Skip to content

[Stats Refresh] Periods loading view#11634

Merged
danielebogo merged 14 commits intodevelopfrom
issues/10380-periods-loading
May 16, 2019
Merged

[Stats Refresh] Periods loading view#11634
danielebogo merged 14 commits intodevelopfrom
issues/10380-periods-loading

Conversation

@danielebogo
Copy link
Contributor

Refs. #10380

This PR adds the loading view in SiteStatsPeriodTableViewController
The behaviour is the same implemented here

stats-periods-loading-view

Error Handling

I will push another PR for the error handling. This because it requires some change on the Store.

To test:

• From My Sites select a site and open its dashboard
• Select Stats to open the Stats section.
• Select one of the tab period tab Days, Weeks, Months, Years.
• You should see the loading view
• The loading view should disappear as soon the data is loaded

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@danielebogo danielebogo added this to the 12.4 milestone May 3, 2019
@danielebogo danielebogo requested a review from ScoutHarris May 3, 2019 20:27
@danielebogo danielebogo self-assigned this May 3, 2019
@ScoutHarris
Copy link
Contributor

Hey @danielebogo .

I think we need to show the loading view when the period is changed, and when the date changes (via the date selector). Some of the queries take a bit to run, especially the one for the overview card (aka summary). Without a loading view, it looks done when it's actually not. The result is the overview card shows 0 values for a while until the query is finished.

What do you think?

@jkmassel
Copy link
Contributor

jkmassel commented May 6, 2019

Bumping this to 12.5. If it needs to be released in the 12.4 timeframe, please let me know and we can do an additional beta release.

@jkmassel jkmassel modified the milestones: 12.4, 12.5 May 6, 2019
@danielebogo
Copy link
Contributor Author

Hey @ScoutHarris this is ready for another round!

@ScoutHarris
Copy link
Contributor

ScoutHarris commented May 14, 2019

Hey @danielebogo . This is shaping up nicely!

However, I noticed the loading view is removed before the stats are finished loading.

  1. I see this in the logs:
2019-05-14 13:01:07:978 WordPress[6994:8974788] Stats: All fetching operations finished.
2019-05-14 13:01:19:706 WordPress[6994:8974788] Stats: Finished fetching summary.
2019-05-14 13:01:19:718 WordPress[6994:8974788] Stats: All fetching operations finished.

Which seemed strange. Until I discovered state.fetchingSummary is never set to true. This is not your error, but would you mind fixing it by adding this line to this method in StatsPeriodStore:

func setAllAsFetchingOverview(fetching: Bool = true) {
    state.fetchingSummary = fetching
    ...
  1. While Implements featured image zoom in and an adjusted size for previewing it, depending on available screen space, in Post Setings controller #1 makes it better, it doesn't actually fix the problem. The Period view is still updated before all the stats are finished. Watching the logs, I can see it's not finished yet, but I'm no longer seeing the loading view.
2019-05-14 13:27:42:803 WordPress[3323:2026611] Stats: Finished fetching published.
2019-05-14 13:27:42:878 WordPress[3323:2026611] Stats: Finished fetching videos.
2019-05-14 13:27:42:953 WordPress[3323:2026611] Stats: Finished fetching search terms.
2019-05-14 13:27:43:044 WordPress[3323:2026611] Stats: Finished fetching posts.
2019-05-14 13:27:43:115 WordPress[3323:2026611] Stats: Finished fetching countries.
2019-05-14 13:27:43:943 WordPress[3323:2026611] Stats: Finished fetching clicks.
2019-05-14 13:27:43:972 WordPress[3323:2026611] Stats: Finished fetching authors.

< Loading view removed here. >

2019-05-14 13:27:45:891 WordPress[3323:2026611] Stats: Finished fetching referrers.
2019-05-14 13:28:07:902 WordPress[3323:2026611] Stats: Finished fetching summary.
2019-05-14 13:28:07:937 WordPress[3323:2026611] Stats: All fetching operations finished.

This is much easier to notice for longer periods, like months and years, where the summary query takes longer (to note, without cached data). For those periods, changing the date via the date bar, you should be able to repro it easily enough.

@danielebogo
Copy link
Contributor Author

Hey @ScoutHarris ! Thanks for the hint about the missed flag! I fixed it!

While #1 makes it better, it doesn't actually fix the problem. The Period view is still updated before all the stats are finished. Watching the logs, I can see it's not finished yet, but I'm no longer seeing the loading view

I fixed the way we check the cached data and the way the loading view is handled. I separate the logic from the onChange block (which is still there) because the new one is called only when it's required.

Let me know what you think!

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah!!

I fixed the way we check the cached data and the way the loading view is handled. I separate the logic from the onChange block (which is still there) because the new one is called only when it's required.

Oh this is much better. Thank you!!

:shipit:

@danielebogo
Copy link
Contributor Author

Thanks @ScoutHarris !

@danielebogo danielebogo merged commit 5b27bb6 into develop May 16, 2019
@danielebogo danielebogo deleted the issues/10380-periods-loading branch May 16, 2019 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants