Add close button for store alerts #37710

nathanss · 2023-04-13T14:18:03Z

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:

We were having issues with Undismissable notices.

Currently we have multiple messages that manually add a "Dismiss" button, which need to have a url to navigate to. This is not ideal because it can create a race condition, and also, navigating outside the home page when dismissing an alert doesn't make sense.

I'm adding an X button for all alerts which should always dismiss it.

Closes #36913 .

How to test the changes in this Pull Request:

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

Run the SQL commands DELETE FROM wp_customer_wc_admin_notes; and DELETE FROM wp_customer_wc_admin_note_actions;
Using WCA test helper, search for options including 'remote_inbox' and delete them.
Install the plugin woocommerce-gateway-eway version 3.3.0.
Go to WooCommerce > Home.
You should see the 'Security vulnerability patched in WooCommerce Eway Gateway' banner message
Click the X button
Check if the message disappears.

github-actions · 2023-04-13T14:45:26Z

Test Results Summary

Commit SHA: 0896303

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	267	0	0	2	0	269	1m 11s
E2E Tests	187	0	0	10	0	197	21m 52s

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.

codecov · 2023-04-17T17:12:04Z

Codecov Report

Merging #37710 (c91f9dc) into trunk (7a63646) will increase coverage by 0.1%.
The diff coverage is n/a.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             trunk   #37710     +/-   ##
==========================================
+ Coverage     51.5%    51.6%   +0.1%     
+ Complexity   17281    17261     -20     
==========================================
  Files          430      429      -1     
  Lines        80030    79942     -88     
==========================================
- Hits         41215    41212      -3     
+ Misses       38815    38730     -85

see 9 files with indirect coverage changes

github-actions · 2023-04-20T13:11:51Z

Hi @mattsherman,

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

mattsherman · 2023-04-20T18:08:59Z

Run the SQL commands DELETE FROM wp_customer_wc_admin_notes; and DELETE FROM wp_customer_wc_admin_note_actions;

Using WCA test helper, search for options including 'remote_inbox' and delete them.

Install the plugin woocommerce-gateway-eway version 3.3.0.

Go to WooCommerce > Home.

You should see the 'Security vulnerability patched in WooCommerce Eway Gateway' banner message

Click the X button

Check if the message disappears.

@nathanss Some suggestions on how to improve testing instructions in the future:

Include a link to specific plugin versions referenced
Include screenshots of before/after to make it easy for tester to verify that things are as they are supposed to be

mattsherman · 2023-04-20T18:16:29Z

Run the SQL commands DELETE FROM wp_customer_wc_admin_notes; and DELETE FROM wp_customer_wc_admin_note_actions;

You can also just use the "Delete all admin notes" action in the WCA Test Helper > Admin notes screen.

mattsherman

It would be good to add a test for the new dismiss button. To make sure it shows the previous alert, and dismisses the current alert.

Should we get rid of the Dismiss button for notes that have them? I iamgine that involves modiying the notes data source on WooCommerce.com.

mattsherman · 2023-04-20T18:35:36Z

plugins/woocommerce-admin/client/layout/store-alerts/test/index.js

+		expect( getByText( 'Click me!' ) ).toBeVisible();
+		expect( getByText( 'Click me!' ).getAttribute( 'href' ) ).toBe( '#' );


mattsherman · 2023-04-20T18:39:59Z

plugins/woocommerce-admin/client/layout/store-alerts/index.js

+							await removeNote( alert.id );
+							this.previousAlert();
+							invalidateResolutionForStoreSelector( 'getNotes' );


It would be nice to move this to a function outside of the JSX, to keep the code a bit more readable.

mattsherman · 2023-04-20T18:45:50Z

plugins/woocommerce-admin/client/layout/store-alerts/index.js

+							invalidateResolutionForStoreSelector( 'getNotes' );
+						} }
+					>
+						<Icon icon={ close } />


It would be good to have a tooltip or some other accessible way to know what this button is.

How about Dismiss message?

nathanss · 2023-04-24T17:37:39Z

@mattsherman

You can also just use the "Delete all admin notes" action in the WCA Test Helper > Admin notes screen.

That's awesome. Thanks for sharing!

Should we get rid of the Dismiss button for notes that have them? I iamgine that involves modiying the notes data source on WooCommerce.com.

Yes, it involves modifying it on woocommerce.com. That would be pretty hard. I prefer not to do this for now.

mattsherman · 2023-04-25T19:53:07Z

Should we get rid of the Dismiss button for notes that have them? I iamgine that involves modiying the notes data source on WooCommerce.com.

Yes, it involves modifying it on woocommerce.com. That would be pretty hard. I prefer not to do this for now.

I'm concerned that the UX is kinda confusing with both the "Dismiss" buttons and the "X" close button. It shouldn't be hard to remove the "Dismiss" button for notes. We should create a follow-up issue to do so.

@jarekmorawski Do you have any thoughts on having both the "Dismiss" and the "X" button on notes in the mean time?

mattsherman

Overall, code looks good (I did not re-test yet).

Left a question about a div added.

I'll re-test later this evening.

mattsherman · 2023-04-25T19:46:22Z

plugins/woocommerce-admin/client/layout/store-alerts/index.js

-								{ interpolateComponents( {
-									mixedString: __(
-										'{{current /}} of {{total /}}',
+					<div>


Why was this extra div needed?

This is needed to group the pagination buttons with the close button, since they're inside a div with display: flex

mattsherman

This is overall looking quite good. Nice job!

I think we should have a Tracks event when the "X" is clicked and the note is removed (see comment in code).

mattsherman · 2023-04-26T00:14:47Z

plugins/woocommerce-admin/client/layout/store-alerts/index.js

+		const { removeNote, invalidateResolutionForStoreSelector } = this.props;
+
+		const removeNoteAndInvalidate = async () => {
+			await removeNote( alert.id );


We should record a Tracks event when the "X" is clicked and the note is removed.

Something similar to wcadmin_store_alert_action, with similar params if possible.

woocommerce/plugins/woocommerce/src/Admin/Notes/Notes.php

Line 387 in 8cfff8a

wc_admin_record_tracks_event( 'store_alert_action', $event_params );

nathanss · 2023-04-26T13:35:43Z

I'm concerned that the UX is kinda confusing with both the "Dismiss" buttons and the "X" close button. It shouldn't be hard to remove the "Dismiss" button for notes. We should create a follow-up issue to do so.

@jarekmorawski Do you have any thoughts on having both the "Dismiss" and the "X" button on notes in the mean time?

@mattsherman I'm just not sure how we're going to maintain support for previous versions, since the admin notes don't seem to have versions, so if we just removed the Dismiss actions from woocommerce.com, all alerts would become undismissable on installations without this code. There might be a way, though. I just couldn't think of it.

jarekmorawski · 2023-04-26T13:45:21Z

@jarekmorawski Do you have any thoughts on having both the "Dismiss" and the "X" button on notes in the mean time?

There's no need to have both since this could confuse people regarding each button's function. Before we go any further, can you explain why we need the Dismiss button? It isn't clear to me what the problem is and why the Dismiss button needs a URL (even more so, why it's linked to an external page). Shouldn't the button, you know, dismiss the notice?

nathanss · 2023-04-26T13:50:51Z

@jarekmorawski I agree with you. The problem is that all the buttons that come from remote inbox are generic, there's no "native" dismiss button. Here's an example of what is received from the server:

{
"name": "woocommerce-core-paypal-march-2022-dismiss",
"locales": [
{
"locale": "en_US",
"label": "Dismiss"
}
],
"url": "",
"url_is_admin_query": false,
"is_primary": false,
"status": "actioned"
}

There's no good way of knowing this is a Dismiss button from my point of view. The best I could do is test if the button label is equal "Dismiss" (accounting for translation as well),which seems to be a pattern currently, and hide it in that case. @mattsherman what do you think?

mattsherman · 2023-04-27T19:37:18Z

plugins/woocommerce-admin/client/layout/store-alerts/index.js

 			this.previousAlert();
 			invalidateResolutionForStoreSelector( 'getNotes' );
+			recordEvent( 'store_alert_close', {
+				alert_id: alert.id,


Is this alert.id unique to the store/db? If so, there is probably no benefit in including it.

mattsherman · 2023-04-27T19:39:34Z

plugins/woocommerce-admin/client/layout/store-alerts/index.js

+				alert_name: alert.name,
+				alert_title: alert.title,
+				alert_content: alert.content,


For consistency with the store_alert_action, I would name these:

note_name

note_title

note_content

Do you have access to note_type? I see that in the store_alert_action event, as well as screen... those would be useful too.

mattsherman · 2023-04-27T21:33:21Z

@jarekmorawski I agree with you. The problem is that all the buttons that come from remote inbox are generic, there's no "native" dismiss button. Here's an example of what is received from the server:
{
"name": "woocommerce-core-paypal-march-2022-dismiss",
"locales": [
{
"locale": "en_US",
"label": "Dismiss"
}
],
"url": "",
"url_is_admin_query": false,
"is_primary": false,
"status": "actioned"
}
There's no good way of knowing this is a Dismiss button from my point of view. The best I could do is test if the button label is equal "Dismiss" (accounting for translation as well),which seems to be a pattern currently, and hide it in that case. @mattsherman what do you think?

Note: I'm leaving this rambling at the beginning of this comment just to show my thought process. Skip to POSSIBLE SOLUTION below for what I think actually is what we should do!

Okay, so, could we safely conclude that if a note's action meets the following criteria it is just a "stock" Dismiss action that we could hook up our own handling to?

label === "Dismiss" for locale === "en_US"
url === "" (they aren't expecting us to redirect the user anywhere specific)
- Update: No, that won't work... some actions, such as needs-update-eway-payment-gteway-rin-dismiss-button-2022-12-20 have url set to #.
url_is_admin_query === false
status === "actioned"

If an action met all those criteria, what if we replaced the default action handling hooked up to it with the new code you wrote for the "X" button? And, got rid of the "X" button (unless Jarek likes that UX better).

Seems pretty safe and robust?

The one unknown I wonder about is whether there is any code on the backend that is tracking the "actioning" of the dismisses... I see that name set to woocommerce-core-paypal-march-2022-dismiss and it makes me feel that there is some analysis being done somewhere that is using those names.

Also, there are filters that are called when actions are triggered:

woocommerce_note_action

woocommerce/plugins/woocommerce/src/Admin/Notes/Notes.php

Line 359 in 8cfff8a

do_action( 'woocommerce_note_action', $triggered_action->name, $note );

woocommerce_note_action_woocommerce-core-paypal-march-2022-dismiss

woocommerce/plugins/woocommerce/src/Admin/Notes/Notes.php

Line 367 in 8cfff8a

do_action( 'woocommerce_note_action_' . $triggered_action->name, $note );

Thinking about this more right now though... why does your implementation of calling removeNote() work (always) when the regular action handling of calling triggerNoteAction() doesn't (always)?

I'm beginning to question whether this is the correct approach to fixing the issue.

POSSIBLE SOLUTION

We believe the heart of the problem is that some actions have a url and that is sometimes causing a race condition with the onClick handling that calls triggerNoteAction(). So, let's fix that implementation! (And get rid of the "X" that we considered adding.)

woocommerce/plugins/woocommerce-admin/client/layout/store-alerts/index.js

Lines 76 to 77 in 64dbeba

    
           href={ action.url || undefined } 
        
           onClick={ () => triggerNoteAction( alert.id, action.id ) }

We don't have any guarantee that the triggerNoteAction() will complete before the navigation occurs.

Here is a quick test I did changing the implementation...

onClick={ async ( event ) => {
	const url = event.currentTarget.getAttribute( 'href' );
	event.preventDefault();

	console.log( 'triggering action...' );

	await triggerNoteAction( alert.id, action.id );

	console.log( '...action triggered' );

	if ( url ) {
		console.log( `navigating to url (${ url })...` );

		window.location.href = url;
	}
} }

This should guarantee that the note action is triggered before we navigate.

We could even go further and "ignore" url if it was set to things like "#" to avoid "incorrect" redirects (we'd want to audit that list of "incorrect" redirects to make sure we weren't missing valid reasons for the redirect).

leonardola · 2023-04-28T14:45:35Z

Just o let you know that in the inbox list we actually have a dismiss button and not an x button. It would probably be better to keep the same style for the banner. I was testing RINs yesterday and took this screenshot. There are two dismiss buttons because I've added one manually while using the info type for the RIN which already adds a dismiss button automatically

nathanss · 2023-04-28T18:30:19Z

Closed in favor of #38047

github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Apr 13, 2023

nathanss force-pushed the fix/undismissable-notice branch from 69dc45c to 50ab26b Compare April 13, 2023 16:37

nathanss marked this pull request as draft April 14, 2023 13:59

nathanss changed the title ~~Add fixed dismiss button for store alerts~~ Add close button for store alerts Apr 17, 2023

nathanss marked this pull request as ready for review April 17, 2023 16:01

mattsherman self-requested a review April 20, 2023 13:11

mattsherman suggested changes Apr 20, 2023

View reviewed changes

nathanss requested a review from mattsherman April 24, 2023 18:36

mattsherman reviewed Apr 25, 2023

View reviewed changes

mattsherman suggested changes Apr 26, 2023

View reviewed changes

mattsherman suggested changes Apr 27, 2023

View reviewed changes

nathanss added 8 commits April 27, 2023 17:03

Add fixed dismiss button for store alerts

5e7728d

Fix a bug when deleting the last index of the array of alerts

a43f82d

Replace dismiss button with X button

603cb58

Revert unnecessary changes

eb1c67a

Fix fragile unit test

b3eefed

Extract function to outside of jsx

c977742

Add test for StoreAlert

a11d581

Add tracks event for store_alert_close

3daf9ec

Improve tracks event properties

0896303

nathanss force-pushed the fix/undismissable-notice branch from 2eaa5dc to 0896303 Compare April 27, 2023 20:10

nathanss requested a review from mattsherman April 27, 2023 20:12

nathanss mentioned this pull request Apr 28, 2023

Await triggerNoteAction before navigating when action is clicked #38047

Merged

nathanss closed this Apr 28, 2023

kalessil deleted the fix/undismissable-notice branch February 14, 2025 07:48

		expect( getByText( 'Click me!' ) ).toBeVisible();
		expect( getByText( 'Click me!' ).getAttribute( 'href' ) ).toBe( '#' );

Add close button for store alerts #37710

Add close button for store alerts #37710

Uh oh!

Conversation

nathanss commented Apr 13, 2023 • edited by mattsherman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Uh oh!

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

Uh oh!

Test Results Summary

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!

github-actions bot commented Apr 20, 2023

Uh oh!

mattsherman commented Apr 20, 2023

Uh oh!

mattsherman commented Apr 20, 2023

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

mattsherman Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

mattsherman Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

mattsherman Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

jarekmorawski Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

mattsherman commented Apr 25, 2023

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

mattsherman Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

nathanss Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

mattsherman Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

nathanss commented Apr 26, 2023

Uh oh!

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

Uh oh!

Uh oh!

nathanss commented Apr 26, 2023

Uh oh!

mattsherman Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

mattsherman Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsherman commented Apr 27, 2023

Uh oh!

leonardola commented Apr 28, 2023

Uh oh!

nathanss commented Apr 28, 2023

Uh oh!

nathanss commented Apr 13, 2023 •

edited by mattsherman

Loading

github-actions bot commented Apr 13, 2023 •

edited

Loading

codecov bot commented Apr 17, 2023 •

edited

Loading

nathanss commented Apr 24, 2023 •

edited

Loading

nathanss Apr 26, 2023 •

edited

Loading

jarekmorawski commented Apr 26, 2023 •

edited

Loading

mattsherman Apr 27, 2023 •

edited

Loading