Skip to content

[Stats Refresh] StatsPeriodStore: make use of the new API#11342

Merged
jklausa merged 10 commits intofeature/stats-core-data-finishedfrom
feature/stats-timeinterval-new-api
Mar 28, 2019
Merged

[Stats Refresh] StatsPeriodStore: make use of the new API#11342
jklausa merged 10 commits intofeature/stats-core-data-finishedfrom
feature/stats-timeinterval-new-api

Conversation

@jklausa
Copy link
Contributor

@jklausa jklausa commented Mar 27, 2019

This switches StatsPeriodStore to the new StatsServiceRemoteV2 based API, also updating the appropriate ViewModels.

I found two more changes (grr.....) I'll need to make to the data model when working on this —

  • Videos endpoint doesn't return a media ID, just a URL
  • we need to move the country-flag icons from WPStatsiOS module to WPiOS. This was outside of scope for this PR so I decided to leave it broken for now.

To test:

  • Go to Stats
  • Look at Period-type data
  • Verify everything still works! (modulo the two broken things above)

@jklausa jklausa added this to the 12.2 milestone Mar 27, 2019
@jklausa jklausa self-assigned this Mar 27, 2019
@jklausa jklausa requested a review from ScoutHarris March 27, 2019 05:35
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@ScoutHarris
Copy link
Contributor

ScoutHarris commented Mar 27, 2019

Hey @jklausa .

It was really difficult to verify the data due to these 2 issues, primarily the first:

  1. Switching tabs doesn't always update the data. It seems like the selected period is not updated.
  • For example, if I'm on Years and switch to Days, I see the log messages that the queries finished, but Year data is still shown.
  • If I go back to the site details and select Stats again, the tab is still Days, and the data is still Years.
  1. Fetching the data is taking a lot longer. It appears to be related to the summary fetching.
    statsRemote.getData(for: period, endingOn: date) { (summary: StatsSummaryTimeIntervalData?, error: Error?)

I was able to identify these as well:

  • All the Details lists are now limited to 10. They should be showing all results.
  • Clicks should not show icons.
    - Referrers > Search Engines: children and grandchildren rows should not show the icon.
  • Every web view that displays a WP link prompts me to log in. I don't think we want this. A user shouldn't have to log in just view a site. They don't currently, and don't on Android.

I think I saw some data discrepancies, but again it was hard to tell with the tab switching issue. Once that's resolved, I'll take another look.

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.

See comment about issues.

@ScoutHarris
Copy link
Contributor

Hey @jklausa . Please ignore the comment about the Referrers > Search Engines icons. I've marked it out. Sorry!

@jklausa
Copy link
Contributor Author

jklausa commented Mar 28, 2019

Updated!

I temporarily removed fetching the summary data, since we're not currently making use of it anywhere. I'll fix that in a separate issue.

Switching tabs should be fixed now — because we don't currently have any loading indicators, it may appear as nothing is happening, so you need to observe the console log.

I fixed clicks to not show icons.

The issue with "details" list is due to #11359 — if you "manually" pull-to-refresh it, it'll work. Note that there are also some (new) performance issues with that view (#11360).

As for the webviews — I don't think this is due to anything could have done here, but I'll continue to investigate.

}

if !isFetchingOverview {
DDLogInfo("Stats: All fetching operations finished.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!!

@ScoutHarris
Copy link
Contributor

Hey @jklausa .

I found a few more issues:

Period > Posts & Pages:

  • Days: P & P shows No data for this period when there actually are (ex: en.blog).
  • Switch to another Period, then back to Days. P & P doesn't update.

Period > Published:

  • Seems to be incorrect. Example, for en.blog, on develop Weeks and Months show more results than in this branch.

@ScoutHarris
Copy link
Contributor

Hey @jklausa .

I nuked my Pods directory, and that resolved the Posts and Pages issues.

However, the Published issue remains.

@jklausa
Copy link
Contributor Author

jklausa commented Mar 28, 2019

@ScoutHarris confirmed, I'd right to tackle that in a follow-up if you don't mind :)

@ScoutHarris
Copy link
Contributor

Works for me! :shipit:

@jklausa
Copy link
Contributor Author

jklausa commented Mar 28, 2019

heyo!

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.

2 participants