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

Fix E2E gtag events for a blockified confirmation page. #2161

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

jorgemd24
Copy link
Contributor

@jorgemd24 jorgemd24 commented Nov 21, 2023

Changes proposed in this Pull Request:

Currently, there's a fatal error due to this issue: woocommerce/woocommerce-blocks#11851 and a "blockified" confirmation page. This problem is caused because the hook woocommerce_before_thankyou is being called with incorrect parameters.

Additionally, our E2E tests are failing. Check here: https://github.com/woocommerce/google-listings-and-ads/actions/runs/6942346039/job/18885109983?pr=2160. The blockified confirmation page uses a different class for the order status.

This PR introduces a temporary solution for the mentioned issue and resolves the E2E test problem.

Screenshots:

Detailed test instructions:

Fatal Error in the confirmation page. This has been fixed in WC 8.3.1 p1700588910958219-slack-C01DT6U03HC
1. Use WC 8.3
2. Use a theme that supports blocks, for example twenty twenty-two.
3. Blockify your confirmation page. See instructions here: https://woo.com/document/cart-checkout-blocks-status/ (Search for "Replacing the order confirmation page" )
4. Buy a product and complete the order, you shouldn't get any fatal error.
5. Checkout develop and repeat the steps and you will get a fatal error.

E2E Tests

  1. Check that E2E are ✅ .

Additional details:

  • I added await page.waitForTimeout( 3000 ); because it was clicking the button too quickly making the tests fail.

Changelog entry

Fix - E2E gtag evens tests.

@jorgemd24 jorgemd24 self-assigned this Nov 21, 2023
@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Nov 21, 2023
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #2161 (f3ea025) into release/2.5.12 (7264052) will decrease coverage by 0.0%.
Report is 2 commits behind head on release/2.5.12.
The diff coverage is 0.0%.

❗ Current head f3ea025 differs from pull request most recent head d28e604. Consider uploading reports for the commit d28e604 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##             release/2.5.12   #2161     +/-   ##
==================================================
- Coverage              58.3%   58.3%   -0.0%     
- Complexity             4119    4120      +1     
==================================================
  Files                   452     452             
  Lines                 17679   17681      +2     
==================================================
  Hits                  10305   10305             
- Misses                 7374    7376      +2     
Flag Coverage Δ
php-unit-tests 58.3% <0.0%> (-<0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Google/GlobalSiteTag.php 0.0% <0.0%> (ø)

@jorgemd24 jorgemd24 requested a review from a team November 21, 2023 14:15
@jorgemd24 jorgemd24 marked this pull request as ready for review November 21, 2023 14:15
@jorgemd24 jorgemd24 changed the title Fix/gtag event with incorrect hook param Fix E2E gtag events for a blockified confirmation page. Nov 21, 2023
Copy link
Member

@eason9487 eason9487 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 fixing the E2E tests. LGTM.

Left an alternative method to avoid clicking the button too quickly and it's not a blocker for this PR.

Comment on lines +103 to +104
//TODO: See if there's an alternative method to click the button without relying on waitForTimeout.
await page.waitForTimeout( 3000 );
Copy link
Member

Choose a reason for hiding this comment

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

I commented out this line and ran the related tests several times, and all attempts can pass.

  • I added await page.waitForTimeout( 3000 ); because it was clicking the button too quickly making the tests fail.

Maybe an alternative method is to wait for the button to be enabled:

Suggested change
//TODO: See if there's an alternative method to click the button without relying on waitForTimeout.
await page.waitForTimeout( 3000 );
await expect( page.locator( 'text=Place order' ) ).toBeEnabled();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Locally the E2E tests failed for me, even with the suggestion that you mentioned. For now, I will keep it as it is. If I find the root cause I will change it.

@jorgemd24 jorgemd24 merged commit 5848d0c into release/2.5.12 Nov 22, 2023
5 checks passed
@jorgemd24 jorgemd24 deleted the fix/gtag-event-with-incorrect-hook-param branch November 22, 2023 10:17
@jorgemd24 jorgemd24 mentioned this pull request Nov 22, 2023
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants