Skip to content

Fix code standards in orders code, and check payment method name instead of title on get order totals #30468

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.

Sign up for GitHub

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

Jump to bottom
Merged
jorgeatorres merged 6 commits into from
Jul 22, 2022
Merged

Fix code standards in orders code, and check payment method name instead of title on get order totals #30468

jorgeatorres merged 6 commits into from
Jul 22, 2022

Conversation

@Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Aug 10, 2021

All Submissions:

Changes proposed in this Pull Request:

Fix code standards errors in order details metabox (class-wc-meta-box-order-data.php)

  • Sanitize all input.
  • Escape all output.
  • Verify nonce on save.
  • Verify that all required POST parameters are present on save, throw an exception if not.
  • Minor coding standards fixes (periods at end of comments, function docs, Yoda conditions...)

Check payment method name instead of display title in WC_Order::add_order_item_totals_payment_method_row

This method, used by get_order_item_totals, now adds a "payment method" row to the returned data only if the payment method name is different from other (the payment method title was being checked instead, but both are different since #30256 was merged).

How to test the changes in this Pull Request:

For the code standards fixes: open an order in the admin area and verify that all the data in the order dettails box is rendered correctly. Do some changes in the order, click "Update" and verify that the changes are saved correctly.

For the change in get_order_item_totals:

  1. Open an order in the admin area.
  2. Edit the billing details, change the payment method to "Other", save.
  3. In the "Order actions" box, run the "Email invoice/order details to customer" action.
  4. Verify that the email sent does not contain a "payment method" row in the order details table (without the fix it will contain "Payment method: other").

You may want to use the email logger plugin to see the email sent.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

N/A - It's basically a fix for #30256, which is to be included in the same release.

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@Konamiman Konamiman self-assigned this Aug 10, 2021
claudiosanches
claudiosanches suggested changes Aug 10, 2021
View reviewed changes
claudiosanches
claudiosanches reviewed Aug 10, 2021
View reviewed changes
claudiosanches
claudiosanches suggested changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@claudiosanches claudiosanches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, there's only a minor change pending to remove check for $_POST['_wpnonce'].

claudiosanches
claudiosanches previously approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@claudiosanches claudiosanches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great now!

@claudiosanches
Copy link
Contributor

claudiosanches commented Sep 27, 2021

There's still an error with PHP Unit tests, but seems like just need to rebase with trunk to fix it. Anyway, doesn't seems to be a requirement before merging this PR.

@ObliviousHarmony ObliviousHarmony added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 21, 2022
@jorgeatorres jorgeatorres self-assigned this Feb 24, 2022
@jorgeatorres jorgeatorres removed their assignment Jun 16, 2022
Konamiman added 4 commits June 17, 2022 16:52
@Konamiman @jorgeatorres
Fix code standards errors in class-wc-meta-box-orrder-data.php
d0b4612 
- Sanitize all input
- Escape all output
- Verify nonce on save
- Verify that all required POST parameters are present on save
- Minor fixes (periods at end of comments, function docs, Yodas...)
@Konamiman @jorgeatorres
Check payment method name, not title, on add_order_item_totals_paymen…
0385fed 
…t_method_row
@Konamiman @jorgeatorres
Remove unnecessary nonce verification in "save" for orders.
7a33564 
Also move comment to new line for readability.
@Konamiman @jorgeatorres
Remove unnecessary nonce check in WC_Meta_Box_Order_Data::save
8f3c2f7 
 The check is already performed in the code that invokes "save".
@jorgeatorres jorgeatorres force-pushed the fix/orders-code-standards branch from 29a6f42 to 8f3c2f7 Compare June 17, 2022 19:52
@botwoo
Copy link
Collaborator

botwoo commented Jun 17, 2022

📊 Test reports for this pull request have been published and are accessible through the following links:

Latest commit referenced in the reports: Remove unnecessary nonce check in WC_Meta_Box_Order_Data::save 8f3c2f7
This comment will automatically be updated with the latest referenced commit when you push new changes to this pull request.

Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them.

@jorgeatorres
Copy link
Member

jorgeatorres commented Jun 17, 2022

Hi @Konamiman!

FYI I've rebased this with the latest trunk to fix conflicts. CI is now complaining about some other codestyle violations. It's been a while since you last worked on this, so let me know if you still want to work on those and I'd be more than happy to review.

We could also switch places if you'd rather have me fix the remaining issues.

Thanks!

@jorgeatorres jorgeatorres added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Jun 17, 2022
@Konamiman
Copy link
Contributor Author

Konamiman commented Jul 18, 2022

Hi @jorgeatorres, yes, let's switch places and I'll review this time. Thank you!

@github-actions github-actions bot added needs: triage feedback Issues for which we requested feedback from the author and received it. and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Jul 18, 2022
@Konamiman
Copy link
Contributor Author

Konamiman commented Jul 21, 2022

Hi @jorgeatorres, after a second look it seems that all the code style violations are bout hooks not having documentation comments. I don't think it makes sense to add all those in the context of this pull request, so maybe we can merge it as is.

jorgeatorres
jorgeatorres requested changes Jul 21, 2022
View reviewed changes
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Konamiman!

This looks good to me. I've only requested one tiny change. Once that's addressed, we're good to go.

Thanks!

@jorgeatorres jorgeatorres added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Jul 21, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2022 •
edited
Loading

Test Results Summary

Commit SHA: 316be4d

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests11500201170m 42s
E2E Tests185001018616m 24s
 To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@Konamiman Konamiman force-pushed the fix/orders-code-standards branch from 51e412c to f96737e Compare July 22, 2022 14:55
jorgeatorres
jorgeatorres approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks Néstor!

@jorgeatorres jorgeatorres merged commit 7d2a992 into trunk Jul 22, 2022
@jorgeatorres jorgeatorres deleted the fix/orders-code-standards branch July 22, 2022 17:54
@github-actions github-actions bot added this to the 6.9.0 milestone Jul 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 22, 2022

Hi @jorgeatorres, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

jacob-sewell pushed a commit that referenced this pull request Jul 29, 2022
@Konamiman @jorgeatorres @jacob-sewell
Fix code standards in orders code, and check payment method name inst…
d8dc5cb 
…ead of title on get order totals (#30468)

* Fix code standards errors in class-wc-meta-box-orrder-data.php

- Sanitize all input
- Escape all output
- Verify nonce on save
- Verify that all required POST parameters are present on save
- Minor fixes (periods at end of comments, function docs, Yodas...)

* Check payment method name, not title, on add_order_item_totals_payment_method_row

* Remove unnecessary nonce verification in "save" for orders.

Also move comment to new line for readability.

* Remove unnecessary nonce check in WC_Meta_Box_Order_Data::save

 The check is already performed in the code that invokes "save".

* Add transaction id to payment method string if URL doesn't exist

* Minor fix

Co-authored-by: Jorge A. Torres <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorgeatorres jorgeatorres jorgeatorres approved these changes

+1 more reviewer

@claudiosanches claudiosanches claudiosanches left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Konamiman Konamiman

Labels

needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. needs: triage feedback Issues for which we requested feedback from the author and received it. plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

6.9.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Konamiman @claudiosanches @botwoo @jorgeatorres @ObliviousHarmony