Replace hardcoded notices with the correct wp_print_notice function #37514

mikejolley · 2023-03-30T13:49:38Z

All Submissions:

[-] Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

The contribution guidelines do not cover changelog entry creation—I had an error from a pre-commit hook when pushing.

Changes proposed in this Pull Request:

When working on woocommerce/woocommerce-blocks#8659 we realised several of the notices rendered by WooCommerce core across several frontend template files did not use the notice function helpers (wp_print_notice) or templates, and were instead hardcoded.

This PR replaces hardcoded notices with the correct functions and makes it possible to modify the notices via template overrides.

Additionally, a small modification to wp_print_notice was added so that the notice could be returned as a string instead of rendered. This was to make it more flexible. e.g.

$notice_html = wp_print_notice( 'Example', 'error', [], true );

How to test the changes in this Pull Request:

Have you followed the Writing high-quality testing instructions guide?

These instructions cover how to test notices that were changed.

As a logged in customer with no orders, go to the My Account page
- Click "orders" navigation item. Confirm the "no orders" notice is shown
- Click "downloads" navigation item. Confirm the "no downloads" notice is shown
- Navigate direct to "my-account/payment-methods". Confirm the "no methods" notice is shown
- Navigate direct to "my-account/add-payment-method". Confirm the notice is shown.
Go to the cart page (ensure there are no items in your cart). Confirm the "empty cart" notice is shown.
Add an item to your cart.
Go back to the cart page.
Remove the item from your cart. Confirm you see both an "undo" notice, and an "empty cart" notice.
Go to the shop page. Append ?s=noresults to the URL. Confirm you see the "No products were found matching your selection" notice.
Add some items to your cart then place an order via the checkout
Go back to the My Account Page
Click the "orders" navigation item
Click the button to view an order
Change the URL and replace the order ID there with something invalid e.g. 123
Confirm you see the invalid order notice

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 created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
Have you included testing instructions?

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.

github-actions · 2023-03-30T13:52:03Z

Hi @coreymckrill,

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 · 2023-03-30T14:14:53Z

Test Results Summary

Commit SHA: e8fcfbb

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 48s
E2E Tests	187	0	0	10	0	197	13m 33s

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.

opr

Tested and the notices work for me, and the code changes look mostly OK, I added some comments that might need action, but also might not so I will approve and let you decide whether to action them.

plugins/woocommerce/includes/class-wc-ajax.php

plugins/woocommerce/templates/myaccount/lost-password-confirmation.php

opr · 2023-04-04T11:24:27Z

plugins/woocommerce/includes/wc-template-functions.php

-	echo '<p class="cart-empty woocommerce-info">' . wp_kses_post( apply_filters( 'wc_empty_cart_message', __( 'Your cart is currently empty.', 'woocommerce' ) ) ) . '</p>';
+	echo '<div class="cart-empty">' .
+		wc_print_notice(
+			wp_kses_post( apply_filters( 'wc_empty_cart_message', __( 'Your cart is currently empty.', 'woocommerce' ) ) ),


Does this need esc_html__ or is it safe to skip because it's wrapped in wp_kses_post?

Escaping should come last so I think its ok to let KSES deal with this one. The filter could also return HTML.

mikejolley · 2023-04-04T11:40:34Z

Thanks @opr I fixed up those template versions which would have been blocking. Escaping is left as it was before for the cart-empty message. KSES was used previously and should continue to be in place.

opr

Thanks @mikejolley that's great now.

coreymckrill

Thanks @mikejolley, this all looks good! Testing instructions worked as expected, and it appears that no translation strings were changed 👍

Just one tweak needs to be made to some e2e tests (see below) and then this should be good to go.

coreymckrill · 2023-04-07T17:27:00Z

plugins/woocommerce/includes/wc-template-functions.php

-	echo '<p class="cart-empty woocommerce-info">' . wp_kses_post( apply_filters( 'wc_empty_cart_message', __( 'Your cart is currently empty.', 'woocommerce' ) ) ) . '</p>';
+	echo '<div class="cart-empty">';
+	wc_print_notice( wp_kses_post( apply_filters( 'wc_empty_cart_message', __( 'Your cart is currently empty.', 'woocommerce' ) ) ), 'notice' ); // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment
+	echo '</div>';


It looks like this change is causing some e2e tests to fail, because they assume the message container is p.cart-empty. Can probably just update the three tests: 1, 2, 3

codecov · 2023-04-17T14:46:32Z

Codecov Report

Merging #37514 (e8fcfbb) into trunk (c226fa1) will decrease coverage by 0.0%.
The diff coverage is 4.0%.

Additional details and impacted files

@@            Coverage Diff             @@
##             trunk   #37514     +/-   ##
==========================================
- Coverage     51.6%    51.5%   -0.0%     
- Complexity   17260    17261      +1     
==========================================
  Files          429      429             
  Lines        79928    79957     +29     
==========================================
+ Hits         41213    41215      +2     
- Misses       38715    38742     +27

Impacted Files	Coverage Δ
...ocommerce/includes/admin/class-wc-admin-assets.php	`0.0% <0.0%> (ø)`
plugins/woocommerce/includes/class-wc-ajax.php	`4.5% <0.0%> (-<0.1%)`	⬇️
...ugins/woocommerce/includes/class-wc-post-types.php	`1.1% <0.0%> (-<0.1%)`	⬇️
...commerce/includes/class-wc-rest-authentication.php	`3.4% <0.0%> (-<0.1%)`	⬇️
plugins/woocommerce/includes/class-woocommerce.php	`20.4% <ø> (ø)`
...ncludes/shortcodes/class-wc-shortcode-checkout.php	`0.0% <0.0%> (ø)`
...ludes/shortcodes/class-wc-shortcode-my-account.php	`0.0% <0.0%> (ø)`
...ludes/tracks/events/class-wc-products-tracking.php	`0.0% <ø> (ø)`
...ins/woocommerce/includes/wc-template-functions.php	`12.1% <0.0%> (-<0.1%)`	⬇️
plugins/woocommerce/woocommerce.php	`15.4% <ø> (ø)`
... and 1 more

... and 1 file with indirect coverage changes

mikejolley · 2023-04-17T15:03:13Z

@coreymckrill Those tests are updated but there are seemingly unrelated e2e fails. Can you take a look please?

coreymckrill · 2023-04-17T21:16:01Z

🤔 @mikejolley I think the e2e's are still failing because of the three related empty cart tests (example). Given the following assertion:

await expect( page.locator( '.cart-empty' ) ).toContainText(
	'Your cart is currently empty.'
);

I suspect the reason this is failing may be because Playwright expects that the cart-empty container itself contains the given text, rather than containing another child container that contains that text. @lanej0 can hopefully confirm.

mikejolley · 2023-04-18T12:28:43Z

@coreymckrill I don't think its that because should display no item in the cart test is passing, and that uses the same selector and the same notice. So something else is preventing the message rendering I guess.

mikejolley · 2023-04-18T12:41:44Z

update_wc_div in cart.js moves it out of the div into another wrapper. Ill find a workaround.

coreymckrill

e2e tests passing now 👍

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 30, 2023

mikejolley self-assigned this Mar 30, 2023

mikejolley mentioned this pull request Mar 30, 2023

Feature Branch: Updated Shopper Notices woocommerce/woocommerce-blocks#8659

Merged

19 tasks

mikejolley requested review from a team and opr and removed request for a team March 30, 2023 13:51

github-actions bot added the focus: e2e tests Issues related to e2e tests label Mar 31, 2023

opr previously approved these changes Apr 4, 2023

View reviewed changes

mikejolley dismissed opr’s stale review via e1a510f April 4, 2023 11:39

opr previously approved these changes Apr 4, 2023

View reviewed changes

mikejolley dismissed opr’s stale review via e78e04f April 5, 2023 12:47

mikejolley force-pushed the fix/remove-hardcoded-notices branch from e1a510f to e78e04f Compare April 5, 2023 12:47

mikejolley requested review from a team and coreymckrill and removed request for a team April 5, 2023 15:17

coreymckrill suggested changes Apr 7, 2023

View reviewed changes

mikejolley requested a review from coreymckrill April 17, 2023 15:02

mikejolley added 7 commits April 18, 2023 12:22

Allow notice to return instead of echo

d0ea42a

Replace hardcoded info notices with wp_print_notice

ec6ed5c

Replace hardcoded error notices with wp_print_notice

abfe701

Changelog

64995db

Update selector for test notice

65038cc

template version bumps

bb0d83d

Revert version bump in lost password template

078f54d

mikejolley added 9 commits April 18, 2023 12:22

Revert template bump in form-coupon

82a4698

Revert template bump in form-login

3c8bef0

Escape return to shop

f330bc7

Remove array short syntax

123dff3

Add full stop to docblock param

5ba6a3d

phpcs fixes

93665bd

Fix indents for phpcs

2cbbbbb

Update cart empty selector

73fd1a5

Target inner div within notices in tests

b800a42

mikejolley force-pushed the fix/remove-hardcoded-notices branch from 06aad2c to b800a42 Compare April 18, 2023 11:59

mikejolley added 3 commits April 18, 2023 14:11

Append cart-empty class to notice div

5a7be6f

phpcs rules

22c5dbb

Bump templates to 7.8.0

e8fcfbb

coreymckrill approved these changes Apr 18, 2023

View reviewed changes

coreymckrill merged commit 1650489 into trunk Apr 18, 2023

coreymckrill deleted the fix/remove-hardcoded-notices branch April 18, 2023 19:07

github-actions bot added this to the 7.8.0 milestone Apr 18, 2023

Replace hardcoded notices with the correct wp_print_notice function #37514

Replace hardcoded notices with the correct wp_print_notice function #37514

Uh oh!

Conversation

mikejolley commented Mar 30, 2023 • edited by opr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

Uh oh!

github-actions bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

opr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

opr Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

mikejolley Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

mikejolley commented Apr 4, 2023

Uh oh!

opr left a comment

Choose a reason for hiding this comment

Uh oh!

coreymckrill left a comment

Choose a reason for hiding this comment

Uh oh!

coreymckrill Apr 7, 2023

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikejolley commented Apr 17, 2023

Uh oh!

coreymckrill commented Apr 17, 2023

Uh oh!

mikejolley commented Apr 18, 2023

Uh oh!

mikejolley commented Apr 18, 2023

Uh oh!

coreymckrill left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mikejolley commented Mar 30, 2023 •

edited by opr

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023 •

edited

Loading

codecov bot commented Apr 17, 2023 •

edited

Loading