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

Order wrapper class proposal #3654

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open

Conversation

wjrosa
Copy link
Contributor

@wjrosa wjrosa commented Dec 12, 2024

Changes proposed in this Pull Request:

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 should make it easier to reference and change metas.

Testing instructions

  • Take a look at the code. Do you think it makes sense to move forward?
  • Check if the tests are still passing
  • Perform some smoke tests:
    -- 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


  • Covered with tests (or have a good reason not to test in description ☝️)
  • Added changelog entry in both changelog.txt and readme.txt (or does not apply)
  • Tested on mobile (or does not apply)

Post merge

@wjrosa wjrosa self-assigned this Dec 12, 2024
@wjrosa
Copy link
Contributor Author

wjrosa commented Dec 13, 2024

Hey team! I would like to have your opinion on this proposal (@Mayisha , @diegocurbelo , @mattallan , @annemirasol ). What do you think?

@annemirasol
Copy link
Contributor

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.

@wjrosa
Copy link
Contributor Author

wjrosa commented Dec 26, 2024

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.

@wjrosa
Copy link
Contributor Author

wjrosa commented Dec 27, 2024

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.

@wjrosa wjrosa marked this pull request as ready for review January 2, 2025 12:02
@wjrosa wjrosa requested a review from mattallan January 2, 2025 12:02
Copy link
Contributor

@Mayisha Mayisha left a 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 latest develop 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-
Screenshot 2025-01-07 at 8 05 52 PM

@diegocurbelo diegocurbelo removed the request for review from mattallan January 16, 2025 20:48
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