Skip to content
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

Introduce grace period before asking guests to verify their email address #39191

Merged
merged 11 commits into from Jul 19, 2023

Conversation

barryhughes
Copy link
Member

@barryhughes barryhughes commented Jul 11, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

In our latest release, we introduced a change that requires shoppers to verify their email address before seeing the order confirmation page, or order payment page, but only if WooCommerce is unable to recognize them.

Naturally, the desire was to make this as friction-free as possible and, for example, shoppers should not have to verify their email address immediately after checking out ... unfortunately, depending on the payment gateway in use, that became possible.

Therefore, this change introduces a grace period during which email verification will not be required. Further, the grace period is filterable so can be removed (or extended) as desired.

💡 To clarify, this softens the requirement for email verification for guest shoppers. By itself, it does not touch the requirement for authenticated shoppers to be logged in before they can see their order confirmation.

How to test the changes in this Pull Request:

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

  1. As a guest shopper, make a purchase.
  2. After checking out, you should see the Order Received page.
  3. Copy the URL and open this page in a new browser context (ie, an incognito mode tab or else a container tab, if you are using Firefox):
    • If we are within 10 minutes of the order being placed, then the Order Received page should be visible in both tabs (ie, the original tab where the shopper is 'known' and the separate tab where the shopper is unknown).
    • After 10 minutes, refreshing the second tab should result in you being asked to verify your email address (but in the original tab, the Order Received page should still be accessible).
  4. There is no change for authenticated shoppers. In this case, if you repeat the above exercise, there is no grace period and if we cannot identify the shopper (as in the second browser context) they will be required to login.

Notes

  1. You don't have to wait 10 minutes when testing, as you can temporarily modify this line of code and replace 10 * MINUTE_IN_SECONDS with 0 to simulate having waited 10 minutes. Or, if you don't like to touch core code, you could setup the following snippet in a suitable place (such as wp-content/mu-plugins/test-39191.php) then remove after testing:
<?php
add_filter( 'woocommerce_order_email_verification_grace_period', '__return_zero' );
  1. I am aware of this related discussion. We still need to decide on the best course of action there (possibly following up with a second PR).

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

Message

Adds a grace period during which email verification will not be needed before the order confirmation (or payment) page is rendered.

Comment

…rder pay/conf page).

When the order confirmation (or payment) page is requested, we often want to ensure the visitor is associated with the order. However, this relies heavily on information stored in the user session and, depending on the payment gateway in use, this may not be dependable. Therefore, we've introduced a grace period during which no such verification will take place.
@github-actions github-actions bot added focus: e2e tests Issues related to e2e tests plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Jul 11, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2023

Test Results Summary

Commit SHA: 2343236

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 51s
E2E Tests1890019020813m 27s

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.

@barryhughes barryhughes requested review from a team and vedanshujain and removed request for a team July 12, 2023 13:46
@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2023

Hi @rodelgc,

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

@barryhughes
Copy link
Member Author

barryhughes commented Jul 12, 2023

  • Requesting review, but let's not merge until we have agreement from all parties (for instance, we may want to request a further review from a payments-focused team).
  • Also interested in non-technical perspectives on this (from a high-level, are there any concerns with this strategy?).
  • Requested a Solaris review, partly in relation to my last point but also because of the E2E changes (maybe there is an existing and more elegant approach that I missed, regarding temporarily setting filters from E2E tests...or maybe that is simply a malpractice).

@barryhughes barryhughes marked this pull request as ready for review July 12, 2023 16:01
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

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

Left a minor optional comment, I tested dby eleting the cookie to simulate the condition where the session won't have the email and worked as expected. Went again after 10 minutes, and as expected, I was required to verify the email.

Screenshot 2023-07-13 at 10 21 18 AM

@barryhughes barryhughes changed the title Fix/order confirmation access Introduce grace period before asking guests to verify their email address Jul 14, 2023
Copy link
Contributor

@rodelgc rodelgc left a comment

Choose a reason for hiding this comment

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

Done with my review and left some comments. Thanks so much for making the necessary updates on the E2E tests @barryhughes! They tested well on localhost. I requested some changes so that E2E changes you made would also work on permanent test sites.

Summary of tests I did for this PR:

  • ✔️ Ran the allows guest customer to place an order test just by itself
  • ✔️ Ran allows guest customer to place an order twice in a row to ensure it's repeatable. I used the arguments --retries=0 --repeat-each=2 --workers=1
  • ✔️ Ran all tests in shopper/checkout.spec.js
  • ⚠️ allows guest customer to place an order test failed when I ran all tests in shopper/checkout.spec.js against an external WooCommerce test site.

plugins/woocommerce/tests/e2e-pw/utils/filters.js Outdated Show resolved Hide resolved
plugins/woocommerce/tests/e2e-pw/utils/filters.js Outdated Show resolved Hide resolved
@coreymckrill coreymckrill self-assigned this Jul 17, 2023
@coreymckrill
Copy link
Collaborator

@rodelgc I'm taking over this PR since Barry is AFK. I've addressed your feedback, let me know what you think.

@nigeljamesstevenson
Copy link
Contributor

Thanks a lot for making these updates @coreymckrill !

⚠️ allows guest customer to place an order test still failed when I ran all tests in shopper/checkout.spec.js against a JN.

Interestingly, it was failing on the line 349 assertion

		await expect( page.locator( 'h1.entry-title' ) ).toContainText(
		 	'Order received'
		);

but looked good when I updated it to


		await expect(
			page.getByRole( 'heading', { name: 'Order received' } )
		).toBeVisible();

(which you updated on a previous similar update in this PR) - so it certainly seem those locators are more stable..
I think it would be good to get this one updated also..

@rodelgc - any idea if this was the location at which your test was failing originally?

@coreymckrill
Copy link
Collaborator

🤔 @nigeljamesstevenson Line 349 is in a separate test, allows existing customer to place order, which isn't modified in this PR. Can you confirm which test is failing for you?

@nigeljamesstevenson
Copy link
Contributor

nigeljamesstevenson commented Jul 19, 2023

Line 349 is in a separate test, allows existing customer to place order, which isn't modified in this PR. Can you confirm which test is failing for you?

Doh! 🤦‍♂️ My bad @coreymckrill ! - It was indeed the allows existing customer to place order that was failing for me. I will create a task for myself and take a peek at this. allows guest customer to place an order looks great!

@coreymckrill
Copy link
Collaborator

Since Nigel re-ran the tests and they are working, and we already have one approval here, I'm going to go ahead and merge.

@coreymckrill
Copy link
Collaborator

Actually, looks like I can't until @rodelgc confirms that requested changes were made.

Copy link
Contributor

@lanej0 lanej0 left a comment

Choose a reason for hiding this comment

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

Going to try another approval to get this out of "approval gridlock"

Narrator: the attempt didn't work.

@lanej0 lanej0 dismissed rodelgc’s stale review July 19, 2023 18:56

Confirmed that tests pass locally and on a remote site and that changes have been made.

@lanej0
Copy link
Contributor

lanej0 commented Jul 19, 2023

I did re-run the tests locally and against a Jurassic Ninja site and everything is good. So I've unblocked this.

@lanej0 lanej0 merged commit e97eda1 into trunk Jul 19, 2023
19 checks passed
@lanej0 lanej0 deleted the fix/order-confirmation-access branch July 19, 2023 18:57
@github-actions github-actions bot added this to the 8.1.0 milestone Jul 19, 2023
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jul 19, 2023
@tammullen tammullen added needs: analysis Indicates if the PR requires a PR testing scrub session. and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Jul 19, 2023
@coreymckrill
Copy link
Collaborator

Thank you!

@coreymckrill coreymckrill mentioned this pull request Jul 19, 2023
@nigeljamesstevenson nigeljamesstevenson added the needs: internal testing Indicates if the PR requires further testing conducted by Solaris label Jul 19, 2023
@jonathansadowski jonathansadowski modified the milestones: 8.1.0, 8.0.0 Jul 20, 2023
github-actions bot pushed a commit that referenced this pull request Jul 20, 2023
…ress (#39191)

* Add a grace period during which email verification is not required (order pay/conf page).

When the order confirmation (or payment) page is requested, we often want to ensure the visitor is associated with the order. However, this relies heavily on information stored in the user session and, depending on the payment gateway in use, this may not be dependable. Therefore, we've introduced a grace period during which no such verification will take place.

* Provide a mechanism for establishing server-side filters from our E2E tests.

* Make our utilities for setting up filters from E2E available in the test env.

* Update guest shopper workflow to account for order conf/payment access grace period.

* Tidy verbiage.

* Add changefile(s) from automation for the following project(s): woocommerce

* Only activate the Filter Setter (e2e utility) during e2e tests.

* Coding standard fixes for E2E utility plugin.

* e2e: Update locators for headings to use getByRole, add await to clearFilters

* e2e: Abstract the cookie domain to work on non-localhost test sites

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Corey McKrill <916023+coreymckrill@users.noreply.github.com>
jonathansadowski pushed a commit that referenced this pull request Jul 20, 2023
* Introduce grace period before asking guests to verify their email address (#39191)

* Add a grace period during which email verification is not required (order pay/conf page).

When the order confirmation (or payment) page is requested, we often want to ensure the visitor is associated with the order. However, this relies heavily on information stored in the user session and, depending on the payment gateway in use, this may not be dependable. Therefore, we've introduced a grace period during which no such verification will take place.

* Provide a mechanism for establishing server-side filters from our E2E tests.

* Make our utilities for setting up filters from E2E available in the test env.

* Update guest shopper workflow to account for order conf/payment access grace period.

* Tidy verbiage.

* Add changefile(s) from automation for the following project(s): woocommerce

* Only activate the Filter Setter (e2e utility) during e2e tests.

* Coding standard fixes for E2E utility plugin.

* e2e: Update locators for headings to use getByRole, add await to clearFilters

* e2e: Abstract the cookie domain to work on non-localhost test sites

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Corey McKrill <916023+coreymckrill@users.noreply.github.com>

* Prep for cherry pick 39191

---------

Co-authored-by: Barry Hughes <3594411+barryhughes@users.noreply.github.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Corey McKrill <916023+coreymckrill@users.noreply.github.com>
Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
coreymckrill added a commit that referenced this pull request Jul 25, 2023
On the heels of #38983 and #39191, this changes the flow a bit so that
the "thank you" message will always be shown when loading the "order
received" URL. However, the verification requirement still prevents
the details of the order being shown unless the customer is verified,
either by confirming the email or by logging in. If unverified, the
login/email form is shown on the thank you screen instead of the order
details.
coreymckrill added a commit that referenced this pull request Aug 1, 2023
On the heels of #38983 and #39191, this changes the flow a bit so that
the "thank you" message will always be shown when loading the "order
received" URL. However, the verification requirement still prevents
the details of the order being shown unless the customer is verified,
either by confirming the email or by logging in. If unverified, the
login/email form is shown on the thank you screen instead of the order
details.
tommyshellberg pushed a commit that referenced this pull request Aug 7, 2023
…ress (#39191)

* Add a grace period during which email verification is not required (order pay/conf page).

When the order confirmation (or payment) page is requested, we often want to ensure the visitor is associated with the order. However, this relies heavily on information stored in the user session and, depending on the payment gateway in use, this may not be dependable. Therefore, we've introduced a grace period during which no such verification will take place.

* Provide a mechanism for establishing server-side filters from our E2E tests.

* Make our utilities for setting up filters from E2E available in the test env.

* Update guest shopper workflow to account for order conf/payment access grace period.

* Tidy verbiage.

* Add changefile(s) from automation for the following project(s): woocommerce

* Only activate the Filter Setter (e2e utility) during e2e tests.

* Coding standard fixes for E2E utility plugin.

* e2e: Update locators for headings to use getByRole, add await to clearFilters

* e2e: Abstract the cookie domain to work on non-localhost test sites

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Corey McKrill <916023+coreymckrill@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: e2e tests Issues related to e2e tests needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants