treeherder/tbpl comments are not automatically collapsed

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
User Interface: Modal
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production
x86
Mac OS X
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1150541
(Assignee)

Updated

3 years ago
Priority: -- → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → glob
Depends on: 1146775
(Assignee)

Updated

3 years ago
Depends on: 1146769
(Assignee)

Updated

3 years ago
Summary: tbpl comments are not automatically collapsed → treeherder/tbpl comments are not automatically collapsed
(Assignee)

Comment 1

3 years ago
Created attachment 8617753 [details] [diff] [review]
1146774_1.patch

- 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.
(Assignee)

Comment 3

3 years ago
(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-
(Assignee)

Comment 5

3 years ago
(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).
(Assignee)

Comment 6

3 years ago
Created attachment 8621443 [details] [diff] [review]
1146774_2.patch

- 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+
(Assignee)

Comment 8

3 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   ac09f71..c290f3e  master -> master
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

3 years ago
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
You need to log in before you can comment on or make changes to this bug.