Skip to:

Opened 11 years ago

Closed 11 years ago

#4844 closed defect (bug) (fixed)

Show all X comments should update when comments are deleted

Reported by: modemlooper's profile modemlooper Owned by: boonebgorges's profile boonebgorges
Milestone: 1.8 Priority: low
Severity: minor Version: 1.7
Component: Activity Keywords: has-patch


When you delete comments the amount in the show all link doesn't change. If you delete and then click the show all, it show's all the comments including the previously deleted comments.

Create 7 comments
Refresh page to get "show all X comments" link
Delete a few comments no change to amount
Click the show all link
previously deleted comments reappear

Attachments (1)

4844.patch (1.2 KB) - added by merty 11 years ago.

Download all attachments as: .zip

Change History (10)

#1 @boonebgorges
11 years ago

  • Keywords reporter-feedback added

Can you be more specific about what you mean by "comments"? Do you mean activity replies? If so, I can't reproduce the issue - the "x comments" button changes as I'd expect (and also, I don't see a "show all" link).

#2 @modemlooper
11 years ago

You can see the show all. when you are logged in it says show all 6 comments. If you delete a comment the number doesn't change and if you delete and then click the show all link it shows the previously deleted comments.

#4 @boonebgorges
11 years ago

  • Component changed from Core to Activity
  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 1.8
  • Priority changed from normal to low
  • Severity changed from normal to minor

Thanks for the additional details. I've confirmed both issues. Neither one is a regression; the same problems appear in the 1.6 branch.

#5 @boonebgorges
11 years ago

Marked #3942 as duplicate.

11 years ago

#6 @merty
11 years ago

  • Keywords has-patch added; needs-patch removed

fadeOut just sets the item's display property to none and leaves it in the DOM. Added a callback to it to remove it from the DOM after the effect is completed.

I don't think updating the X in "Show all X comments" is worth the trouble. I think it's pretty understandable from the user's point of view.

#7 @boonebgorges
11 years ago

In 7000:

When activity comments are deleted via AJAX, remove them from the DOM entirely

Previously, deleted comments were merely hidden using jQuery.fadeOut(). As a
result, when the show/hide comment button was toggled - applying $.show() to
all comment items - the comments would appear again, even though they had been
deleted from the server. Removing them from the DOM ensures that this won't

See #4844

Props merty

#8 @boonebgorges
11 years ago

I don't think updating the X in "Show all X comments" is worth the trouble. I think it's pretty understandable from the user's point of view.

I might've been tempted to agree with this. But then I looked at the way this string is currently generated, and was so repulsed that I knew it had to be rewritten :) Basically, we were piecing it together like this: "Show all" + " " + comment_count + " " + "comments". Beyond being breathtakingly ugly, this is extremely untranslatable.

I've refactored the way that the show-all text is generated, so that (a) it's possible to translate it, and (b) it's easy to write javascript to update the number after deletion. Win-win!

#9 @boonebgorges
11 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 7002:

Update 'Show all x comments' count after creating or deleting activity comments

On activity permalink pages, the comment list is collapsed if the item has more
than 6 responses, and a link is appended that reads 'Show all x comments'. This
changeset introduces additional JavaScript to update this count when a new
activity comment is created, or an existing one is deleted, via AJAX.

In the process of making the change, the logic for constructing the 'Show all
x comments' string was changed, so that it's now properly translatable.

Also cached a JS variable that was being used many times, to avoid unnecessary
DOM traversal.

Fixes #4844

Note: See TracTickets for help on using tickets.