778731 - Add email flag to NEEDINFO state

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: User Interface, enhancement)

Description

[Andrew Overholt [:overholt]]

In Red Hat's bugzilla instance, there is the ability to add an email address indicating from whom more info is needed.  This would help us triage, for example, B2G bugs where we're using [blocked-on-input] in the whiteboard but it's a bit messy to track who needs to provide said input.

dkl and I discussed this via email and he said it's likely possible to do this via an extension if not a part of core Bugzilla.

I've CC'd a few people who were around when I mentioned this.  I think they liked the idea so their +1s could help justify this :)

Comment 2

[:glob ✱]

oh, sorry, an email flag.
/me drinks more coffee

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch creating Needinfo extension (v1)

I had this patch done a bit ago but didnt get a chance to attach it for review til now. Please take a look and see if you can see any glaring issues so far. It is fairly complete and functional based on what was in place when I was at Red Hat, but could use some UI clean up possibly.

dkl

Comment 4

[:glob ✱]

Comment on attachment 659869 [details] [diff] [review]
Patch creating Needinfo extension (v1)

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

::: extensions/Needinfo/Extension.pm
@@ +43,5 @@
> +    $dbh->do("INSERT INTO flagexclusions
> +              (type_id, product_id, component_id)
> +              VALUES (?, NULL, NULL)", undef, $id);
> +
> +    $dbh->bz_commit_transaction();

please use the objects to create, instead of hitting the db directly.

@@ +89,5 @@
> +                if $needinfo_role eq 'reporter';
> +
> +            # Use qa_contact as requestee
> +            $needinfo_flag->{requestee} = $bug->qa_contact->login
> +                if $needinfo_role eq 'qa_contact';

this would read better as an if/elsif/elsif/... block

@@ +138,5 @@
> +    }
> +
> +    if (@flags || @new_flags) {
> +        $bug->set_flags(\@flags, \@new_flags);
> +        $bug->update($timestamp);

calling update from within this hook results in duplicating any comment made at the same time the flag is cleared.

ie. if you set a targetted needinfo flag, then comment as the requestee, your comment is added to the bug twice.

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +32,5 @@
> +  [% IF needinfo_requested %]
> +    <script>
> +      var summary_container = document.getElementById('summary_alias_container');
> +      var needinfo_text = document.createTextNode(' [NEEDINFO]');
> +      summary_container.insertBefore(needinfo_text, summary_container.childNodes[0]);

i think a better place for this would be after the status..
  Status: ASSIGNED (edit) [NEEDINFO]

@@ +40,5 @@
> +  <div id="needinfo_container">
> +    [% IF needinfo_requested %]
> +      [% IF needinfo_from == user.login || needinfo_from_any %]
> +        Additional information was requested for this [% terms.bug %].
> +        Adding comment will automatically clear needinfo request from you[% " or from anyone " IF needinfo_from_any %].

i'm not sold on the value of either of these sentences.

there's already another two indicators on the bug that needinfo has been set, a third isn't required.

i don't think the second sentence serves any purpose; if the flag is going to be automatically cleared there's no point telling the user, because there isn't any action they can take aside from not commenting which will avoid that.  as it stands it's confusing.

@@ +44,5 @@
> +        Adding comment will automatically clear needinfo request from you[% " or from anyone " IF needinfo_from_any %].
> +      [% ELSE %]
> +        <input type="checkbox" id="needinfo_override" name="needinfo_override" value="1">
> +        <label for="needinfo_override">
> +          I am providing the requested information for this [% terms.bug %] (Will clear needinfo request).

s/Will clear/this will clear/

@@ +68,5 @@
> +                  name     => "needinfo_from"
> +                  size     => 30
> +                  value    => ""
> +                  onchange => "document.changeform.needinfo.checked=true;"
> +      %]

i was a bit confused initially by the current presentation.

it would be clearer if you made 'other' an option in the needinfo_role select, which controls the visibility of the needinfo_from userselect.

Comment 5

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #4)
> ::: extensions/Needinfo/Extension.pm
> @@ +43,5 @@
> > +    $dbh->do("INSERT INTO flagexclusions
> > +              (type_id, product_id, component_id)
> > +              VALUES (?, NULL, NULL)", undef, $id);
> > +
> > +    $dbh->bz_commit_transaction();
> 
> please use the objects to create, instead of hitting the db directly.

The create() methods for FlagType were added in 4.2 and do not exist for 4.0. I will do this as a 4.2+ only extension.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch creating Needinfo extension (ver4.2) (v2)

Comment 7

[:glob ✱]

Comment on attachment 660996 [details] [diff] [review]
Patch creating Needinfo extension (ver4.2) (v2)

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

note: even though the setup is 4.2 specific, we can still deploy to 4.0 by creating manually the flag prior to deployment.

what are your thoughts on limiting the 'need info' blurb near the comment textarea to users with canconfirm or higher?

::: extensions/Needinfo/Extension.pm
@@ +140,5 @@
> +
> +        $bug->set_flags(\@flags, \@new_flags);
> +        $bug->update($timestamp);
> +
> +        $bug->{added_comments} = $added_comments;

this is a hack, and has a high possibility to cause problems -- we shouldn't be calling update() from withing this hook, as it's called from update().

an example of this problem if you use the normal flags ui to clear a flag, you end up with two entries in the bugs_activity table:
  Flags: needinfo?([e-mail]) → needinfo-
  Flags: needinfo-

you should do something like this instead:

if (@flags || @new_flags) {
    $bug->set_flags(\@flags, \@new_flags);
    my ($removed, $added) = Bugzilla::Flag->update_flags($bug, $old_bug, $timestamp);
    if ($removed || $added) {
        $changes->{'flagtypes.name'} = [$removed, $added]; 
    }
}

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +52,5 @@
> +      <script>
> +        function needinfoRole (select) {
> +          YAHOO.util.Dom.get('needinfo').checked = true;
> +          if (select.value == 'other') {
> +            YAHOO.util.Dom.removeClass('needinfo_from_container', 'bz_default_hidden');

it would be nice to focus() needinfo_from_container here.

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch creating Needinfo extension (v3)

1. Updated to required canconfirm permissions to show the needinfo UI. We can also update the flags request group to canconfirm as well using editflagtypes.cgi.
2. Use .focus() when selecting 'other' from the needinfo role drop down.
3. Updated backend code to properly set the flags without adding duplicate changes. Had to call LogActivityEntry explicitly as bug_end_of_update comes after in Bug.pm. Also did some extra work to properly format $changes if the user had also set other flags before needinfo.

dkl

Comment 9

[:glob ✱]

Comment on attachment 661686 [details] [diff] [review]
Patch creating Needinfo extension (v3)

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

r=glob

::: extensions/Needinfo/Extension.pm
@@ +142,5 @@
> +        my ($removed, $added) = Bugzilla::Flag->update_flags($bug, $old_bug, $timestamp);
> +        if ($removed || $added) {
> +            my $field = 'flagtypes.name';
> +            LogActivityEntry($bug->id, $field, $removed, $added, $user->id, $timestamp);
> +            $changes->{$field} = [$removed, $added];

the code in Bug.pm changes $removed and $added from undef to '', so we should do the same here.

Comment 10

[David Lawrence [:dkl]]

Thanks

Committing to: bzr+ssh://[e-mail]/bmo/4.0
added extensions/Needinfo
added extensions/Needinfo/Config.pm
added extensions/Needinfo/Extension.pm
added extensions/Needinfo/template
added extensions/Needinfo/template/en
added extensions/Needinfo/template/en/default
added extensions/Needinfo/template/en/default/bug
added extensions/Needinfo/template/en/default/hook
added extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
added extensions/Needinfo/template/en/default/hook/attachment
added extensions/Needinfo/template/en/default/hook/bug
added extensions/Needinfo/template/en/default/hook/attachment/create-form_before_submit.html.tmpl
added extensions/Needinfo/template/en/default/hook/attachment/edit-after_comment_textarea.html.tmpl
added extensions/Needinfo/template/en/default/hook/bug/edit-after_comment_textarea.html.tmpl
Committed revision 8330.

Committing to: bzr+ssh://[e-mail]/bmo/4.2
added extensions/Needinfo
added extensions/Needinfo/Config.pm
added extensions/Needinfo/Extension.pm
added extensions/Needinfo/template
added extensions/Needinfo/template/en
added extensions/Needinfo/template/en/default
added extensions/Needinfo/template/en/default/bug
added extensions/Needinfo/template/en/default/hook
added extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
added extensions/Needinfo/template/en/default/hook/attachment
added extensions/Needinfo/template/en/default/hook/bug
added extensions/Needinfo/template/en/default/hook/attachment/create-form_before_submit.html.tmpl
added extensions/Needinfo/template/en/default/hook/attachment/edit-after_comment_textarea.html.tmpl
added extensions/Needinfo/template/en/default/hook/bug/edit-after_comment_textarea.html.tmpl
Committed revision 8349.

Comment 11

[Andre Klapper]

First of all: Awesome. Thanks so much.

Two questions:

If I set this below the comment box, I'll get a "needinfo?" flag in the Flags section on show_bug.cgi. But what is the semantic meaning of (and difference between) needinfo+ or needinfo- then?
And can I set it again for a followup needinfo after (insufficient) info has been provided and the needinfo flag status has changed from needinfo? to something else?

I'd test this but I don't want to create bugmail, and it's not available on landfill 4-4.

Comment 12

[:glob ✱]

(In reply to Andre Klapper from comment #11)
> If I set this below the comment box, I'll get a "needinfo?" flag in the
> Flags section on show_bug.cgi. But what is the semantic meaning of (and
> difference between) needinfo+ or needinfo- then?

if you try to set needinfo+ or needinfo-, the needinfo flag will be cleared instead.

> And can I set it again for a followup needinfo after (insufficient) info has
> been provided and the needinfo flag status has changed from needinfo? to
> something else?

yes.

> I'd test this but I don't want to create bugmail, and it's not available on
> landfill 4-4.

bmo's playpen environment is http://bugzilla-stage-tip.mozilla.org/