Deprecated WC_Cart->is_coupon_emails_allowed in favour of static DiscountsUtil::is_coupon_emails_allowed

[wavvves]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR refactors code and deprecates and moves is_coupon_emails_allowed() from WC_Cart into a new DiscountsUtil class as it is better suited to live there. This enables 3rd party code to use WC_Discounts in Order context, where a cart might not exist (specific use case linked in the mentioned issue below), thus preventing a fatal error from occurring when trying to access the non-existent cart.

Closes #47330 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

1. Create a new coupon named "exclusive", and set email restriction on it
2. Install the Code Snippets extensions and add the following snippet:

add_action("wp_loaded","test_coupon",20,1);
function test_coupon(){
	WC()->cart = null; // making sure there is no cart
	$order = new WC_Order();
	$order->apply_coupon('exclusive');
}

3. Refresh any store page and ensure no critical error is displayed
4. Repeat the steps on an unpatched installation and verify that after adding the snippet a critical error is produced.

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Fixed a fatal error when programmatically using the WC_Discounts::class in a context where no cart exists.

Comment

[github-actions]

Hi @Konamiman, @opr,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[github-actions]

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

[Konamiman]

Given that:

1. We are trying to not add any new methods to classes in includes (everything new should go in src), and
2. The code in is_coupon_emails_allowed is acting exclusively on the input (it doesn't depend on any state)

then I think the is_coupon_emails_allowed method should be a static method in a new CouponUtil class (or perhaps DiscountUtil) in src/Utilities (normally I would suggest src/Internal/Utilities instead, but the method is already public anyway).

Also this is a case where I would add unit tests. I can help with that if you want.

[wavvves]

Given that:

1. We are trying to not add any new methods to classes in includes (everything new should go in src), and
2. The code in is_coupon_emails_allowed is acting exclusively on the input (it doesn't depend on any state)

then I think the is_coupon_emails_allowed method should be a static method in a new CouponUtil class (or perhaps DiscountUtil) in src/Utilities (normally I would suggest src/Internal/Utilities instead, but the method is already public anyway).

Also this is a case where I would add unit tests. I can help with that if you want.

I 100% agree with going the static route. I can make the change, but I do have a question. Do you think it is worth considering if, at this point, anything else should qualify for moving into a DiscountsUtil class and move it, or should I just highlight it for a future refactor?
I'm not a big fan of segmented Utility classes, as that doesn't ever sound too "clean cody" to me (maybe the object scope should be reconsidered if anything kinda falls into its responsibilities but it doesn't fit like a glove). So it becomes harder to justify opening a new Util class with just one method, that might never fit anything else in there becoming a "one trick dog" 🙄

[barryhughes]

Adding a couple of thoughts:

Should be a static method in a new CouponUtil class
...
[Should] anything else should qualify for moving into a DiscountsUtil class

Small thing, but if a new util class is the way we go here then, in relation to the proposed classname I also prefer Discounts over Coupons in this particular case.

Or should I just highlight it for a future refactor?

I think this is the way to do it. I understand and agree with the concerns around segmentation, but even superficially simple refactorings tend to increase risk and so minimizing the surface area of a change has its own value (in terms of release stability).

[wavvves]

@Konamiman @barryhughes I pushed some changes:

• Created a DiscountsUtil class w/ the static method as suggested e26ca24
• Moved tests that already existed 16d3bd1
• Took a step further, updating all calls in our codebase for the deprecated method and added a doing it wrong warning to the wrapper. I can revert this if needed 37d95a9

[barryhughes]

Looks good!

🗣️ Left one note about a possible i18n improvement.

🛒 Tested in the cart and checkout, using coupons with 1 allowed email, 2 allowed emails and no allowed emails, and different combinations of user data.

💻 Tested in the WP shell, with WC()->cart nullified, as a compliment to the provided testing instructions.

🛠️ Additional tests look great!

[barryhughes]

Thanks! Good to merge on my end, unless we want either of the other reviewers to approve first?

The requested changes were included.