Store balance transaction ID in order metadata #7945
Merged
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 method has too many params whose values can easily be derived from a single intent object. This commit replaces all derivable params with an intent param. This will make it easier to add new intent-based data values to order meta.
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
… a more accurate mock object
anu-rock
commented
Jan 2, 2024
| * @param WC_Order $order The order. | ||
| * @param WC_Payments_API_Abstract_Intention $intent The payment or setup intention object. | ||
| * @param WC_Order $order The order. | ||
| * @param WC_Payments_API_Payment_Intention|WC_Payments_API_Setup_Intention $intent The payment or setup intention object. |
There was a problem hiding this comment.
I was getting Psalm UndefinedMethod linting error, so replaced the abstract class type hint with a list of possible concrete types. I wonder if there's a proper way to use abstract types as type hints without Psalm complaining.
Looks like new reference of the method that came with the latest code from develop.
dpaun1985
reviewed
Jan 4, 2024
| * | ||
| * @throws Order_Not_Found_Exception | ||
| */ | ||
| public function attach_intent_info_to_order( WC_Order $order, $intent ) { |
There was a problem hiding this comment.
part of this method ( calling the setters and calling the save method) is identical to attach_intent_info_to_order__legacy .
This method extracts the data that needs to be added to the order, and saves it on the order. attach_intent_info_to_order__legacy only saves the information to the order.
We can keep this method to extract the data and call the attach_intent_info_to_order__legacy in this method.
So, we could:
- change this method name to
add_intent_info_to_order - keep the old name for
attach_intent_info_to_order__legacy(attach_intent_info_to_order) - call
attach_intent_info_to_orderin this method
There was a problem hiding this comment.
I agree with this, common code can be extracted to a separate function as part of this refactor.
There was a problem hiding this comment.
@dpaun1985 @naman03malhotra Thanks for the feedback. I fixed the code duplication in a7bef87.
In the long term, I think it's desirable to remove the legacy method and keep only the refactored one. This will allow us to care about only one method when adding new intent properties to order meta.
change this method name to add_intent_info_to_order
Unfortunately, I cannot change the name as I'd like my changes to apply to all uses of this method.
| * | ||
| * @throws Order_Not_Found_Exception | ||
| */ | ||
| public function attach_intent_info_to_order( WC_Order $order, $intent ) { |
There was a problem hiding this comment.
I agree with this, common code can be extracted to a separate function as part of this refactor.
|
Some more feedback from Gamma: p1704266984371909-slack-C01BPL3ALGP |
Co-authored-by: Naman Malhotra <[email protected]>
This reverts commit 1f4681a.
dpaun1985
approved these changes
Jan 15, 2024
|
@dpaun1985 Thanks for your re-testing and approval. But I wonder why the key is named |
I think I didn't updated the branch and was an older commit. I just tested and it's the correct key |
mordeth
approved these changes
Jan 16, 2024
Jinksi
pushed a commit
that referenced
this pull request
Jan 30, 2024
Co-authored-by: Cvetan Cvetanov <[email protected]> Co-authored-by: Naman Malhotra <[email protected]> Co-authored-by: Dan Paun <[email protected]>
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 #7856
Changes proposed in this Pull Request
_wcpay_payment_transaction_idin order metadata to store Stripe's balance transaction ID for each payment to aid in transaction reconciliation in third-party accounting systems.attach_intent_info_to_ordermethod to make it easier to add new intent-based data values to order meta.Testing instructions
/wp-json/wc/v3/orders/<order_id>(see Retrieve an order API)._payment_transaction_idin the response order object's meta_data property with a value starting with 'txn_'.Screen.Recording.2024-01-16.at.4.12.05.PM.mp4
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge