456743 - Add the ability to disable field values (mark them as inactive)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Frédéric Buclin]

I thought we had a bug already, but I cannot find it...

Currently, you can disable custom fields, which hides them in the UI; you can delete a field value, if not in use by any bug; but you cannot disable (mark as inactive) a field value.

For instance, this would be very useful when you don't want to use "Mac System 7" for OS anymore, but still want to keep bugs which already have this OS as is, rather than having to edit these (old) bugs to use another OS.

Bug 77193 is only about product-specific fields (versions, components and milestones), and bug 69267 is only about keywords. My bug is to do it for *all* fields, including hardcoded and custom fields. This shouldn't be a big deal as all DB tables related to bug fields already have an 'isactive' column. All we need to do is to add a checkbox in editvalues.cgi.

Comment 2

[Max Kanat-Alexander]

Just because there's an isactive column doesn't mean it was ever implemented or used. The addition of the isactive column was a mistake--something that I added but is actually never supposed to be used (until we implement its use).

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> Just because there's an isactive column doesn't mean it was ever implemented or
> used.

I never said it was. I'm just saying it's something useful and which needs a UI to use it.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> I never said it was. I'm just saying it's something useful and which needs a UI
> to use it.

  For sure.

Comment 5

[Frédéric Buclin]

I really want this so that we can fix bug 77193.

Comment 6

[Frédéric Buclin]

too late for 3.4.

Comment 7

[Greg Hendricks]

So should this be merged with bug 77193 or should it be a dependency?

Comment 8

[Frédéric Buclin]

The dependency is already set. This bug will fix bug 77193.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> The dependency is already set. This bug will fix bug 77193.

  Well, no, this bug should be fixed first, and then you should fix bug 77193. Being able to disable non-product-specific fields is the first step before we add the ability to disable product-specific fields.

Comment 10

[Greg Hendricks]

So how do (In reply to comment #0)
> This shouldn't be a big deal as
> all DB tables related to bug fields already have an 'isactive' column. All we
> need to do is to add a checkbox in editvalues.cgi.

I don't see an isactive column in the Product specific field tables. Am I looking in the wrong place?

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> I don't see an isactive column in the Product specific field tables. Am I
> looking in the wrong place?

  You're not looking in the wrong place. There is no isactive field on the per-product tables, but per-product field values should have disabling implemented in bug 77193, not this bug. This bug should be fixed first.

Comment 12

[Greg Hendricks]

OK, since I am working on bug 77193, am I ok just adding those fields to the tables directly? This is what I have so far:

in Schema.pm added 
   isactive   =>  {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'},
to versions, milestones, and components

in Bugzilla/InstallDB.pm calls to bz_add_column for the same tables.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> OK, since I am working on bug 77193

  I was imagining you'd be working on this bug first, since it doesn't have a patch.

> am I ok just adding those fields to the tables directly?

>    isactive   =>  {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'},

  Nowadays we'd probably call it is_active, but consistency is more important. Yes, that definition looks fine, but we should probably have the conversation about it in the appropriate bug, not this one.

Comment 14

[Greg Hendricks]

I am curious then how these bugs relate. Do they need to be dependent? It seems to me that they can both be implemented separately. I had already been working on the other bug when I was made aware of this one, hence my questions.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> I am curious then how these bugs relate. Do they need to be dependent? It seems
> to me that they can both be implemented separately. I had already been working
> on the other bug when I was made aware of this one, hence my questions.

  Disabling custom field values and non-per-product values is much simpler than disabling per-product values, and will share a lot of code with disabling per-product values, so this bug should be implemented first.

Comment 16

[Greg Hendricks]

Attachment: Part1-V1

This is the first part. It deals only with editing the field values to set isactive to true or false. I will post a separate patch dealing with hiding from bug entry/edit.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 376802 [details] [diff] [review]
Part1-V1

>Index: template/en/default/admin/fieldvalues/create.html.tmpl
> [snip]

  You don't need to support (and in fact, shouldn't support) disabling field values when they are being created.

>Index: Bugzilla/Field.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v
>retrieving revision 1.43
>diff -u -r1.43 Field.pm
>--- Bugzilla/Field.pm	8 Feb 2009 19:42:21 -0000	1.43
>+++ Bugzilla/Field.pm	11 May 2009 22:48:19 -0000
>@@ -874,12 +874,12 @@
> =cut
> 
> sub get_legal_field_values {
>-    my ($field) = @_;
>+    my ($field, $isactive) = @_;

  I'd think that $isactive should default to 1 if undefined, right?

>Index: editvalues.cgi
> [snip]
>@@ -116,6 +116,7 @@
>         value   => scalar $cgi->param('value'), 
>         sortkey => scalar $cgi->param('sortkey'),
>         is_open => scalar $cgi->param('is_open'),
>+        isactive => $cgi->param('isactive') ? 1 : 0,

  You don't need to to the ternary operator there, that's the validator's job. Just do "scalar".

>+    $value->set_isactive($cgi->param('isactive') ? 1 : 0);

  Same there, you don't need the ternary operator.

>-sub sortkey { return $_[0]->{'sortkey'}; }
>+sub sortkey  { return $_[0]->{'sortkey'};  }
>+sub isactive { return $_[0]->{'isactive'}; }

  I'd prefer if the accessor was called is_active for consistency's sake with almost every other modern accessor (and perlstyle).

  Also, this should be in some sort of alphabetical order with the other accessor.

>@@ -303,6 +306,10 @@
> 
> sub set_name    { $_[0]->set('value', $_[1]);   }
> sub set_sortkey { $_[0]->set('sortkey', $_[1]); }
>+sub set_isactive {
>+    my ($self, $isactive) = @_;
>+    $self->{isactive} = $isactive;
>+}

  You don't need to make this different from the above setters. Also, put it in alphabetical order.


  As LpSolit pointed out, you're missing a validator for isactive.

Comment 18

[Max Kanat-Alexander]

Also, "Active" is an unclear field label. I believe for NASA we called it something like "Show as a valid choice on bugs" or something like that--maybe something shorter.

Comment 19

[Greg Hendricks]

Attachment: V2

I can agree that we don't need to allow disabling on create for field values. However, I would argue that it makes sense for the product specific fields. I may want to define a set of milestones at the beginning of a project but not allow new bugs against them until a future date for example. So I have removed it from here, but it retained this feature in the patch for 77193.

Comment 20

[Max Kanat-Alexander]

(In reply to comment #19)
> I may want to define a set of milestones at the beginning of a project but not
> allow new bugs against them until a future date for example. So I have removed
> it from here, but it retained this feature in the patch for 77193.

  That's a rare enough case that we don't need it, so I would recommend that you remove it from bug 77193 as well. We can support it in the WebServices at some point if people absolutely need to do something like that for some crazy yet very rare reason.

Comment 21

[Greg Hendricks]

Attachment: Full Patch V1

The question arises as to how to deal with values that are inactive in search forms. Since bugs might still exists whith disabled values, it stands to reason that they should be searchable. The search forms call get_legal_field_values to populate the multiselect fields as get_legal_field_values already returns only active values. I propose creating a get_all_field_values subroutine for the search fields which is reflected in this patch.

Comment 22

[Max Kanat-Alexander]

FWIW, you should be able to use Bugzilla::Field::Choice->match instead of any existing get_blah and do whatever you want with it, no?

Comment 23

[Max Kanat-Alexander]

Comment on attachment 377186 [details] [diff] [review]
Full Patch V1

>Index: template/en/default/admin/fieldvalues/list.html.tmpl
>      {
>+       name => "isactive"
>+       heading => "Active for new $terms.bugs"

  No, it's not "Active for new bugs". That makes it sounds like it doesn't affect the editing screen.

>Index: template/en/default/admin/fieldvalues/edit.html.tmpl
>+      <tr>
>+        <th><label for="isactive">Active:</label></th>

  I think we'll need some descriptive text on the page somewhere if we make the label that short.

>+        my $values = Bugzilla::Field::Choice->type($self)->match({'WHERE' => {'isactive = ?' => '1'}});

  Wow, you don't need to use the WHERE hack. You can just do {isactive => 1}.

>+        $self->{'legal_values'} = $values;
>+    }
>+    return $self->{'legal_values'};
>+}
>+
>+sub all_values {

  Instead of having two subroutines you should probably just allow an argument to be passed to legal_values. I'm not sure about that, though.

  Anyhow, the separation between "legal_values" and "all_values" is not going to make sense once we can also disable values on the search page separately.

>+sub get_all_field_values {
>+    my ($field) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    my $result_ref = $dbh->selectcol_arrayref(
>+         "SELECT value FROM $field
>+        ORDER BY sortkey, value");

  You could also do this with Bugzilla::Field::Choice and a map(), which I would somewhat prefer.

>+sub set_isactive { $_[0]->set('isactive', $_[1]); }

  set_is_active.

>+sub _check_isactive {

  _check_is_active. Also, you don't even need to create this validator. \&Bugzilla::Object::check_boolean.

Comment 24

[Max Kanat-Alexander]

Also, I didn't see you handle the case where a bug still has the value set on it?

Comment 25

[Greg Hendricks]

(In reply to comment #23)
>   No, it's not "Active for new bugs". That makes it sounds like it doesn't
> affect the editing screen.

I am really bad at coming up with descriptive yet terse descriptions. I could use a little help here.

>   Wow, you don't need to use the WHERE hack. You can just do {isactive => 1}.

Really? That didn't seem clear from the documentation... :(

> >+sub all_values {
> 
>   Instead of having two subroutines you should probably just allow an argument
> to be passed to legal_values. I'm not sure about that, though.
> 
>   Anyhow, the separation between "legal_values" and "all_values" is not going
> to make sense once we can also disable values on the search page separately.

I went back and forth on this, but I was unaware we had plans to disable them on searches. If so, we should rename it from legal_values to just get_field_values or something. Legal values sounds like it should be limited in some way (as opposed to what, illegal values)?

Again, I could use some help in naming these things.

> 
> >+sub get_all_field_values {
> >+    my ($field) = @_;
> >+    my $dbh = Bugzilla->dbh;
> >+    my $result_ref = $dbh->selectcol_arrayref(
> >+         "SELECT value FROM $field
> >+        ORDER BY sortkey, value");
> 
>   You could also do this with Bugzilla::Field::Choice and a map(), which I
> would somewhat prefer.

I am not sure how you would do this. I know this way works.

>   _check_is_active. Also, you don't even need to create this validator.
> \&Bugzilla::Object::check_boolean.

OK

Comment 26

[Greg Hendricks]

(In reply to comment #24)
> Also, I didn't see you handle the case where a bug still has the value set on
> it?

DOH!
I got bit by that in Testopia eons ago. I should have remembered to check that.

Of course, with all the new abstraction in the templates with bug/field, this is going to be a challenge...

Comment 27

[Greg Hendricks]

So there is one more issue here. Since on editing bugs, bug/field is called for products, it complains that there is no isactive field in the products table. Do we add one? How would that jive with disallownew? Should disallownew be changed to isactive?

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> So there is one more issue here. Since on editing bugs, bug/field is called for
> products, it complains that there is no isactive field in the products table.
> Do we add one? How would that jive with disallownew? Should disallownew be
> changed to isactive?

  Where does it complain about isactive? We could also possibly just remove isactive from its DB_COLUMNS list, no? Or give it a $product->is_active that's always 1.

Comment 29

[Greg Hendricks]

Attachment: V2

There is a lot of confusion potential with get_legal_field_values and Field::legal_values. I am not sure I am happy with this patch, but tell me if you think it should go in this direction or some other.

Comment 30

[Frank Becker]

(In reply to comment #29)
> Created an attachment (id=380544) [details]
> V2
> 
> There is a lot of confusion potential with get_legal_field_values and
> Field::legal_values. I am not sure I am happy with this patch, but tell me if
> you think it should go in this direction or some other.

I think that config.rdf.tmpl must reflect the new usage to. Mylyn actually use only this to show the options.

Comment 31

[Greg Hendricks]

Given the numerous places this could be used, my recommendation would be to create a method called get_field_values and pass a flag to limit to only active values. Either that or have a separate get_active_values method. 

Thoughts?

Comment 32

[Max Kanat-Alexander]

No, I think we should just have an { obsolete => 1 } argument that we can pass to Bugzilla::Field->legal_values when we want to include INactive values, and we should be using legal_values everywhere that we can.

Comment 33

[Frédéric Buclin]

Comment on attachment 380544 [details] [diff] [review]
V2

>Index: template/en/default/search/form.html.tmpl

>+        <option value="[% value.name == "" ? '---' : value.name FILTER html %]"

1) Field values cannot be "", except for the resolution. I think it worths a comment to make that clear.
2) Nit: also [% value.name OR '---' FILTER html %] would be cleaner, as we don't accept 0 as a valid field value, IIRC.


>           [% ELSIF sel.name == "resolution" %]
>+            [% '---' IF value.name == "" %]
>+            [% get_resolution(value.name) FILTER html %]

This is a bit confusing. [% get_resolution(value.name) OR '---' FILTER html %] is probably cleaner, with a comment explaining that a missing resolution is displayed as '---'.



>Index: template/en/default/admin/fieldvalues/edit.html.tmpl
>     [% END %]
>+      <tr>
>+        <th align="right"><label for="isactive">Allow for selection in [% terms.bugs %]:</label></th>
>+        <td><input id="isactive" name="is_active" type="checkbox" value="1" 
>+             [% ' checked="checked"' IF value.is_active %]
>+             [% ' disabled="disabled"' IF value.is_default %]></td>

1) The indentation is wrong. <tr> must be aligned with [% END %]. This also affects subsequent lines.
2) Also, the ID should be "is_active", to match the field name (as recommended by the HTML4 spec, IIRC).
3) Nit: write [%+ 'checked="checked"' IF foo %] rather than adding a leading whitespace in quotes.
4) There is no reason to disable the checkbox if this value is the default one for the given field. It should always be editable.


>+[%# This is a fun hack. If a checkbox is disabled, the value is not sent
>+  # With the rest of the form, so we have to set a value here. %] 
>+[% IF value.is_default %]
>+  <input type="hidden" name="is_active" value="[% value.is_active ? 1 : 0 FILTER html %]">
>+[% END %]

This "fun hack" can go away with what I said in 4) above.



>Index: Bugzilla/Field.pm

>-        $legalsRef = get_legal_field_values($name);
>+        $legalsRef = Bugzilla::Field->new({name => $name})->legal_values;
>+        my @values = map($_->name, @$legalsRef);
>+        $legalsRef = \@values;

Is there any reason to do this change? I'm pretty sure this makes the code slower, and offers no benefit.



>Index: template/en/default/bug/field.html.tmpl

>+            [% IF legal_value.is_active OR legal_value.name == bug.${field.name} %]

This doesn't work for multi-select fields. In this case, bug.${field.name} is an arrayref, not a string.


>             <option value="[% legal_value.name FILTER html %]"

The indentation is wrong. It shouldn't be aligned with [% IF %].

Comment 34

[Max Kanat-Alexander]

Why does this patch even touch search/form.html.tmpl? This shouldn't be affecting searches at all, yet.

Comment 35

[Frédéric Buclin]

(In reply to comment #34)
> Why does this patch even touch search/form.html.tmpl?

It does because query.cgi now passes objects to the template.

Comment 36

[Max Kanat-Alexander]

(In reply to comment #35)
> (In reply to comment #34)
> > Why does this patch even touch search/form.html.tmpl?
> 
> It does because query.cgi now passes objects to the template.

  Ah, okay, that makes sense!

Comment 37

[Greg Hendricks]

(In reply to comment #33)
> >+             [% ' checked="checked"' IF value.is_active %]
> >+             [% ' disabled="disabled"' IF value.is_default %]></td>
> 
> 3) Nit: write [%+ 'checked="checked"' IF foo %] rather than adding a leading
> whitespace in quotes.

You and mkanat should fight it out sometime. The leading space was his idea.

> 4) There is no reason to disable the checkbox if this value is the default one
> for the given field. It should always be editable.

This is because if you disable the default, it will still show up in the list of valid options on bug entry. When you try to submit it will complain that a legal value was not set.
 
> >Index: Bugzilla/Field.pm
> 
> >-        $legalsRef = get_legal_field_values($name);
> >+        $legalsRef = Bugzilla::Field->new({name => $name})->legal_values;
> >+        my @values = map($_->name, @$legalsRef);
> >+        $legalsRef = \@values;
> 
> Is there any reason to do this change? I'm pretty sure this makes the code
> slower, and offers no benefit.

This is left over from when I was trying to remove get_legal_field_values. I still think it should go away.

> 
> >+            [% IF legal_value.is_active OR legal_value.name == bug.${field.name} %]
> 
> This doesn't work for multi-select fields. In this case, bug.${field.name} is
> an arrayref, not a string.

*sigh*

Comment 38

[Max Kanat-Alexander]

Keep the leading whitespace. Otherwise you get <tdclass=""> or something like name="foo"class="blah".

Comment 39

[Frédéric Buclin]

(In reply to comment #38)
> Keep the leading whitespace. Otherwise you get <tdclass=""> or something like
> name="foo"class="blah".

It's cleaner to write [%+ foo %]. The "+" sign exactly means "add a whitespace". This way you are sure the whitespace is present.

Comment 40

[Max Kanat-Alexander]

But it often results in messier HTML:

    <td


   class="blah"

vs:

  <td class="blah"

Comment 41

[Greg Hendricks]

> 
> >+            [% IF legal_value.is_active OR legal_value.name == bug.${field.name} %]
> 
> This doesn't work for multi-select fields. In this case, bug.${field.name} is
> an arrayref, not a string.

Ok, I am pulling my hair out on this one. I am not coming up with any good ways to do a proper check for multi selct fields. One, there is no "ref" in TT so I can't tell if the bug.${field.name} is a scalar or a list. I can add a ref vmethod to the TT Stash but it sounds like lpsolit has an easier way in mind. If so, I would love to hear it.

Comment 42

[Frédéric Buclin]

(In reply to comment #41)
> it sounds like lpsolit has an easier way in mind.
> If so, I would love to hear it.

Just call grep(). TT doesn't mind if it gets a scalar or an arrayref:

http://www.template-toolkit.org/docs/manual/VMethods.html#section_Automagic_Promotion_of_Scalar_to_List_for_Virtual_Methods

Comment 43

[Max Kanat-Alexander]

Greg--there's already code that handles that in field.html.tmpl, just look at the existing code.

Comment 44

[Greg Hendricks]

Attachment: V3

Comment 45

[Greg Hendricks]

Attachment: V3

Now I see what mkanat was talking about...

Comment 46

[Frédéric Buclin]

Comment on attachment 388496 [details] [diff] [review]
V3

Your patch is working fine. There are some remaining points to fix, though, but nothing hard to do.


>Index: template/en/default/admin/fieldvalues/edit.html.tmpl

>+      <th align="right"><label for="isactive">Allow for selection in [% terms.bugs %]:</label></th>
>+      <td><input id="is_active" name="is_active" type="checkbox" value="1" 

1) The field ID and name are "is_active", as expected by editvalues.cgi, but the <label> points to "isactive".
2) I take back what I said in my previous review: I agree you should disable the checkbox if it's the default value, else this breaks editparams.cgi when you view the "Bug Fields" panel. But there is no need for your hack (your hidden field). All you need to do is to skip $value->set_is_active(...) in editvalues.cgi if the value is the default one for this field. This way, we are sure one cannot hack the URL to cause trouble.
3) You must also disable this checkbox if the value is non-deletable (bug states and resolutions have some). I tried to disable the DUPLICATE resolution, and I no longer can mark bugs as dupes; annoying. Also, in editvalues.cgi, you should also ignore $value->set_is_active(...) for non-deletable values, as they must always be active. Alternately, you can force is_active = 1 for default and non-deletable values, if you prefer; I don't really care.


>   </table>
>-
>   <input type="hidden" name="value" value="[% value.name FILTER html %]">

Nit: no reason to remove this empty line. It should remain, for readability.


I like that your patch doesn't prevent to set values to inactive ones. This way, we can still hack the URL if necessary. If we decide to prevent this anyway, I would prefer to do it in another bug.

Comment 47

[Greg Hendricks]

Attachment: V4

I decided I like "Enable for bugs" better than "Allow for selection in bugs". If this needs to change I can do it on checkin.

Comment 48

[Frédéric Buclin]

Comment on attachment 389196 [details] [diff] [review]
V4

>Index: editvalues.cgi

>+    if (!($value->is_static || $value->is_default)){

Nit: missing space before {.


Your patch is working fine. Thanks! r=LpSolit

Comment 49

[Greg Hendricks]

    Checking in template/en/default/admin/fieldvalues/edit.html.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/edit.html.tmpl,v  <--  edit.html.tmpl
    new revision: 1.15; previous revision: 1.14
    done
    Checking in template/en/default/admin/fieldvalues/list.html.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/list.html.tmpl,v  <--  list.html.tmpl
    new revision: 1.11; previous revision: 1.10
    done
    Checking in editvalues.cgi;
    /cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
    new revision: 1.40; previous revision: 1.39
    done
    Checking in query.cgi;
    /cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
    new revision: 1.185; previous revision: 1.184
    done
    Checking in template/en/default/bug/field.html.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
    new revision: 1.29; previous revision: 1.28
    done
    Checking in Bugzilla/Field/Choice.pm;
    /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field/Choice.pm,v  <--  Choice.pm
    new revision: 1.12; previous revision: 1.11
    done
    Checking in template/en/default/search/form.html.tmpl;
    /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v  <--  form.html.tmpl
    new revision: 1.56; previous revision: 1.55
    done
    Checking in Bugzilla/Field.pm;
    /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
    new revision: 1.44; previous revision: 1.43
    done

Comment 50

[Max Kanat-Alexander]

Added to the release notes in bug 547466.