-
Couldn't load subscription status.
- Fork 217
Handling more Stripe metas with the order helper class #4705
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
Conversation
Co-authored-by: Mayisha <[email protected]>
Co-authored-by: Mayisha <[email protected]>
Co-authored-by: Mayisha <[email protected]>
Co-authored-by: Mayisha <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some feedback, based mostly on the naming, but there are one or two other areas that may drive changes. I'll dive into testing after confirming we don't plan to make any changes.
| * @param string $payment_type | ||
| * @return false|void | ||
| */ | ||
| public function update_stripe_upe_payment_type( ?WC_Order $order = null, string $payment_type = '' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question on the types: might we expect instances of WC_Order_Refund to be supplied to any of these helper methods? If so, we might want to loosen the types to accept ?WC_Abstract_Order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code, I don't think so. I don't see any references to WC_Order_Refund in the flows where this method is called. But happy to make this change if you think it is safer 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my side, I was mostly flagging it as a possible concern. I don't know the full codebase well enough to know what objects we expect, so having the strict type might be a subtle compatibility issue. Using WC_Abstract_Order would be less likely to break, even if we never see a refund or similar, while the slightly more restrictive type would throw an exception for an unexpected object.
Maybe this is something we should tackle in a separate, dedicated follow-up if we decide to make any changes.
Co-authored-by: daledupreez <[email protected]>
…d and implementations
Co-authored-by: daledupreez <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes LGTM, and the tests are clean, so I think we should 🚢 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These updates look good to me, and smoke tests didn't reveal any issues.
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Base PR #4613
P2 peNR48-1Jc-p2
Changes proposed in this Pull Request:
As part of the Order Helper class project, I am including some additional meta keys to be handled by the new class and implementing the new methods across our codebase.
The metas handled here are:
_stripe_customer_id,_stripe_card_id,_stripe_upe_payment_type,_stripe_upe_waiting_for_redirect, and_stripe_upe_redirect_processed.Testing instructions
Changelog entry
Changelog Entry Comment
Comment
Post merge