Shiny Updates: Prevent navigating away while plugins are updating

[johnbillion]

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

[jorbin]

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

[adamsilverstein]

warn before unload when updating plugins

[adamsilverstein]

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?

[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.

[adamsilverstein]

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.

[adamsilverstein]

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).

[DrewAPicture]

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?

[jorbin]

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.

[jorbin]

In 31989:

Add beforeunload string for shiny updates.

See #31769.
Props johnbillion, adamsilverstein, jorbin.

[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().

[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

[adamsilverstein]

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

[adamsilverstein]

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

[ericlewis]

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

[DrewAPicture]

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

[jorbin]

yoink

Reviewing this now.

[jorbin]

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