69267 - Add the ability to deactivate keywords

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Matthew Tuck [:CodeMachine]]

You should have the ability to specify a keyword can no longer be added to bugs,
without actually having to remove the keyword.

Comment 1

[Matthew Tuck [:CodeMachine]]

This will fall out of the bug #94534 cleanup, although I will likely make
changes to the keywords cgi such that:

If you view it in "global" mode it will show all keywords in alphabetical order.

If you view it in "bug" mode it will firstly show all the keywords on the bug in
alphabetical order, and then the rest, where the inactive keywords on the bug
get strikethru or something.

Comment 2

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 3

[Matthew Tuck [:CodeMachine]]

*** Bug 103577 has been marked as a duplicate of this bug. ***

Comment 4

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 5

[Matthew Tuck [:CodeMachine]]

I'm going to be introducing the new administration architecture here to reduce
the burden on the reviewers of bug #94534.

Comment 6

[Matthew Tuck [:CodeMachine]]

The administration side of this is pretty much done.

What I need to do now is to work out how to do this for show_bug/process_bug, in
particular handling inactive keywords.  Given the nature of keywords, I'm
thinking I should give an error from process_bug if you try and add an inactive
keyword.

The tricky bit is handling makeexact from mass change.  If you're using this
with an inactive keyword, you may well not be adding it to any bugs.  I'm
wondering whether I should just give an error or try to make sure it is actually
added to something before giving an error.

I'm also puzzling over the way inactivity is done for groups.  As best as I can
tell, inactive groups are simply dropped.  Given the keyword UI is different,
this is probably unacceptable for keywords.

Comment 7

[Jouni Heikniemi]

>The tricky bit is handling makeexact from mass change.  If you're using this
>with an inactive keyword, you may well not be adding it to any bugs.  I'm
>wondering whether I should just give an error or try to make sure it is actually
>added to something before giving an error.

Hmm. There's definitely point in checking for addition before bugging the user,
but on the practical side, I can't come up with many situations where it would
be totally unreasonable to just complain if an inactive keyword is specified.

I'd say go for the simple approach. Let's change the way we do it if people
complain about this.

Comment 8

[Dave Miller [:justdave]]

Taking Jake's bugs...  his Army Reserve unit has been deployed.

Comment 9

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 10

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 11

[dolphinling]

*** Bug 272160 has been marked as a duplicate of this bug. ***

Comment 12

[Frédéric Buclin]

*** Bug 312145 has been marked as a duplicate of this bug. ***

Comment 13

[Frédéric Buclin]

*** Bug 318122 has been marked as a duplicate of this bug. ***

Comment 14

[:glob ✱]

keyword's don't need a sortkey, however they should be able to be deactivated.

they should be handled on bulk bug changing in the same way as deactivated versions/milestones/components/etc

Comment 15

[Dylan Hardison [:dylan] (he/him)]

Lots of places to add in the check for is_active. bug_fields.keywords, bug.keywords, etc. Should deactivated keywords show up in search?

Comment 16

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #15)
> Should deactivated keywords show up in search?

yes.

i suspect the only places where we need to remove deactivated keywords is when creating or updating a bug.

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Alright, I decided it would be sane (and rewarding) to split this up into three micro tasks:

1) hack the SQL definition and admin UI to disable keywords
2) Remove them from show_bug
3) Disallow them from being added to any bug during a bug update (including API calls)

I flipped a three-sided coin and decided to focus on #2.
Unfortunately this was not an optimal course, as they show up in keyword completion still.

A second solution is just making Bugzilla::Bug->keyword_objects to ignore them. Durp durp.

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v1.patch

And another thing to review. The way keywords are presented made this especially annoying, but in the end it's not too horrible.

Note the patch includes minor code cleanups, such as replacing five-space indentations and aligning some fat-commas in a template.
And trailing white spaces.

Comment 19

[:glob ✱]

Comment on attachment 8539009 [details] [diff] [review]
bug-69267-v1.patch

Review of attachment 8539009 [details] [diff] [review]:
-----------------------------------------------------------------

trying to deactive a keyword throws:
> The requested method 'Bugzilla::Keyword::set_is_active' was not found.

Comment 20

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v1.5.patch

Silly brain! "No, I don't need to stage that hunk. it's never called" because I thought Bugzilla::Object didn't use the accessor methods.

Comment 21

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v1.5.patch

without fat-fingering a 't' while copying to my local text editor,
and with correct pod formatting.

Comment 22

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v1.5.patch

Comment 23

[:glob ✱]

Comment on attachment 8539025 [details] [diff] [review]
bug-69267-v1.5.patch

Review of attachment 8539025 [details] [diff] [review]:
-----------------------------------------------------------------

if a keyword is disabled, it still needs to be visible on bugs that had it set prior when it was active.  updating that bug shouldn't remove the keyword.

ie. disabling a keyword should only prevent you from adding it to bugs.

Comment 24

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› from comment #23)
> Comment on attachment 8539025 [details] [diff] [review]
> bug-69267-v1.5.patch
> if a keyword is disabled, it still needs to be visible on bugs that had it
> set prior when it was active.  
OK

> updating that bug shouldn't remove the
> keyword.
This is the case now. Although without the aforementioned change,
it becomes impossible to ever remove inactive keywords from bugs.

Comment 25

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #24)
> This is the case now. Although without the aforementioned change,
> it becomes impossible to ever remove inactive keywords from bugs.

sorry, not sure i follow you here.


when viewing a bug all values should be visible, even if they are no longer active.

you should be able to remove inactive keywords from bugs that already have it, but you should not be able to add inactive keywords to bugs.  inactive keywords must not be removed from bugs that already have that keyword set when the bug is updated unless explicitly removed by the user.

Comment 26

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› (unavailable until jan 4th) from comment #25)
> (In reply to Dylan William Hardison [:dylan] from comment #24)
> > This is the case now. Although without the aforementioned change,
> > it becomes impossible to ever remove inactive keywords from bugs.
> 
> sorry, not sure i follow you here.
> 
> 
> when viewing a bug all values should be visible, even if they are no longer
> active.
> 
> you should be able to remove inactive keywords from bugs that already have
> it, but you should not be able to add inactive keywords to bugs.  inactive
> keywords must not be removed from bugs that already have that keyword set
> when the bug is updated unless explicitly removed by the user.

Yep! that's what the (incoming soon) patch does.

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v2.patch

Alright, this patch shows all keywords on edit bug, but will not complete inactive ones. The behavior should be as expected -- deactivated keywords can be removed but not re-added.

Comment 28

[:glob ✱]

Comment on attachment 8544324 [details] [diff] [review]
bug-69267-v2.patch

Review of attachment 8544324 [details] [diff] [review]:
-----------------------------------------------------------------

most of this looks excellent.

disabled keywords are not visible on the advanced search form (see comment 16)

it's possible to use an inactive keyword when creating a bug.  using _check_keywords should fix that.

> Bugzilla/WebService/Bug.pm
> 236:        my @legal_keywords = Bugzilla::Keyword->get_all;
this should return active keywords only.

::: Bugzilla/Bug.pm
@@ +958,5 @@
> +    }
> +
> +    foreach my $keyword (@{$self->keyword_objects}) {
> +        unless ($keyword->is_active || $old_kw_id{$keyword->id}) {
> +            ThrowUserError('object_does_not_exist',

value_inactive would be a better error to throw instead of object_does_not_exist

@@ +961,5 @@
> +        unless ($keyword->is_active || $old_kw_id{$keyword->id}) {
> +            ThrowUserError('object_does_not_exist',
> +                           { name  => $keyword->name, class => ref $keyword });
> +        }
> +    }

this belongs in _check_keywords

@@ +3612,5 @@
>      my $dbh = Bugzilla->dbh;
>      my $ids = $dbh->selectcol_arrayref(
>           "SELECT keywordid FROM keywords WHERE bug_id = ?", undef, $self->id);
> +    my @keywords = @{ Bugzilla::Keyword->new_from_list($ids) };
> +    $self->{'keyword_objects'} = \@keywords;

this change appears to be pointless; what's going on here?

Comment 29

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› (limited availability until 19th jan) from comment #28)
> Comment on attachment 8544324 [details] [diff] [review]
> bug-69267-v2.patch
> 
> Review of attachment 8544324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> most of this looks excellent.
> 
> disabled keywords are not visible on the advanced search form (see comment
> 16)

They're accidentally not visible. I guess that one should use all_keywords.
But this results in...

> it's possible to use an inactive keyword when creating a bug.  using
> _check_keywords should fix that.
> 
> > Bugzilla/WebService/Bug.pm
> > 236:        my @legal_keywords = Bugzilla::Keyword->get_all;
> this should return active keywords only.

If it it does, how can I return all keywords for the advanced search form to use?


> ::: Bugzilla/Bug.pm
> @@ +958,5 @@
> > +    }
> > +
> > +    foreach my $keyword (@{$self->keyword_objects}) {
> > +        unless ($keyword->is_active || $old_kw_id{$keyword->id}) {
> > +            ThrowUserError('object_does_not_exist',
> 
> value_inactive would be a better error to throw instead of
> object_does_not_exist

Ah, yes. That is a much better error.

> 
> @@ +961,5 @@
> > +        unless ($keyword->is_active || $old_kw_id{$keyword->id}) {
> > +            ThrowUserError('object_does_not_exist',
> > +                           { name  => $keyword->name, class => ref $keyword });
> > +        }
> > +    }
> 
> this belongs in _check_keywords

OK.

> @@ +3612,5 @@
> >      my $dbh = Bugzilla->dbh;
> >      my $ids = $dbh->selectcol_arrayref(
> >           "SELECT keywordid FROM keywords WHERE bug_id = ?", undef, $self->id);
> > +    my @keywords = @{ Bugzilla::Keyword->new_from_list($ids) };
> > +    $self->{'keyword_objects'} = \@keywords;
> 
> this change appears to be pointless; what's going on here?

Ah, cruft from revising after the last review. Reverted now.

Comment 30

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #29)
> > > Bugzilla/WebService/Bug.pm
> > > 236:        my @legal_keywords = Bugzilla::Keyword->get_all;
> > this should return active keywords only.
> 
> If it it does, how can I return all keywords for the advanced search form to
> use?

the search form doesn't call the webservice.

Comment 31

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v3.patch

changes: logic moved into _check_keywords, active_keywords replaced by a re-defined all_keywords. legal keywords from the skips inactive keywords. I had a more radical idea for re-working this, but this change ends up being pretty similar to the earlier attempts.

Comment 32

[Frédéric Buclin]

Comment on attachment 8589283 [details] [diff] [review]
bug-69267-v3.patch

>+++ b/Bugzilla/Bug.pm

>-    $self->{'keyword_objects'} = Bugzilla::Keyword->new_from_list($ids);
>+    my @keywords = @{ Bugzilla::Keyword->new_from_list($ids) };
>+    $self->{'keyword_objects'} = \@keywords;

Err.... what?? This change is useless.

Comment 33

[:glob ✱]

Comment on attachment 8589283 [details] [diff] [review]
bug-69267-v3.patch

Review of attachment 8589283 [details] [diff] [review]:
-----------------------------------------------------------------

this patch doesn't apply to trunk (multiple issues), and cruft from comment 28 and comment 32 needs to be removed.

Comment 34

[Dylan Hardison [:dylan] (he/him)]

I wonder if I uploaded the wrong patch. Let's find out.

Comment 35

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-69267-v4.patch

Should apply cleanly. The only non-essential changes left involve fixing whitespace.

Comment 36

[:glob ✱]

Comment on attachment 8590298 [details] [diff] [review]
bug-69267-v4.patch

Review of attachment 8590298 [details] [diff] [review]:
-----------------------------------------------------------------

the following issues from the last review have not been addressed:
- disabled keywords are not visible on the advanced search form
- it's possible to use an inactive keyword when creating a bug

Comment 37

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› from comment #36)
> Comment on attachment 8590298 [details] [diff] [review]
> bug-69267-v4.patch
> 
> Review of attachment 8590298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> the following issues from the last review have not been addressed:
> - disabled keywords are not visible on the advanced search form
Ah. that's why I originally had both all_keywords and active_keywords as template functions.
Sorry

> - it's possible to use an inactive keyword when creating a bug
when creating, but not when updating. I didn't expect those to be different.

Comment 38

[Dylan Hardison [:dylan] (he/him)]

Attachment: 69267_5.patch

re-added all_keywords, only search uses it. First patch using bz-dev-manager.

Comment 39

[Dylan Hardison [:dylan] (he/him)]

Oh, and non-active keywords trigger value_inactive during ->create() instead of just ->update(). That was the original intent,  but the change was lost in the chaos patch management.

Comment 40

[:glob ✱]

Comment on attachment 8593009 [details] [diff] [review]
69267_5.patch

Review of attachment 8593009 [details] [diff] [review]:
-----------------------------------------------------------------

this looks great.

the describe-keywords page needs some work, sorry i didn't pick up on that earlier.

::: Bugzilla/Bug.pm
@@ +1858,5 @@
> +        %old_kw_id  = map { $_->id => 1 } @old_keywords;
> +    }
> +
> +    foreach my $keyword (values %keywords) {
> +        next if $keyword->is_active || $old_kw_id{$keyword->id};

nit: use |exists $old_kw_id{..}| to avoid autovivification

::: describekeywords.cgi
@@ +25,4 @@
>  # Run queries against the shadow DB.
>  Bugzilla->switch_to_shadow_db;
>  
> +@{ $vars->{'keywords'} } = grep { $_->is_active } @{ Bugzilla::Keyword->get_all_with_bug_count() };

only showing active keywords here can be confusing, because if you navigate from a bug with a deactivated keyword to this page in order to see its description, it will not be listed.

i think this page should display all keywords, with a column that indicates if a keyword is active or not.

(time passes)

the object_does_not_exist error for keywords links here with "see the list of available keywords", which complicates things slightly.

probably the best thing is to display all keywords by default (with an 'is active' column), add an query-string param to just enabled keywords (without the 'active' column), and update the error to include that param.

::: template/en/default/admin/keywords/edit.html.tmpl
@@ +23,5 @@
>                   value="[% keyword.name FILTER html %]" required></td>
>      </tr>
>      <tr>
> +      <th><label for="is_active">Enabled For [% terms.Bugs %]</label></th>
> +      <td><input id="is_active" name="is_active" type="checkbox" [% IF keyword.is_active %]checked[% END %]></td>

nit: [%= "checked" IF keyword.is_active %]

::: template/en/default/admin/keywords/list.html.tmpl
@@ +34,5 @@
> +    {
> +        name        => "description"
> +        heading     => "Description"
> +        allow_html_content => 1
> +    },

swap "is_active" and "description" - all our admin pages have name,description,other,columns,...

::: template/en/default/list/edit-multiple.html.tmpl
@@ +227,4 @@
>        <td colspan="3">
>          [% INCLUDE bug/field.html.tmpl
>             field = bug_fields.keywords, editable = 1, value = keywords
> +           possible_values = active_keywords

this needs to be all_keywords, in order to be able to remove inactive keywords in bulk

Comment 41

[Dylan Hardison [:dylan] (he/him)]

Attachment: 69267_6.patch

The active column is always displayed, but it can be shown/hidden with either javascript or a query string parameter. The query string is included in the link from the object_does_not_exist error. Note that we don't raise that error any more, we use value_inactive, which does not link to describe keywords. Overall I think this is an improvement, but the placement of the link is sort of ugly. But really, the whole table is ugly (the blue heading especially)... but it is what it is.

Comment 42

[:glob ✱]

Comment on attachment 8596243 [details] [diff] [review]
69267_6.patch

Review of attachment 8596243 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Dylan, Byron, many thanks for your work. It is really appreciated and it will simplify our life as release manager!

Comment 44

[Frédéric Buclin]

The documentation must also be fixed, because it currently says that you *cannot* deactivate keywords, see http://bugzilla.readthedocs.org/en/latest/administering/keywords.html.

Comment 45

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   0e68998..c3c2ecc  master -> master

The documentation mentions deactivation now -- is that sufficient or should it describe it a bit more clearly? Perhaps the keywords section could re-factored a bit.

Comment 46

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   c3c2ecc..7b85e3b  master -> master

+ slightly clearer documentation.

Comment 47

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #45)
> The documentation mentions deactivation now -- is that sufficient

This should be enough for now. Thanks! :)