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

RESOLVED FIXED in Bugzilla 3.6

Status

()

task
P2
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: LpSolit, Assigned: gregaryh)

Tracking

Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [3.6 Focus])

Attachments

(1 attachment, 6 obsolete attachments)

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.
Duplicate of this bug: 381674
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).
Summary: Add UI to disable field values (mark them as inactive) → Add the ability to disable field values (mark them as inactive)
(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.
(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.
I really want this so that we can fix bug 77193.
Assignee: administration → LpSolit
Priority: -- → P2
Target Milestone: --- → Bugzilla 3.4
too late for 3.4.
Target Milestone: Bugzilla 3.4 → Bugzilla 4.0
Whiteboard: [3.6 Focus]
So should this be merged with bug 77193 or should it be a dependency?
The dependency is already set. This bug will fix bug 77193.
(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.
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?
(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.
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.
(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.
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.
(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.
Posted patch Part1-V1 (obsolete) — Splinter Review
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.
Assignee: LpSolit → ghendricks
Status: NEW → ASSIGNED
Attachment #376802 - Flags: review?(mkanat)
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.
Attachment #376802 - Flags: review?(mkanat) → review-
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.
Posted patch V2 (obsolete) — Splinter Review
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.
Attachment #376802 - Attachment is obsolete: true
Attachment #376978 - Flags: review?(mkanat)
(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.
Posted patch Full Patch V1 (obsolete) — Splinter Review
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.
Attachment #376978 - Attachment is obsolete: true
Attachment #377186 - Flags: review?(mkanat)
Attachment #376978 - Flags: review?(mkanat)
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 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.
Attachment #377186 - Flags: review?(mkanat) → review-
Also, I didn't see you handle the case where a bug still has the value set on it?
(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
(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...
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?
(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.
Depends on: 493090
Posted patch V2 (obsolete) — Splinter Review
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.
Attachment #377186 - Attachment is obsolete: true
Attachment #380544 - Flags: review?(mkanat)
Attachment #380544 - Flags: review?(mkanat) → review?(LpSolit)
(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.
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?
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 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 %].
Attachment #380544 - Flags: review?(LpSolit) → review-
Why does this patch even touch search/form.html.tmpl? This shouldn't be affecting searches at all, yet.
(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.
(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!
(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*
Keep the leading whitespace. Otherwise you get <tdclass=""> or something like name="foo"class="blah".
(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.
But it often results in messier HTML:

    <td


   class="blah"

vs:

  <td class="blah"
> 
> >+            [% 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.
(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
Greg--there's already code that handles that in field.html.tmpl, just look at the existing code.
Posted patch V3 (obsolete) — Splinter Review
Attachment #380544 - Attachment is obsolete: true
Attachment #388490 - Flags: review?(LpSolit)
Posted patch V3 (obsolete) — Splinter Review
Now I see what mkanat was talking about...
Attachment #388490 - Attachment is obsolete: true
Attachment #388496 - Flags: review?(LpSolit)
Attachment #388490 - Flags: review?(LpSolit)
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.
Attachment #388496 - Flags: review?(LpSolit) → review-
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6
Posted patch V4Splinter Review
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.
Attachment #388496 - Attachment is obsolete: true
Attachment #389196 - Flags: review?(LpSolit)
Attachment #389196 - Flags: review?(LpSolit) → review+
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
Flags: approval?
Flags: approval? → approval+
    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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Keywords: relnote
Blocks: 519481
Blocks: 535309
Added to the release notes in bug 547466.
Keywords: relnote
Duplicate of this bug: 543102
You need to log in before you can comment on or make changes to this bug.