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

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Administration
P1
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: bigstijn, Assigned: timello)

Tracking

({selenium})

3.3.2
Bugzilla 4.2
selenium
Bug Flags:
approval +
testcase +

Details

(Whiteboard: [es-redhat])

Attachments

(2 attachments, 10 obsolete attachments)

v8
17.88 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
1.26 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Depends on: 371995

Comment 1

9 years ago
a duplicate of bug 458436

Comment 2

9 years ago
(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.
Assignee: general → administration
Status: UNCONFIRMED → NEW
Component: Bugzilla-General → Administration
Ever confirmed: true
Version: unspecified → 3.3.2

Comment 3

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

Updated

9 years ago
Priority: -- → P1

Updated

9 years ago
Duplicate of this bug: 494394

Comment 5

9 years ago
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

9 years ago
(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.

Updated

8 years ago
Duplicate of this bug: 520757

Comment 8

8 years ago
Created attachment 405394 [details] [diff] [review]
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.
Attachment #405394 - Flags: review?(mkanat)

Comment 9

8 years ago
Created attachment 405401 [details] [diff] [review]
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.
Attachment #405394 - Attachment is obsolete: true
Attachment #405401 - Flags: review?(mkanat)
Attachment #405394 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #405401 - Flags: review?(mkanat) → review?(LpSolit)

Comment 10

8 years ago
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.
Attachment #405401 - Flags: review?(LpSolit) → review-
Created attachment 437954 [details] [diff] [review]
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
Attachment #437954 - Flags: review?(mkanat)

Comment 12

8 years ago
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 dkl@redhat.com - Bug 310450
>     $dbh->bz_add_column('bugs_activity', 'comment_id', {TYPE => 'INT3'});
> 
>+    # 2010-04-05 dkl@redhat.com - 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.
Attachment #437954 - Flags: review?(mkanat) → review-
(Assignee)

Comment 13

8 years ago
Hey David, any chance to update the patch?

Updated

8 years ago
Assignee: administration → dkl
Target Milestone: --- → Bugzilla 3.8

Updated

8 years ago
Status: NEW → ASSIGNED

Comment 14

8 years ago
Any hope of this being updated before the hard freeze?
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

8 years ago
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

8 years ago
Created attachment 454752 [details] [diff] [review]
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.
Attachment #454752 - Flags: review?(mkanat)

Comment 18

8 years ago
I think that at this point, it's not realistic that this is going to make 4.0. Sorry.
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2
(Assignee)

Comment 19

8 years ago
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
Attachment #454752 - Flags: review-

Comment 20

8 years ago
(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

8 years ago
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?
(Assignee)

Comment 22

8 years ago
(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

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

just removing the - on timello reviews given previous comment.
Attachment #454752 - Flags: review- → review?

Updated

8 years ago
Attachment #454752 - Flags: review? → review?(timello)

Updated

8 years ago
Summary: Show or hide particular custom fields based on multiple values of another field → Show or hide particular custom fields based on multiple values of another field (visibility controllers)

Updated

8 years ago
Whiteboard: [es-redhat]

Updated

8 years ago
Assignee: dkl → Alex.Eiser
(Assignee)

Comment 24

8 years ago
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">
Attachment #454752 - Flags: review?(timello) → review-
(Assignee)

Comment 25

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

Comment 26

8 years ago
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.
Attachment #454752 - Flags: review?(mkanat) → review-

Comment 27

8 years ago
(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

8 years ago
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

8 years ago
(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

8 years ago
Created attachment 464702 [details] [diff] [review]
V4 Added Timello's & Max's comments
Attachment #454752 - Attachment is obsolete: true
Attachment #464702 - Flags: review?(timello)

Updated

8 years ago
Attachment #464702 - Flags: review?(mkanat)

Comment 31

8 years ago
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.

Updated

8 years ago
Attachment #464702 - Flags: review?(mkanat)

Comment 32

8 years ago
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.
(Assignee)

Comment 33

8 years ago
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.
Attachment #464702 - Flags: review?(timello) → review-
(Assignee)

Comment 34

8 years ago
(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

8 years ago
(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

8 years ago
Created attachment 465662 [details] [diff] [review]
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
Attachment #464702 - Attachment is obsolete: true
Attachment #464703 - Attachment is obsolete: true
Attachment #465662 - Flags: review?
(Assignee)

Comment 37

8 years ago
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*?
Attachment #465662 - Flags: review? → review-

Comment 38

8 years ago
(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)
(Assignee)

Comment 39

8 years ago
(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.
(Assignee)

Comment 40

8 years ago
Created attachment 465929 [details] [diff] [review]
v6

Taking the bug since Alex permitted.
Assignee: Alex.Eiser → timello
Attachment #405401 - Attachment is obsolete: true
Attachment #437954 - Attachment is obsolete: true
Attachment #465662 - Attachment is obsolete: true
Attachment #465929 - Flags: review?(mkanat)

Comment 41

8 years ago
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.
Attachment #465929 - Flags: review?(mkanat) → review-
(Assignee)

Comment 42

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 44

8 years ago
Created attachment 469323 [details] [diff] [review]
v7
Attachment #465929 - Attachment is obsolete: true
Attachment #469323 - Flags: review?(mkanat)
(Assignee)

Comment 45

8 years ago
Created attachment 469327 [details] [diff] [review]
v8

Dang! Fixing an error in the code.
Attachment #469323 - Attachment is obsolete: true
Attachment #469327 - Flags: review?(mkanat)
Attachment #469323 - Flags: review?(mkanat)

Comment 46

8 years ago
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! :-)
Attachment #469327 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval+

Updated

8 years ago
Keywords: relnote
(Assignee)

Comment 47

8 years ago
(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.
(Assignee)

Comment 48

8 years ago
Committing to: bzr+ssh://timello%40gmail.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 49

8 years ago
This is breaking the checksetup tests:

http://tinderbox.mozilla.org/showlog.cgi?log=Bugzilla/1282880632.1282885996.25969.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 50

8 years ago
Created attachment 469876 [details] [diff] [review]
Adding visibility_value_id to the standard fields
Attachment #469876 - Flags: review?(mkanat)

Comment 51

8 years ago
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?
Attachment #469876 - Flags: review?(mkanat) → review-

Comment 52

8 years ago
Comment on attachment 469327 [details] [diff] [review]
v8

>=== modified file 'Bugzilla/Install/DB.pm'
>+    # 2010-04-05 dkl@redhat.com - 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.
(Assignee)

Comment 53

8 years ago
Created attachment 470071 [details] [diff] [review]
Bringing _add_visiblity_value_to_value_tables() back.
Attachment #469876 - Attachment is obsolete: true
Attachment #470071 - Flags: review?(mkanat)

Comment 54

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

Sweet. :-)
Attachment #470071 - Flags: review?(mkanat) → review+
(Assignee)

Comment 55

8 years ago
Committing to: bzr+ssh://timello%40gmail.com@bzr.mozilla.org/bugzilla/trunk/                                                                                                             
modified Bugzilla/Install/DB.pm
Committed revision 7448.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 56

7 years ago
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

7 years ago
(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?

Updated

6 years ago
Flags: testcase?

Comment 58

6 years ago
Selenium script attached to bug 308253.
Flags: testcase? → testcase+
Keywords: selenium

Comment 59

6 years ago
Added to relnotes in bug 713346.
Keywords: relnote

Comment 60

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

Updated

5 years ago
Duplicate of this bug: 803203
You need to log in before you can comment on or make changes to this bug.