Enhancement/update success error messages to wp admin notices #85

MaxwellGarceau · 2024-12-19T21:29:01Z

Description of the Change

On the plugin settings page, replace the custom Mailchimp UI for admin notices to the standard WP admin notices API

Drawbacks

Two systems for displaying messages. WP admin notices and the Mailchimp custom messages for the front end.

Possible regressions

While I've tested for all of these scenarios and fixed any issues I've found the following items are the most likely regressions.

It's possible that some messages that should display as WP admin notices were missed and will still display as custom Mailchimp messages
- It's possible that those messages that were not updated will not display at all due to removing custom message output on the plugin settings page
It's possible that specialized message text will be stripped out by sanitization functions that have been introduced or tweaked

Closes #62

How to test the Change

WP Admin messages

A video recording can be made on request for any of the testing items below.

I've documented all of the possible testing scenarios that I found while making the changes, but due to the extensive list of areas to test I recommend making a note of any items that are too difficult or time consuming to efficiently test. We can then look at the items that were not tested and decide on a plan.

For example, testing the error message for an invalid Mailchimp API token would require finding a way to fail the OAuth flow while still navigating to the plugin settings page. It might be better to talk about items like that and brainstorm easier ways of ensuring regression proof code, such as relying on the OAuth test in /tests/cypress/e2e/connect.test.js or extending it.

Success message

Settings page URL: /wp-admin/admin.php?page=mailchimp_sf_options

Save any option and the success message will display
- (optional) If we really want to go above and beyond we could save every single option on the page to trigger all of the locations where the success message will display
- Pro tip: Multiple admin notices will display at the same time. You can turn every option on and then every option off again. Caution should be taken for options that will conditionally hide or display other options.

Error message

Set the users list to "-- Select a List --" and click "Update List"
If a users lists can not be retrieved from Mailchimp then an error will display
If a user does not have any lists in their Mailchimp account defined then an error will display

FE messages

FE messages should remain unchanged

Success Messages

Submit the form after filling out all required fields to trigger the success message

Users that already exist as a contact will be subscribed without email verification required (I think)
Users not already saved as a contact will receive a success message to verify their email

Error Messages

Submit the form without entering any details to trigger the error message
The following scenarios will also throw errors on the FE

Required fields not filled out or invalid
If Mailchimp API client won't work (invalid token, etc) then an error will be thrown
If update existing is turned off in the plugin settings and the subscriber email already exists then an error will be thrown
Error with API request adding subscriber to list (could trigger with chrome dev tools -> mobile device view -> go offline)

Changelog Entry

Changed - Plugin settings page success and error messages will now use WP admin notices
Changed - Renamed mailchimp_sf_global_msg function to mailchimp_sf_frontend_msg to differentiate functionality from WP admin notices that are used exclusively in the WP Admin

Credits

Props @MaxwellGarceau

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly. - Asked in pull request if this step is worth it
I have added tests to cover my change.
All new and existing tests pass.

Lint project and fix errors

Only admin notices should be displayed here

This better explains the functions purpose and will prevent confusion in the future. Updated mailchimp_sf_frontend_msg comments to explain how WP admin notices are used in the WP admin

MaxwellGarceau

I've highlighted some areas that I think would benefit from additional scrutiny. If anyone would like to request changes I would be happy to make them =).

includes/admin/admin-notices.php

includes/class-mailchimp-admin.php

MaxwellGarceau · 2024-12-19T21:35:14Z

mailchimp.php

@@ -253,7 +253,7 @@ function mailchimp_sf_request_handler() {
 						if ( ! headers_sent() ) { // just in case...
 							header( 'Last-Modified: ' . gmdate( 'D, d M Y H:i:s' ) . ' GMT', true, 200 );
 						}
-						echo wp_kses_post( mailchimp_sf_global_msg() );
+						echo wp_kses_post( mailchimp_sf_frontend_msg() );


Renamed mailchimp_sf_global_msg to mailchimp_sf_frontend_msg to reflect new role of this msg function.

Flagging for extra scrutiny.

I'm fine with the rename though I guess not sure if it's accurate? Seems that function still outputs admin messages, while I'd expect a function called frontend message to only output messages on the front-end, not admin?

Hey @dkotter, thanks for calling this out. Are you referring to the mailchimp_sf_frontend_msg call in mailchimp_sf_request_handler at line 256? I believe this fetches the error messages accumulated during the BE validation handling.

There's some validation on the accounts creation page in the admin, but that looks like it's being handled via JS validation in assets/js/admin.js.

I double checked the codebase for other areas where mailchimp_sf_request_handler might be used in the admin, but I couldn't find any. Please correct me if I'm wrong. I'd be happy to revert the same or update the codebase.

mailchimp.php

MaxwellGarceau · 2024-12-19T21:51:32Z

@dkotter @jeffpaul currently admin notice messages will display in the admin and the plugins own message system for displaying messages on the front end.

Do you think it's worth it to add a section in the readme regarding this pattern for documentation or do you think it's overkill?

includes/class-mailchimp-admin.php

mailchimp.php

dkotter · 2024-12-20T15:41:43Z

mailchimp.php

@@ -253,7 +253,7 @@ function mailchimp_sf_request_handler() {
 						if ( ! headers_sent() ) { // just in case...
 							header( 'Last-Modified: ' . gmdate( 'D, d M Y H:i:s' ) . ' GMT', true, 200 );
 						}
-						echo wp_kses_post( mailchimp_sf_global_msg() );
+						echo wp_kses_post( mailchimp_sf_frontend_msg() );


I'm fine with the rename though I guess not sure if it's accurate? Seems that function still outputs admin messages, while I'd expect a function called frontend message to only output messages on the front-end, not admin?

jeffpaul · 2024-12-23T20:56:51Z

Do you think it's worth it to add a section in the readme regarding this pattern for documentation or do you think it's overkill?

I don't think this needs to be called out in the readme.

MaxwellGarceau · 2024-12-26T21:00:36Z

I removed the composer autoload and changes to the build process to keep this PR modular.

Composer autoload discussion continued on issue #96 and PR #95.

MaxwellGarceau added 6 commits December 19, 2024 15:06

Add WP admin notices to plugin settings page

b97dc72

Move admin-notices to separate file

cfcf2d5

Lint project and fix errors

Add wp_kses to allow links for admin notices

17075e8

Use admin notices for MC list errors

c569f73

Remove global message output from settings page

a8c7500

Only admin notices should be displayed here

Rename global message to frontend message

ec55395

This better explains the functions purpose and will prevent confusion in the future. Updated mailchimp_sf_frontend_msg comments to explain how WP admin notices are used in the WP admin

github-actions bot assigned MaxwellGarceau Dec 19, 2024

github-actions bot added this to the 1.7.0 milestone Dec 19, 2024

MaxwellGarceau added 3 commits December 19, 2024 16:29

Fix typo

e1504ae

Remove void return type to maintain compatibility with php 7.0

fcce0a8

Update comment

bffedf7

MaxwellGarceau commented Dec 19, 2024

View reviewed changes

Update success message selectors in settings.test.js

09eab4f

MaxwellGarceau marked this pull request as ready for review December 19, 2024 21:55

github-actions bot added the needs:code-review This requires code review. label Dec 19, 2024

MaxwellGarceau requested review from dkotter and jeffpaul December 19, 2024 21:55

dkotter reviewed Dec 20, 2024

View reviewed changes

MaxwellGarceau mentioned this pull request Dec 26, 2024

Remove deprecated sopresto code #94

Open

1 task

MaxwellGarceau added 4 commits December 26, 2024 14:05

Namespace admin notice functions

4fda85e

Add autoloading via composer PSR-4

3532e0f

Fix lint errors

ac21b86

Update build workflows to install composer dependencies

7d2fc21

This comment was marked as outdated.

Sign in to view

MaxwellGarceau added 4 commits December 26, 2024 15:50

Revert autoload commits

eba7ecb

Remove autoload inclusion in root plugin file

4529f40

Change todo note to info note

6b0d9e7

Fix lint issue

f0c253e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/update success error messages to wp admin notices #85

Enhancement/update success error messages to wp admin notices #85

MaxwellGarceau commented Dec 19, 2024 •

edited

Loading

MaxwellGarceau left a comment

MaxwellGarceau Dec 19, 2024

dkotter Dec 20, 2024

MaxwellGarceau Dec 26, 2024

MaxwellGarceau commented Dec 19, 2024

dkotter Dec 20, 2024

jeffpaul commented Dec 23, 2024

This comment was marked as outdated.

MaxwellGarceau commented Dec 26, 2024

Enhancement/update success error messages to wp admin notices #85

Are you sure you want to change the base?

Enhancement/update success error messages to wp admin notices #85

Conversation

MaxwellGarceau commented Dec 19, 2024 • edited Loading

Description of the Change

Drawbacks

Possible regressions

How to test the Change

WP Admin messages

Success message

Error message

FE messages

Success Messages

Error Messages

Changelog Entry

Credits

Checklist:

MaxwellGarceau left a comment

Choose a reason for hiding this comment

MaxwellGarceau Dec 19, 2024

Choose a reason for hiding this comment

dkotter Dec 20, 2024

Choose a reason for hiding this comment

MaxwellGarceau Dec 26, 2024

Choose a reason for hiding this comment

MaxwellGarceau commented Dec 19, 2024

dkotter Dec 20, 2024

Choose a reason for hiding this comment

jeffpaul commented Dec 23, 2024

This comment was marked as outdated.

MaxwellGarceau commented Dec 26, 2024

MaxwellGarceau commented Dec 19, 2024 •

edited

Loading