663751 - create reports to assist triage

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[:glob ✱]

we should add some new reports to make finding bug that need attention easier.

sdaugherty has suggested:

- UNCONFIRMED where the reporter or user without canconfirm is the last commenter
- UNCONFIRMED without a comment for 30 days

Comment 1

[Stephanie Daugherty [:sdaugherty]]

The second of these is already searchable I think. The first is the interesting one.

The idea is that if the last commenter was empowered to confirm the bug, and didn't, they didn't have the info needed to confirm it. If the last commenter didn't have the ability to confirm the bug, they may have added info to be able to confirm it, so someone needs to look at the bug again, either to confirm it, or to note what information will be needed to be able to confirm it.

The report would give some ability to filter for which bugs are "ready for triage", without the noise of bugs that are waiting for information, without changing any workflows. It's basically a workaround, but it's an easy workaround until we get somewhere with a better triage workflow.

Comment 2

[:glob ✱]

this has been committed to bugzilla-stage-tip to gather feedback

Committing to: bzr+ssh://[e-mail]/bmo/4.0-dev/
modified extensions/BMO/Extension.pm
modified extensions/BMO/lib/Reports.pm
modified extensions/BMO/template/en/default/hook/reports/menu-end.html.tmpl
added extensions/BMO/template/en/default/pages/triage_last_commenter.html.tmpl
Committed revision 7754.

https://bugzilla-stage-tip.mozilla.org/report.cgi

Comment 3

[:glob ✱]

Attachment: patch v1

here's the patch to bring both the reports from stage-tip to the mainline branch.

Comment 4

[Tyler Downer [He/Him]]

What also be nice (you can kinda do it in a query, but since we are already making these changes maybe add it in), is instead of just "Last commenter is reporter or without canconfirm" be able to specify a custom Email address. For example, I run through bugs that have gone stale, put a comment requesting more information. They aren't CLOSEME ready, but I would like to go through to the ones that were never replied to, and see who didn't reply. maybe being able to run this by date as well.

So it would be like "Show UNCONFIRMED bugs in [Product/Component]  where the last commenter   [is reporter/without canconfirm/email address] and have not been changed since [date]"

Should also be able to filter down to the individual components

Comment 5

[:glob ✱]

(In reply to comment #4)
> So it would be like "Show UNCONFIRMED bugs in [Product/Component]  where the
> last commenter   [is reporter/without canconfirm/email address] and have not
> been changed since [date]"

by "have not been changed", do you mean across all fields, or do you mean "has not had a comment since [date]" ?

Comment 6

[Tyler Downer [He/Him]]

Sorry, yes I mean a comment. It is somewhat like your second report, just combining them. sometimes I want to look at both of the queries at the same time, makes for more control and and more powerful report

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 539551 [details] [diff] [review]
patch v1

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

Just a few changes needed. Looking good though.

::: extensions/BMO/lib/Reports.pm
@@ +345,5 @@
> +
> +    my @selectable_products = sort { lc($a->name) cmp lc($b->name) }
> +        @{$user->get_selectable_products};
> +    Bugzilla::Product::preload(\@selectable_products);
> +    $vars->{'products'} = \@selectable_products;

This is wholly unnecessary as you are using user.get_selectable_products directly in the templates.

@@ +348,5 @@
> +    Bugzilla::Product::preload(\@selectable_products);
> +    $vars->{'products'} = \@selectable_products;
> +
> +    if (Bugzilla->params->{'useclassification'}) {
> +        $vars->{'classifications'} = $user->get_selectable_classifications;

To be consistent with what you are doing in the templates with user.get_selectable_products, you could just call user.get_selectable_classifications in the templates as well.

@@ +353,5 @@
> +    }
> +
> +    my @products = @{Bugzilla->user->get_accessible_products()};
> +    @products = sort { lc($a->name) cmp lc($b->name) } @products;
> +    $vars->{'accessible_products'} = \@products;

I don't see that you are using this anywhere in the templates. Also this section is not needed anyway due to comment above.

@@ +394,5 @@
> +        my $attach_sql = "
> +            SELECT description
> +              FROM attachments
> +             WHERE attach_id = ?
> +        ";

You need to check for is_insider for private attachments similar to comments.

@@ +408,5 @@
> +
> +            if ($type == CMT_ATTACHMENT_CREATED) {
> +                ($comment) = $dbh->selectrow_array($attach_sql, undef, $extra);
> +                $comment = "(Attachment) " . $comment;
> +            }

If user is no insider and attachment is private, then skip this comment altogether.

@@ +422,5 @@
> +            $bug->{creation_ts} = $creation_ts;
> +            $bug->{commenter} = Bugzilla::User->new($commenter_id);
> +            $bug->{comment_ts} = $comment_ts;
> +            $bug->{comment} = $comment;
> +            $bug->{comment_count} = $comment_count;

Nit: nice if all lined up.

::: extensions/BMO/template/en/default/pages/triage_last_commenter.html.tmpl
@@ +19,5 @@
> +  #%]
> +
> +[% INCLUDE global/header.html.tmpl
> +  title = "UNCONFIRMED Last Commenter Reports"
> +  yui = [ 'autocomplete', 'calendar' ]

I don't see where either of these are needed in the templates.

@@ +40,5 @@
> +
> +#report-header {
> +  background: #dddddd;
> +}
> +</style>

Move this all to separate css file.

@@ +54,5 @@
> +Show UNCONFIRMED bugs in
> +<select name="product" id="product">
> +  <option value=""></option>
> +  [% IF Param('useclassification') %]
> +    [% FOREACH c = classifications %]

Use user.get_selectable_classifications instead.

@@ +129,5 @@
> +  </table>
> +
> +  <p>
> +    <a href="buglist.cgi?bug_id=
> +    [%- FOREACH bug = bugs %][% bug.id %],[% END -%]

FILTER url_quote

::: extensions/BMO/template/en/default/pages/triage_stale.html.tmpl
@@ +19,5 @@
> +  #%]
> +
> +[% INCLUDE global/header.html.tmpl
> +  title = "UNCONFIRMED Stale Reports"
> +  yui = [ 'autocomplete', 'calendar' ]

I don't see where either of these are needed in the templates.

@@ +40,5 @@
> +
> +#report-header {
> +  background: #dddddd;
> +}
> +</style>

Move this all to a separate css file.

@@ +54,5 @@
> +Show UNCONFIRMED bugs in
> +<select name="product" id="product">
> +  <option value=""></option>
> +  [% IF Param('useclassification') %]
> +    [% FOREACH c = classifications %]

Use user.get_selectable_classifications instead.

@@ +131,5 @@
> +  </table>
> +
> +  <p>
> +    <a href="buglist.cgi?bug_id=
> +    [%- FOREACH bug = bugs %][% bug.id %],[% END -%]

FILTER url_quote

Comment 8

[:glob ✱]

based on tyler's feedback i've merged the two reports into a single page:

https://bugzilla-stage-tip.mozilla.org/report.cgi

i'll attach a updated bmo/4.0 patch for this soon.

Comment 9

[:glob ✱]

Attachment: patch v2

Comment 10

[:glob ✱]

Attachment: patch v3

same as patch 2, but with the javascript in its own file.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 539776 [details] [diff] [review]
patch v2

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

Quick code review. Will test it out as well.

::: extensions/BMO/lib/Reports.pm
@@ +413,5 @@
> +                    } elsif ($filter_commenter_on eq 'is') {
> +                        next if $commenter_id != $filter_commenter_id;
> +                    }
> +                }
> +            if ($filter_last) {

Somehow the indentation went astray here. Fix on checkin.

@@ +428,5 @@
> +
> +            if ($type == CMT_ATTACHMENT_CREATED) {
> +                my ($description, $is_private) = $dbh->selectrow_array($attach_sql, undef, $extra);
> +                next if $is_private && !Bugzilla->user->is_insider;
> +                $comment = "(Attachment) " . $description;

This will overwrite the comment if the submitter had a comment along with the new attachment. Maybe you should prepend the attachment data to the actual comment instead? Similar to how this displays in show_bug.cgi.

@@ +447,5 @@
> +            $bug->{creation_ts} = $creation_ts;
> +            $bug->{commenter} = $commenter || Bugzilla::User->new($commenter_id);
> +            $bug->{comment_ts} = $comment_ts;
> +            $bug->{comment} = $comment;
> +            $bug->{comment_count} = $comment_count;

Nit: still would be nice if these all lined up. Can do on checkin.

@@ +451,5 @@
> +            $bug->{comment_count} = $comment_count;
> +            push @bugs, $bug;
> +        }
> +
> +        @bugs = sort { $b->{comment_ts} cmp $a->{comment_ts} } @bugs;

Never sorted on date strings personally. Does this give the results you expect?

::: extensions/BMO/template/en/default/pages/triage_reports.html.tmpl
@@ +19,5 @@
> +  #%]
> +
> +[% PROCESS global/variables.none.tmpl %]
> +
> +[% js_data = BLOCK %]

As discussed in IRC, move most of this to external js file.

@@ +28,5 @@
> +var last_sel = [];
> +var cpts = new Array();
> +[% n = 1 %]
> +[% FOREACH p = user.get_selectable_products %]
> +  cpts['[% n %]'] = [

n FILTER js to satisfy the test suite.

@@ +115,5 @@
> +  yui = [ 'autocomplete', 'calendar' ]
> +  javascript = js_data
> +  javascript_urls = [ "js/util.js", "js/field.js", "js/productform.js" ]
> +  style_urls = [ "skins/standard/buglist.css",
> +    "extensions/BMO/web/styles/triage_reports.css" ]

Nit: line them up.

@@ +125,5 @@
> +
> +[% PROCESS "global/field-descs.none.tmpl" %]
> +
> +<form id="activity_form" name="activity_form" action="page.cgi" method="get"
> +  onSubmit="return onGenerateReport()">

Nit: line up with id=

@@ +153,5 @@
> +    [% 'checked' IF input.filter_commenter %]>
> +    <label for="filter_commenter">where the last commenter</label>
> +    <select name="commenter" id="commenter" onChange="onCommenterChange()">
> +      <option value="reporter" [% 'selected' IF input.commenter == 'reporter' %]>is the reporter</option>
> +      <option value="canconfirm" [% 'selected' IF input.commenter == 'canconfirm' %]>does not have canconfirm</option>

Nit: Should this be nocanconfirm to be more indicative of what it means? Otherwise it var seems like it means "does have canconfirm".

@@ +181,5 @@
> +      <input type="text" id="last_is" name="last_is" size="11" maxlength="10"
> +        value="[% input.last_is FILTER html %]"
> +        onChange="updateCalendarLastIs(this)">
> +      <button type="button" class="calendar_button" id="button_calendar_last_is"
> +        onClick="showCalendar('last_is')"><span>Calendar</span>

Nit: line up with type=

Comment 12

[:glob ✱]

(In reply to comment #11)
> This will overwrite the comment if the submitter had a comment along with
> the new attachment. Maybe you should prepend the attachment data to the
> actual comment instead? Similar to how this displays in show_bug.cgi.

oops, i'll just make it use the attachment description only if the comment is empty.  concatenating both is probably useless given it'll be truncated to 80 chars.

> > +        @bugs = sort { $b->{comment_ts} cmp $a->{comment_ts} } @bugs;
> 
> Never sorted on date strings personally. Does this give the results you
> expect?

yes, dates are yyyy-mm-dd, so sort correctly as a string.

> > +    [% 'checked' IF input.filter_commenter %]>
> > +    <label for="filter_commenter">where the last commenter</label>
> > +    <select name="commenter" id="commenter" onChange="onCommenterChange()">
> > +      <option value="reporter" [% 'selected' IF input.commenter == 'reporter' %]>is the reporter</option>
> > +      <option value="canconfirm" [% 'selected' IF input.commenter == 'canconfirm' %]>does not have canconfirm</option>
> 
> Nit: Should this be nocanconfirm to be more indicative of what it means?
> Otherwise it var seems like it means "does have canconfirm".

i'd agree with you if it was a visible value :)

Comment 13

[David Lawrence [:dkl]]

UI review:

When selecting "where the last comment is older than [the date]" and leaving the date field empty, it should give an alert for that field before allowing to submit. Seems that if I select "the date" it should require a date.

Some reason the even/odd row coloring is not working for me. Looking in Firebug, it looks like bz_row_even/bz_row_odd is conflicting with the same in buglist.css.

Nit: Generate Report is in a strange location. Maybe cleaner just to break it out of the table and have it display in the normal page flow beneath Components?

Also maybe have the Comment: be the end of the first row and move the inputs down to the second row below Comment:. For example:

Comment:
[ ] where the last commenter ...
[ ] where the last comment is older than ...

All else looks nice.

dkl

Comment 14

[:glob ✱]

(In reply to comment #13)
> Some reason the even/odd row coloring is not working for me. Looking in
> Firebug, it looks like bz_row_even/bz_row_odd is conflicting with the same
> in buglist.css.

i'm unable to replicate this; do you have a sample url?

i'm using the same classnames from buglist.css to match the styling of a normal bug list.

Comment 15

[David Lawrence [:dkl]]

(In reply to comment #14)
> (In reply to comment #13)
> > Some reason the even/odd row coloring is not working for me. Looking in
> > Firebug, it looks like bz_row_even/bz_row_odd is conflicting with the same
> > in buglist.css.
> 
> i'm unable to replicate this; do you have a sample url?
> 
> i'm using the same classnames from buglist.css to match the styling of a
> normal bug list.

My bad. If I hold my head just right, I can barely make out the different colored rows. It is the same for buglist.cgi as well so this report is not at fault. Something screwy with my monitor. 

My other comments still apply and then I will r+

dkl

Comment 16

[:glob ✱]

Attachment: patch v4

this should address all the review points.

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 540401 [details] [diff] [review]
patch v4

Looks good. r=dkl

Comment 18

[:glob ✱]

thanks dkl :)

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/BMO/Extension.pm
modified extensions/BMO/lib/Reports.pm
modified extensions/BMO/template/en/default/hook/reports/menu-end.html.tmpl
added extensions/BMO/template/en/default/pages/triage_reports.html.tmpl
added extensions/BMO/web/js/triage_reports.js
added extensions/BMO/web/styles/triage_reports.css
Committed revision 7770.