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

[antonrinas]

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.

[antonrinas]

Dashboard screenshot

[rachelbaker]

@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

[rachelbaker]

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.

[afercia]

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.

[rachelbaker]

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

[vagios]

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.

[rachelbaker]

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.

[adamsilverstein]

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

[azaozz]

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]

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.

[rachelbaker]

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

[adamsilverstein]

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.

[adamsilverstein]

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

[adamsilverstein]

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?

[adamsilverstein]

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

[adamsilverstein]

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

[pento]

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.

[adamsilverstein]

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.