Bug 713307: Please add FlagTypeComments for tracking/approval flags

10 files changed, 203 insertions, 91 deletions

diff --git a/extensions/TrackingFlags/Extension.pm b/extensions/TrackingFlags/Extension.pm
index dd852e472..b107c0393 100644
--- a/extensions/TrackingFlags/Extension.pm
+++ b/extensions/TrackingFlags/Extension.pm
@@ -25,6 +25,8 @@ use Bugzilla::Field;
 use Bugzilla::Install::Filesystem;
 use Bugzilla::Product;
 
+use JSON;
+
 our $VERSION = '1';
 
 BEGIN {
@@ -66,13 +68,15 @@ sub template_before_process {
     if ($file eq 'bug/create/create.html.tmpl'
         || $file eq 'bug/create/create-winqual.html.tmpl')
     {
-        $vars->{'tracking_flags'} = Bugzilla::Extension::TrackingFlags::Flag->match({
+        my $flags = Bugzilla::Extension::TrackingFlags::Flag->match({
             product   => $vars->{'product'}->name,
             enter_bug => 1,
             is_active => 1,
         });
 
-        $vars->{'tracking_flag_types'} = FLAG_TYPES;
+        $vars->{tracking_flags}      = $flags;
+        $vars->{tracking_flags_json} = _flags_to_json($flags);
+        $vars->{tracking_flag_types} = FLAG_TYPES;
     }
     elsif ($file eq 'bug/edit.html.tmpl'|| $file eq 'bug/show.xml.tmpl'
            || $file eq 'email/bugmail.html.tmpl' || $file eq 'email/bugmail.txt.tmpl')
@@ -81,12 +85,15 @@ sub template_before_process {
         my $bug = exists $vars->{'bugs'} ? $vars->{'bugs'}[0] : $vars->{'bug'};
 
         if ($bug && !$bug->{error}) {
-            $vars->{'tracking_flags'} = Bugzilla::Extension::TrackingFlags::Flag->match({
+            my $flags = Bugzilla::Extension::TrackingFlags::Flag->match({
                 product     => $bug->product,
                 component   => $bug->component,
                 bug_id      => $bug->id,
                 is_active   => 1,
             });
+
+            $vars->{tracking_flags}      = $flags;
+            $vars->{tracking_flags_json} = _flags_to_json($flags);
         }
 
         $vars->{'tracking_flag_types'} = FLAG_TYPES;
@@ -99,6 +106,37 @@ sub template_before_process {
     }
 }
 
+sub _flags_to_json {
+    my ($flags) = @_;
+
+    my $json = {
+        flags => {},
+        types => [],
+        comments => {},
+    };
+
+    my %type_map = map { $_->{name} => $_ } @{ FLAG_TYPES() };
+    foreach my $flag (@$flags) {
+        my $flag_type = $flag->flag_type;
+
+        $json->{flags}->{$flag_type}->{$flag->name} = $flag->bug_flag->value;
+
+        if ($type_map{$flag_type}->{collapsed}
+            && !grep { $_ eq $flag_type } @{ $json->{types} })
+        {
+            push @{ $json->{types} }, $flag_type;
+        }
+
+        foreach my $value (@{ $flag->values }) {
+            if (defined($value->comment) && $value->comment ne '') {
+                $json->{comments}->{$flag->name}->{$value->value} = $value->comment;
+            }
+        }
+    }
+
+    return encode_json($json);
+}
+
 sub db_schema_abstract_schema {
     my ($self, $args) = @_;
     $args->{'schema'}->{'tracking_flags'} = {
diff --git a/extensions/TrackingFlags/lib/Admin.pm b/extensions/TrackingFlags/lib/Admin.pm
index 0b19fcf80..1bae18ef8 100644
--- a/extensions/TrackingFlags/lib/Admin.pm
+++ b/extensions/TrackingFlags/lib/Admin.pm
@@ -133,6 +133,7 @@ sub admin_edit {
                     value           => '---',
                     setter_group_id => '',
                     is_active       => 1,
+                    comment         => '',
                 },
             ]);
             $vars->{visibility} = '';
@@ -337,6 +338,7 @@ sub _update_db_values {
             setter_group_id => $value->{setter_group_id},
             is_active       => $value->{is_active},
             sortkey         => $sortkey,
+            comment         => $value->{comment},
         };
 
         if ($value->{id}) {
@@ -345,13 +347,11 @@ sub _update_db_values {
             my $old_value = $value_obj->value;
             $value_obj->set_all($object_set);
             $value_obj->update();
-            if ($object_set->{value} ne $old_value) {
-                Bugzilla::Extension::TrackingFlags::Flag::Bug->update_all_values({
-                    value_obj => $value_obj,
-                    old_value => $old_value,
-                    new_value => $value_obj->value,
-                });
-            }
+            Bugzilla::Extension::TrackingFlags::Flag::Bug->update_all_values({
+                value_obj => $value_obj,
+                old_value => $old_value,
+                new_value => $value_obj->value,
+            });
         } else {
             $object_set->{tracking_flag_id} = $flag_obj->flag_id;
             Bugzilla::Extension::TrackingFlags::Flag::Value->create($object_set);
@@ -406,6 +406,7 @@ sub _flag_values_to_json {
             value           => $value->{value},
             setter_group_id => $value->{setter_group_id},
             is_active       => $value->{is_active} ? JSON::true : JSON::false,
+            comment         => $value->{comment} // '',
         };
     }
     return encode_json(\@data);
diff --git a/extensions/TrackingFlags/lib/Flag/Value.pm b/extensions/TrackingFlags/lib/Flag/Value.pm
index 4023e191d..964d76810 100644
--- a/extensions/TrackingFlags/lib/Flag/Value.pm
+++ b/extensions/TrackingFlags/lib/Flag/Value.pm
@@ -14,7 +14,7 @@ use warnings;
 
 use Bugzilla::Error;
 use Bugzilla::Group;
-use Bugzilla::Util qw(detaint_natural);
+use Bugzilla::Util qw(detaint_natural trim);
 use Scalar::Util qw(blessed);
 
 ###############################
@@ -30,6 +30,7 @@ use constant DB_COLUMNS => qw(
     value
     sortkey
     is_active
+    comment
 );
 
 use constant LIST_ORDER => 'sortkey';
@@ -39,6 +40,7 @@ use constant UPDATE_COLUMNS => qw(
     value
     sortkey
     is_active
+    comment
 );
 
 use constant VALIDATORS => {
@@ -47,6 +49,7 @@ use constant VALIDATORS => {
     value            => \&_check_value,
     sortkey          => \&_check_sortkey,
     is_active        => \&Bugzilla::Object::check_boolean,
+    comment          => \&_check_comment,
 };
 
 ###############################
@@ -86,6 +89,13 @@ sub _check_sortkey {
     return $sortkey;
 }
 
+sub _check_comment {
+    my ($invocant, $value) = @_;
+    return undef unless defined $value;
+    $value = trim($value);
+    return $value eq '' ? undef : $value;
+}
+
 ###############################
 ####       Setters         ####
 ###############################
@@ -94,6 +104,7 @@ sub set_setter_group_id { $_[0]->set('setter_group_id', $_[1]); }
 sub set_value           { $_[0]->set('value', $_[1]);           }
 sub set_sortkey         { $_[0]->set('sortkey', $_[1]);         }
 sub set_is_active       { $_[0]->set('is_active', $_[1]);       }
+sub set_comment         { $_[0]->set('comment', $_[1]);         }
 
 ###############################
 ####      Accessors        ####
@@ -104,6 +115,7 @@ sub setter_group_id  { return $_[0]->{'setter_group_id'};  }
 sub value            { return $_[0]->{'value'};            }
 sub sortkey          { return $_[0]->{'sortkey'};          }
 sub is_active        { return $_[0]->{'is_active'};        }
+sub comment          { return $_[0]->{'comment'};          }
 
 sub tracking_flag {
     return $_[0]->{'tracking_flag'} ||= Bugzilla::Extension::TrackingFlags::Flag->new({
diff --git a/extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl b/extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
index b2b6efca7..4e2c97dfa 100644
--- a/extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
+++ b/extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
@@ -30,7 +30,8 @@
       [% IF user.id %]
         <input type="hidden" id="[% flag.name FILTER html %]_dirty">
         <select id="[% flag.name FILTER html %]"
-                name="[% flag.name FILTER html %]">
+                name="[% flag.name FILTER html %]"
+                onchange="tracking_flag_change(this)">
           [% FOREACH value = flag.values %]
             [% IF new_bug || value.name != flag_bug_value  %]
               [% NEXT IF !value.is_active || !flag.can_set_value(value.name) %]
@@ -55,3 +56,7 @@
     </td>
   </tr>
 [% END %]
+
+<script type="text/javascript">
+  TrackingFlags = [% tracking_flags_json FILTER none %];
+</script>
diff --git a/extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl b/extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
index e0411b512..b66bd3df4 100644
--- a/extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
+++ b/extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ -6,49 +6,41 @@
   # defined by the Mozilla Public License, v. 2.0.
   #%]
 
-[% IF tracking_flags.size %]
-  [% FOREACH type = tracking_flag_types %]
-    [% flag_list = [] %]
-    [% FOREACH flag = tracking_flags %]
-      [% flag_list.push(flag) IF flag.flag_type == type.name %]
-    [% END %]
-    [% IF flag_list.size %]
-      <tr>
-        <td class="field_label">
-          <label>[% type.description FILTER html %]:</label>
-        </td>
-        <td>
-          [% IF bug.check_can_change_field('flagtypes.name', 0, 1) %]
-            [% IF user.id && type.collapsed %]
-              <span id="edit_[% type.name FILTER html %]_flags_action">
-                (<a href="#" name="[% type.name FILTER html %]" class="edit_tracking_flags_link">edit</a>)
-              </span>
-            [% END %]
-            <table class="tracking_flags">
-            [% INCLUDE bug/tracking_flags.html.tmpl
-               flag_list = flag_list %]
-            </table>
-            [% IF type.collapsed %]
-              <script type="text/javascript">
-                TrackingFlags.flags['[% type.name FILTER js %]'] = {};
-                [% FOREACH flag = flag_list %]
-                  TrackingFlags.flags['[% type.name FILTER js %]']['[% flag.name FILTER js %]'] = '[% flag.bug_flag.value FILTER js %]';
-                [% END %]
-                TrackingFlags.types.push('[% type.name FILTER js %]');
-            </script>
-            [% END %]
-          [% ELSE %]
-            [% FOREACH flag = flag_list %]
-              [% NEXT IF flag.status == '---' %]
-              [% flag.description FILTER html %]: [% flag.bug_flag.value FILTER html %]<br>
-            [% END %]
+[% RETURN UNLESS tracking_flags.size %]
+
+[% FOREACH type = tracking_flag_types %]
+  [% flag_list = [] %]
+  [% FOREACH flag = tracking_flags %]
+    [% flag_list.push(flag) IF flag.flag_type == type.name %]
+  [% END %]
+  [% IF flag_list.size %]
+    <tr>
+      <td class="field_label">
+        <label>[% type.description FILTER html %]:</label>
+      </td>
+      <td>
+        [% IF bug.check_can_change_field('flagtypes.name', 0, 1) %]
+          [% IF user.id && type.collapsed %]
+            <span id="edit_[% type.name FILTER html %]_flags_action">
+              (<a href="#" name="[% type.name FILTER html %]" class="edit_tracking_flags_link">edit</a>)
+            </span>
+          [% END %]
+          <table class="tracking_flags">
+          [% INCLUDE bug/tracking_flags.html.tmpl
+              flag_list = flag_list %]
+          </table>
+        [% ELSE %]
+          [% FOREACH flag = flag_list %]
+            [% NEXT IF flag.status == '---' %]
+            [% flag.description FILTER html %]: [% flag.bug_flag.value FILTER html %]<br>
           [% END %]
-        </td>
-      </tr>
-    [% END %]
+        [% END %]
+      </td>
+    </tr>
   [% END %]
 [% END %]
 
 <script type="text/javascript">
+  TrackingFlags = [% tracking_flags_json FILTER none %];
   hide_tracking_flags();
 </script>
diff --git a/extensions/TrackingFlags/template/en/default/hook/global/header-start.html.tmpl b/extensions/TrackingFlags/template/en/default/hook/global/header-start.html.tmpl
new file mode 100644
index 000000000..2bf1c75c3
--- /dev/null
+++ b/extensions/TrackingFlags/template/en/default/hook/global/header-start.html.tmpl
@@ -0,0 +1,11 @@
+[%# This Source Code Form is subject to the terms of the Mozilla Public
+  # License, v. 2.0. If a copy of the MPL was not distributed with this
+  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
+  #
+  # This Source Code Form is "Incompatible With Secondary Licenses", as
+  # defined by the Mozilla Public License, v. 2.0.
+  #%]
+
+[% IF template.name == "bug/create/create.html.tmpl" && tracking_flags.size %]
+  [% javascript_urls.push('extensions/TrackingFlags/web/js/tracking_flags.js') %]
+[% END %]
diff --git a/extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl b/extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl
index 12c8d2c3b..60406490f 100644
--- a/extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl
+++ b/extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl
@@ -78,7 +78,7 @@ var selected_components = [
       <option value=""></option>
       [% FOREACH type = tracking_flag_types %]
         <option value="[% type.name FILTER html %]"
-                [% 'selected="selected"' IF flag.flag_type == type.name %]>
+                [% " selected" IF flag.flag_type == type.name %]>
           [% type.name FILTER html %]</option>
       [% END %]
     </select>
diff --git a/extensions/TrackingFlags/web/js/admin.js b/extensions/TrackingFlags/web/js/admin.js
index e3eb81714..58bdd294f 100644
--- a/extensions/TrackingFlags/web/js/admin.js
+++ b/extensions/TrackingFlags/web/js/admin.js
@@ -80,7 +80,7 @@ function update_flag_values() {
     for (var i = 0, l = flag_values.length; i < l; i++) {
         var value = flag_values[i];
 
-        var row = tbl.insertRow(2 + i);
+        var row = tbl.insertRow(2 + (i * 2));
         var cell;
 
         // value
@@ -157,10 +157,31 @@ function update_flag_values() {
                 ? '<span class="txt_icon">&nbsp;-&nbsp;</span>'
                 : '<a class="txt_icon" href="#" onclick="value_move_down(' + i + ');return false"> &nabla; </a>'
             );
-        if (value.value != '---')
-            html += '| <a href="#" onclick="remove_value(' + i + ');return false">Remove</a>';
-        html += ']';
+        if (value.value != '---') {
+            var lbl = value.comment == '' ? 'Set Comment' : 'Edit Comment';
+            html +=
+                '|<a href="#" onclick="remove_value(' + i + ');return false">Remove</a>' +
+                '|<a href="#" onclick="toggle_value_comment(this, ' + i + ');return false">' + lbl + '</a>'
+
+        }
+        html += ' ]';
         cell.innerHTML = html;
+
+        row = tbl.insertRow(3 + (i * 2));
+        row.className = 'bz_default_hidden';
+        row.id = 'comment_row_' + i;
+        cell = row.insertCell(0);
+        cell = row.insertCell(1);
+        cell.colSpan = 3;
+        var ta = document.createElement('textarea');
+        ta.className = 'value_comment';
+        ta.id = 'value_comment_' + i;
+        ta.rows = 5;
+        ta.value = value.comment;
+        cell.appendChild(ta);
+        Event.addListener(ta, 'blur', function(e, idx) {
+            flag_values[idx].comment = e.target.value;
+        }, i);
     }
 
     tag_invalid_values();
@@ -229,6 +250,19 @@ function update_value(e, o) {
     flag_values[i].value = o.value;
 }
 
+function toggle_value_comment(btn, idx) {
+    var row = Dom.get('comment_row_' + idx);
+    if (Dom.hasClass(row, 'bz_default_hidden')) {
+        Dom.removeClass(row, 'bz_default_hidden');
+        btn.innerHTML = 'Hide Comment';
+        Dom.get('value_comment_' + idx).select();
+        Dom.get('value_comment_' + idx).focus();
+    } else {
+        Dom.addClass(row, 'bz_default_hidden');
+        btn.innerHTML = flag_values[idx].comment == '' ? 'Set Comment' : 'Edit Comment';
+    }
+}
+
 // visibility
 
 function update_flag_visibility() {
diff --git a/extensions/TrackingFlags/web/js/tracking_flags.js b/extensions/TrackingFlags/web/js/tracking_flags.js
index 135b93dba..17513ffe4 100644
--- a/extensions/TrackingFlags/web/js/tracking_flags.js
+++ b/extensions/TrackingFlags/web/js/tracking_flags.js
@@ -8,49 +8,64 @@
 
 var Dom = YAHOO.util.Dom;
 
-var TrackingFlags = {
-  flags: {},
-  types: []
-};
-
 function hide_tracking_flags() {
-  for (var i = 0, l = TrackingFlags.types.length; i < l; i++) {
-    var flag_type = TrackingFlags.types[i];
-    for (var field in TrackingFlags.flags[flag_type]) {
-      var el = Dom.get(field);
-      var value = el ? el.value : TrackingFlags.flags[flag_type][field];
-      if (el && (value != TrackingFlags.flags[flag_type][field])) {
-        show_tracking_flags(flag_type);
-        return;
-      }
-      if (value == '---') {
-        Dom.addClass('row_' + field, 'bz_default_hidden');
-      } else {
-        Dom.addClass(field, 'bz_default_hidden');
-        Dom.removeClass('ro_' + field, 'bz_default_hidden');
-      }
+    for (var i = 0, l = TrackingFlags.types.length; i < l; i++) {
+        var flag_type = TrackingFlags.types[i];
+        for (var field in TrackingFlags.flags[flag_type]) {
+            var el = Dom.get(field);
+            var value = el ? el.value : TrackingFlags.flags[flag_type][field];
+            if (el && (value != TrackingFlags.flags[flag_type][field])) {
+                show_tracking_flags(flag_type);
+                return;
+            }
+            if (value == '---') {
+                Dom.addClass('row_' + field, 'bz_default_hidden');
+            } else {
+                Dom.addClass(field, 'bz_default_hidden');
+                Dom.removeClass('ro_' + field, 'bz_default_hidden');
+            }
+        }
     }
-  }
 }
 
 function show_tracking_flags(flag_type) {
-  Dom.addClass('edit_' + flag_type + '_flags_action', 'bz_default_hidden');
-  for (var field in TrackingFlags.flags[flag_type]) {
-    if (Dom.get(field).value == '---') {
-      Dom.removeClass('row_' + field, 'bz_default_hidden');
+    Dom.addClass('edit_' + flag_type + '_flags_action', 'bz_default_hidden');
+    for (var field in TrackingFlags.flags[flag_type]) {
+        if (Dom.get(field).value == '---') {
+            Dom.removeClass('row_' + field, 'bz_default_hidden');
+        } else {
+            Dom.removeClass(field, 'bz_default_hidden');
+            Dom.addClass('ro_' + field, 'bz_default_hidden');
+        }
+    }
+}
+
+function tracking_flag_change(e) {
+    var value = e.value;
+    if (!TrackingFlags.comments[e.name])
+        return;
+    var prefill = TrackingFlags.comments[e.name][e.value];
+    if (!prefill)
+        return;
+    var commentEl = document.getElementById('comment');
+    if (!commentEl)
+        return;
+    var value = commentEl.value;
+    if (value == prefill)
+        return;
+    if (value == '') {
+        commentEl.value = prefill;
     } else {
-      Dom.removeClass(field, 'bz_default_hidden');
-      Dom.addClass('ro_' + field, 'bz_default_hidden');
+        commentEl.value = prefill + "\n\n" + value;
     }
-  }
 }
 
 YAHOO.util.Event.onDOMReady(function() {
-  var edit_tracking_links = Dom.getElementsByClassName('edit_tracking_flags_link');
-  for (var i = 0, l = edit_tracking_links.length; i < l; i++) {
-    YAHOO.util.Event.addListener(edit_tracking_links[i], 'click', function(e) {
-      e.preventDefault();
-      show_tracking_flags(this.name);
-    });
-  }
+    var edit_tracking_links = Dom.getElementsByClassName('edit_tracking_flags_link');
+    for (var i = 0, l = edit_tracking_links.length; i < l; i++) {
+        YAHOO.util.Event.addListener(edit_tracking_links[i], 'click', function(e) {
+            e.preventDefault();
+            show_tracking_flags(this.name);
+        });
+    }
 });
diff --git a/extensions/TrackingFlags/web/styles/admin.css b/extensions/TrackingFlags/web/styles/admin.css
index 374409ce6..51c6ab966 100644
--- a/extensions/TrackingFlags/web/styles/admin.css
+++ b/extensions/TrackingFlags/web/styles/admin.css
@@ -87,6 +87,10 @@
     width: 10em;
 }
 
+.value_comment {
+    width: 100%;
+}
+
 .hidden {
     display: none;
 }