-
Notifications
You must be signed in to change notification settings - Fork 33
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
Make the admin notification mechanics contextually aware (of the user and of the admin screen) #767
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including unit tests and nice work on this one @barryhughes !!
Also amazing PR description :)
I've left a few initial comments, but so far these change look good!
Co-authored-by: Matt Allan <[email protected]>
… clearing the queue).
If a plugin tries to call wcs_add_admin_notice() and no user is currently logged in, then a transient will be created and (if this happens periodically) it will A) not expire B) continue to grow in size. This change guards against that possibility.
@mattallan requesting your further review. I think I addressed all of your concerns, but of course let me know if I missed anything/if any of the adjustments are off-key. Thanks again for all the great feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great @barryhughes !
I tested:
- existing uses of
wcs_display_admin_notices()
throughout subscriptions are still working as expected - transient is properly cleared after noticed is displayed
- tested user specific notices and screen specific:
wcs_add_admin_notice( 'This is a test notice.', 'error' );
wcs_add_admin_notice( 'Show this to user 2.', 'error', 2 );
wcs_add_admin_notice( 'Only show this notice on subscription list.', 'success', 1, 'edit-shop_subscription' );
wcs_add_admin_notice( 'Invalid guest notice.', 'success', 0 );
All appears to be working as expected 💯 I've left a small comment to fix a typo but apart from that this is good !
Thanks again
Co-authored-by: Matt Allan <[email protected]>
If an admin notice is added via the
wcs_add_admin_notice()
function, then it will display on the next request for an admin screen—regardless of who the current user is or what screen they are viewing. Here's a hypothetical example:In this change, we tighten things up a little such that:
How can this code break?
We previously stored these notices in a transient named
_wcs_admin_notices
, but will now use one or more transients named_wcs_admin_notices_<user_id>
. So, an enqueued notice could be lost upon update (I think this is acceptable, and would be a very temporary problem that probably would impact very few users indeed ... but we could soften the landing if we think it's necessary to do so).Updates http://github.com/woocommerce/woocommerce-subscriptions/issues/4776
How to test this PR
Let's first test that existing admin notices continue work as expected. To do this, select an existing subscription and contrive a problem in which the subscription end date is earlier than the next payment date:
Normally it's not possible to do this, so setting it up will require direct edits to the relevant row of the
wc_orders_meta
table (assuming you are using HPOS). Find the_schedule_end
entry, and alter the date manually as needed.With that done, return to the editor and click update. Upon reload, you should see that an admin notice is present on the page telling you that the end date is incorrect:
On to the real testing! Let's confirm that admin notices are 'constrained' to the current user. Probably the easiest way to do this is to have two separate browser contexts: in one, you should be logged in as an admin user or shop manager, in the other you should be logged in as a contributor (or other less privileged user). You also need to add the following snippet to a suitable location, such as
wp-content/mu-plugins/test-pr-767.php
:example.com/wp-admin?create-wcs-message=1
. This triggers the code in the snippet.?create-wcs-message=1
is no longer in the URL.Tip
In case it isn't clear, note that you need to run through the whole set of testing steps once without this fix, and then again with the fix. You can't flip between branches as you go along, or you will get unexpected results.
Note
The change goes beyond what these testing instructions describe. For example, as noted earlier, it now becomes possible to lock a message to a specific screen. I have not detailed testing steps for this but will probably do so in a further PR that takes advantage of that functionality. Nonetheless, feel free to explore this area now if you feel so inclined!
Product impact