956892 - collapsed comments should display tags to indicate the reason for auto-collapsing

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Nickolay_Ponomarev]

bug 793963 implemented the ability to tag comments. Per https://wiki.mozilla.org/BMO/comment_tagging#Implementation_Notes comments with certain tags (e.g. 'spam', 'obsolete') are automatically collapsed.

Collapsed comments look like this:

>> Nickname 2011-06-22 01:47:27 PDT  [tag] [reply] [+] Comment 99

...with no indication as to why they are collapsed.

I think it would be useful to show the comment's tags on collapsed comments:

>> Nickname 2011-06-22 01:47:27 PDT (spam) [tag] [reply] [+] Comment 99

to hint at the reason they are collapsed.

Comment 1

[Andre Klapper]

This is bmo only (see "BMO" in the URL) - moving.

Comment 2

[Frédéric Buclin]

This is an upstream feature, available in Bugzilla 4.5.2.

Comment 3

[Justin Dolske [:Dolske]]

Actually, I somewhat disagree with this, and would actually suggest that collapsed comments be made even more subtle. But we might end up with a similar end result.

It's good, effective moderation policy to minimize the aftermath of a problem, and avoid drawing attention to disturbances. Not to mention just keeping things clean and orderly.

A minimal change to get closer to what I'm thinking would be to change collapsed comments from:

>> Nickname 2011-06-22 01:47:27 PDT  [tag] [reply] [+] Comment 99

to:

>> Spam comment removed. [+]

or even just

>> Comment removed. [+]

Bonus points for some text style to easier to skim over (eg gray text, smaller font, etc tbd).

Double bonus points for collapsing multiple comments together ("5 comments removed"), but that's a different bug. :)

Comment 4

[Nickolay_Ponomarev]

dolske: I agree completely. The reason I asked for tags to show up on collapsed comments was:
1) to make it clearer why it was collapsed (because it had "bad" tags) - your suggestions both make that clear.
2) to distinguish between useless comments (spam/advocacy) and obsolete comments that may have some value when reading through an old bug.

Comment 5

[:glob ✱]

Attachment: 956892_1.patch

- comments which are collapsed by default (ie tagged with any of the collapsed_comment_tags tags) now have a much simpler header
- the "comment hidden" text has a tooltip with the comment author and time
- expanding a comment will restore the standard header

Comment 6

[:glob ✱]

Attachment: screenshot showing collapsed comments

Comment 7

[Gervase Markham [:gerv]]

Comment on attachment 8418608 [details] [diff] [review]
956892_1.patch

Review of attachment 8418608 [details] [diff] [review]:
-----------------------------------------------------------------

The graphic looks nice :-)

Gerv

::: template/en/default/bug/comments.html.tmpl
@@ +116,4 @@
>        <div class="[% class_name FILTER html %]">
>          [% IF mode == "edit" %]
>            <span class="bz_comment_actions">
> +            <span class="[% "collapsed" IF comment.collapsed %]">

This will produce a random empty <span> if the comment is not collapsed, i.e. in the common case. For page weight reasons (every little helps!) isn't it better to have the whole span conditional on comment.collapsed? Or were you trying to avoid two IF tests?

@@ +173,4 @@
>            [% END %]
>          </span>
>  
> +        <span class="bz_comment_time [% " collapsed" IF comment.collapsed %]">

If all these added "collapsed" classes are used to hide things, can't we put the class on a node higher up (probably the <div> that you are already adding bz_default_collapsed to) and then use descendant CSS rules to collapse the things we want?

div.collapsed > .bz_comment_time, ... { display: none; }

or whatever? (If the "collapsed" class is already used elsewhere for directly collapsed things, call it "collapsed_comment" instead.) This would also, AIUI, avoid the new loop in the expand_comment() function.

@@ +177,4 @@
>            [%+ comment.creation_ts FILTER time %]
>          </span>
> +
> +        [% IF comment.collapsed %]

So we are treating comments collapsed when the page is loaded differently to comments collapsed by the user?

Not objecting, just asking.

@@ +179,5 @@
> +
> +        [% IF comment.collapsed %]
> +          <span id="cr[% comment.count %]" class="bz_comment_collapse_reason"
> +             title="[% comment.author.name || comment.author.login FILTER html %]
> +                    [%~%] [[% comment.creation_ts FILTER time %]]">

Doesn't this "~" mean the time will follow the author name without even a space?

Comment 8

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #7)

> div.collapsed > .bz_comment_time, ... { display: none; }

that sounds good, i'll experiment with that.

> So we are treating comments collapsed when the page is loaded differently to
> comments collapsed by the user?

yes.  the goal here is to de-focus comments which are collapsed by default (especially those which are spam/offtopic/etc).  comments collapsed by the user while viewing a bug don't need this as the user has already seen them.

> > +                    [%~%] [[% comment.creation_ts FILTER time %]]">
> 
> Doesn't this "~" mean the time will follow the author name without even a
> space?

no.
i'll replace it with [%~ %] for clarity in my next revision.

Comment 9

[:glob ✱]

Attachment: 956892_2.patch

- <span>s no longer created for non-collapsed comments
- using css inheritence to hide comment parts instead of explicit classes

Comment 10

[:glob ✱]

Comment on attachment 8423630 [details] [diff] [review]
956892_2.patch

bug 1013986 pointed out that automatically collapsed comments are not expandable unless you're logged in.

i'll fix that here since it's the same area of code and somewhat related.

Comment 11

[:glob ✱]

Attachment: 956892_3.patch

- shows the [+] expander to logged out users for collapsed-by-default comments

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 8426852 [details] [diff] [review]
956892_3.patch

r=gerv.

Gerv

Comment 13

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   80c434b..384a1c6  master -> master

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   9916f57..dd10df6  master -> master