714786 - The FlagTypeComment extension should allow more than one flag to have have comment text associated with it

Status: RESOLVED FIXED

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

Description

[David Lawrence [:dkl]]

Originally discovered in bug 713307:

(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 1

[David Lawrence [:dkl]]

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

Here is a patch that allows more than one flag on a bug or attachment to have flag type comment text.

dkl

Comment 2

[:glob ✱]

Comment on attachment 585408 [details] [diff] [review]
Patch to allow more that one flag to have flag type comment text (v1)

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

gah, i can't believe i missed this during development; thanks for catching and fixing this :)

this looks good, however now when editing some flags (try editflagtypes.cgi?action=edit&id=5) the text areas are no longer present.

::: extensions/FlagTypeComment/Extension.pm
@@ +163,2 @@
>      foreach my $state (FLAGTYPE_COMMENT_STATES) {
> +        my $text = $input->{"ftc_$flagtype_id" . "_$state"} || '';

nit: "ftc_${flagtype_id}_$state"

::: extensions/FlagTypeComment/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
@@ +26,4 @@
>      <td>add text into the comment box when flag is changed to a state</td>
>    </tr>
>  
> +  [% FOREACH type_id = ftc_flags.keys %]

should probably sort the keys here to ensure they are always displayed in the same order.

Comment 3

[David Lawrence [:dkl]]

Attachment: Path to allow more that one flag to have flag type comment text (v2)

Thanks. New patch with suggested fixes.

dkl

Comment 4

[:glob ✱]

Comment on attachment 585885 [details] [diff] [review]
Path to allow more that one flag to have flag type comment text (v2)

r=glob

Comment 5

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.0
modified extensions/FlagTypeComment/Extension.pm
modified extensions/FlagTypeComment/template/en/default/flag/type_comment.html.tmpl
modified extensions/FlagTypeComment/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
Committed revision 8003.