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

[Resilience] Customize error banner message when data can't be loaded due to parsing errors #10260

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

rachelmcr
Copy link
Contributor

@rachelmcr rachelmcr commented Jul 19, 2023

Part of #10162

Description

This is a followup to #10252. In that PR, we improved the message on the Orders list error banner when there is a parsing error while loading data. This PR makes the same change for Products, Reviews, and the empty state screen.

If the error was a parsing (decoding) error, the banner message now mentions possible plugin conflicts.

Changes

  • On the products or reviews screens, we now keep track of what error occurred during sync and use it to create the corresponding error banner.
  • On the empty state screen, we now use the error to create the corresponding error banner in showTopBannerView(for:).

Note: The banner isn't updated yet on the My store screen (see the issue in #10259).

Testing instructions

You can test using a tool like Charles Proxy or Proxyman to intercept and modify the API response:

  1. Set a breakpoint in your tool of choice for requests to the /wc/v3/products endpoint.
  2. Build and run the app.
  3. Open the Products tab.
  4. Modify the response so there is an invalid product object (e.g. remove a required field).
  5. Execute the response and confirm the error banner appears with the new message, mentioning a possible plugin conflict.
  6. Disable your internet connection and pull to refresh the Products list (to trigger a non-decoding error).
  7. Confirm the error banner appears with the general error message.

Repeat the steps with a breakpoint for requests to the /wc/v3/products/reviews endpoint, opening Menu > Reviews in the app.

Screenshots

/ General error Parsing error
Products Simulator Screen Shot - iPhone 14 Pro - 2023-07-19 at 11 28 18 Simulator Screen Shot - iPhone 14 Pro - 2023-07-19 at 11 28 07
Reviews Simulator Screen Shot - iPhone 14 Pro - 2023-07-19 at 11 27 12 Simulator Screen Shot - iPhone 14 Pro - 2023-07-19 at 11 26 57
Empty state Simulator Screen Shot - iPhone 14 Pro - 2023-07-19 at 11 25 31 Simulator Screen Shot - iPhone 14 Pro - 2023-07-19 at 11 26 17

  • 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: review list Related to the reviews list. feature: product list Related to the product list. 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 10:32
@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 pr10260-72ca05f on your iPhone

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

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.

LGTM! It was the first time for me to modify the response using a breakpoint in Charles Proxy, took me some time to figure out why I couldn't edit the response (had to tap Cancel instead of Execute while online tutorials said to tap Execute) 😓

Non-blocking: if it's easy to identify the offline error, maybe we can show a different message about it to indicate the device is currently offline.


// When
viewModel.synchronizeReviews(pageNumber: 1, pageSize: 25, onCompletion: nil)

// Then
XCTAssertTrue(viewModel.hasErrorLoadingData)
XCTAssertNotNil(viewModel.dataLoadingError)
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 can also assert that the error is SampleError.first

@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@rachelmcr
Copy link
Contributor Author

Non-blocking: if it's easy to identify the offline error, maybe we can show a different message about it to indicate the device is currently offline.

We do have the offline banner when the device is offline, so we could show a different message in the error banner but maybe that's overkill?

@rachelmcr rachelmcr enabled auto-merge July 20, 2023 09:08
@rachelmcr rachelmcr merged commit 364a95d into trunk Jul 20, 2023
@rachelmcr rachelmcr deleted the issue/10162-parsing-error-banners branch July 20, 2023 09:40
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: product list Related to the product list. feature: review list Related to the reviews list.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants