WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15530 closed defect (bug) (fixed)

AJAX trashing of comments doesn't update on screen comment count

Reported by: johnbillion Owned by: garyc40
Milestone: 3.1 Priority: normal
Severity: minor Version: 3.1
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description

When you trash a comment or mark a comment as spam, the "X items" text next to the pagination buttons doesn't update.

Attachments (10)

garyc40.15530.diff (8.6 KB) - added by garyc40 7 years ago.
return localized number of items for each delete-comment ajax request
15530.diff (9.2 KB) - added by nacin 7 years ago.
15530.2.diff (8.0 KB) - added by nacin 7 years ago.
15530.3.diff (2.7 KB) - added by nacin 7 years ago.
garyc40.15530.pagination.diff (12.1 KB) - added by garyc40 7 years ago.
addressed all problems in comment:10
15530.4.diff (4.4 KB) - added by nacin 7 years ago.
15530.5.diff (6.4 KB) - added by nacin 7 years ago.
15530.6.diff (6.4 KB) - added by nacin 7 years ago.
15530.7.diff (7.2 KB) - added by nacin 7 years ago.
15530.debug.diff (530 bytes) - added by markmcwilliams 7 years ago.

Download all attachments as: .zip

Change History (31)

#1 @johnbillion
7 years ago

Sort of related: #6530

#2 @nacin
7 years ago

  • Milestone changed from Awaiting Review to Future Release

#3 @nacin
7 years ago

  • Milestone changed from Future Release to 3.1

Per discussion in IRC, this is a regression and should have a bit of love in the context of #16262. I believe garyc40 hopes to handle this.

@garyc40
7 years ago

return localized number of items for each delete-comment ajax request

#4 @garyc40
7 years ago

  • Keywords has-patch added
  • Owner set to garyc40
  • Status changed from new to assigned

#5 @markjaquith
7 years ago

Works great in my testing!

#6 @ryan
7 years ago

Looks good.

#7 @PeteMall
7 years ago

  • Keywords commit added

#8 @nacin
7 years ago

Confused why there's a giant chunk of red in the middle of that patch. There's also now a TODO that mentions pagination links, but it looks like that block of code is supposed to handle pagination links. Would like a second opinion from the JS side as well.

#9 @garyc40
7 years ago

That block output the old pagination links ([1] [2] [3] ... type of links), so it's useless in 3.1 anyways. We need to re-do it using WP_List_Table API for pagination link, or recalculate total pages using JS. That's why I put TODO there and delete the whole block.

Perhaps another ticket for restoring the proper pagination links? It's only a minor problem.

@nacin
7 years ago

#10 @garyc40
7 years ago

The problem is a bit more complicated that I thought.

Try these:

  • Go to a filter (like Pending or Approved), and trash comment there. The pagination links returned do not include comment_status argument (or any arguments for that matter).
  • Search something, then delete a comment. The ajax handler does not take argument s into account, so wrong total items and total number of pages are returned.
  • Go to the last page of the list, delete all comments on that page one by one. The page number becomes to "1 of xx". I think the expected behavior should be redirecting to the new last page.

Working on a patch. Seems like more reds and greens are going to be added to this minor ticket :(

@nacin
7 years ago

#11 @markjaquith
7 years ago

Maybe I'm missing something, but why does this need an AJAX call at all? If the item disappears, decrement the appropriate comment counts locally. And if you have comments per page and number of comments, decrement number of comments, divide by cpp, round up, and that's your new number of total pages. Do the opposite for undo.

#12 @garyc40
7 years ago

This is because total item number needs to be localized (xx items or xx item).

@nacin
7 years ago

@garyc40
7 years ago

addressed all problems in comment:10

#13 @garyc40
7 years ago

The comment river is not properly refilled if there's a search query ($.query.get() automatically fill comment_type with a true value).

I included a fix for this in garyc40.15530.pagination.diff as well.

@nacin
7 years ago

@nacin
7 years ago

#14 @nacin
7 years ago

  • Keywords no-really-commit added

Latest patch, 15530.6.diff, is the result of more than four hours of iterations. koopersmith assisted. I also pulled in mdawaffe as he knew the history of the comment counts: [10204/trunk/wp-admin/admin-ajax.php]. As we continued, we noticed particular bugs that garyc40 also ran into, and thus I largely incorporated his fixes where appropriate.

In a nutshell, this patch fixes pagination for comment moderation. Some major points:

  • It does not remove a chunk of code from the ajax handler. It keeps the client in sync as before, using the original code with a twist. We now always send an ajax response, rather than a die( time() ), but still avoids the comment counts in most situations.
  • It avoids touching any list table logic, as we can play dumb and just use the arguments we get from the client.
  • It avoids messing with any pagination links themselves, with the exception of the disabled class for the next and last links. Clicking these will redirect you to the last page anyway (even if out of bounds), so I'm perfectly happy with that behavior for 3.1.

@nacin
7 years ago

@nacin
7 years ago

#15 @nacin
7 years ago

15530.7.diff includes a change that isn't directly related to everything else here, but is an easy fix. Basically, we stick $.query.get() directly into ajax, but $.query.get() sets comment_type to true, which then becomes 'true', rather than '' as it should be. Simple check fixes that. $.query might include a way to avoid such casts. I didn't check.

Considering .7 ready for commit.

#16 @ryan
7 years ago

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

(In [17354]) Update counts and pagination when trashing and moderating comments. Props garyc40, koopersmith, mdawaffe, nacin. fixes #15530

#17 @solarissmoke
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There is some debug code lurking in the patch that was applied - class-wp-comments-list-table.php line 51 needs to be culled:

error_log( var_export( $comment_type, true ) );

#18 @markmcwilliams
7 years ago

Attached a patch above.

#19 @scribu
7 years ago

  • Keywords no-really-commit removed

#20 @ryan
7 years ago

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

(In [17383]) Remove debug cruft. Props markmcwilliams. fixes #15530

#21 @ryan
7 years ago

(In [17384]) Remove debug cruft. Props markmcwilliams. fixes #15530

Note: See TracTickets for help on using tickets.