WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 45 hours ago

#48333 reopened defect (bug)

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 (3)

48333.diff (1.6 KB) - added by desrosj 9 months ago.
48333.patch (2.3 KB) - added by Clorith 7 days ago.
48333.1.diff (2.2 KB) - added by whyisjake 7 days ago.

Download all attachments as: .zip

Change History (29)

@desrosj
9 months ago

#1 @desrosj
9 months ago

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.

#2 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 5.3.1

#3 @sathyapulse
8 months ago

@desrosj Do we need to consider the user's locale when displaying the success message? Refer #48313

#4 @Clorith
8 months ago

This will need a refresh as soon as #48334 lands (which I think is an important element to the reminder), but after that it should go in with 5.3.1

@sathyapulse #48313 shouldn't affect this, as the success notice is shown when you reach wp-admin.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

#6 @whyisjake
8 months ago

  • Owner set to whyisjake
  • Status changed from new to assigned

#7 @azaozz
8 months 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.

Last edited 8 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


7 months ago

#9 @audrasjb
7 months 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 months ago

#11 @Clorith
5 months 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 @azaozz
5 months 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.

Last edited 5 months ago by azaozz (previous) (diff)

#13 @Clorith
5 months 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.


3 months ago

#15 @afragen
3 months 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.

#16 @afragen
3 months ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


2 weeks ago

#21 @Clorith
2 weeks 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.

@Clorith
7 days ago

#22 @Clorith
7 days 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.

@whyisjake
7 days ago

#23 @whyisjake
7 days ago

@Clorith, updated patch that has been through composer lint.

#24 @whyisjake
2 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 48359:

Site Health: Ensure that the user will be notified after a successful snooze action.

After clicking remind me later, the user is shown an admin notification.

Fixes #48333.

Props desrosj, sathyapulse, Clorith, azaozz, audrasjb, afragen, whyisjake.

#25 @SergeyBiryukov
45 hours ago

In 48363:

Site Health: Correct translator comment for the message displayed after clicking "remind me later" on the admin email confirmation.

Adjust the logic for displaying the message for better readability.

Follow-up to [48359].

See #48333.

#26 @SergeyBiryukov
45 hours 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().
Note: See TracTickets for help on using tickets.