-
Notifications
You must be signed in to change notification settings - Fork 207
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
Order wrapper class proposal #3654
base: develop
Are you sure you want to change the base?
Conversation
Hey team! I would like to have your opinion on this proposal (@Mayisha , @diegocurbelo , @mattallan , @annemirasol ). What do you think? |
I love that we're consolidating order metadata getters and setters, and abstracting specifics, like the actual key names. I haven't looked at all the changes closely, but I think it's a refactor worth pursuing, @wjrosa. Thanks for making the effort. |
Nice, Anne! I will update this PR and properly move this to review so everyone can have a better look at the idea. |
Hey @woocommerce/quark! I have finished polishing my idea here. I'd appreciate it if you could at least tell me if it is OK to move this forward. I believe it is a great quality of life improvement. All possible meta keys, getters, setters, and retrieval methods are in one place. This also makes our WC_Stripe_Helper class smaller. |
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.
This PR is a proposal for introducing a new children class (WC_Stripe_Order, extended from WC_Order) that wraps all our custom metadata and extension-specific business logic.
This looks like a good refactor to me. 👍
I am not done smoke testing all the flows. But here are a few points @wjrosa -
- There are a few conflicts with the latest changes. Let's do the functional tests after they are resolved.
- We need to move lock_order_refund and unlock_order_refund to
WC_Stripe_Order
class. Please recheck the latestdevelop
branch to identify similar missing functions/usages that were merged after this PR was created. - I have tested a simple product purchase and got this error in the logs-
Changes proposed in this Pull Request:
This PR is a proposal for introducing a new children class (
WC_Stripe_Order
, extended fromWC_Order
) that wraps all our custom metadata and extension-specific business logic. This should make it easier to reference and change metas.Testing instructions
-- Connect your Stripe account
-- Enable some payment methods
-- As a shopper, make some purchases
* Unit tests for the new class will be added if this proposal gets approved
changelog.txt
andreadme.txt
(or does not apply)Post merge