Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[My store] Fix error banner so it appears when stats data can't be loaded #10262

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

rachelmcr
Copy link
Contributor

@rachelmcr rachelmcr commented Jul 19, 2023

Closes: #10259

Description

The error banner (to explain there is a problem loading data) was not showing up on the My store dashboard. This PR fixes 3 related issues:

  1. The check for sync errors was happening before the syncing was complete (and before any sync error was set).
    • Fix: In StoreStatsAndTopPerformersViewController there are several dispatch groups used for syncing (group, periodGroup, and periodStoreStatsGroup). We now call group.leave() last to make sure all the work for the time period is complete (including setting any period sync error) before checking for and handling sync errors for the dashboard as a whole.
  2. We were attempting to fetch WPCom stats (visit and summary stats) even for sites not connected to WPCom, causing sync errors.
    • Fix: Now in DashboardViewModel we check if the store is authenticated with WPCom before dispatching those actions, to avoid extra API calls and unnecessary sync errors.
  3. The pull to refresh mechanism has changed since the error banner was first introduced, and the error banner wasn't removed if the dashboard was refreshed after it appeared.
    • Fix: Now in DashboardViewController we hide the error banner optimistically on pull to refresh.

Testing instructions

  1. Build and run the app.
  2. Disable your internet connection and pull to refresh the dashboard (to trigger an error).
  3. Confirm the error banner appears at the top of the screen (under the site title).
  4. Enable your internet connection and pull to refresh again.
  5. Confirm the error banner is removed and the dashboard loads as expected.

You can test using a store with or without Jetpack, to confirm the expected stats load in both cases.

Screenshots

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-19.at.14.10.56.mp4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@rachelmcr rachelmcr added feature: stats Related to stats, including Top Performers. category: reliability Related to app’s reliability, accuracy, and perceived waiting time labels Jul 19, 2023
@rachelmcr rachelmcr added this to the 14.6 milestone Jul 19, 2023
@rachelmcr rachelmcr marked this pull request as ready for review July 19, 2023 14:43
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 19, 2023

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr10262-519ae01 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@jaclync jaclync self-assigned this Jul 20, 2023
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

Nice catch on the issue, works as expected :shipit:

periodGroup.leave()
periodStoreStatsGroup.leave()
group.leave()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe it's worth making a comment so that we don't accidentally reorder it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good thought!

@rachelmcr rachelmcr enabled auto-merge July 20, 2023 09:13
@rachelmcr rachelmcr merged commit 6a158b5 into trunk Jul 20, 2023
@rachelmcr rachelmcr deleted the issue/10259-dashboard-error-banner branch July 20, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: reliability Related to app’s reliability, accuracy, and perceived waiting time feature: stats Related to stats, including Top Performers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[My store] Error banner does not appear when stats data can't be loaded on dashboard
3 participants