Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#48333 closed defect (bug) (fixed)

Show success notice when the admin verification screen is "snoozed"

Reported by: desrosj's profile desrosj Owned by: whyisjake's profile 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)

48333.diff (1.6 KB) - added by desrosj 5 years ago.
48333.patch (2.3 KB) - added by Clorith 4 years ago.
48333.1.diff (2.2 KB) - added by whyisjake 4 years ago.
48333.2.diff (417 bytes) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (38)

@desrosj
5 years ago

#1 @desrosj
5 years 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
5 years ago

  • Milestone changed from Awaiting Review to 5.3.1

#3 @sathyapulse
5 years ago

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

#4 @Clorith
5 years 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.


5 years ago

#6 @whyisjake
5 years ago

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

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

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

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


5 years ago

#9 @audrasjb
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 @Clorith
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 @azaozz
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.

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

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

#16 @afragen
5 years ago

  • Keywords has-patch added; needs-patch removed

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 @Clorith
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.

@Clorith
4 years ago

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

@whyisjake
4 years ago

#23 @whyisjake
4 years ago

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

#24 @whyisjake
4 years 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
4 years 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
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

@desrosj
4 years ago

#28 @desrosj
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 @whyisjake
4 years ago

  • Keywords commit added

Patch looks great, I can't think of anything that would keep that query arg in the URL.

#30 @whyisjake
4 years ago

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

In 48522:

Site Health: Ensure that there is a human readable time for snoozed email verifications.

This will be the time the was proposed, added to the reminder interval.

Fixes #48333.
Props SergeyBiryukov, desrosj.

#31 @SergeyBiryukov
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.

#32 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#33 @SergeyBiryukov
4 years ago

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

In 48528:

Site Health: Correct the message for snoozed email verifications.

Follow-up to [48522].

Fixes #48333.

#34 @desrosj
4 years ago

  • Keywords commit removed

@whyisjake if that scenario is not reproducible, then the conditional this notice is wrapped in is probably unnecessary. Wouldn't hurt to keep it in there, but not sure it's needed.

Note: See TracTickets for help on using tickets.