WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#15500 closed defect (bug) (fixed)

Ajax comment delete display bug

Reported by: hew Owned by: scribu
Milestone: 3.1 Priority: high
Severity: major Version: 3.1
Component: Comments Keywords: has-patch needs-testing
Focuses: Cc:

Description (last modified by westi)

I think [16486] may have introduced a new small bug with comment delete behavior.

To observe (trunk build), go to the last page of the comment admin interface and trash comments. I think other oddness with the Javascript on delete may be observable elsewhere, but the last page specifically is the easiest place to see it.

The deleted comment is trashed behind the scenes but it (or an entirely different comment) is appended erratically to the bottom of the page.

Attachments (2)

garyc40-15500.patch (2.9 KB) - added by garyc40 4 years ago.
garyc40-15500-rev2.patch (2.9 KB) - added by garyc40 4 years ago.
fixed unnecessary event check as well as doing ajax check

Download all attachments as: .zip

Change History (17)

comment:1 hew4 years ago

Meh, linked a ticket up above.. n00b. Here's the actual changeset:

http://core.trac.wordpress.org/changeset/16486

comment:2 nacin4 years ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 3.1

Changesets can be linked via [16486] or r16486.

comment:3 hew4 years ago

Anyone seen this yet (via my loose steps for reproduction up top or otherwise)?

I'm going to get a more consistent demonstrable behavior as someone I was corresponding with just told me this (seems to be the same issue):

"...clicking on Approve when on the Pending page causes a new comment to immediately appear at the bottom of the page [which is where I begin approving comments]. And if I correct a typo in a comment while on the Pending page, then that comments remains on the page. If no changes are made, the comment disappears upon approval, as is usual working in that page."

comment:4 westi4 years ago

  • Description modified (diff)

comment:5 westi4 years ago

Reproduced on Current trunk.

  • 11 Comments - Paging set to 10 per page.
  • Trash the 1 comment on the 2nd page and it appears both marked as trashed and in the list again

comment:6 westi4 years ago

  • Cc westi added
  • Owner set to scribu
  • Priority changed from normal to high
  • Severity changed from normal to major
  • Status changed from new to assigned

theExtraList doesn't get repopulated on AJAX page switching so it is way out of date when the trashing happens on non-page zero.

I can't see an easy fix for this.

comment:7 hew4 years ago

This bug also happens in all the other sections (pending, approved, spam, etc) of the comments admin ui. You can reproduce by following Westi's steps above and taking the appropriate action in that section (approve, unapprove, not spam, etc).

comment:8 PeteMall4 years ago

  • Keywords needs-patch added
  • Version set to 3.1

garyc404 years ago

comment:9 garyc404 years ago

  • Cc garyc40@… added
  • Keywords has-patch needs-testing added; needs-patch removed

Patch attached.

  • The Extra List is now populated correctly everytime the user navigates to another page.
  • Fixed WP_Comments_List_Table::prepare_items() to accept both $_GETnumber? and $_POSTnumber? in case of AJAX requests.

comment:10 scribu4 years ago

  • Keywords changed from has-patch, needs-testing to has-patch needs-testing

comment:11 scribu4 years ago

I like how you use .trigger() and .bind() but is it really necessary to use arguments.length ?

comment:12 follow-up: garyc404 years ago

Thanks! That's just a way to check whether refillTheExtraList() was called when an event trigger. Is there a better way to do that?

By the way, I noticed that some variables were not set correctly at the beginning of the file edit-comments.dev.js (line 7, 8, 9):

totalInput = $('.tablenav input[name="_total"]', '#comments-form');
perPageInput = $('.tablenav input[name="_per_page"]', '#comments-form');
pageInput = $('.tablenav input[name="_page"]', '#comments-form');

These all yield empty arrays. Removing the ".tablenav" part of the query would return the correct element. I tried that, but there would be weird behaviors. Did some debugging and it seemed like total pages count, pagination links and comment count are not properly updated when comments are being deleted (see #15530 as well).

If you try removing .tablenav, and have 61 comments. Number of comments per page is 20, number of pages is 4. If you trash one comment, then suddenly the existing pagination link elements are appended with a list of [1] [2] [3] pagination links.

I don't know if anyone notices this, and if a fix is planned for this as well?

comment:13 in reply to: ↑ 12 scribu4 years ago

Replying to garyc40:

Thanks! That's just a way to check whether refillTheExtraList() was called when an event trigger. Is there a better way to do that?

Something like this:

function refillTheExtraList(ev) {

  if ( ev ) {
  ...
  }
}

Fixed WP_Comments_List_Table::prepare_items() to accept both $_GETnumber? and $_POSTnumber? in case of AJAX requests.

I think simply switching to $_REQUEST all the time would be enough.

As for totalInput etc. I think you should open a new ticket if it's causing problems.

comment:14 garyc404 years ago

I wasn't sure about refillTheExtraList(ev) when I created the patch (thought there could be some error), but just did another test and it works fine. Embarrassing mistake :)

I'll revise the patch.

garyc404 years ago

fixed unnecessary event check as well as doing ajax check

comment:15 scribu4 years ago

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

(In [16911]) Fix error when trashing a comment on the last page. Props garyc40. Fixes #15500

Note: See TracTickets for help on using tickets.