WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#31769 closed enhancement (fixed)

Shiny Updates: Prevent navigating away while plugins are updating

Reported by: johnbillion Owned by: jorbin
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: has-patch commit
Focuses: javascript, administration Cc:

Description

We should have an onbeforeunload event active while a plugin is being updated via shiny updates. Especially for bulk updates.

Attachments (4)

31769.diff (1.2 KB) - added by adamsilverstein 4 years ago.
warn before unload when updating plugins
31769.2.diff (1.4 KB) - added by adamsilverstein 4 years ago.
31769.3.diff (2.1 KB) - added by adamsilverstein 4 years ago.
31769.4.diff (527 bytes) - added by ericlewis 4 years ago.

Download all attachments as: .zip

Change History (30)

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


4 years ago

#2 @jorbin
4 years ago

I like the idea of adding an AYS check if an update is in process or if there are items in the queue.

@adamsilverstein
4 years ago

warn before unload when updating plugins

#3 @adamsilverstein
4 years ago

  • Focuses javascript added
  • Keywords has-patch dev-feedback added; needs-patch removed

In 31769.2.diff:

  • leverage window.onbeforeunload to warn users navigating away while plugin updates still running
  • remove onbeforeunload when all updates complete

Tested in firefox: does this work correctly in IE8?

#4 @DrewAPicture
4 years ago

  • Keywords needs-testing added

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


4 years ago

#6 @DrewAPicture
4 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#7 @DrewAPicture
4 years ago

  • Keywords 4.2-strings added

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


4 years ago

#9 follow-ups: @johnbillion
4 years ago

  • Keywords needs-refresh added

This doesn't clear the onbeforeunload event if an error occurs with a plugin update.

It looks like wp.updates.updateLock = false; needs to be called in wp.updates.updateError() in the same way that it is in wp.updates.updateSuccess(), but I'm not overly familiar with this code.

@jorbin @pento @adamsilverstein thoughts?

Also patch needs a refresh to apply cleanly.

#10 in reply to: ↑ 9 @adamsilverstein
4 years ago

I will update the patch and double check to make sure the lock is cleared if the update fails, that sounds right.

I will also test in IE :)

Replying to johnbillion:

This doesn't clear the onbeforeunload event if an error occurs with a plugin update.

It looks like wp.updates.updateLock = false; needs to be called in wp.updates.updateError() in the same way that it is in wp.updates.updateSuccess(), but I'm not overly familiar with this code.

@jorbin @pento @adamsilverstein thoughts?

Also patch needs a refresh to apply cleanly.

#11 follow-up: @adamsilverstein
4 years ago

In 31769.3.diff clear updateLock, recheck queue and possible clear onbeforeunload in always callback, which happens after success or failure.

Resolves an issue where onbeforeunload would not be cleared when updates fail.

Could use some additional testing: test with failures, test in IE.

I noticed one JS issue updating, i reverted to an old version of Jetpack then updated it, the slug in the response didn’t match the data-slug attribute looked for on L 234: {{ var $pluginRow = $( '[data-slug="' + response.slug + '"]' ).first(); }} - possibly because the slug changed? we should ensure the slug always matches here, or at least check and not fail if it changed, this failure broke the rest of my updates (I had queued multiple).

#12 @DrewAPicture
4 years ago

  • Keywords needs-refresh removed

The only thing in this ticket blocking beta 4 is the new string change in script-loader.php. Can we get that one change committed in lieu of finishing up this entire ticket?

#13 @jorbin
4 years ago

I wonder if the string is exactly what we want. It doesn't provide much actual feedback. What do people think of:

Plugins are currently being updated. If you navigate away there may be unexpected behavior.

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


4 years ago

#15 @jorbin
4 years ago

In 31989:

Add beforeunload string for shiny updates.

See #31769.
Props johnbillion, adamsilverstein, jorbin.

#16 @DrewAPicture
4 years ago

  • Keywords 4.2-strings removed

@ericlewis
4 years ago

#17 follow-up: @ericlewis
4 years ago

In attachment:31769.4.diff,

  • move the beforeunload handler to a callback bound via $( window ).on( 'beforeunload' ), rather than setting and unsetting the callback to window.onbeforeunload. We don't need to play hot potato with the callback because the internal conditional logic will decide whether to pop open the "Are you sure?" alert.
  • remove the wp.ajax.post().always() callback, which will cause spurious requests when a user inputs the wrong credentials. See this comment. The lock is released in wp.updates.updateSuccess(), and the request modal is re-opened in the error modal, so the lock doesn't need to be released in wp.updates.updateError().

#18 in reply to: ↑ 11 ; follow-up: @ericlewis
4 years ago

Replying to adamsilverstein:

I noticed one JS issue updating, i reverted to an old version of Jetpack then updated it, the slug in the response didn’t match the data-slug attribute looked for on L 234: {{ var $pluginRow = $( '[data-slug="' + response.slug + '"]' ).first(); }} - possibly because the slug changed? we should ensure the slug always matches here, or at least check and not fail if it changed, this failure broke the rest of my updates (I had queued multiple).

Can we open a separate ticket and get clear steps to reproduce here? :D

#19 in reply to: ↑ 17 @adamsilverstein
4 years ago

Nice!

Replying to ericlewis:

In attachment:31769.4.diff,

  • move the beforeunload handler to a callback bound via $( window ).on( 'beforeunload' ), rather than setting and unsetting the callback to window.onbeforeunload. We don't need to play hot potato with the callback because the internal conditional logic will decide whether to pop open the "Are you sure?" alert.
  • remove the wp.ajax.post().always() callback, which will cause spurious requests when a user inputs the wrong credentials. See this comment. The lock is released in wp.updates.updateSuccess(), and the request modal is re-opened in the error modal, so the lock doesn't need to be released in wp.updates.updateError().

#20 in reply to: ↑ 18 @adamsilverstein
4 years ago

Will do, lets get this in and I will open a new ticket.

Replying to ericlewis:

Replying to adamsilverstein:

I noticed one JS issue updating, i reverted to an old version of Jetpack then updated it, the slug in the response didn’t match the data-slug attribute looked for on L 234: {{ var $pluginRow = $( '[data-slug="' + response.slug + '"]' ).first(); }} - possibly because the slug changed? we should ensure the slug always matches here, or at least check and not fail if it changed, this failure broke the rest of my updates (I had queued multiple).

Can we open a separate ticket and get clear steps to reproduce here? :D

#21 in reply to: ↑ 9 @ericlewis
4 years ago

Replying to johnbillion:

It looks like wp.updates.updateLock = false; needs to be called in wp.updates.updateError() in the same way that it is in wp.updates.updateSuccess(), but I'm not overly familiar with this code.

Yes. See #31892

#22 @ericlewis
4 years ago

  • Summary changed from Prevent navigating away while plugins are updating via shiny updates to Shiny Updates: Prevent navigating away while plugins are updating

#23 @DrewAPicture
4 years ago

  • Keywords commit added; dev-feedback needs-testing removed

The AYS notice comes up as expected when trying to navigate away during a shiny update. Moving for commit.

#24 @jorbin
4 years ago

  • Owner changed from johnbillion to jorbin

yoink

Reviewing this now.

#25 @jorbin
4 years ago

In 32052:

Conditionally add AYS to leaving shiny updates

When a shiny update is happening or pending, we should make sure users don't accidentally leave the page. This simple notification should help prevent users from accidentally not updating when they want to update.

See #31769
Props ericlewis and adamsilverstein for initial patch

#26 @jorbin
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.