Move the guest should verify email logic to the order class

[hsingyuc]

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 move the guest should verify email logic to the order class so we can reuse email is valid check.

How to test the changes in this Pull Request:

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

1. Create a guest pay-for-order order with (ex. WC Pre-Orders)
2. Find the order and update the status to pending payment
3. Open the Customer payment page → in incognito
4. See the pay-for-order order without email verification because we are in the grace period
5. Comment out the grace period or wait till the time pass
6. Go to the pay-for-order page again
7. See the verify email address page.

Screenshot

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

Move the guest should verify email logic to the user utils

Comment

[github-actions]

Test Results Summary

Commit SHA: 1fce2d1

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 37s
E2E Tests	305	0	0	4	0	309	7m 29s

---

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.

[github-actions]

Hi @barryhughes,

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

[leonardola]

Generally the changes look good. Tests went well. But I left a comment about making the code more readable.

Also the wc-order class is already huge so I'm a little unease about moving more code into it.

We'll need to check with a team that is more familiar with WooCommerce core code to also review this PR before it gets merged.

[leonardola]

This one liner is quite hard to understand at first. Let's take the opportunity and refactor it into a more readable code. Maybe change to something like this:

// If we cannot match the order with the current user, ask that they verify their email address.
$supplied_email_matches_order_email = sanitize_email( wp_unslash( filter_input( INPUT_POST, 'email' ) ) ) === $order->get_billing_email();
$nonce_is_valid                     = wp_verify_nonce( filter_input( INPUT_POST, 'check_submission' ), 'wc_verify_email' );
$supplied_email                     = null;

if ( $supplied_email_matches_order_email &&  $nonce_is_valid ) {
	$supplied_email = sanitize_email( wp_unslash( filter_input( INPUT_POST, 'email' ) ) );
}

Also it ends up checking if the email matches with the order again in the new function. Do we really need to also check it here?

[hsingyuc]

This one liner is quite hard to understand at first. Let's take the opportunity and refactor it into a more readable code.

Good idea, thank you!

Also it ends up checking if the email matches with the order again in the new function. Do we really need to also check it here?

Thank you for catching that! Both issues are fixed here cf90234.

[leonardola]

Changes look good now. Thanks for the fixes.

[hsingyuc]

Hi @jonathansadowski, can you help us review this PR, we'll need it before Jan 31st for testing. Thank you!

[jonathansadowski]

Hi @jonathansadowski, can you help us review this PR, we'll need it before Jan 31st for testing. Thank you!

Hey @hsingyuc I cycled the tests on this by closing and reopening the PR, as after the changelog was added by our automation, the checks weren't automatically triggered (a fix for this is in the works). I noticed that there was a lint issue that should be addressed before this was run.

We'll need to check with a team that is more familiar with WooCommerce core code to also review this PR before it gets merged.

I'm going to add @woocommerce/proton as a reviewer to this PR for that. @woocommerce/vortex is primarily involved with the tooling surrounding the monorepo.

[barryhughes]

Also the wc-order class is already huge so I'm a little unease about moving more code into it.

Fwiw, I agree with this. Less because of the size of the existing WC_Order class (though, yep, it's definitely grown pretty big), but because its core job is to model orders, rather than to govern access to a frontend view.

Could this logic live inside OrderAuthorizationTrait? Even though that is in the Blocks namespace, I'm sure the shortcode class could still reference it (or, if that is not palatable, a new trait or utility class could be created).

[hsingyuc]

I noticed that there was a lint issue that should be addressed before this was run.

Thank you, @jonathansadowski ! Fixed in eea1892.

@barryhughes This is more of a permission check, it's only checking if the email is valid. I put it inside class-wc-order because we put key_is_valid there, and they seem like they should be together. I don't think we want to put it inside OrderAuthorizationTrait because that's for endpoints and might be weird for the shortcode to use. I found /Utilities/OrderUtil.php but it doesn't seem to fit well with the other methods in there. Any suggestions?

[barryhughes]

Hi @hsingyuc,

I found /Utilities/OrderUtil.php but it doesn't seem to fit well with the other methods in there.

True. Although, it's described as a spot for any order-related helpers—so it wouldn't be the worst fit (but I take your point). If that's uncomfortable, how about either of:

• Automattic\WooCommerce\Internal\Utilities\Users (and renaming the method to should_user_verify_order_email).
• Or, placing it in a new type Automattic\WooCommerce\Internal\Utilities\Checkout.

Both have the advantage we are not adding frontend validation logic to a data model, and both are in the internal namespace which can be used from both block and traditional shortcode logic, but gives us the freedom to refactor further in the future.

[hsingyuc]

Thank you, @barryhughes! Fixed in bc14f07 and moved the function to user utils.

[barryhughes]

Hi @hsingyuc, I wanted to take a bit of time to test some scenarios, because the rollout of the original change (adding protections to the order confirmation page) resulted in some friction.

One thing I'm noticing (with the 'classic'/shortcode-based flow) is that, after a guest checks out, they are immediately asked to verify their email address. This shouldn't be the case, they should see the confirmation page.

[hsingyuc]

I wanted to take a bit of time to test some scenarios, because the rollout of the original change (adding protections to the order confirmation page) resulted in some friction.

Sure. I'm just trying to make the store API meet the Core expectations and expose the function so we can use it. Let me know how I can help.

[barryhughes]

Definitely. We'd need to start by addressing the issue I just mentioned, we can't merge as-is, or we'd disrupt a large number of sites.

[hsingyuc]

One thing I'm noticing (with the 'classic'/shortcode-based flow) is that, after a guest checks out, they are immediately asked to verify their email address. This shouldn't be the case, they should see the confirmation page.

Thank you for pointing that out! Fixed in 4ee6572

[barryhughes]

Thanks! Closing and re-opening to re-start CI checks (...again 🙃).

Changes are made and tests are passing.

[hsingyuc]

@barryhughes Thank you for the review! I'm not authorized to merge the PR, could you merge it for me? Thanks!