Skip to content

Conversation

@koke
Copy link
Member

@koke koke commented Nov 4, 2022

Closes: #8030

Description

The existing networking code uses DotcomValidator and DotcomError to attempt to extract error information from WordPress.com API responses (error/message). However, the newer endpoints that use the core REST API (wpcom/v2, wp/v2) use a different error format (code/message/data).

This PR got a bit complex, but the main idea is that each type of request can define a validator that might convert a response into an error. This is useful because a DotcomRequest might want to do a different thing depending on the wordpressApiVersion.

Testing instructions

The easiest way to force an error is to modify InAppPurchasesRemote and make sure the X-APP-ID header has an unknown identifier (e.g. Bundle.main.bundleIdentifier?.appending(".unknown")).

Then:

  1. Go to Hub menu > Debug IAP
  2. Wait for the products to load
  3. Look at the console for an error

If you test this on trunk, all you would see is a parsing error:

Error loading products: typeMismatch(Swift.Array, Swift.DecodingError.Context(codingPath: [], debugDescription: "Expected to decode Array but found a dictionary instead.", underlyingError: nil))

With the changes in this PR, you should see a more specific error:

Error loading products: WordPress API Error: [rest_forbidden] Sorry, you are not allowed to do that.


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

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 4, 2022

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 pr8031-0ae120c on your iPhone

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

@koke koke requested a review from toupper November 4, 2022 11:09
@koke koke added this to the 11.1 milestone Nov 4, 2022
@koke koke added type: task An internally driven task. category: architecture Related to architecture such as the database, FluxC, Networking, Core Data, etc. feature: in-app purchases Related to In-app purchases and subscriptions labels Nov 4, 2022
@peril-woocommerce
Copy link

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

Generated by 🚫 dangerJS

@koke koke marked this pull request as ready for review November 4, 2022 11:09
@toupper
Copy link
Contributor

toupper commented Nov 4, 2022

The code looks good to me and it tests well! 🎉 :shipit: It's nice that we have now more specific error messages:

2022-11-04 12:57:32.026167+0100 WooCommerce[12468:3206818] <> Mapping Error: WordPress API Error: [rest_forbidden] Sorry, you are not allowed to do that.
2022-11-04 12:57:32.026460+0100 WooCommerce[12468:3206819] [💰IAP Store] Failed obtaining product list from StoreKit: WordPress API Error: [rest_forbidden] Sorry, you are not allowed to do that.
Error loading products: WordPress API Error: [rest_forbidden] Sorry, you are not allowed to do that.

I got three repeated error messages, but I guess it's because we log them at every level of our IAP architecture.


/// WordPress API Request Error
///
public enum WordPressApiError: Error, Decodable, Equatable {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have an enum for WordPress.com Request Errors (DotcomError), for me to understand, do these two enums have different purposes?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for the core API errors, which might come from WordPress.com (wpcom/v2, wp/v2) or directly from a self-hosted site (via WordPressOrgRequest). There might be some overlap, but error codes might not necessarily match.

@koke
Copy link
Member Author

koke commented Nov 4, 2022

I got three repeated error messages

Oh good catch. I think the "Mapping error" shouldn't be there. Previously it would only log if the error was coming from the mapper, but since I consolidated the error handling, it's too verbose now.

@koke koke merged commit 2266c70 into trunk Nov 4, 2022
@koke koke deleted the issue/8030-dotcom-error-v2-parser branch November 4, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: architecture Related to architecture such as the database, FluxC, Networking, Core Data, etc. feature: in-app purchases Related to In-app purchases and subscriptions type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update DotComError parser to support v2 errors

4 participants