Closed Bug 713307 Opened 8 years ago Closed 5 years ago

Please add FlagTypeComments for tracking/approval flags

Categories

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

Production
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: akeybl, Assigned: glob)

References

Details

Attachments

(1 file, 4 obsolete files)

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: "
(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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
Patch that allows more than one flag at a time to have flagtype comments associated with them.

dkl
Assignee: nobody → dkl
Status: REOPENED → ASSIGNED
Attachment #584468 - Flags: review?(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.
Attachment #584468 - Attachment is obsolete: true
Attachment #584468 - Flags: review?(glob)
Depends on: 714786
Assignee: dkl → glob
Attached patch 713307_1.patch (obsolete) — Splinter Review
Attachment #802127 - Flags: review?(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.
Attachment #802127 - Flags: review?(dkl) → review-
Blocks: 853108
Duplicate of this bug: 853333
Attached patch 713307_2.patch (obsolete) — Splinter Review
- 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
Attachment #802127 - Attachment is obsolete: true
Attachment #8418599 - Flags: review?(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
Attachment #8418599 - Flags: review?(dkl) → review-
(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.
Attached patch 713307_3.patch (obsolete) — Splinter Review
- now working on the new_bug form
- removed the custom inline_javascript hook, now using global/header
Attachment #8418599 - Attachment is obsolete: true
Attachment #8446383 - Flags: review?(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.
Attachment #8446383 - Flags: review?(dkl) → review-
(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"?
(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
(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 :)
Attached patch 713307_4.patchSplinter Review
Attachment #8446383 - Attachment is obsolete: true
Attachment #8448568 - Flags: review?(dkl)
Comment on attachment 8448568 [details] [diff] [review]
713307_4.patch

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

r=dkl with latest fix
Attachment #8448568 - Flags: review?(dkl) → review+
schema-only:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   4b300bd..aa7f123  master -> master
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   235190a..bff9f8e  master -> master
Status: ASSIGNED → RESOLVED
Closed: 8 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.