695698 - Roll out FlagTypeComment extension to bmo

Status: RESOLVED FIXED

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

Description

[Alex Keybl [:akeybl]]

I've created a bz extension that provides the ability to set per-flag text which is subsequently prepended to the comment box when the specified flag state is selected (see attached screenshots for a visual explanation).

The pages that (sometimes) have javascript inserted into the footer are editflagtypes.cgi, show_bug.cgi, and attachment.cgi. A new table "flagtypecomments" is also created in the DB.

This extension would allow Release Management to request specific info when an approval request is made, but the ability is generic enough to be applicable for other flags on bmo.

I've tested this extension on a local instance of Bugzilla 4.0.2. This bug is intended to cover any additional testing/review that would need to occur in order to roll out this extension. The source is available https://github.com/akeybl/FlagTypeComment.

Comment 1

[Alex Keybl [:akeybl]]

Attachment: editflagtypes screenshot

Comment 2

[Alex Keybl [:akeybl]]

Attachment: show_bug screenshot

Comment 3

[:glob ✱]

interesting :)

are you able to provide a specific use-case for this extension?  (ie. which flags, and what text would you provide).

Comment 4

[Alex Keybl [:akeybl]]

Sure thing - here's one use case


* An engineer wants to request that we take a patch on our aurora or beta channel, instead of riding the normal m-c->aurora->beta train
* They would add the approval-mozilla-{aurora,beta} to the attachment with a state of '?'
* At this point the bug would come up during daily triage, and we many times have to ask additional questions of the requester, thus delaying a decision by at least a day
* If instead a multiline comment is prepended to the comment box including requested info like:

"Regression from: 
User impact if declined: 
Risk to patch: 
Testing completed: "

we could get a lot of the info that we needed at the time of request.
* At this point a release driver would approve or deny, and we could auto-add comments like (in this case a -/denial)

"Reasoning behind declined request: 
Expected shipping version: "


This extension is meant to make the request/approval/denial process much cleaner and more precise.

Comment 5

[LegNeato]

Yep, I think we want this...it shouldn't need a security review as it is so trivial, right?

Comment 6

[:glob ✱]

this sounds very reasonable, let's get the ball rolling on this.

i'm extremely delighted that your proposal is bundled with code!  thanks :)


a quick visual review of the code follows (will do a more thorough review later).
don't hesitate to grab me on irc if you need further clarification or help.
if you prefer, i'm happy to tidy up your code for you.


your extension needs a Config.pm file.  there are some parts of bugzilla that will not work correctly if this file doesn't exist.

to inject the javascript you should save it in a file under the extension's web/ directory, and use the 'end' hook in bug/show-header to add the url to the javascript_urls array.  doing it this way has a few advantages: the browser can cache the javascript, it's loaded as part of the head block, and your code will only run on show_bug pages, rather than on every page. 

> my(@attachtemplates) = ("attachment/edit.html.tmpl");
you could use perl's constants for these:
> use constant ATTACH_TEMPLATES => ['attachment/edit.html.tmpl']

> my $db_base = "SELECT * FROM flagtypecomments WHERE ";
please don't do "select *" :)

> my $db_query = "SELECT comment_prepend FROM flagtypecomments WHERE flagtype=" . $dbh->quote($type->id) . " AND on_status=" . $dbh->quote($state);
> my $text = $dbh->selectrow_array( "$db_query", {Slice=>{}} );
use sql placeholders rather than dbh->quote..
> my $text = $dbh->selectrow_array(
>     "SELECT comment_prepend FROM flagtypecomments WHERE flagtype=? AND on_status=?",
>     undef,
>     $type->id, $state);

> my $sth = $dbh->prepare("UPDATE flagtypecomments SET comment_prepend = ? WHERE flagtype=" . $dbh->quote($tid) . " AND on_status=" . $dbh->quote($state)
> $sth->bind_param_array(1, $text);
> $sth->execute_array(undef) or die $sth->errstr;
> $sth->finish();
this can be simplified to:
> $dbh->do(
>     "UPDATE flagtypecomments SET comment_prepend=? WHERE flagtype=? AND on_status=?",
>     undef,
>     $text, $tid, $state);
bugzilla will automatically catch and deal with db errors.

> var selects = document.querySelectorAll( 'select[id^=flag_type-]' );
> selects[i].addEventListener('change',onFlagChange,false);
you're using javascript which won't work on older IE :(
the YUI lib should be of some assistance here.

> [% notfirst = 0 %]
> [% FOREACH ft_comment = flag_type_comments %]
> [% IF notfirst == 1 %] ...
> ...
> [% IF notfirst == 1 %]
Template-Toolkit has a built in variable for tracking first/last iterations:
> [% FOREACH ft_comment = flag_type_comments %]
>   [% IF loop.first %] ...

> [% i = 0 %]
> [% FOREACH state = states %]
>   var row = document.getElementById("form").insertRow(12 + [% i %]);
likewise there's a counter, loop.index
> [% FOREACH state = states %]
>   var row = document.getElementById("form").insertRow(12 + [% loop.index %]);

> var text = 'for [% state %]</br><textarea id="ftc_text_[% i %]" name="ftc_text_[% i %]" cols="50" rows="2">[% texts.${state} %]</textarea>';
bugzilla isn't xhtml, <br> not </br>

with your template you _always_ need to FILTER data when it's being output..
for example:
> prependComment("[% ft_comment.comment_prepend %]");
this will break if comment_prepend contains a quote
> prependComment('[% ft_comment.comment_prepend FILTER js %]');




> Yep, I think we want this...it shouldn't need a security review as it is so trivial, right?
i don't think a sec review is required.

Comment 7

[:glob ✱]

Attachment: patch v1

after a quick chat with alex on irc i'll be finishing up the extension.
(time passes)
and here it is.

i've simplified the extension by adding in hooks to flagtypes.  we can't use the normal object_* hooks because, while Bugzilla::FlagTypes subclasses Bugzilla::Object, the admin edit cgi updates the database directly.  when that is fixed, these hooks can be removed.

this implementation works on both bug and attachment flags; however it would be excellent to avoid the extra db work when viewing every bug -- alex: do you have any objections if i change this to only work on attachment flags?

dkl, i don't need a review on this just yet as this may need to be updated based on alex's response.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 572445 [details] [diff] [review]
patch v1

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

Move extensions/FlagTypeComment/template/en/default/flag_type_comment.html.tmpl => 
extensions/FlagTypeComment/template/en/default/flag/type_comment.html.tmpl

Then reference that from extensions/FlagTypeComment/template/en/default/hook/attachment/create-end.html.tmpl,
extensions/FlagTypeComment/template/en/default/hook/attachment/edit-end.html.tmpl, and
extensions/FlagTypeComment/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl

I don't see anywhere in this patch to add the template hook for:
extensions/FlagTypeComment/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
since it doesn't seem to exist already in bmo/4.0.

::: extensions/FlagTypeComment/Extension.pm
@@ +34,5 @@
> +    "bug/comments.html.tmpl",
> +);
> +use constant STATES => (
> +    "?", "+", "-"
> +);

Nit: extensions/FlagTypeComment/lib/Constants.pm

@@ +44,5 @@
> +sub db_schema_abstract_schema {
> +    my ($self, $args) = @_;
> +    $args->{'schema'}->{'flagtype_comments'} = {
> +        FIELDS => [
> +            flagtype => {

type_id to be consistent.

@@ +63,5 @@
> +                NOTNULL => 1
> +            }, 
> +        ],
> +        INDEXES => [
> +            flagtype_comments_idx => ['flagtype'],

flagtype_comments_idx => ['type_id'],

@@ +90,5 @@
> +    my $db_states;
> +    if ($file =~ /^admin\//) {
> +        # admin
> +        $db_states = $dbh->selectall_hashref(
> +            "SELECT flagtype,on_status AS state,comment_prepend AS text

Nit. Add spaces after commas.

::: extensions/FlagTypeComment/template/en/default/flag_type_comment.html.tmpl
@@ +27,5 @@
> +    for (var i = 0; i < selects.length; i++) {
> +      YAHOO.util.Event.on(selects[i], 'change', ftc_on_change);
> +    }
> +  }
> +  ftc_init();

YAHOO.util.Event.onDOMReady(ftc_init);

Comment 9

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #8)
> I don't see anywhere in this patch to add the template hook for:
> extensions/FlagTypeComment/template/en/default/hook/admin/flag-type/edit-
> rows.html.tmpl
> since it doesn't seem to exist already in bmo/4.0.

the hook is in template/en/default/admin/flag-type/edit.html.tmpl and is included in the patch.
 
> @@ +44,5 @@
> > +sub db_schema_abstract_schema {
> > +    my ($self, $args) = @_;
> > +    $args->{'schema'}->{'flagtype_comments'} = {
> > +        FIELDS => [
> > +            flagtype => {
> 
> type_id to be consistent.

i assume you mean flag_type_id.

Comment 10

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #9)
> the hook is in template/en/default/admin/flag-type/edit.html.tmpl and is
> included in the patch.

See it now. Stupid Splinter put the link on the next line where I didn't see it :)
Must fix that to look better.
  
> > @@ +44,5 @@
> > > +sub db_schema_abstract_schema {
> > > +    my ($self, $args) = @_;
> > > +    $args->{'schema'}->{'flagtype_comments'} = {
> > > +        FIELDS => [
> > > +            flagtype => {
> > 
> > type_id to be consistent.
> 
> i assume you mean flag_type_id.

No, type_id. Look at the flags, flaginclusions, and flagexclusions tables in Bugzilla/DB/Schema.pm

dkl

Comment 11

[:glob ✱]

Attachment: patch v2

this addresses the review points.

the extension is now enabled for attachment flags only to avoid the overhead of another db hit on each bug page.  this can be toggled via lib/Config.pm

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 574539 [details] [diff] [review]
patch v2

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

Change extensions/FlagTypeComment/lib/Config.pm => extensions/FlagTypeComment/lib/Constants.pm

Also what should we do when the person is editing the patch as comment? Should we prepend the flag comment text to the beginning/end? Forget it altogether
and only focus on standalone comments? I am fine with the latter but I would imagine quite a few people still use the edit as comment functionality.

This reminds me that once this is live I should patch Splinter to use this as well.

dkl

::: extensions/FlagTypeComment/Extension.pm
@@ +113,5 @@
> +            'bug_id'       => $bug->id,
> +            'is_active'    => 1
> +        });
> +        my $types = join(',', map { $_->id }
> +                              grep { $_->is_active } 

You are already specifying is_active => 1, so I pretty sure grep { $_->is_active } is redundant.

Comment 13

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #12)
> Comment on attachment 574539 [details] [diff] [review] [diff] [details] [review]
>
> Change extensions/FlagTypeComment/lib/Config.pm =>
> extensions/FlagTypeComment/lib/Constants.pm

as the values in that file aren't constant, i don't think Constants was an appropriate name.
 
> Also what should we do when the person is editing the patch as comment?

forget about it; if you're editing as a comment you're doing a review, which doesn't generally mesh well with canned responses.

> This reminds me that once this is live I should patch Splinter to use this
> as well.

i don't agree, for the reasons above.

> You are already specifying is_active => 1, so I pretty sure grep {
> $_->is_active } is redundant.


Bugzilla::FlagType::match() doesn't support the extra arguments, so it's ignored.  sorry, i thought i'd removed it from the call, i'll fix that.

Comment 14

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #13) 
> as the values in that file aren't constant, i don't think Constants was an
> appropriate name.

Bugzilla/Constants.pm and Bugzilla/WebService/Constants.pm are a mix of constants and subroutines as well.

> Bugzilla::FlagType::match() doesn't support the extra arguments, so it's
> ignored.  sorry, i thought i'd removed it from the call, i'll fix that.

Bugzilla::FlagType::match({ is_active => 1 }) is a valid param. Line 441-444
in Bugzilla/FlagType.pm.

dkl

Comment 15

[:glob ✱]

Attachment: patch v3

(In reply to David Lawrence [:dkl] from comment #14)
> Bugzilla/Constants.pm and Bugzilla/WebService/Constants.pm are a mix of
> constants and subroutines as well.

correct you are :)  this revision also uses constants.

> > Bugzilla::FlagType::match() doesn't support the extra arguments, so it's
> > ignored.  sorry, i thought i'd removed it from the call, i'll fix that.
> 
> Bugzilla::FlagType::match({ is_active => 1 }) is a valid param. Line 441-444
> in Bugzilla/FlagType.pm.

i swear it didn't work before :D redundant grep removed.

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 575805 [details] [diff] [review]
patch v3

Almost there. All looks good except for an issue found with my testing. I get the following error when an attachment has zero active flag types:

DBD::mysql::db selectall_hashref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 2 [for Statement "SELECT type_id AS flagtype, on_status AS state, comment AS text FROM flagtype_comments WHERE type_id IN ()"] at ./extensions/FlagTypeComment/Extension.pm line 118

Need to skip the SQL call if no types are returned by Bugzilla::FlagType::match. You can fix on checkin. 

dkl

Comment 17

[:glob ✱]

committed to obtain timings from staging:

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified editflagtypes.cgi
added extensions/FlagTypeComment
added extensions/FlagTypeComment/Config.pm
added extensions/FlagTypeComment/Extension.pm
added extensions/FlagTypeComment/lib
added extensions/FlagTypeComment/template
added extensions/FlagTypeComment/lib/Constants.pm
added extensions/FlagTypeComment/template/en
added extensions/FlagTypeComment/template/en/default
added extensions/FlagTypeComment/template/en/default/flag
added extensions/FlagTypeComment/template/en/default/hook
added extensions/FlagTypeComment/template/en/default/flag/type_comment.html.tmpl
added extensions/FlagTypeComment/template/en/default/hook/admin
added extensions/FlagTypeComment/template/en/default/hook/attachment
added extensions/FlagTypeComment/template/en/default/hook/bug
added extensions/FlagTypeComment/template/en/default/hook/admin/flag-type
added extensions/FlagTypeComment/template/en/default/hook/admin/flag-type/edit-rows.html.tmpl
added extensions/FlagTypeComment/template/en/default/hook/attachment/create-end.html.tmpl
added extensions/FlagTypeComment/template/en/default/hook/attachment/edit-end.html.tmpl
added extensions/FlagTypeComment/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
modified template/en/default/admin/flag-type/edit.html.tmpl
Committed revision 7976.

Comment 18

[:glob ✱]

fix for the cosmetic issue on admin -> edit-flags when editing bug-flags:

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

Comment 19

[:glob ✱]

this is now live.

Comment 20

[Alex Keybl [:akeybl]]

Thanks so much for your help! We'll be filing bugs soon for setting the comment for some approval/tracking flags.

Comment 21

[LegNeato]

w00t! Thanks!