479400 - Show or hide particular custom fields based on multiple values of another field (visibility controllers)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P1)

Description

[bigstijn]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6
Build Identifier: Bugzilla 3.3.2

Bug 371995 did introduce the ability to have a custom field which is visible if one particular product has been choosen.

A further enhancement is the posibility of having a custom field which is visible for multiple products.  Or generic: show/hide particular custom fields based on multiple values of another field.

As stated in Bug 371995 comment 5, the existing inclusion/exclusion code for flags could be reused (introducing inclusion/exclusion table for custom fields).


Reproducible: Always

Comment 1

[Vitaly Fedrushkov]

a duplicate of bug 458436

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> a duplicate of bug 458436

Hum no, not a dupe. Bug 458436 only lets you select a *single* value, not multiple ones. This is a valid request.

Comment 3

[Max Kanat-Alexander]

It won't be necessary to re-use the *clusion code, I'm pretty sure. But we might be able to. We'll see.

Comment 5

[SKropp]

It would be nice, if we could map a custom field to a classification. IMHO the mapping to just one product isn't enough. I've tried to enable this about setting the mapping directly in the database, but this only caused an error.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> It would be nice, if we could map a custom field to a classification.

  Okay, you should file a separate bug for that if it can't already be done in Bugzilla 3.4, and if there isn't already an existing bug for it.

Comment 8

[Tiago Stürmer Daitx]

Attachment: visibility control by multiple field values

This is the first patch that I'm submitting to Bugzilla, I tried my best to follow the dev guidelines so if you see a problem just let me know what it is and I'll get it fixed.

I would like to see this feature on bugzilla 3.6, let me know if I have to set any additional flag.

Comment 9

[Tiago Stürmer Daitx]

Attachment: visibility control by multiple field values - v2

Sorry, the last patch was missing a file and a fix. It also was generated by hg instead of svn.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 405401 [details] [diff] [review]
visibility control by multiple field values - v2

>Index: editfields.cgi
>+    use Data::Dumper;
>+    print Dumper($cgi);
>+    print "<br>\n";
>+    print Dumper(@visibility_values);
>+    print "<br>\n";

  This is debugging code.

>+    if (scalar @visibility_values) {
>+        $field->set_visibility_values(\@visibility_values);
>+        $field->update();
>+    }

  Don't create() and then update(), particularly not outside of a transaction. Implement create() fully.

>+    my @visibility_values = $cgi->param('visibility_value_ids');
>+    $field->set_visibility_values(\@visibility_values) if scalar 
>+                                                          @visibility_values;

  Why that if?

>+use constant VISIBILITY_VALUE_DB_TABLE => 'fieldvisibilityvalue';

  You don't need this in a constant.

  The table should be called field_visibility.

>+    return [] if !$field;
>+    my $value_obj_ids = [];

  Why are you creating an arrayref instead of an array?

>+sub visibility_value_ids {

  This method should not exist at all. You should always only be returning Field::Choice objects.


  I don't currently have time to review the rest of this patch.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to hide/show field based on one or more field value (v1)

Here is my try at this feature. I took the previous patch and ported it to the latest trunk code. I also made a few fixes/improvements such as migration in Bugzilla/Install/DB.pm. Please review.

Dave

Comment 12

[Max Kanat-Alexander]

Comment on attachment 437954 [details] [diff] [review]
Patch to hide/show field based on one or more field value (v1)

  This patch has a lot of odd stuff in it, but I'm going to assume that that comes from the previous patch, and not directly from you.

  Overall I'm surprised at how little code was required to do this, though.

>=== modified file 'Bugzilla/DB/Schema.pm'
>+    field_visibility_values => {

  Let's make it singular.

>+        FIELDS => [
>+            field_id => {TYPE => 'INT3', 
>+                         REFERENCES => {TABLE  => 'fielddefs',
>+                                        COLUMN => 'id'}},
>+            visibility_value_id => {TYPE => 'INT2', NOTNULL => 1},
>+        ],
>+        INDEXES => [
>+            fielddefs_field_id_idx => ['field_id'],
>+            fielddefs_visibility_value_id_idx => ['visibility_value_id'],

  Those indexes need their names changed.

>=== modified file 'Bugzilla/Field.pm'
>+=item C<visibility_value_ids>

  As I believe I mentioned in the previous review, this accessor should not exist. Instead, _check_control_values should return objects, and only visibility_values should be used. There should be no visibility_value_ids.

> If we have a L</visibility_field>, then what value 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.
>+be set to in order to show this field? Returns an arrayref of
>+L<Bugzilla::Field::Choice> or an empty arrayref if there is no 
>+C<visibility_field> set.

  Needs to be changed grammatically to accommodate "values" instead of "value".

> [snip]
>+        my @visibility_values;     
>+        foreach my $value_id (@visibility_value_ids) {
>+            push(@visibility_values,
>+                Bugzilla::Field::Choice->type($self->visibility_field)->new($value_id));
>+        }

  You can use new_from_list, there.

>-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->{visibility_value_ids} 
>+        = $self->_check_control_values($value_ids, $self->{visibility_field_id});
>+    delete $self->{visibility_values};

  That change shouldn't be necessary--the validator will be called normally when you do set(). That's why it's in UPDATE_VALIDATORS.

>@@ -839,6 +883,9 @@
>+    # The field visibility values are stored in a separate table
>+    $self->delete_visibility_values();

  This method should be private.

>@@ -893,8 +940,16 @@
> =cut
> 
> sub create {
>-    my $class = shift;
>-    my $field = $class->SUPER::create(@_);
>+    my ($class, $params) = @_;
>+
>+    $class->check_required_create_fields($params);
>+    $params = $class->run_create_validators($params);
>+
>+    # These are not a fields in the bugs table, so we don't pass them to
>+    # insert_create_data.
>+    my $visibility_value_ids = delete $params->{visibility_value_ids};
>+
>+    my $field = $class->insert_create_data($params);

  You don't need this level of complexity. You can simply delete visibility_values from $params before calling SUPER::create, and then use its data after SUPER::create is done.

  (This is also a note that the argument to create() should now probably become visibility_values.)

>+    $field->update_visibility_values($visibility_value_ids) if $visibility_value_ids; 

  This method should be private.

>@@ -958,11 +1015,38 @@
>+        $self->delete_visibility_values($self->id);
>+    } 
>+    elsif (scalar $self->{visibility_value_ids}) {

  Why "scalar" on that?

>+sub update_visibility_values {
>+    my ($self, $visibility_value_ids) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    # If not set, fetch from self
>+    if (!defined $visibility_value_ids) {
>+        $visibility_value_ids = $self->{visibility_value_ids};
>+    }

  Why is this using a direct hash access instead of an accessor of some sort?

  Also, the block could be reduced to $visibility_values ||=

>+    $dbh->bz_start_transaction();
>+    $self->delete_visibility_values($self->id);
>+    for my $value_id (@$visibility_value_ids) {
>+        $dbh->do("INSERT INTO field_visibility_values (field_id, visibility_value_id)
>+                  VALUES (?,?)", undef, $self->id, $value_id);
>+    }
>+    $dbh->bz_commit_transaction();

  I'm not quite sure why this method creates a transaction all by itself. Seems like callers would always be in a transaction already. I mean, it's not harmful, but do make sure that the callers all have transactions externally.

>+}
>+
>+sub delete_visibility_values {
>+    my ($self, $id) = @_;

  This takes $self. Why would it need an $id argument?

>=== modified file 'Bugzilla/Install/DB.pm'
>@@ -602,6 +601,9 @@
>     # 2009-11-14 [e-mail] - Bug 310450
>     $dbh->bz_add_column('bugs_activity', 'comment_id', {TYPE => 'INT3'});
> 
>+    # 2010-04-05 [e-mail] - Bug XXXXXX
>+    _migrate_field_visibility_values();

  This is a change to fielddefs, so it needs to go into the fielddefs-updating code, not here.

>+sub _migrate_field_visibility_values {
>+    my $dbh = Bugzilla->dbh;
>+
>+    if ($dbh->bz_column_info('fielddefs', 'value_field_id')) {

  This should be checking for visibility_value_id instead.

>+        print "Populating new field_visibility_values table...\n";
>+
>+        $dbh->bz_start_transaction();
>+
>+        my $results = $dbh->selectall_arrayref(
>+            "SELECT fielddefs.id, fielddefs.visibility_value_id
>+               FROM fielddefs
>+              WHERE fielddefs.visibility_value_id IS NOT NULL", 

  You don't need to prefix all the column names with "fielddefs".

  Also, if you want, you could just do this as a selectcol_arrayref with {Columns=>[1,2]} and put it straight into a hash.

>+        my $insert_sth = $dbh->prepare(
>+            "INSERT INTO field_visibility_values (field_id, visibility_value_id) VALUES (?, ?)");

  Nit: Line too long.

>@@ -393,13 +393,21 @@
>+    var selected = false;
>+    for (var i = 0; i < values.length; i++) {
>+        if (bz_valueSelected(controller, values[i])) {
>+            selected = true;
>+            break;
>+        }
>+    }

  For the sake of efficiency, this should be re-implemented as bz_anyValueSelected, probably.

  I'm guessing that "values" will be shorted that the controller's options, so a faster loop would be to loop through all the controller's options and check each one to see if is contained in the "values".

>=== modified file 'template/en/default/admin/custom_fields/create.html.tmpl'
>+        <label for="visibility_value_ids"><strong>is set to:</strong></label>
>+        <select multiple="multiple" size="5" name="visibility_value_ids" id="visibility_value_ids" style="vertical-align: middle">

  style="" is, more or less, the Devil.

>=== modified file 'template/en/default/admin/custom_fields/edit.html.tmpl'
>+        <label for="visibility_value_ids"><strong>is set to:</strong></label>
>+        <select multiple="multiple" size="5" name="visibility_value_ids" 
>+                id="visibility_value_ids" style="vertical-align: middle">

  There too.

  And this can be called visibility_values.

>-                IF field.visibility_value.id == value.id %]>
>+              [% " selected" IF lsearch(field.visibility_value_ids, value.id) >= 0 %]>

  Don't use lsearch for that. Use "contains"--it's a custom array vmethod.

>=== modified file 'template/en/default/bug/field-events.js.tmpl'
>+  [%- FOREACH visibility_value = controlled_field.visibility_values -%]
>+                '[%- visibility_value.name FILTER js -%]',
>+  [%- END %]

  At some point, we need to change this to using IDs, so that it produces less HTML. Probably after this bug is fixed. (Or maybe before this bug is fixed?)

  One advantage of passing IDs is that we can use getElementById instead of using bz_valueSelected, which should be much faster.

>=== modified file 'template/en/default/bug/field.html.tmpl'
>+[% IF field.visibility_field.defined AND bug %]
>+  [% SET field_show = 0 %]
>+  [% FOREACH visibility_value = field.visibility_values %]
>+    [% SET field_show = 1 IF visibility_value.is_set_on_bug(bug) %]
>+  [% END %]
>+  [% SET hidden = 1 IF NOT field_show %]
> [% END %]

  This crazy logic reversal is not necessary. Just use hidden, don't create field_show.

Comment 13

[Tiago Mello [:timello]]

Hey David, any chance to update the patch?

Comment 14

[Max Kanat-Alexander]

Any hope of this being updated before the hard freeze?

Comment 15

[David Lawrence [:dkl]]

Seems like I was not able to find free time to revise this before my PTO starting this weekend. I am going to have time to work on it during my off time but will be out of internet contact til next sunday so if you need it before then it may need to be taken up by someone. Otherwise I will upload a new patch as soon as I return.

Dave

Comment 16

[Alex Eiser]

I'll take a stab at updating the patch, and correcting the issues identified by Max. I am currently about 50% complete. I should have a patch ready for review Monday Jun 29th.

Comment 17

[Alex Eiser]

Attachment: V3 Incorporating Maxs comments

Incorporated all of Maxs comments.

This does not address back button issues. 
EG: Mark a field as mandatory, and have it hidden and have no values on a record at page load. Then make the field "visible" and leave its value empty. Hit save. This brings up the mandatory field empty error. Hit back button, even though the field should be visible, its hidden again. Hitting save will still produce the error, since the visability_field is still set.

Comment 18

[Max Kanat-Alexander]

I think that at this point, it's not realistic that this is going to make 4.0. Sorry.

Comment 19

[Tiago Mello [:timello]]

Comment on attachment 454752 [details] [diff] [review]
V3 Incorporating Maxs comments

It is not working. The Error Console says:

Error: select_values is undefined
Source File: http://localhost/~timello/bugzilla/editfields.cgi?action=add
Line: 386

Comment 20

[Alex Eiser]

(In reply to comment #19)
> Comment on attachment 454752 [details] [diff] [review]
> V3 Incorporating Maxs comments
> 
> It is not working. The Error Console says:
> 
> Error: select_values is undefined
> Source File: http://localhost/~timello/bugzilla/editfields.cgi?action=add
> Line: 386

I am not getting that error, nor do I have anything related to select_values on that page at that line number... aside from adding a new custom field, what actions/database state where you in?

Comment 21

[Alex Eiser]

AKA, the line number will be different depending on the set of custom fields/drop downs/components etc you have. Can you post a copy of the "create file" page?

Comment 22

[Tiago Mello [:timello]]

(In reply to comment #21)
> AKA, the line number will be different depending on the set of custom
> fields/drop downs/components etc you have. Can you post a copy of the "create
> file" page?

nm, my fault, it's working. I'll continue with the tests.

Comment 23

[Alex Eiser]

Comment on attachment 454752 [details] [diff] [review]
V3 Incorporating Maxs comments

just removing the - on timello reviews given previous comment.

Comment 24

[Tiago Mello [:timello]]

Comment on attachment 454752 [details] [diff] [review]
V3 Incorporating Maxs comments

>=== modified file 'Bugzilla/DB/Schema.pm'
>@@ -688,6 +687,22 @@
>+    field_visibility_value => {

Maybe it could be field_visibility_values instead?

>=== modified file 'Bugzilla/Field.pm'
>@@ -613,16 +615,28 @@
>-sub visibility_value {
>+sub visibility_values {
>+        if (scalar @$visibility_value_ids) {
>+            $visibility_values = Bugzilla::Field::Choice->type($self->visibility_field)->new_from_list($visibility_value_ids);

This line is too long.

>@@ -700,10 +714,13 @@
>sub is_visible_on_bug {
>    my ($self, $bug) = @_;
>
>-    my $visibility_value = $self->visibility_value;
>-    return 1 if !$visibility_value;
>-
>-    return $visibility_value->is_set_on_bug($bug);
>+    my $visibility_values = $self->visibility_values;
>+    return 1 if !$visibility_values;

This is wrong because you don't return an undef but a [] if
there is no visibility values. You should do:

return 1 if !scalar @$visibility_values;

>@@ -806,12 +823,23 @@
>+sub set_visibility_values {
>+    my ($self, $value_ids) = @_;
>+    $self->set('visibility_values', $value_ids);
>+    # update the cached values of visability_values for update
>+    
>+    # set is going to change $self->{visibility_values} to a list of IDs, convert back to objects

Break the comment into more lines. I think the blank line between the comments shouldn't exist, right?

>@@ -949,7 +980,12 @@
>    # the parameter isn't sent to create().
>    $params->{sortkey} = undef if !exists $params->{sortkey};
>    $params->{type} ||= 0;
>+    
>+    my $visibility_values = delete $params->{visibility_values};
>+
>    my $field = $class->SUPER::create(@_);
>+    $field->set_visibility_values($visibility_values);
>+    $field->update_visibility_values();

This method is not used outside of Field.pm so I think it should be
an internal method: _update_visibility_values. Also you shouldn't be
using the same method for both: create and update. There is unnecessary
code in the method when creating a field.

>@@ -978,12 +1014,28 @@
>+sub update_visibility_values {
>+    my $visibility_value_ids = [map($_->id, @{$self->visibility_values})];

Use:
    my @visibility_value_ids = map($_->id, @{$self->visibility_values});
Instead.

>+    $self->_delete_visibility_values($self->id);
>+    for my $value_id (@$visibility_value_ids) {
>+        $dbh->do("INSERT INTO field_visibility_value (field_id, visibility_value_id)

The line is too long.

>+sub _delete_visibility_values {
>+    my ($self, $id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    $dbh->do("DELETE FROM field_visibility_value WHERE field_id = ?", undef, $id);

The line is too long too.

>=== modified file 'Bugzilla/Install/DB.pm'

How about _add_visiblity_value_to_value_tables method, shouldn't it be changed somehow?

>=== modified file 'template/en/default/admin/custom_fields/create.html.tmpl'
>@@ -130,8 +130,8 @@
>+        <select multiple="multiple" size="5" name="visibility_values" id="visibility_values" class="field_value">

Lets break this line:

<select multiple="multiple" size="5" name="visibility_values"
        id="visibility_values" class="field_value">

Comment 25

[Tiago Mello [:timello]]

Hey Alex, are you available to update the patch? If not, I can update it. Please, let me know. Thanks.

Comment 26

[Max Kanat-Alexander]

Comment on attachment 454752 [details] [diff] [review]
V3 Incorporating Maxs comments

Hmm, I frequently have trouble typing the word "visibility" (so many I's...). Perhaps we should just name the table field_vis_value instead? (It should be singular, though, contrary to timello's comment.)

You shouldn't be updating the cache inside set_, you should just be deleting it.

You might want to check out "any" in List::MoreUtils.

Those are just a few comments from a quick look-over.

Comment 27

[Alex Eiser]

(In reply to comment #26)
> Comment on attachment 454752 [details] [diff] [review]
> V3 Incorporating Maxs comments
> 
> Hmm, I frequently have trouble typing the word "visibility" (so many I's...).
> Perhaps we should just name the table field_vis_value instead? (It should be
> singular, though, contrary to timello's comment.)

I dont really love the idea of using "vis" as does not obviously mean a shortening of visibility. I can probably just do a find/replace and upload both patches... but I would rather some one else maybe timello agree/disagree with this change.

> 
> You shouldn't be updating the cache inside set_, you should just be deleting
> it.

The problem is that visibility_values is a list of objects, not a list of IDs. After Set, its a list of IDs, and is in a bad state for people expecting to call visibility_values and get objects. Bellow is a quote from an IM conversation on this topic.

7:01:50 PM Alex: You made a large point in the bug that you don't want an accessor for visibility_value_ids. Which makes sense. But I am little concerned about the "Set" semantics. Do you set an internal hash of visibility_value_ids, or do you force it to populate the visability_values content with real objects? Ii can do it either way, but wondering if you have a preference
7:02:45 PM Max: Modify the visibility_values content.
7:02:59 PM Max: That will make it simpler in update().
7:03:19 PM Max: (That's also what we do in other similar situations.)

I can change it so that there is a different call for IDs vs Objects.

RE: Timello: I can update the patch.

Comment 28

[Max Kanat-Alexander]

Oh, I see what you're talking about, with the visibility_values. We solve this same problem in many other places, though. The validator should return objects--that will solve the problem, no?

Comment 29

[Alex Eiser]

(In reply to comment #28)
> Oh, I see what you're talking about, with the visibility_values. We solve this
> same problem in many other places, though. The validator should return
> objects--that will solve the problem, no?

That makes sense. And of course is obvious in hind sight. I will make the changes, and upload a new patch as soon as I have retested.

Note: RE _add_visiblity_value_to_value_tables. The entire function will be removed, since its no longer relevant.

Comment 30

[Alex Eiser]

Attachment: V4 Added Timello's & Max's comments

Comment 31

[Alex Eiser]

The new patch corrects the following:
1) Fixes a delete field foreign key issue
2) removes _add_visiblity_value_to_value_tables
3) cleans up the set_visability_values manipulation of the cache object.

notes: 
A) Left the shared code alone between create/update. The amount of unneeded activity (delete by fieldid) is very small, and with the index... should not cause any issues.
B) return 1 if !scalar @$visibility_values; This behavior was incorrect because it meant that if you had zero values selected the object is visible... but the JS requires a value to be selected for the code to be visible. Changed to check to see if their is a visibility_field instead.
C) Did not add the s to database table per Max's instructions.

Will upload the "vis" version shortly.

Comment 32

[Alex Eiser]

Attachment: V4B (find/replace for DB column)

This is the alternate to "Attachment 464702 [details] [diff]: V4 Added Timello's & Max's comments"
I don't like the fact that only the DB column uses vis, and I think changing all the calls from visibility_values to vis_values will make the code more unclear then it needs to be.

Comment 33

[Tiago Mello [:timello]]

Comment on attachment 464702 [details] [diff] [review]
V4 Added Timello's & Max's comments

>=== modified file 'Bugzilla/Field.pm'
>@@ -366,11 +365,16 @@
>     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;
>+    my @visibility_values;
>+    my $choice = Bugzilla::Field::Choice->type($field);
>+    foreach my $value_id (@$value_ids) {

At least one value must be selected. If a field has been selected, it should
have a value, otherwise, it should throw an error.

>@@ -700,10 +717,12 @@
> sub is_visible_on_bug {
>     my ($self, $bug) = @_;
> 
>-    my $visibility_value = $self->visibility_value;
>-    return 1 if !$visibility_value;
>+    my $visibility_values = $self->visibility_values;
>+    return 1 unless $self->{visibility_field_id}; # always true if not visiability controlled

You shouldn't be allowing the user to save the custom field with no visibility value selected.
It should return an error saying a value must be selected for the visibility field, and then, the line above
will be unnecessary because 'any' below will check that.

>@@ -978,12 +1005,29 @@
>+    $dbh->do("DELETE FROM field_visibility_value WHERE field_id = ?", undef, $self->id);

This is still too long.

Comment 34

[Tiago Mello [:timello]]

(In reply to comment #32)
> Created attachment 464703 [details] [diff] [review]
> V4B (find/replace for DB column)
> 
> This is the alternate to "Attachment 464702 [details] [diff]: V4 Added Timello's & Max's
> comments"
> I don't like the fact that only the DB column uses vis, and I think changing
> all the calls from visibility_values to vis_values will make the code more
> unclear then it needs to be.

Also, I think vis_values is hard to understand. I prefer visibility.

Comment 35

[Alex Eiser]

(In reply to comment #33)
> Comment on attachment 464702 [details] [diff] [review]
> V4 Added Timello's & Max's comments
> 
> >=== modified file 'Bugzilla/Field.pm'
> >@@ -366,11 +365,16 @@
> >     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;
> >+    my @visibility_values;
> >+    my $choice = Bugzilla::Field::Choice->type($field);
> >+    foreach my $value_id (@$value_ids) {
> 
> At least one value must be selected. If a field has been selected, it should
> have a value, otherwise, it should throw an error.

Sure.

> 
> >@@ -700,10 +717,12 @@
> > sub is_visible_on_bug {
> >     my ($self, $bug) = @_;
> > 
> >-    my $visibility_value = $self->visibility_value;
> >-    return 1 if !$visibility_value;
> >+    my $visibility_values = $self->visibility_values;
> >+    return 1 unless $self->{visibility_field_id}; # always true if not visiability controlled
> 
> You shouldn't be allowing the user to save the custom field with no visibility
> value selected.
> It should return an error saying a value must be selected for the visibility
> field, and then, the line above
> will be unnecessary because 'any' below will check that.

I disagree. Not all cases that check is_visible_on_bug actually check to see if the field should hidden by a visabilty_value_field. I think is_visible_on_bug should return the correct value when ever its called. This check just says yes I am visible, because I am not being hidden by another field. Not their are no values selected so I am not visible.


> 
> >@@ -978,12 +1005,29 @@
> >+    $dbh->do("DELETE FROM field_visibility_value WHERE field_id = ?", undef, $self->id);
> 
> This is still too long.

My column inidicator for being over sized was set to 100, not 80. Sorry about that. 

I will fix the 1,3d changes and resubmit.

Comment 36

[Alex Eiser]

Attachment: V5

Added user error code for not selecting any visibility values.
Moved check on create, to "fail fast" and not create a record if the previous error condition is met.
Simplified a check in field.html.tmpl to no longer look for if the field has a value_field before calling is_visible_on_bug

Comment 37

[Tiago Mello [:timello]]

Comment on attachment 465662 [details] [diff] [review]
V5

>=== modified file 'Bugzilla/Field.pm'
>@@ -366,11 +365,28 @@
>+
>+    if (scalar @visibility_values == 0) {

The verification shouldn't happen at the end. Use @$value_ids and put it right after the:

return [] if !$field;

if (!scalar @$value_ids) {
...
}

>+        ThrowUserError('controlled_field_requires_visibility_values', { field_name => $field_name });

I think the name should be: 'visibility_values_must_be_selected'.
Also $field is already defined at this point, so just use:

ThrowUserError('visibility_values_must_be_selected',
               { field => $field });

Then in the template, use [% field.description ...

>@@ -700,10 +730,14 @@
> sub is_visible_on_bug {
>+    # always return visible, if this field is not visability controlled

visability?

>@@ -949,7 +986,15 @@
>+    my $visibility_values = delete $params->{visibility_values};

You have to delete it within run_create_validators, if you do it here, the
validator will fail.

You should override the method like this:

sub run_create_validators {
    my ($class, $params) = @_;
    my $field_values = $class->SUPER::run_create_validators($params);
    delete $field_values->{visibility_values};
    return $field_values;
}

We have an example in the Version.pm which is similar... we need to run the
validator but the field does not exist in the table and then we have to delete
it before inserting the data into the database.

>+    # since _check_visibility_values can fail, run the validate step 
>+    # BEFORE running create.
>+    $class->_check_visibility_values($visibility_values, undef, $params);

You don't need that. The validator is called already by the superclass.
Otherwise, it will be called twice.

>=== modified file 'template/en/default/global/user-error.html.tmpl'
>@@ -459,6 +459,11 @@
>+  [% ELSIF error == "controlled_field_requires_visibility_values" %]
>+    [% title = "Custom Field Will Never Be Displayed" %]

I think the title should mention what is the problem and not what will happen.

>+    '[% field_name FILTER html %]' will never be shown to users because you

[% field.name FILTER html %] as per comment above.

>+    have not selected any values for when the field should show up.

any value*?

Comment 38

[Alex Eiser]

(In reply to comment #37)
> Comment on attachment 465662 [details] [diff] [review]
> V5
> 
> >=== modified file 'Bugzilla/Field.pm'
> >@@ -366,11 +365,28 @@
> >+
> >+    if (scalar @visibility_values == 0) {
> 
> The verification shouldn't happen at the end. Use @$value_ids and put it right
> after the:
> 
> return [] if !$field;
> 
> if (!scalar @$value_ids) {
> ...
> }


I put it after since it uses the actual "verified/validated" field objects... instead of relaying on the unverified input from the user. Since this is an admin area, we can probably "trust" our admins more then random users... so I don't disagree with the suggestion... but I chose to put it at the bottom to perform the check on what would actually be saved, not what was passed in.



> 
> >+        ThrowUserError('controlled_field_requires_visibility_values', { field_name => $field_name });
> 
> I think the name should be: 'visibility_values_must_be_selected'.
> Also $field is already defined at this point, so just use:
> 
> ThrowUserError('visibility_values_must_be_selected',
>                { field => $field });
> 
> Then in the template, use [% field.description ...
> 

the field object in this case is the "visability_field" not the actual object that the check is being made on. Worse yet the check can happen on create, or on update. In the create case, no object exists. For those reasons I decided to use field_name and set the specific name not use the object.

> >@@ -700,10 +730,14 @@
> > sub is_visible_on_bug {
> >+    # always return visible, if this field is not visability controlled
> 
> visability?

oops.


> 
> >@@ -949,7 +986,15 @@
> >+    my $visibility_values = delete $params->{visibility_values};
> 
> You have to delete it within run_create_validators, if you do it here, the
> validator will fail.
> 
> You should override the method like this:
> 
> sub run_create_validators {
>     my ($class, $params) = @_;
>     my $field_values = $class->SUPER::run_create_validators($params);
>     delete $field_values->{visibility_values};
>     return $field_values;
> }
> 
> We have an example in the Version.pm which is similar... we need to run the
> validator but the field does not exist in the table and then we have to delete
> it before inserting the data into the database.
> 

Sure, that makes sense.


> >+    # since _check_visibility_values can fail, run the validate step 
> >+    # BEFORE running create.
> >+    $class->_check_visibility_values($visibility_values, undef, $params);
> 
> You don't need that. The validator is called already by the superclass.
> Otherwise, it will be called twice.
> 

The validator does get called twice, but if I don't call it here, the object will be created and unless I wrap the entire thing in a transaction you will end up with an object that is invalid. (visability_field, but no values). I will move the check into the 
run_create_validators call... but I think its still required.. or a transaction. You choose.

> >=== modified file 'template/en/default/global/user-error.html.tmpl'
> >@@ -459,6 +459,11 @@
> >+  [% ELSIF error == "controlled_field_requires_visibility_values" %]
> >+    [% title = "Custom Field Will Never Be Displayed" %]
> 
> I think the title should mention what is the problem and not what will happen.
> 
> >+    '[% field_name FILTER html %]' will never be shown to users because you
> 

sure.

> [% field.name FILTER html %] as per comment above.
> 
> >+    have not selected any values for when the field should show up.
> 
> any value*?

admins may not really know regular expressions. we could put...
any value(s)

Comment 39

[Tiago Mello [:timello]]

(In reply to comment #38)
> (In reply to comment #37)
> > Comment on attachment 465662 [details] [diff] [review] [details]
> > V5
> > 
> > >=== modified file 'Bugzilla/Field.pm'
> > >@@ -366,11 +365,28 @@
> > >+
> > >+    if (scalar @visibility_values == 0) {
> > 
> > The verification shouldn't happen at the end. Use @$value_ids and put it right
> > after the:
> > 
> > return [] if !$field;
> > 
> > if (!scalar @$value_ids) {
> > ...
> > }
> 
> 
> I put it after since it uses the actual "verified/validated" field objects...
> instead of relaying on the unverified input from the user. Since this is an
> admin area, we can probably "trust" our admins more then random users... so I
> don't disagree with the suggestion... but I chose to put it at the bottom to
> perform the check on what would actually be saved, not what was passed in.

We don't need to care about validated data at that point... we just need to know if any value has been passed... that's the way we normally do it in Bugzilla. First we check if the parameters have been passed as expected and then we validate them.

> 
> 
> 
> > 
> > >+        ThrowUserError('controlled_field_requires_visibility_values', { field_name => $field_name });
> > 
> > I think the name should be: 'visibility_values_must_be_selected'.
> > Also $field is already defined at this point, so just use:
> > 
> > ThrowUserError('visibility_values_must_be_selected',
> >                { field => $field });
> > 
> > Then in the template, use [% field.description ...
> > 
> 
> the field object in this case is the "visability_field" not the actual object
> that the check is being made on. Worse yet the check can happen on create, or
> on update. In the create case, no object exists. For those reasons I decided to
> use field_name and set the specific name not use the object.

Indeed! Well, I think for simplicity propose we can just mention the selected field saying it needs a at least one value. We know that is related to the custom field we are trying to create/edit.

> > 
> > >@@ -949,7 +986,15 @@
> > >+    my $visibility_values = delete $params->{visibility_values};
> > 
> > You have to delete it within run_create_validators, if you do it here, the
> > validator will fail.
> > 
> > You should override the method like this:
> > 
> > sub run_create_validators {
> >     my ($class, $params) = @_;
> >     my $field_values = $class->SUPER::run_create_validators($params);
> >     delete $field_values->{visibility_values};
> >     return $field_values;
> > }
> > 
> > We have an example in the Version.pm which is similar... we need to run the
> > validator but the field does not exist in the table and then we have to delete
> > it before inserting the data into the database.
> > 
> 
> Sure, that makes sense.

For this one, instead of overriding the method above we are going to call check_required_values, run_create_validators, insert_create_data individually instead of calling SUPER::create. 

> 
> > [% field.name FILTER html %] as per comment above.
> > 
> > >+    have not selected any values for when the field should show up.
> > 
> > any value*?
> 
> admins may not really know regular expressions. we could put...
> any value(s)

Yeah, sorry... I didn't mean '*' but that it should be in the singular and not in plural.

Comment 40

[Tiago Mello [:timello]]

Attachment: v6

Taking the bug since Alex permitted.

Comment 41

[Max Kanat-Alexander]

Comment on attachment 465929 [details] [diff] [review]
v6

>=== modified file 'Bugzilla/DB/Schema.pm'
>+    field_visibility_value => {

  Why not just call it field_visibility?

>+        FIELDS => [
>+            field_id => {TYPE => 'INT3', 
>+                         REFERENCES => {TABLE  => 'fielddefs',
>+                                        COLUMN => 'id'}},

  That should have a DELETE => 'CASCADE'.

>+            visibility_value_id => {TYPE => 'INT2', NOTNULL => 1},

  Let's just call it value_id.

>+        ],
>+        INDEXES => [
>+            field_visibility_value_field_id_idx => ['field_id'],
>+            field_visibility_value_visibility_value_id_idx => 
>+                ['visibility_value_id'],

  One of those should be a two-column index with a UNIQUE constraint on it.

  Why do you need an index on visibility_value_id?

>=== modified file 'Bugzilla/Field.pm'
>+sub visibility_values {
>+        my $visibility_value_ids = Bugzilla->dbh->selectcol_arrayref(
>+            "SELECT visibility_value_id
>+             FROM field_visibility_value
>+             WHERE field_id = ?",
>+            undef, $self->id
>+        );

  Nit: SQL isn't aligned correctly.

  Nit: There's no reason for "undef, $self->id" and ");" to have their own lines. Actually, there's probably not even a reason for "WHERE field_id = ?" to have its own line.

>+        my $visibility_values = [];

  Why not just do my @visibility_values?

>+
>+        if (scalar @$visibility_value_ids) {

  You don't need that if. new_from_list can deal with an empty list for you.

>@@ -700,10 +725,15 @@
>+    # Always return visible, if this field is not
>+    # visibility controlled.
>+    return 1 unless $self->{visibility_field_id};

  Nit: I think "if !" would be clearer there than "unless".

>+    my $visibility_values = $self->visibility_values;
>+    return 1 if any { $_->is_set_on_bug($bug) } @$visibility_values;
>+
>+    # If reached this point, return false.
>+    return 0;

  You could just put a ? 1 : 0 after the "any" call, too, no? But maybe the "return 0" this way is clearer.

>+    # The field visibility values are stored in a separate table
>+    # delete first to avoid foreign key issues.
>+    $self->_delete_visibility_values();

  With DELETE => 'CASCADE', you won't need that.

>@@ -945,13 +978,24 @@
> 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;

  Note: We need to fix this later--the sortkey line is no longer necessary (because of the changes to check_required_create_fields) and the {type} line shouldn't be necessary either anymore, for the same reason. Then the $params = @_ line above could go away too.

>-    my $field = $class->SUPER::create(@_);
>+    
>+    $dbh->bz_start_transaction();
>+    $class->check_required_create_fields($params);

  You should pass @_ there, not just $params, in case the API changes some day.

>+    my $field_values      = $class->run_create_validators($params);
>+    my $visibility_values = delete $field_values->{visibility_values};
>+    my $field             = $class->insert_create_data($field_values);
>+    $dbh->bz_commit_transaction();

  Don't commit the transaction before you insert the visibility values.

>+    my @visibility_value_ids = map { $_->id } @$visibility_values;
>+    $field->set_visibility_values(\@visibility_value_ids);
>+    $field->_update_visibility_values();

  That's pretty clever. However, you're going to run the validator twice, this way. Instead, you should allow the validator to accept objects, and just pass the objects.

>@@ -978,12 +1022,32 @@
>     my $self = shift;
>     my $changes = $self->SUPER::update(@_);
>     my $dbh = Bugzilla->dbh;
>-    if ($changes->{value_field_id} && $self->is_select) {
>-        $dbh->do("UPDATE " . $self->name . " SET visibility_value_id = NULL");
>-    }
>+    $self->_update_visibility_values();

  You deleted code for value_field_id and then added code for visibility_field_id, there.

>+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_value 
>+                 (field_id, visibility_value_id) VALUES (?,?)",
>+            undef, $self->id, $value_id
>+        );

  Nit: The ");" doesn't need its own line.

>=== modified file 'Bugzilla/Install/DB.pm'
>+        my $insert_sth = $dbh->prepare(
>+            "INSERT INTO field_visibility_value 
>+                (field_id, visibility_value_id) VALUES (?, ?)"
>+        );

  Nit: ); on its own line.

>+        foreach my $id (keys %results) {
>+            $insert_sth->execute($id, $results{$id});
>+        }
>+
>+        $dbh->bz_drop_column('fielddefs', 'visibility_value_id');
>+
>+        $dbh->bz_commit_transaction();

  You should drop the column outside the transaction, because dropping a column implicitly commits a transaction.

>=== modified file 'template/en/default/admin/custom_fields/create.html.tmpl'
>+        <label for="visibility_values"><strong>is set to:</strong></label>

  Let's make it say "is set to any of:"

>=== modified file 'template/en/default/admin/custom_fields/edit.html.tmpl'
>+        <label for="visibility_values"><strong>is set to:</strong></label>

  And there to.

>+        [% SET visibility_value_ids = [] %]
>+        [% FOREACH value = field.visibility_values %]
>+          [% visibility_value_ids.push(value.id) %]
>+        [% END %]

  You don't need to do that, because "contains" has special magic for Bugzilla::Object instances.

>+              [% " selected" IF visibility_value_ids.contains(value.id) %]>

  So that can just be visibility_values.contains(value)

>=== modified file 'template/en/default/bug/field-events.js.tmpl'
>-                '[% 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 -%]',
>+  [%- END %]

  Wow, all that extra indentation is not necessary. :-)

  This will produce an extra comma a the end of the list, which will break IE. You can check UNLESS loop.last.


  Very nice patch, in general. Most of the above needs to be fixed before this can be r+ though.

Comment 42

[Tiago Mello [:timello]]

(In reply to comment #41)
> Comment on attachment 465929 [details] [diff] [review]
> v6
> >=== modified file 'Bugzilla/Field.pm'
> 
> >@@ -945,13 +978,24 @@
> > 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;
> 
>   Note: We need to fix this later--the sortkey line is no longer necessary
> (because of the changes to check_required_create_fields) and the {type} line
> shouldn't be necessary either anymore, for the same reason. Then the $params =
> @_ line above could go away too.

Do you want me to change that in this patch?

> 
> >@@ -978,12 +1022,32 @@
> >     my $self = shift;
> >     my $changes = $self->SUPER::update(@_);
> >     my $dbh = Bugzilla->dbh;
> >-    if ($changes->{value_field_id} && $self->is_select) {
> >-        $dbh->do("UPDATE " . $self->name . " SET visibility_value_id = NULL");
> >-    }
> >+    $self->_update_visibility_values();
> 
>   You deleted code for value_field_id and then added code for
> visibility_field_id, there.

I didn't get that.

Comment 43

[Max Kanat-Alexander]

(In reply to comment #42)
> >   Note: We need to fix this later--the sortkey line is no longer necessary
> > (because of the changes to check_required_create_fields) and the {type} line
> > shouldn't be necessary either anymore, for the same reason. Then the $params =
> > @_ line above could go away too.
> 
> Do you want me to change that in this patch?

  No, let's file a separate bug.
> > >-    if ($changes->{value_field_id} && $self->is_select) {
> > >-        $dbh->do("UPDATE " . $self->name . " SET visibility_value_id = NULL");
> > >-    }
> > >+    $self->_update_visibility_values();
> > 
> >   You deleted code for value_field_id and then added code for
> > visibility_field_id, there.
> 
> I didn't get that.

  Look at the code you deleted. You deleted code that should not be deleted as part of this patch.

Comment 44

[Tiago Mello [:timello]]

Attachment: v7

Comment 45

[Tiago Mello [:timello]]

Attachment: v8

Dang! Fixing an error in the code.

Comment 46

[Max Kanat-Alexander]

Comment on attachment 469327 [details] [diff] [review]
v8

>=== modified file 'Bugzilla/Field.pm'
>+sub visibility_values {
>+    if (!defined $self->{visibility_values}) {
>+        return [] unless $self->{visibility_field_id};

  That return can probably be outside of the if, no?

  Other than that, this looks great! Thanks, timello! :-)

Comment 47

[Tiago Mello [:timello]]

(In reply to comment #46)
> Comment on attachment 469327 [details] [diff] [review]
> v8
> 
> >=== modified file 'Bugzilla/Field.pm'
> >+sub visibility_values {
> >+    if (!defined $self->{visibility_values}) {
> >+        return [] unless $self->{visibility_field_id};
> 
>   That return can probably be outside of the if, no?

Yes. Lets do:

return [] if !$self->{visibility_field_id};

Instead. Outside of the if.

Comment 48

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                              
modified editfields.cgi
modified Bugzilla/Field.pm                                                                                                                                
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified js/field.js
modified template/en/default/admin/custom_fields/cf-js.js.tmpl
modified template/en/default/admin/custom_fields/create.html.tmpl
modified template/en/default/admin/custom_fields/edit.html.tmpl
modified template/en/default/bug/field-events.js.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7446.

Comment 49

[Max Kanat-Alexander]

This is breaking the checksetup tests:

http://tinderbox.mozilla.org/showlog.cgi?log=Bugzilla/1282880632.1282885996.25969.gz

Comment 50

[Tiago Mello [:timello]]

Attachment: Adding visibility_value_id to the standard fields

Comment 51

[Max Kanat-Alexander]

Comment on attachment 469876 [details] [diff] [review]
Adding visibility_value_id to the standard fields

Wait, I don't understand this. Why should we need to add this for the standard fields here? I mean, what does this even have to do with this patch?

Comment 52

[Max Kanat-Alexander]

Comment on attachment 469327 [details] [diff] [review]
v8

>=== modified file 'Bugzilla/Install/DB.pm'
>+    # 2010-04-05 [e-mail] - Bug 479400
>+    _migrate_field_visibility_value();

  The problem is that this should not be here.

>-    _add_visiblity_value_to_value_tables();
>-

  And that should not be removed.

  You should always be adding function to the *end* of update_table_definitions, unless that's completely impossible.

Comment 53

[Tiago Mello [:timello]]

Attachment: Bringing _add_visiblity_value_to_value_tables() back.

Comment 54

[Max Kanat-Alexander]

Comment on attachment 470071 [details] [diff] [review]
Bringing _add_visiblity_value_to_value_tables() back.

Sweet. :-)

Comment 55

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                                             
modified Bugzilla/Install/DB.pm
Committed revision 7448.

Comment 56

[Frank Becker]

We have a problem with the latest commited version. Now Webservice Bug.fields did not work.

Line 114 from Bug.pm 
my $vis_value = $field->visibility_value;
did no longer work there is an visibility_values but this is a list

Comment 57

[Max Kanat-Alexander]

(In reply to comment #56)
> Line 114 from Bug.pm 
> my $vis_value = $field->visibility_value;
> did no longer work there is an visibility_values but this is a list

  Okay. Could you file a separate bug about that?

Comment 58

[Frédéric Buclin]

Selenium script attached to bug 308253.

Comment 59

[Frédéric Buclin]

Added to relnotes in bug 713346.

Comment 60

[Albert Ting]

For the record, I tried the patches with 4.0.3, seems to work as expected.  Any issues I may not be aware of?