713307 - Please add FlagTypeComments for tracking/approval flags

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Alex Keybl [:akeybl]]

If approval-mozilla-aurora, approval-mozilla-beta, approval-mozilla-release are set to ?, please prepend the following comment to the comment box (minus quotation marks):

"[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): "


If approval-mozilla-aurora, approval-mozilla-beta, approval-mozilla-release are set to + or -, please prepend the following comment to the comment box (minus quotation marks):

"[Triage Comment]"


If tracking-firefox9, tracking-firefox10, tracking-firefox11, or tracking-firefox12 are set to ?, please prepend the following comment to the comment box (minus quotation marks): 

"[Tracking Request Comment]
User impact and significance if left unresolved: "

Comment 1

[David Lawrence [:dkl]]

(In reply to Alex Keybl [:akeybl] from comment #0)
> If approval-mozilla-aurora, approval-mozilla-beta, approval-mozilla-release
> are set to ?, please prepend the following comment to the comment box (minus
> quotation marks):
> 
> "[Approval Request Comment]
> Regression caused by (bug #): 
> User impact if declined: 
> Testing completed (on m-c, etc.): 
> Risk to taking this patch (and alternatives if risky): "
> 
> 
> If approval-mozilla-aurora, approval-mozilla-beta, approval-mozilla-release
> are set to + or -, please prepend the following comment to the comment box
> (minus quotation marks):
> 
> "[Triage Comment]"

Done

> If tracking-firefox9, tracking-firefox10, tracking-firefox11, or
> tracking-firefox12 are set to ?, please prepend the following comment to the
> comment box (minus quotation marks): 
> 
> "[Tracking Request Comment]
> User impact and significance if left unresolved: "

Hmm, these are custom fields in Bugzilla and not normal flags like approval-* as above. So the new extension has no affect on those fields. Please create a separate new bug that asks the ability to do the same commenting with custom tracking fields.

dkl

Comment 2

[Alex Keybl [:akeybl]]

(In reply to David Lawrence [:dkl] from comment #1)
> (In reply to Alex Keybl [:akeybl] from comment #0)
> > If approval-mozilla-aurora, approval-mozilla-beta, approval-mozilla-release
> > are set to ?, please prepend the following comment to the comment box (minus
> > quotation marks):
> > 
> > "[Approval Request Comment]
> > Regression caused by (bug #): 
> > User impact if declined: 
> > Testing completed (on m-c, etc.): 
> > Risk to taking this patch (and alternatives if risky): "
> > 
> > 
> > If approval-mozilla-aurora, approval-mozilla-beta, approval-mozilla-release
> > are set to + or -, please prepend the following comment to the comment box
> > (minus quotation marks):
> > 
> > "[Triage Comment]"
> 
> Done

approval‑mozilla‑beta?/+ is working great but I'm having issues with approval-mozilla-aurora?/+ and approval-mozilla-release?/+ where the comment does not appear to be added to the text box. Were those set too?

> > If tracking-firefox9, tracking-firefox10, tracking-firefox11, or
> > tracking-firefox12 are set to ?, please prepend the following comment to the
> > comment box (minus quotation marks): 
> > 
> > "[Tracking Request Comment]
> > User impact and significance if left unresolved: "
> 
> Hmm, these are custom fields in Bugzilla and not normal flags like
> approval-* as above. So the new extension has no affect on those fields.
> Please create a separate new bug that asks the ability to do the same
> commenting with custom tracking fields.

Thanks! Will do.

Comment 3

[David Lawrence [:dkl]]

(In reply to Alex Keybl [:akeybl] from comment #2)
> approval‑mozilla‑beta?/+ is working great but I'm having issues with
> approval-mozilla-aurora?/+ and approval-mozilla-release?/+ where the comment
> does not appear to be added to the text box. Were those set too?

Hmm, there is a bug in the extension code where it will only work if one flag in a
list of flags for an attachment has any comment pretext associated with it. In this
case we have three flags set up so only one of the three is working properly (approval-mozilla-beta). I have a patch to fix this that I will post for review and we will try
to get this fixed as soon as possible.

dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to allow more that one flag to have flag type comment text (v1)

Patch that allows more than one flag at a time to have flagtype comments associated with them.

dkl

Comment 5

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #4)
> Created attachment 584468 [details] [diff] [review]
> Patch to allow more that one flag to have flag type comment text (v1)

can you please file a new bug to track this code change.

Comment 6

[:glob ✱]

Attachment: 713307_1.patch

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 802127 [details] [diff] [review]
713307_1.patch

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

Slight bit-rot (needs to go instead in bug/tracking_flags.html.tmpl):

patching file extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
Hunk #1 FAILED at 136.
1 out of 1 hunk FAILED -- saving rejects to file extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl.rej

When editing the comment text for each value, I have to hide the comment in order to have the text saved for the value. If I do not hide the related textarea, it does not save. IMO, if you move focus out of the textarea (i.e. leave the textarea open but with text included) then it should then save the value to flag_values[idx].comment. Also if you hit "Save" before unfocusing, it should also accept the new value as well.

::: extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ +138,5 @@
>        TrackingFlags.flags['tracking']['[% flag.name FILTER js %]'] = '[% flag.bug_flag.value FILTER js %]';
> +      [% FOREACH value = flag.values %]
> +        [% IF value.comment %]
> +          YAHOO.util.Event.addListener(
> +            '[% flag.name FILTER js %]', 'change', tracking_flag_change, '[% value.comment FILTER js %]');

This did not work for me as if you have more than one comment for different values of the same flag name, you get multiple even fires on a single change.

For example I had different text for leo? and leo+ for the cf_blocking_b2g flag:

      YAHOO.util.Event.addListener(
        'cf_blocking_b2g', 'change', tracking_flag_change, 'leo? comment:');
      YAHOO.util.Event.addListener(
        'cf_blocking_b2g', 'change', tracking_flag_change, 'leo+ comment:');

Maybe TrackingFlags.flags['tracking']['[% flag.name FILTER js %]'] should be an object with { value: value, comment: comment } instead of a simple string for 'value'. Then tracking_flag_change can simply look up the comment given the flag name and value name.

Comment 9

[:glob ✱]

Attachment: 713307_2.patch

- changes the column type of 'comments' to TEXT
- moves construction of the TrackingFlags js object from template iteration to perl
- fixes saving of the comment on the admin ui
- fixes setting the comment on show_bug

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8418599 [details] [diff] [review]
713307_2.patch

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

Looks/works good but with couple small changes:

1. Focus comment field when editing a comment for a value
2. Either make this work for entering a new bug (preferrable) or check for newbug == 1 before adding onchange to remove error from console

-                onchange="tracking_flag_change(this)">
+                [% IF !new_bug %]onchange="tracking_flag_change(this)"[% END %]>

::: template/en/default/bug/edit.html.tmpl
@@ +90,5 @@
>    [%- END %]
>  [% END %]
> +
> +[%# BMO - hook for trackingflags %]
> +[% Hook.process("inline_javascript") %]

Seems like a nice one to go upstream as well

Comment 11

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #10)
> > +[%# BMO - hook for trackingflags %]
> > +[% Hook.process("inline_javascript") %]
> 
> Seems like a nice one to go upstream as well

i'll file an upstream bug for that, but i don't think it's worth blocking this work on it.

Comment 12

[:glob ✱]

Attachment: 713307_3.patch

- now working on the new_bug form
- removed the custom inline_javascript hook, now using global/header

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 8446383 [details] [diff] [review]
713307_3.patch

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

I get the following console error when creating a new bug or processing a current bug:

ReferenceError: TrackingFlags is not defined

So when I select a value, it will not populate the comment field with the proper text. Works if I reload the page and show_bug.cgi from scratch.
header-start.html.tmpl and Extension.pm need to also load for the templates used when a bug id created or edited.

Admin UI changes worked nicely.

Comment 14

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #13)
> I get the following console error when creating a new bug or processing a
> current bug:
> 
> ReferenceError: TrackingFlags is not defined

odd; i can't reproduce this issue.
does this happen on all products?

> Works if I reload the page and show_bug.cgi from scratch.

i'm not sure what you mean by this - specifically how were you loading the page if not "from scratch"?

Comment 15

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #14)
> i'm not sure what you mean by this - specifically how were you loading the
> page if not "from scratch"?

post_bug.cgi and process_bug.cgi is when i get the js error. Those both have their urls rewritten after completion to show_bug.cgi. So "from scratch" meaning if I just load the bug using show_bug.cgi.

dkl

Comment 16

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #15)
> post_bug.cgi and process_bug.cgi is when i get the js error.

ah, gotcha :)

Comment 17

[:glob ✱]

Attachment: 713307_4.patch

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 8448568 [details] [diff] [review]
713307_4.patch

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

r=dkl with latest fix

Comment 19

[:glob ✱]

schema-only:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   4b300bd..aa7f123  master -> master

Comment 20

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   235190a..bff9f8e  master -> master