Bug 479400: Add the ability to show or hide particular custom fields

based on multiple values of another field (visibility controllers)
r/a=mkanat

11 files changed, 171 insertions, 69 deletions

diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index a4d44d191..2efdbefc4 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -671,7 +671,6 @@ use constant ABSTRACT_SCHEMA => {
             visibility_field_id => {TYPE => 'INT3', 
                                     REFERENCES => {TABLE  => 'fielddefs',
                                                    COLUMN => 'id'}},
-            visibility_value_id => {TYPE => 'INT2'},
             value_field_id => {TYPE => 'INT3',
                                REFERENCES => {TABLE  => 'fielddefs',
                                               COLUMN => 'id'}},
@@ -688,6 +687,25 @@ use constant ABSTRACT_SCHEMA => {
         ],
     },
 
+    # Field Visibility Information
+    # -------------------------
+
+    field_visibility => {
+        FIELDS => [
+            field_id => {TYPE => 'INT3', 
+                         REFERENCES => {TABLE  => 'fielddefs',
+                                        COLUMN => 'id',
+                                        DELETE => 'CASCADE'}},
+            value_id => {TYPE => 'INT2', NOTNULL => 1}
+        ],
+        INDEXES => [
+            field_visibility_field_id_idx => {
+                FIELDS => [qw(field_id value_id)],
+                TYPE   => 'UNIQUE'
+            },
+        ],
+    },
+
     # Per-product Field Values
     # ------------------------
 
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index daf708179..41b90a644 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -77,6 +77,7 @@ use base qw(Exporter Bugzilla::Object);
 use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Util;
+use List::MoreUtils qw(any);
 
 use Scalar::Util qw(blessed);
 
@@ -99,7 +100,6 @@ use constant DB_COLUMNS => qw(
     enter_bug
     buglist
     visibility_field_id
-    visibility_value_id
     value_field_id
     reverse_desc
     is_mandatory
@@ -118,7 +118,7 @@ use constant VALIDATORS => {
     type         => \&_check_type,
     value_field_id      => \&_check_value_field_id,
     visibility_field_id => \&_check_visibility_field_id,
-    visibility_value_id => \&_check_control_value,
+    visibility_values => \&_check_visibility_values,
     is_mandatory => \&Bugzilla::Object::check_boolean,
 };
 
@@ -127,7 +127,7 @@ use constant VALIDATOR_DEPENDENCIES => {
     type => ['custom'],
     reverse_desc => ['type'],
     value_field_id => ['type'],
-    visibility_value_id => ['visibility_field_id'],
+    visibility_values => ['visibility_field_id'],
 };
 
 use constant UPDATE_COLUMNS => qw(
@@ -138,7 +138,6 @@ use constant UPDATE_COLUMNS => qw(
     enter_bug
     buglist
     visibility_field_id
-    visibility_value_id
     value_field_id
     reverse_desc
     is_mandatory
@@ -357,8 +356,8 @@ sub _check_visibility_field_id {
     return $field->id;
 }
 
-sub _check_control_value {
-    my ($invocant, $value_id, undef, $params) = @_;
+sub _check_visibility_values {
+    my ($invocant, $values, undef, $params) = @_;
     my $field;
     if (blessed $invocant) {
         $field = $invocant->visibility_field;
@@ -366,11 +365,24 @@ sub _check_control_value {
     elsif ($params->{visibility_field_id}) {
         $field = $invocant->new($params->{visibility_field_id});
     }
-    # When no field is set, no value is set.
-    return undef if !$field;
-    my $value_obj = Bugzilla::Field::Choice->type($field)
-                    ->check({ id => $value_id });
-    return $value_obj->id;
+    # When no field is set, no values are set.
+    return [] if !$field;
+
+    if (!scalar @$values) {
+        ThrowUserError('field_visibility_values_must_be_selected',
+                       { field => $field });
+    }
+
+    my @visibility_values;
+    my $choice = Bugzilla::Field::Choice->type($field);
+    foreach my $value (@$values) {
+        if (!blessed $value) {
+            $value = $choice->check({ id => $value });
+        }
+        push(@visibility_values, $value);
+    }
+
+    return \@visibility_values;
 }
 
 sub _check_reverse_desc {
@@ -603,9 +615,9 @@ sub visibility_field {
 
 =over
 
-=item C<visibility_value>
+=item C<visibility_values>
 
-If we have a L</visibility_field>, then what value does that field have to
+If we have a L</visibility_field>, then what values does that field have to
 be set to in order to show this field? Returns a L<Bugzilla::Field::Choice>
 or undef if there is no C<visibility_field> set.
 
@@ -613,16 +625,23 @@ or undef if there is no C<visibility_field> set.
 
 =cut
 
-
-sub visibility_value {
+sub visibility_values {
     my $self = shift;
-    if ($self->{visibility_field_id}) {
-        require Bugzilla::Field::Choice;
-        $self->{visibility_value} ||=
-            Bugzilla::Field::Choice->type($self->visibility_field)->new(
-                $self->{visibility_value_id});
+    my $dbh = Bugzilla->dbh;
+    
+    return [] if !$self->{visibility_field_id};
+
+    if (!defined $self->{visibility_values}) {
+        my $visibility_value_ids =
+            $dbh->selectcol_arrayref("SELECT value_id FROM field_visibility
+                                      WHERE field_id = ?", undef, $self->id);
+
+        $self->{visibility_values} =
+            Bugzilla::Field::Choice->type($self->visibility_field)
+            ->new_from_list($visibility_value_ids);
     }
-    return $self->{visibility_value};
+
+    return $self->{visibility_values};
 }
 
 =pod
@@ -700,10 +719,13 @@ See L<Bugzilla::Field::ChoiceInterface>.
 sub is_visible_on_bug {
     my ($self, $bug) = @_;
 
-    my $visibility_value = $self->visibility_value;
-    return 1 if !$visibility_value;
+    # Always return visible, if this field is not
+    # visibility controlled.
+    return 1 if !$self->{visibility_field_id};
 
-    return $visibility_value->is_set_on_bug($bug);
+    my $visibility_values = $self->visibility_values;
+
+    return (any { $_->is_set_on_bug($bug) } @$visibility_values) ? 1 : 0;
 }
 
 =over
@@ -784,7 +806,7 @@ They will throw an error if you try to set the values to something invalid.
 
 =item C<set_visibility_field>
 
-=item C<set_visibility_value>
+=item C<set_visibility_values>
 
 =item C<set_value_field>
 
@@ -806,12 +828,11 @@ sub set_visibility_field {
     my ($self, $value) = @_;
     $self->set('visibility_field_id', $value);
     delete $self->{visibility_field};
-    delete $self->{visibility_value};
+    delete $self->{visibility_values};
 }
-sub set_visibility_value {
-    my ($self, $value) = @_;
-    $self->set('visibility_value_id', $value);
-    delete $self->{visibility_value};
+sub set_visibility_values {
+    my ($self, $value_ids) = @_;
+    $self->set('visibility_values', $value_ids);
 }
 sub set_value_field {
     my ($self, $value) = @_;
@@ -945,13 +966,24 @@ C<is_mandatory> - boolean - Whether this field is mandatory. Defaults to 0.
 sub create {
     my $class = shift;
     my ($params) = @_;
+    my $dbh = Bugzilla->dbh;
+
     # This makes sure the "sortkey" validator runs, even if
     # the parameter isn't sent to create().
     $params->{sortkey} = undef if !exists $params->{sortkey};
     $params->{type} ||= 0;
-    my $field = $class->SUPER::create(@_);
+    
+    $dbh->bz_start_transaction();
+    $class->check_required_create_fields(@_);
+    my $field_values      = $class->run_create_validators($params);
+    my $visibility_values = delete $field_values->{visibility_values};
+    my $field             = $class->insert_create_data($field_values);
+    
+    $field->set_visibility_values($visibility_values);
+    $field->_update_visibility_values();
+
+    $dbh->bz_commit_transaction();
 
-    my $dbh = Bugzilla->dbh;
     if ($field->custom) {
         my $name = $field->name;
         my $type = $field->type;
@@ -981,9 +1013,29 @@ sub update {
     if ($changes->{value_field_id} && $self->is_select) {
         $dbh->do("UPDATE " . $self->name . " SET visibility_value_id = NULL");
     }
+    $self->_update_visibility_values();
     return $changes;
 }
 
+sub _update_visibility_values {
+    my $self = shift;
+    my $dbh = Bugzilla->dbh;
+
+    my @visibility_value_ids = map($_->id, @{$self->visibility_values});
+    $self->_delete_visibility_values();
+    for my $value_id (@visibility_value_ids) {
+        $dbh->do("INSERT INTO field_visibility (field_id, value_id)
+                  VALUES (?, ?)", undef, $self->id, $value_id);
+    }
+}
+
+sub _delete_visibility_values {
+    my ($self) = @_;
+    my $dbh = Bugzilla->dbh;
+    $dbh->do("DELETE FROM field_visibility WHERE field_id = ?",
+        undef, $self->id);
+    delete $self->{visibility_values};
+}
 
 =pod
 
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index bfce7779f..b786e0624 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -88,7 +88,6 @@ sub update_fielddefs_definition {
     }
 
     $dbh->bz_add_column('fielddefs', 'visibility_field_id', {TYPE => 'INT3'});
-    $dbh->bz_add_column('fielddefs', 'visibility_value_id', {TYPE => 'INT2'});
     $dbh->bz_add_column('fielddefs', 'value_field_id', {TYPE => 'INT3'});
     $dbh->bz_add_index('fielddefs', 'fielddefs_value_field_id_idx',
                        ['value_field_id']);
@@ -113,6 +112,9 @@ sub update_fielddefs_definition {
     $dbh->bz_add_index('fielddefs', 'fielddefs_is_mandatory_idx',
                        ['is_mandatory']);
 
+    # 2010-04-05 dkl@redhat.com - Bug 479400
+    _migrate_field_visibility_value();
+
     # Remember, this is not the function for adding general table changes.
     # That is below. Add new changes to the fielddefs table above this
     # comment.
@@ -561,8 +563,6 @@ sub update_table_definitions {
     # 2008-09-07 LpSolit@gmail.com - Bug 452893
     _fix_illegal_flag_modification_dates();
 
-    _add_visiblity_value_to_value_tables();
-
     # 2009-03-02 arbingersys@gmail.com - Bug 423613
     _add_extern_id_index();
 
@@ -3208,20 +3208,6 @@ sub _fix_illegal_flag_modification_dates {
     print "$rows flags had an illegal modification date. Fixed!\n" if ($rows =~ /^\d+$/);
 }
 
-sub _add_visiblity_value_to_value_tables {
-    my $dbh = Bugzilla->dbh;
-    my @standard_fields = 
-        qw(bug_status resolution priority bug_severity op_sys rep_platform);
-    my $custom_fields = $dbh->selectcol_arrayref(
-        'SELECT name FROM fielddefs WHERE custom = 1 AND type IN(?,?)',
-        undef, FIELD_TYPE_SINGLE_SELECT, FIELD_TYPE_MULTI_SELECT);
-    foreach my $field (@standard_fields, @$custom_fields) {
-        $dbh->bz_add_column($field, 'visibility_value_id', {TYPE => 'INT2'});
-        $dbh->bz_add_index($field, "${field}_visibility_value_id_idx", 
-                           ['visibility_value_id']);
-    }
-}
-
 sub _add_extern_id_index {
     my $dbh = Bugzilla->dbh;
     if (!$dbh->bz_index_info('profiles', 'profiles_extern_id_idx')) {
@@ -3395,6 +3381,33 @@ sub _remove_attachment_isurl {
     }
 }
 
+sub _migrate_field_visibility_value {
+    my $dbh = Bugzilla->dbh;
+
+    if ($dbh->bz_column_info('fielddefs', 'visibility_value_id')) {
+        print "Populating new field_visibility table...\n";
+
+        $dbh->bz_start_transaction();
+
+        my %results =
+            @{ $dbh->selectcol_arrayref(
+                "SELECT id, visibility_value_id FROM fielddefs
+                 WHERE visibility_value_id IS NOT NULL",
+               { Columns => [1,2] }) };
+
+        my $insert_sth =
+            $dbh->prepare("INSERT INTO field_visibility (field_id, value_id)
+                           VALUES (?, ?)");
+
+        foreach my $id (keys %results) {
+            $insert_sth->execute($id, $results{$id});
+        }
+
+        $dbh->bz_commit_transaction();
+        $dbh->bz_drop_column('fielddefs', 'visibility_value_id');
+    }
+}
+
 1;
 
 __END__
diff --git a/editfields.cgi b/editfields.cgi
index e207a2ee4..cdf1008f7 100755
--- a/editfields.cgi
+++ b/editfields.cgi
@@ -66,7 +66,7 @@ elsif ($action eq 'new') {
         custom      => 1,
         buglist     => 1,
         visibility_field_id => scalar $cgi->param('visibility_field_id'),
-        visibility_value_id => scalar $cgi->param('visibility_value_id'),
+        visibility_values => [ $cgi->param('visibility_values') ],
         value_field_id => scalar $cgi->param('value_field_id'),
         reverse_desc => scalar $cgi->param('reverse_desc'),
         is_mandatory => scalar $cgi->param('is_mandatory'),
@@ -114,7 +114,7 @@ elsif ($action eq 'update') {
     $field->set_obsolete($cgi->param('obsolete'));
     $field->set_is_mandatory($cgi->param('is_mandatory'));
     $field->set_visibility_field($cgi->param('visibility_field_id'));
-    $field->set_visibility_value($cgi->param('visibility_value_id'));
+    $field->set_visibility_values([ $cgi->param('visibility_values') ]);
     $field->set_value_field($cgi->param('value_field_id'));
     $field->set_reverse_desc($cgi->param('reverse_desc'));
     $field->update();
diff --git a/js/field.js b/js/field.js
index a64369d33..8e1f53991 100644
--- a/js/field.js
+++ b/js/field.js
@@ -465,12 +465,12 @@ function setClassification() {
  * a certain value. May only be called after the controller has already
  * been added to the DOM.
  */
-function showFieldWhen(controlled_id, controller_id, value) {
+function showFieldWhen(controlled_id, controller_id, values) {
     var controller = document.getElementById(controller_id);
     // Note that we don't get an object for "controlled" here, because it
     // might not yet exist in the DOM. We just pass along its id.
-    YAHOO.util.Event.addListener(controller, 'change', 
-        handleVisControllerValueChange, [controlled_id, controller, value]);
+    YAHOO.util.Event.addListener(controller, 'change',
+        handleVisControllerValueChange, [controlled_id, controller, values]);
 }
 
 /**
@@ -480,13 +480,21 @@ function showFieldWhen(controlled_id, controller_id, value) {
 function handleVisControllerValueChange(e, args) {
     var controlled_id = args[0];
     var controller = args[1];
-    var value = args[2];
+    var values = args[2];
 
     var label_container = 
         document.getElementById('field_label_' + controlled_id);
     var field_container =
         document.getElementById('field_container_' + controlled_id);
-    if (bz_valueSelected(controller, value)) {
+    var selected = false;
+    for (var i = 0; i < values.length; i++) {
+        if (bz_valueSelected(controller, values[i])) {
+            selected = true;
+            break;
+        }
+    }
+
+    if (selected) {
         YAHOO.util.Dom.removeClass(label_container, 'bz_hidden_field');
         YAHOO.util.Dom.removeClass(field_container, 'bz_hidden_field');
     }
diff --git a/template/en/default/admin/custom_fields/cf-js.js.tmpl b/template/en/default/admin/custom_fields/cf-js.js.tmpl
index f9c31b5c7..cc1a4e4aa 100644
--- a/template/en/default/admin/custom_fields/cf-js.js.tmpl
+++ b/template/en/default/admin/custom_fields/cf-js.js.tmpl
@@ -65,7 +65,7 @@ function onChangeType(type_field) {
 
 function onChangeVisibilityField() {
     var vis_field = document.getElementById('visibility_field_id');
-    var vis_value = document.getElementById('visibility_value_id');
+    var vis_value = document.getElementById('visibility_values');
 
     if (vis_field.value) {
         var values = select_values[vis_field.value];
diff --git a/template/en/default/admin/custom_fields/create.html.tmpl b/template/en/default/admin/custom_fields/create.html.tmpl
index b96738473..f89f979fd 100644
--- a/template/en/default/admin/custom_fields/create.html.tmpl
+++ b/template/en/default/admin/custom_fields/create.html.tmpl
@@ -130,8 +130,11 @@ YAHOO.util.Event.onDOMReady(function() {onChangeType(document.getElementById('ty
             </option>
           [% END %]
         </select>
-        <label for="visibility_value_id"><strong>is set to:</strong></label>
-        <select name="visibility_value_id" id="visibility_value_id">
+        <label for="visibility_values">
+          <strong>is set to any of:</strong>
+        </label>
+        <select multiple="multiple" size="5" name="visibility_values"
+                id="visibility_values" class="field_value">
           <option value=""></option>
         </select>
       </td>
diff --git a/template/en/default/admin/custom_fields/edit.html.tmpl b/template/en/default/admin/custom_fields/edit.html.tmpl
index 48c3396f3..5ce2b7fd5 100644
--- a/template/en/default/admin/custom_fields/edit.html.tmpl
+++ b/template/en/default/admin/custom_fields/edit.html.tmpl
@@ -118,12 +118,14 @@
             </option>
           [% END %]
         </select>
-        <label for="visibility_value_id"><strong>is set to:</strong></label>
-        <select name="visibility_value_id" id="visibility_value_id">
+        <label for="visibility_values">
+          <strong>is set to any of:</strong>
+        </label>
+        <select multiple="multiple" size="5" name="visibility_values" 
+                id="visibility_values" class="field_value">
           [% FOREACH value = field.visibility_field.legal_values %]
             <option value="[% value.id FILTER html %]"
-             [% ' selected="selected"' 
-                IF field.visibility_value.id == value.id %]>
+              [% " selected" IF field.visibility_values.contains(value) %]>
               [% IF field.visibility_field.name == 'component' %]
                 [% display_value('product', value.product.name) FILTER html %]:
               [% END %]
diff --git a/template/en/default/bug/field-events.js.tmpl b/template/en/default/bug/field-events.js.tmpl
index f9e0ea93d..94dd298f5 100644
--- a/template/en/default/bug/field-events.js.tmpl
+++ b/template/en/default/bug/field-events.js.tmpl
@@ -23,8 +23,11 @@
 
 [% FOREACH controlled_field = field.controls_visibility_of %]
   showFieldWhen('[% controlled_field.name FILTER js %]',
-                '[% field.name FILTER js %]',
-                '[% controlled_field.visibility_value.name FILTER js %]');
+                '[% field.name FILTER js %]', [
+  [%- FOREACH visibility_value = controlled_field.visibility_values -%]
+    '[%- visibility_value.name FILTER js -%]'[% "," UNLESS loop.last %]
+  [%- END %]
+  ]);
 [% END %]
 [% FOREACH legal_value = field.legal_values %]
   [% FOREACH controlled_field = legal_value.controlled_values.keys %]
diff --git a/template/en/default/bug/field.html.tmpl b/template/en/default/bug/field.html.tmpl
index 49cf718cc..34347b21c 100644
--- a/template/en/default/bug/field.html.tmpl
+++ b/template/en/default/bug/field.html.tmpl
@@ -42,9 +42,7 @@
   #%]
 
 [% SET hidden = 0 %]
-[% IF field.visibility_field.defined AND bug
-      AND !field.visibility_value.is_set_on_bug(bug) 
-%]
+[% IF bug AND !field.is_visible_on_bug(bug) %]
   [% SET hidden = 1 %]
 [% END %]
 
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 59fa47bf9..f63092c96 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -482,6 +482,11 @@
     Only Drop-Down or Multi-Select fields can have a field that
     controls their values.
 
+  [% ELSIF error == "field_visibility_values_must_be_selected" %]
+    [% title = "Missing visibility field values" %]
+    At least one value must be selected for the visibility field
+    '[% field.name FILTER html %]'.
+
   [% ELSIF error == "fieldname_invalid" %]
     [% title = "Specified Field Does Not Exist" %]
     The field '[% field.name FILTER html %]' does not exist or