Make WordPress Core

Opened 7 years ago

Closed 9 months ago

Last modified 9 months ago

#40966 closed defect (bug) (fixed)

Double clicking the Update plugins-button gives... interesting experience!

Reported by: bitnissen's profile bitnissen Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version: 4.6
Component: Upgrade/Install Keywords: shiny-updates good-first-bug has-patch commit
Focuses: Cc:

Description

Hi there,

I just updated to 4.8 on a bunch of sites and in one site I accidentally clicked the "Update"-button multiple times. What then happened, was a rather weird experience: After the update of the plugins successfully completed, it did another run where all updates apparently failed.

I tried this on multiple 4.8-installs, all has the same behaviour, so I guess it's fairly easy to reproduce. I've also recorded a video:

http://d.pr/v/qmoc7E/5MFtrVpY.mp4

Steps to reproduce:

  • Upgrade to 4.8.
  • Go to "Plugins" » "Updates available" (you need at least a plugin that has updates ready for it)
  • Select all update-ready plugins.
  • Click the "Update"-button. Then click it again.
  • Observe the table of plugins :-)

Kind regards,
Morten

Attachments (8)

40966.diff (1.2 KB) - added by MarcGuay 7 years ago.
40966.2.diff (900 bytes) - added by MarcGuay 7 years ago.
Remove extra line
40966.3.diff (1.9 KB) - added by xkon 7 years ago.
On Update only and re enable on all notices just in case.
40966.4.diff (812 bytes) - added by xkon 7 years ago.
Noop update
40966.5.diff (3.6 KB) - added by swissspidy 7 years ago.
40966.6.patch (2.9 KB) - added by bookdude13 4 years ago.
Refreshed patch
40966.7.diff (3.5 KB) - added by simonemanfre 11 months ago.
Add class 'is-enqueued' to prevent elements being added twice to the update queue
40966.8.diff (1.2 KB) - added by costdev 9 months ago.
Remove unrelated indentation changes, merge removeClass() calls for consistency, add a space before a parenthesis and make a slight improvement to an inline comment.

Download all attachments as: .zip

Change History (42)

#1 @swissspidy
7 years ago

  • Keywords shiny-updates needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to swissspidy
  • Status changed from new to assigned
  • Version changed from 4.8 to 4.6

Hey there,

Welcome to WordPress Trac and thanks for your report!

I'm pretty sure this isn't new in WordPress 4.8. This functionality was introduced in 4.6 with Shiny Updates. We've previously fixed this bug in the feature plugin (see https://github.com/obenland/shiny-updates/issues/145), but I guess not every instance of it.

#2 @MarcGuay
7 years ago

Because the Apply button is a submit input and not an anchor like in the other case, the solution will be a bit different. It seems that it will need the disabled property added somewhere in the click event in wp-admin/js/updates.js starting at line 1999, and then removed once the queue is finished working, but it's not obvious to me at the moment, it's my first time looking at this code.

@MarcGuay
7 years ago

@MarcGuay
7 years ago

Remove extra line

@xkon
7 years ago

On Update only and re enable on all notices just in case.

#3 @xkon
7 years ago

  • Keywords has-patch added; needs-patch removed

For some weird reason ( just in case it was working for you ) @MarcGuay s code was not disabling the button at all on my installation.

Either way I placed the code a little differently on ( 40966.3.diff ). It will disable the Apply button only in case of 'update-selected' and re-enable it either way whenever there's a notice. I think it's safer this way and at least it works for my installation.

Best regards,
Konstantinos

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


7 years ago

#5 @afercia
7 years ago

Please don't set disabled on focused elements because this will trigger a focus loss when using a keyboard. ideally, to avoid double clicks, the button should just no-op. /cc @swissspidy

@xkon
7 years ago

Noop update

#6 @xkon
7 years ago

Changed the diff into 40966.4.diff to fix the issues that @afercia mentioned.

Hopefully this will pass. I already did my tests but more tests are always welcome.

Best regards,
Konstantinos

@swissspidy
7 years ago

#7 @swissspidy
7 years ago

Few notes:

  • Network admin for themes is probably affected too
  • The first patches don't work because of a11y rasons
  • 40966.4.diff is too broad. $('.updating-message') could be anything

What we really need is a way to disallow a specific theme/plugin that has already been updated.

The bulk action handler bails early if an item doesn't have the update class (like, when a plugin doesn't have an update). In theory this be used to fix this bug here. However, wp.updates.updatePlugin doesn't remove that update class after a successful update.

40966.5.diff fixes that by removing update with updating during the process.

#8 @xkon
7 years ago

I just tried your patch @swissspidy and I'm still able to hit the Apply button resulting in plugins update failed.

I thought the idea was to not being able to re-send the update command by 'disabling' the Apply button so we don't get update errors. That's why by reading if the 'updating-message' exists somewhere in the page which is a class added only when something is currently being updated the button is just doing nothing. The 'updating-message' is also binded and checked only if you have the 'update' selected from your list and I haven't seen any other lists containing the 'update-selected' option but I might be wrong here.

Anyway I'm new to this so I might be missing something. Though the .4 patch was cancelling the Apply action and no more update failures where showing. The .5 you uploaded is still giving Errors.

This is by hitting the Apply button 5 times in a row.

I'll take another swing at it as well as I never though of the Theme process as well.

https://cldup.com/U8hak1vi8g.png

Best regards,
Konstantinos

Last edited 7 years ago by xkon (previous) (diff)

#9 @xkon
6 years ago

@swissspidy , now that i see this again with fresh eyes - do you want me to apply your patch and rework my js on top of your new css classes ? Just in case that makes it good to get it onboard as well?

#10 @swissspidy
6 years ago

@xkon Sure, feel free to build upon 40966.5.diff.

@bookdude13
4 years ago

Refreshed patch

#11 @bookdude13
4 years ago

  • Keywords needs-testing added

Hey all! I'm new here, and I hope I'm not stepping on anyone's toes. Patch refreshed. This works for me with a plugin updating. Not sure how to test the theme side. Note that I did not do the css selector change at the end of 40966.5. I wasn't sure if that was still necessary since the last post here was a couple years ago.

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


3 years ago

#13 @swissspidy
3 years ago

  • Owner swissspidy deleted

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


11 months ago

#15 @SergeyBiryukov
11 months ago

  • Milestone changed from Future Release to 6.3

This came up in a recent new contributors meeting. Moving to 6.3 for review.

#17 @giuliapierbon
11 months ago

I have tryed to apply 49066.6.patch to the current dev https://github.com/WordPress/wordpress-develop without success.
I have this error:
error: patch failed: src/js/_enqueues/wp/updates.js:380
error: src/js/_enqueues/wp/updates.js: patch does not apply

#18 @ryokuhi
11 months ago

  • Keywords needs-refresh added; needs-testing removed

Hello @giuliapierbon, and welcome on Trac.
You get the message above because the patch doesn't apply anymore.

#19 @giuliapierbon
11 months ago

Okay, I've just realized!Sorry
Can I have the ownership?

#20 @ryokuhi
11 months ago

  • Owner set to giuliapierbon

@giuliapierbon No problem! Contributing to WordPress can be difficult (especially in the beginning), but the most important part is to give it a try!
If you want to take care of the ticket (which doesn't strictly mean that you have to write the patch for it), I'm giving you ownership.

@simonemanfre
11 months ago

Add class 'is-enqueued' to prevent elements being added twice to the update queue

#21 @simonemanfre
11 months ago

@giuliapierbon I have attached new code to prevent items being added to the update queue again when the user click the update button several times.

To do this I added a new class "is-enqueued" the first time the update is launched for each element, if the user click again on the update button I check if that class is already present and do not add it back to the queue.

The class is then removed in the event of a successful or failed update of each plugin.

I have tested locally for both successful and failed updates and it seems to work correctly.

I don't know if using an HTML class is the best way to solve this bug, maybe it can also be done using local storage, but i see that WordPress use many other class to do similar thing, like "active", "update" or "is-uninstallable".

Last edited 11 months ago by simonemanfre (previous) (diff)

#22 @ryokuhi
11 months ago

  • Keywords needs-testing added; needs-refresh removed

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


9 months ago

#24 @vasilism
9 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/40966/40966.7.diff

Environment

  • OS: Ubuntu 23.04
  • Web Server: NGNIX
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Brave Version 1.52.122 Chromium: 114.0.5735.110 (Official Build) (64-bit)
  • Theme: Twenty Twenty-Three
  • Active Plugins:
    • Gutenberg 15.9.1
    • WP Rollback 1.7.3
    • Akismet Anti-Spam: Spam Protection 5.1

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

On the plugins page, and with updates available for the plugins:

  1. Select the plugins for which updates are available.
  2. Select "Update" from the dropdown menu and click on the "Apply" button
  • Checked with 1 plugin selected to update and several clicks on the "Apply" button
  • Checked with multiple plugins selected to update and several clicks on the "Apply" button
  • Checked with 1 plugin selected to update and one click on the "Apply" button
  • Checked with multiple plugins selected to update and one click on the "Apply" button

Supplemental Artifacts

N/A

@costdev
9 months ago

Remove unrelated indentation changes, merge removeClass() calls for consistency, add a space before a parenthesis and make a slight improvement to an inline comment.

#25 @zunaid321
9 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/40966/40966.8.diff

Environment

  • OS: Windows 11 (22H2)
  • Web Server: nginx/1.23.4
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome Version 114.0.5735.110 (Official Build) (64-bit)
  • Theme: Twenty Twenty-Three

After Applying The Patch

  • Followed the instructions of @vasilism
  • ✅ The patch solves the issue

Plugins Used

  • All In One WP Migration V7.70
  • WP Rollback V1.7.2

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


9 months ago

#27 @costdev
9 months ago

  • Keywords commit added; needs-testing removed

With two successful test reports, I'm adding this for commit consideration.

#28 @audrasjb
9 months ago

  • Owner changed from giuliapierbon to audrasjb
  • Status changed from assigned to reviewing

Thanks for the test reports!
Self assigning for final review.

#29 @audrasjb
9 months ago

The last patch fixes the issue on my side 👍

#30 @audrasjb
9 months ago

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

In 56009:

Upgrade/Install: Prevent users from sending multiple bulk plugin updates.

This changeset improves update.js by adding is-enqueued class to enqueued plugin updates to prevent users from asking for several updates for the same
plugin at the same time, which previously resulted to …an interesting experience.

Props bitnissen, swissspidy, MarcGuay, xkon, afercia, swissspidy, bookdude13, simonemanfre, vasilism, costdev, zunaid321.
Fixes #40966.

#31 @audrasjb
9 months ago

In 56010:

Coding Standards: Fix missing semicolon after [56009].

See #40966.

#33 @audrasjb
9 months ago

In 56011:

Coding Standards: Fix undeclared variable issue.

Follow-up to [56009], [56010].

See #40966.

Note: See TracTickets for help on using tickets.