Add email flag to NEEDINFO state

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: overholt, Assigned: dkl)

Tracking

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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 :)
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 478467
oh, sorry, an email flag.
/me drinks more coffee
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 3

7 years ago
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
Assignee: nobody → dkl
Status: REOPENED → ASSIGNED
Attachment #659869 - Flags: review?(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.
Attachment #659869 - Flags: review?(glob) → review-
(Assignee)

Comment 5

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

Comment 6

7 years ago
Attachment #659869 - Attachment is obsolete: true
Attachment #660996 - Flags: review?(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?(byron.jones@gmail.com) → 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.
Attachment #660996 - Flags: review?(glob) → review-
(Assignee)

Comment 8

7 years ago
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
Attachment #660996 - Attachment is obsolete: true
Attachment #661686 - Flags: review?(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.
Attachment #661686 - Flags: review?(glob) → review+
(Assignee)

Comment 10

7 years ago
Thanks

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/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://dlawrence%40mozilla.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 11

7 years ago
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.
(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/
Duplicate of this bug: 478467
You need to log in before you can comment on or make changes to this bug.