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

Restructure the order transaction state tree. #23822

Merged

Conversation

southp
Copy link
Contributor

@southp southp commented Apr 2, 2018

Summary

This is a follow-up task of making the best use of the newly-deployed order transaction status endpoint, following #23792 . It seems big, but it actually can be boiled down into:

  1. Data layer
    1. Enhance fromApi() to camelCase the keys and convert the processing status into a set of known constants.
  2. State tree
    1. Add isFetching and errors substate under orderTransactions
    2. A new action for dispatching errors.
    3. Add selectors for the above new substates.
    4. Add new test cases for covering them.

Test Plan

  1. Run npm run test-client order-transactions
  2. Run npm run test-client "transactions/order"

@southp southp self-assigned this Apr 2, 2018
@matticbot
Copy link
Contributor

@southp southp added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 2, 2018
@southp southp requested review from a team April 2, 2018 11:11
@southp
Copy link
Contributor Author

southp commented Apr 3, 2018

Hmm, I've found a weird bug when doing a manual e2e test.
When GET /me/transactions/order/:order_id endpoint responds with a 200 response, e.g.

{order_id: 150077, user_id: 107809150, processing_status: "processing"}

Somehow the action passed to https://github.com/Automattic/wp-calypso/blob/master/client/state/data-layer/wpcom-http/utils.js#L382 has meta.dataLayer like this:
image
It makes the following getData() to return '' and make fetchAction to https://github.com/Automattic/wp-calypso/blob/master/client/state/data-layer/wpcom-http/utils.js#L410. Since the fetchAction here is just yet another ORDER_TRANSACTION_FETCH, the whole flow will repeat again and again.

Normally meta.dataLater.data should be filled with the response data and meta.dataLayer.headers is a object containing the parsed header.

@southp
Copy link
Contributor Author

southp commented Apr 10, 2018

@Automattic/payments @Automattic/i18n I've resolved the above issue in 1365c79. This PR should now work and is ready for review 🙇🏻

@southp southp force-pushed the update/match-response-format-of-order-transaction-endpoint branch from 1365c79 to 99ea650 Compare April 10, 2018 03:40
Copy link
Contributor

@yoavf yoavf left a comment

Choose a reason for hiding this comment

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

I don't feel equipped to fully review the state code, but overall:

@southp southp merged commit c608ae5 into master Apr 13, 2018
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 13, 2018
@blowery blowery deleted the update/match-response-format-of-order-transaction-endpoint branch May 16, 2018 13:47
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