Make WordPress Core

Opened 8 years ago

Last modified 5 years ago

#35501 assigned defect (bug)

Dashboard page: incorrect work of "Activity" group box bottom counters

Reported by: antonrinas's profile antonrinas Owned by: adamsilverstein's profile adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4.1
Component: Comments Keywords: has-patch needs-testing
Focuses: javascript, administration Cc:

Description

STEPS TO REPRODUCE
Create new post, add comment through front end, go to dashboard page, click showed up menu items Approve/Unapprove/Spam/Trash in different combinations:

  • click "Approve" and quick click "Trash",
  • click "Unapprove" and quick click "Trash"
  • quick click "Approve" twice

EXPECTED RESULT: bottom counters "All", "Pending", "Approved", "Spam", "Trash" counts correct.
ACTUAL RESULT: see attachment.

Attachments (9)

Dashboard.jpg (126.4 KB) - added by antonrinas 8 years ago.
Dashboard screenshot
35501.patch (1021 bytes) - added by vagios 8 years ago.
35501.3.patch (1.0 KB) - added by adamsilverstein 8 years ago.
35501.4.patch (1.0 KB) - added by adamsilverstein 8 years ago.
35501.5.patch (3.5 KB) - added by vagios 8 years ago.
35501.6.patch (3.6 KB) - added by adamsilverstein 8 years ago.
35501.7.patch (3.8 KB) - added by adamsilverstein 6 years ago.
35501.8.patch (5.6 KB) - added by adamsilverstein 6 years ago.
35501.diff (5.4 KB) - added by adamsilverstein 6 years ago.

Download all attachments as: .zip

Change History (34)

@antonrinas
8 years ago

Dashboard screenshot

#1 @rachelbaker
8 years ago

  • Keywords reporter-feedback 2nd-opinion added

@antonrinas Do you see any errors in your browser's console when you trigger this bug?

I was unable to reproduce this bug, but I may not be able to perform the action changes fast enough. The All, Pending, Approved, Spam, and Trash counts were always updated to match the final action performed on each comment listed on the Dashboard.

Related #13820

#2 follow-up: @rachelbaker
8 years ago

  • Focuses administration added
  • Keywords reporter-feedback 2nd-opinion removed

Finally! I was able to reproduce this by double-clicking on Approve/Unapprove for a comment and the Pending/Approved numbers changed 2x expected result of second click action. Upon refreshing the Dashboard the totals and comments listed only show my first click was recorded.

The fix for this would be to disable clicking on the Comment action links while the ajax action is still in progress.

#3 in reply to: ↑ 2 @afercia
8 years ago

Replying to rachelbaker:

The fix for this would be to disable clicking on the Comment action links while the ajax action is still in progress.

Or abort the AJAX request if there's already one pending, see #25696.
Worth noting similar weird things happen when quickly double clicking Spam or Trash.

#4 @rachelbaker
8 years ago

  • Milestone changed from Awaiting Review to 4.6

#5 @rachelbaker
8 years ago

  • Focuses javascript added
  • Keywords needs-patch good-first-bug added

Would be a good ticket for a new contributor with JavaScript chops.

@vagios
8 years ago

#6 @vagios
8 years ago

  • Keywords needs-testing added

In 35501.patch, I made it to avoid new AJAX requests while there is already one running already (in a similar fashion to #25696).
It should fix the problem with double-clicks on approve/unapprove, spam and trash.

#7 @voldemortensen
8 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.


8 years ago

#9 @rachelbaker
8 years ago

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

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


8 years ago

#11 @rachelbaker
8 years ago

  • Keywords good-first-bug removed

I can't follow the logic in wp-lists.js well enough to review the patch here. Perhaps @afercia or @azaozz can take a look, otherwise this will have to wait for 4.7.

#12 follow-up: @adamsilverstein
8 years ago

  • Keywords commit added; needs-testing removed

@antonrinas thanks for the bug report.

@vagios thanks for the patch, this is an excellent approach, probably better than trying to disable the links themselves. I was a little concerned about browser compatibility, however I tested the patch in ie8, ios7 and an android device (via browserstack) and everything worked as expected. No matter how many times i click the links, the counts stay accurate.

In 35501.4.patch I cleaned up the patch a tiny bit, adding brackets around the single line conditionals, switching to 'yoda conditionals', adding periods at the end of the comment blocks and adding a blank line above each single comment line.

The only possible issue I could see with this code is if the users connection drops the links will remain inactive, however that issue would occur with the 'disabling link' approach; refreshing the page after the connection comes back would of fix the issue. We could add a timeout catch for failures and reset the xhr object, but I think the added complexity isn't worth the effort.

Last edited 8 years ago by adamsilverstein (previous) (diff)

#13 in reply to: ↑ 12 ; follow-ups: @azaozz
8 years ago

  • Keywords needs-patch added; has-patch commit removed

Replying to adamsilverstein:

The only possible issue I could see with this code is if the users connection drops...

Much more common problem with the patch is when the network is slow(er). The users will not be able to quickly approve/unapprove/delete comments, etc. They will have to wait for the previous AJAX request to complete which can be very tedious/slow. To reproduce:

  • Apply the patch.
  • Add sleep(10); to your test install's wp-config.php.
  • Try to moderate several comments.

IMHO this needs different approach.

@vagios
8 years ago

#14 in reply to: ↑ 13 @vagios
8 years ago

  • Keywords has-patch added; needs-patch removed

Thank you all for reviewing the patch.

Replying to azaozz:

Much more common problem with the patch is when the network is slow(er). The users will not be able to quickly approve/unapprove/delete comments, etc. They will have to wait for the previous AJAX request to complete which can be very tedious/slow.

In 35501.5 I tried a slightly different approach,

  • The xhr objects are now saved in an xhrs object using the corresponding comment as identifier. e.g
xhrs = {
    comment-26: { xhr object },
    comment-42: { xhr object }
}
  • An AJAX request will be aborted only if there is already an unfinished AJAX request from the same comment.
  • Added some code at the beginning of ajaxDel and ajaxDim to delete finished requests from xhrs.
  • Clicking the 'Undo' button (after a trash/spam action) while the trash/spam request is still going doesn't create a new request but changes the DOM. To avoid this I added code to abort the request in delBefore (inside edit-comments.js), and to be able to do it I pass xhrs into the settings object. Semantically it doesn't seem very good.

Replying to adamsilverstein:

We could add a timeout catch for failures and reset the xhr object, but I think the added complexity isn't worth the effort.

Indeed, we could catch timeouts and update the DOM (because it changes before the request finishes). I can try that but I would like to agree first on a solution for the previous issue because it seems more common.

#15 @rachelbaker
8 years ago

  • Keywords early 4.7-early added
  • Milestone changed from 4.6 to Future Release

Punting this ticket out of the 4.6 milestone since we are out of time. I would like to try again in 4.7.

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


8 years ago

#17 in reply to: ↑ 13 @adamsilverstein
8 years ago

Replying to azaozz:

IMHO this needs different approach.

Very good point about the slower connection issue.

What do you think about the approach of disabling the links to stop repeated clicks, then re-enable them when the action succeeds. This could be confined to the row the user is interacting with, allowing them to continue interacting with other rows while the potentially slow ajax request completed.

#18 @adamsilverstein
8 years ago

@vagios Thanks for your latest patch.

I tested the patch and verified it works as expected.

I like the way you track the xhrs by element and isolate the blocking that way. This neatly resolves the issue presented by slower connections.

I wonder if we should consider a similar approach on #25696 where slower connections would similarly block the simultaneous editing of multiple rows @azaozz pointed out here.

35501.6.patch is a refreshed patch with a few small changes:

  • For undefined checks I switched to 'undefined' !== typeof var ( because undefined !== var throws an error if var is undefined)
  • Added some whitespace inside square brackets (wp js coding standards)

#19 @adamsilverstein
6 years ago

  • Keywords 4.7-early removed
  • Milestone changed from Future Release to 5.0

In 35501.8.patch:

  • refresh against trunk
  • abstract out xhr functionality to wp.xhrs helper with three methods: clear, inProgress, and setXhrs.
  • needs better docs and some unit tests. Also, not sure about naming could be on wp.ajax or in a package?
Last edited 6 years ago by adamsilverstein (previous) (diff)

#20 @adamsilverstein
6 years ago

  • Keywords needs-testing added; early removed

35501.8.patch slight fixup and refresh against trunk, needs more testing.

#21 @pento
5 years ago

  • Milestone changed from 5.0 to 5.1

#22 @adamsilverstein
5 years ago

@pento planning to test this one again and commit for 5.1 - thoughts?

#23 @pento
5 years ago

wp.xhrs is cute. Are you planning on using it outside of this ticket in the near future? I'm okay with adding new APIs if we need them, but that has to be offset against the maintenance burden as we ultimately rewrite everything in a Gutenberg-y way.

#24 @adamsilverstein
5 years ago

So cute!

The original idea was to make it usable across the wp-admin interface, however i can see this functionality is something can probably bring in from an existing package now that we use webpack or maybe if we need as a wp package we can build in Gutenberg/GitHub.

Punting to future release for further consideration, feel free to close if that is more appropriate.

#25 @adamsilverstein
5 years ago

  • Milestone changed from 5.1 to Future Release
Note: See TracTickets for help on using tickets.