Skip to content

Clean up and reduce confusion around the woocommerce_order_actions filter #30475

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
roykho merged 6 commits into from
Aug 27, 2021
Merged

Clean up and reduce confusion around the woocommerce_order_actions filter #30475

roykho merged 6 commits into from
Aug 27, 2021

Conversation

@danielbitzer
Copy link
Contributor

@danielbitzer danielbitzer commented Aug 11, 2021

All Submissions:

Changes proposed in this Pull Request:

The woocommerce_order_actions is used by a a bunch of plugins like WC Payments, Subscriptions, AutomateWoo and could be improved. Currently a global order object ($theorder) is commonly used to conditionally add items to the array depending on what type of order is being edited. Rather than use a global this should be a filter argument.

This PR adds an $order filter argument to the woocommerce_order_actions filter while maintaining backwards compatibility.

How to test the changes in this Pull Request:

  1. Test that order actions appear on the order edit admin screen

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

Dev - Add order argument to "woocommerce_order_actions" filter

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.

@danielbitzer danielbitzer self-assigned this Aug 11, 2021
haszari
haszari reviewed Aug 11, 2021
View reviewed changes
@vedanshujain vedanshujain requested review from a team and roykho and removed request for a team August 13, 2021 13:01
roykho
roykho reviewed Aug 13, 2021
View reviewed changes
includes/admin/meta-boxes/class-wc-meta-box-order-actions.php
)
);
$theorder = $theorder instanceof WC_Order ? $theorder : null;
$order_actions = self::get_available_order_actions_for_order( $theorder );
Copy link
Contributor

@roykho roykho Aug 13, 2021

Choose a reason for hiding this comment

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

Hi @danielbitzer - Thanks for the PR! So we're trying not to add any new methods unless it is absolutely necessary. And for those cases, we also utilize the internal src namespace to do so. That way it is not publicly exposed. But in this case, I don't think we need a new method for this. Perhaps just put the docblock above the filter for clarity.

Copy link
Contributor Author

@danielbitzer danielbitzer Aug 23, 2021

Choose a reason for hiding this comment

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

HI @roykho! Would making the method private also be acceptable? That way it's not publicly exposed?

Copy link
Contributor

@roykho roykho Aug 23, 2021

Choose a reason for hiding this comment

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

Yes that would be better.

Copy link
Contributor Author

@danielbitzer danielbitzer Aug 26, 2021

Choose a reason for hiding this comment

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

Ok great, I've made that change.

@roykho
Copy link
Contributor

roykho commented Aug 26, 2021

@danielbitzer - thanks for the update. Can you please add the @since version in to 5.8.0 (two places). Thanks!

@danielbitzer
Copy link
Contributor Author

danielbitzer commented Aug 27, 2021

Can you please add the @since version in to 5.8.0 (two places). Thanks!

@roykho done!

roykho
roykho approved these changes Aug 27, 2021
View reviewed changes
Copy link
Contributor

@roykho roykho left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. LGTM!

@roykho roykho merged commit ad955c1 into trunk Aug 27, 2021
@roykho roykho deleted the move-order-actions-variable-to-filter-arg branch August 27, 2021 12:41
@github-actions github-actions bot added this to the 5.8.0 milestone Aug 27, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2021

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

  • Add the status: needs changelog label
  • Add the status: needs testing instructions label

@roykho roykho added release: highlight Issues that have a high user impact and need to be discussed/paid attention to. release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels Aug 27, 2021
@zhongruige zhongruige added release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@haszari haszari haszari left review comments

@roykho roykho roykho approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@danielbitzer danielbitzer

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: highlight Issues that have a high user impact and need to be discussed/paid attention to.

Projects

None yet

Milestone

5.8.0

Development

Successfully merging this pull request may close these issues.

5 participants

@danielbitzer @roykho @haszari @zhongruige