Fix order and subscription numbers on transaction list report #1556
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The order.value was being incorrectly assigned. Since it was not used outside of the CSV download functionality we didn't notice it until the table export was implemented. This commit fixes it, fixing the report download as well.
The subscriptions.value was being incorrectly assigned. Since it was not used outside of the CSV download functionality we didn't notice it until the table export was implemented. This commit fixes it, fixing the report download as well.
|
cc @kalessil as you're handling this release 😄 |
|
Thanks for the ping and quick bug-fix @luizreis, I'll review and test the PR today. |
nb
reviewed
Apr 26, 2021
| ] ); | ||
| wcpaySettings.isSubscriptionsActive && orderSubscriptions | ||
| ? orderSubscriptions.map( ( subscription, i, all ) => [ | ||
| <OrderLink key={ i } order={ subscription } />, |
There was a problem hiding this comment.
First, sorry for the drive-by review. Do we have a better choice than to use the index for the key? If the order changes things can get messy behind the scenes.
There was a problem hiding this comment.
No worries at all, thank you for bringing this to our attention! We have this pattern being used in some other parts of the code, though it's not that common, so I created #1789 to track it and fix it in all of the places it's used.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1548
Changes proposed in this Pull Request
This PR fixes the order and subscription values when generating the transaction list, to fix the CSV download report.
I haven't added a changelog entry as this fix only impacts the transaction list download, which hasn't been shipped yet.
Testing instructions
Make sure the transaction list is rendered correctly and no console errors are thrown when rendering it.
Missing order test:
add_order_info_to_objectin./includes/wc-payment-api/class-wc-payments-api-client.php:1119to simulate an order not found response