#48333 closed defect (bug) (fixed)
Show success notice when the admin verification screen is "snoozed"
Reported by: | desrosj | Owned by: | whyisjake |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | 5.3 |
Component: | Site Health | Keywords: | has-patch |
Focuses: | ui, administration | Cc: |
Description
When a user clicks "Remind me later" on the admin email verification screen, they are brought to the Dashboard and the screen will not show for 3 days. But the user is not shown a notice indicating the action has succeeded.
Attachments (4)
Change History (38)
#3
@
5 years ago
@desrosj Do we need to consider the user's locale when displaying the success message? Refer #48313
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
5 years ago
#7
@
5 years ago
- Keywords 2nd-opinion added
Looking at 48333.diff the problem with this approach is that the query string may get "sticky" in some cases and then the message will be displayed "out of bounds" which will make it confusing.
For example if the user is redirected to a (plugin) screen that has a <form action="">
, the query string will "carry over" after submitting that form and the message will be displayed again. Another edge case is making a browser bookmark when the query string is present. Then every time the bookmark is used, the same message will be displayed...
This was in the initial patch but was removed for that reason.
Another possible solution would be to set a transient when that message is needed, and remove it when the message is displayed. However that writes to the DB, twice. Still probably a better approach as these DB writes will only happen for logged-in users only when they click "remind me later".
Also, may be better to output that message from a function that is added on a hook in the admin? Perhaps admin_init
or similar.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
5 years ago
#9
@
5 years ago
- Milestone changed from 5.3.1 to 5.4
Even if this enhancement would fit very well with WP 5.3.1 scope, we realistically don't have the timeframe to give it proper attention. Moving it to the next release.
This ticket was mentioned in Slack in #core-site-health by clorith. View the logs.
5 years ago
#11
@
5 years ago
- Keywords needs-patch added; has-patch 2nd-opinion removed
I'm agreeing with @azaozz here, and i think the lesser evil is probably to do the transient approach for now, until a better approach for all of WP exists.
#12
@
5 years ago
Yet another way (as discussed in Slack) would be to use sessionStorage
in the browser and store the message text. See https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage.
It will need some "global" js running on all wp-admin pages that will check for stored messages, and if there are any display them and remove them from the sessionStorage. Would probably need to make sure the messages are "plain text" (no HTML tags allowed), then "wrap" them in the standard <div class="notice is-dismissible ...">
that has a "Close" button.
May also need some way to make sure any messages actually come from WP. It would be possible to set sessionStorage from a "third-party" site before navigating to wp-admin. Needs some way to check for that, perhaps a nonce.
#13
@
5 years ago
- Milestone changed from 5.4 to 5.5
Although this would be nice, we're about to hit hit beta for 5.4, so I'm moving this to the next milestone.
This does give us some more time to properly lay a plan and not haphazardly slap this on in a hurry, as I think this is a feature that would make sense to use in other areas of wp-admin as well where currently URLs with status code digits are used.
This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.
5 years ago
#15
@
5 years ago
I've used a very short term transient, 10 seconds, before to move displaying a notice out of a query arg before. It worked very well.
This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.
4 years ago
#21
@
4 years ago
As traction here has slowed down a bit, and it feels like the need for something more robust and safe (to avoid side-injected "admin notices from WP") than just a quick localstorage entry is what should happen, I propose we shelve this for now, and focus towards the notification system as mentioned in https://make.wordpress.org/core/2019/08/05/feature-project-proposal-wp-notify/ first.
A stop-gap solution of adding 48333.diff with a modification to check the age of the delay-action (if it's within 1 minute or hitting the button, show the message, if it's been longer, don't display it for example). This would work around the initial concerns of stickyness, and be a temporary solution.
#22
@
4 years ago
Added 48333.patch which does what I outlined above, it shows the warning, but only if it's been less than a minute since it was postponed.
This should make it so that even if visiting the URL with the wp-admin/?admin_email_remind_later
set, it'll only show the confirmation if it's a recent action.
As mentioned though, this is a stop-gap for a more appropriate implementation once we have a nice notification system to lean on.
#26
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
The new message does not respect the interval set via the admin_email_remind_interval
filter, it will always display "after 3 days", regardless of the actual value.
A couple of options to resolve this:
- Calculate the number of days
$remind_interval
corresponds to. - Use
human_time_diff()
.
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
4 years ago
#28
@
4 years ago
Looking to wrap the bugs with this one up.
48333.2.diff should address the issue @SergeyBiryukov raised above. If I can get a sanity check from someone else, I'm more than happy to commit.
Can someone detail the scenario where the query argument would be stuck in the URL? I'm having trouble visualizing the path to cause this issue. The query var has been added to the list in wp_removable_query_args()
, so it should be removed on page load, regardless of the context. Granted there is still a small window where someone could bookmark or copy the URL, but that seems relatively unlikely to occur.
#29
@
4 years ago
- Keywords commit added
Patch looks great, I can't think of anything that would keep that query arg in the URL.
#31
@
4 years ago
This now displays:
- "The admin email verification page will reappear after 6 days" for the default value of 3 days.
- "The admin email verification page will reappear after 4 days" for a custom value of 1 day.
- "The admin email verification page will reappear after 3 days" for a custom value of 12 hours.
48333.diff adds an admin notification when the user is redirected to the Dashboard after clicking "Remind me later".
For now, I hard coded the number of days that the notice will be snoozed for, but I think that time period should filterable, just like
admin_email_check_interval
. That can be a separate ticket, though.