Make WordPress Core

Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#15530 closed defect (bug) (fixed)

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

Reported by: johnbillion's profile johnbillion Owned by: garyc40's profile 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 14 years ago.
return localized number of items for each delete-comment ajax request
15530.diff (9.2 KB) - added by nacin 14 years ago.
15530.2.diff (8.0 KB) - added by nacin 14 years ago.
15530.3.diff (2.7 KB) - added by nacin 14 years ago.
garyc40.15530.pagination.diff (12.1 KB) - added by garyc40 14 years ago.
addressed all problems in comment:10
15530.4.diff (4.4 KB) - added by nacin 14 years ago.
15530.5.diff (6.4 KB) - added by nacin 14 years ago.
15530.6.diff (6.4 KB) - added by nacin 14 years ago.
15530.7.diff (7.2 KB) - added by nacin 14 years ago.
15530.debug.diff (530 bytes) - added by markmcwilliams 14 years ago.

Download all attachments as: .zip

Change History (31)

#1 @johnbillion
15 years ago

Sort of related: #6530

#2 @nacin
14 years ago

  • Milestone changed from Awaiting Review to Future Release

#3 @nacin
14 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
14 years ago

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

#4 @garyc40
14 years ago

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

#5 @markjaquith
14 years ago

Works great in my testing!

#6 @ryan
14 years ago

Looks good.

#7 @PeteMall
14 years ago

  • Keywords commit added

#8 @nacin
14 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
14 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
14 years ago

#10 @garyc40
14 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
14 years ago

#11 @markjaquith
14 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
14 years ago

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

@nacin
14 years ago

@garyc40
14 years ago

addressed all problems in comment:10

#13 @garyc40
14 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
14 years ago

@nacin
14 years ago

#14 @nacin
14 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
14 years ago

@nacin
14 years ago

#15 @nacin
14 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
14 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
14 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
14 years ago

Attached a patch above.

#19 @scribu
14 years ago

  • Keywords no-really-commit removed

#20 @ryan
14 years ago

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

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

#21 @ryan
14 years ago

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

Note: See TracTickets for help on using tickets.