Closed Bug 1146774 Opened 6 years ago Closed 5 years ago

treeherder/tbpl comments are not automatically collapsed

Categories

(bugzilla.mozilla.org :: User Interface, defect, P1)

Production
x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: glob, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

tbpl comments are not automatically collapsed, and it isn't possible to hide them from the activity stream.

implementation wise we should add a hook to activity_stream() which allows the BMO extension to update relevant comments's collapsed attribute.
Blocks: 1150541
Priority: -- → P1
Assignee: nobody → glob
Depends on: 1146775
Depends on: 1146769
Summary: tbpl comments are not automatically collapsed → treeherder/tbpl comments are not automatically collapsed
Attached patch 1146774_1.patch (obsolete) β€” β€” Splinter Review
- optimise comment default-collapsing code
- add $comment->collapsed_reason
- add list of users which cause comments to be collapsed by default
- add 'hide/show treeherder comments' to view menu
- fixes some bugs when expanding/collapsing collapsed-by-default comments
Attachment #8617753 - Flags: review?(dylan)
Comment on attachment 8617753 [details] [diff] [review]
1146774_1.patch

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

I don't see anything objectionable in this, although I am surprised that the FILTER checker doesn't complain about extra_class in activity_stream.html.tmpl.
(In reply to Dylan William Hardison [:dylan] from comment #2)
> I don't see anything objectionable in this, although I am surprised that the
> FILTER checker doesn't complain about extra_class in
> activity_stream.html.tmpl.

the filter checker is very simple (regex based) and doesn't care much for the inverted style of that line.

ie. if i'd done:
> %]<div class="change-set cc-only [% extra_class %]" id="[% change_set.id %]" style="display:none">[%
instead of
> '<div class="change-set cc-only' _ extra_class _ '" id="' _ change_set.id _ '" style="display:none">';
it would have complained.

fwiw for both extra_class and change_set.id we're guaranteed that their contents are safe, so i'd be using "FILTER none" here anyhow.
Comment on attachment 8617753 [details] [diff] [review]
1146774_1.patch

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

r-

Code works, but the code below may eventually bite us. Comment or DRY. :-)

::: extensions/BugModal/Extension.pm
@@ +257,5 @@
> +
> +    # find the treeherder bot user for the "view -> hide treeherder comments" menu tiem
> +    $vars->{treeherder} =
> +        Bugzilla::User->new({ name => 'tbplbot@gmail.com', cache => 1 })
> +        || Bugzilla::User->new({ name => 'treeherder@bots.tld', cache => 1 });

The user list is coded as a constant in one place, and literal values here. Add a comment referencing the constant here, 
or use first { Bugzilla::User->new({name => $_, cache => 1}) } COLLAPSED_USERS;
Attachment #8617753 - Flags: review?(dylan) → review-
(In reply to Dylan William Hardison [:dylan] from comment #4)
> The user list is coded as a constant in one place, and literal values here.

it's in two places because they are two different use-cases.

the part you quoted is code specific to dealing with treeherder's comments.

COLLAPSED_USERS is more generic, and it's likely we'll add more users to that list.  there's no reason for the two lists to be identical (although that's currently the case).
Attached patch 1146774_2.patch β€” β€” Splinter Review
- provide a central lookup for treeherder's user
Attachment #8617753 - Attachment is obsolete: true
Attachment #8621443 - Flags: review?(dylan)
Comment on attachment 8621443 [details] [diff] [review]
1146774_2.patch

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

r=dylan
Attachment #8621443 - Flags: review?(dylan) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   ac09f71..c290f3e  master -> master
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
oops.. that menu item should only be visible when there are treeherder comments to hide:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   061328f..a48e145  master -> master
Component: User Interface: Modal → User Interface
Assignee: glob → nobody
You need to log in before you can comment on or make changes to this bug.