-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Fix: pass expected checkout validation WP_Error instance #531
Fix: pass expected checkout validation WP_Error instance #531
Conversation
@kidunot89 does this look reasonable to you? I did this based on how woocommerce passes an error object as the second argument to this filter. Thank you for everything you do! |
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.
@jeebay So sorry about the delay review on this. Can you rebase to the develop
branch and confirm if your change is passing CI/CD.
78672e3
to
2027873
Compare
@kidunot89 I rebased, still failing a codeclimate check that wants the class |
@kidunot89 do you mind taking another look? I'm unable to run tests in CI as I'm not a maintainer. |
@jeebay Activated 👌. CI running now |
Code Climate has analyzed commit 2027873 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 77.0% (0.0% change). View more on Code Climate. |
@kidunot89 tests pass except for code climate but it looks like these two issues have been ignored previously. Do you want me to address in this PR? Also happy to do in a followup. |
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.
@jeebay Thanks for the work on this 👍🏿
Your checklist for this pull request
Thanks for sending a pull request! Please make sure you click the link above to view the contribution guidelines, then fill out the blanks below.
🚨Please review the guidelines for contributing to this repository.
What does this implement/fix? Explain your changes.
Passes an empty instance of
WP_Error
to thewoocommerce_after_checkout_validation
action hook. WooCommerce passes an error object as the second argument to the hook and several 3rd party plugins expect it to be provided. Dokan for example doesn't even checkis_wp_error
before calling$errors->add()
resulting in runtime errors.Normally I'd expect plugin developers to know better, do better but in this case, the wp-graphql-woocommerce breaks from Woocommerce in not passing the second argument. Supplying an empty
Does this close any currently open issues?
#448
Any relevant logs, error output, GraphiQL screenshots, etc?
Example error from my application
Any other comments?
I attempted to add a unit test but I the tests for
CheckoutMutationTest
aren't passing locally on a fresh clone. Arguably no test is needed as it would just be testing WordPress internals.It looks like a database issue with some rows not updating? I'm glad to attempt again with some guidance on how to resolve.
Where has this been tested?
N/A