Closed Bug 69267 Opened 19 years ago Closed 4 years ago

Add the ability to deactivate keywords

Categories

(Bugzilla :: Administration, task, P2)

2.15

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: CodeMachine, Assigned: dylan)

References

Details

(Keywords: relnote)

Attachments

(1 file, 8 obsolete files)

You should have the ability to specify a keyword can no longer be added to bugs,
without actually having to remove the keyword.
Target Milestone: --- → Future
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.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: Future → Bugzilla 2.16
Blocks: 97706
Moving to new Bugzilla product ...
Assignee: tara → matty
Status: ASSIGNED → NEW
Component: Bugzilla → Administration
Product: Webtools → Bugzilla
Version: other → 2.15
Status: NEW → ASSIGNED
QA Contact: matty → jake
*** Bug 103577 has been marked as a duplicate of this bug. ***
Blocks: 86547
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
I'm going to be introducing the new administration architecture here to reduce
the burden on the reviewers of bug #94534.
Summary: Allow closing a keyword. → Support keyword sortkeys, inactive/disabled/closed keywords (move keywords over to new administration architecture).
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.
>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.

Taking Jake's bugs...  his Army Reserve unit has been deployed.
QA Contact: jake → 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)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
*** Bug 272160 has been marked as a duplicate of this bug. ***
*** Bug 312145 has been marked as a duplicate of this bug. ***
Assignee: mattyt-bugzilla → administration
Status: ASSIGNED → NEW
QA Contact: justdave → default-qa
Target Milestone: Bugzilla 2.22 → ---
*** Bug 318122 has been marked as a duplicate of this bug. ***
Depends on: 456743
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
Assignee: administration → dylan
No longer blocks: 86547, 97706
Summary: Support keyword sortkeys, inactive/disabled/closed keywords (move keywords over to new administration architecture). → Add the ability to deactivate keywords
Status: NEW → ASSIGNED
Lots of places to add in the check for is_active. bug_fields.keywords, bug.keywords, etc. Should deactivated keywords show up in search?
Flags: needinfo?(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.
Flags: needinfo?(glob)
Priority: P3 → P2
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.
Attached patch bug-69267-v1.patch (obsolete) — Splinter Review
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.
Attachment #8539009 - Flags: review?(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.
Attachment #8539009 - Flags: review?(glob) → review-
Attached patch bug-69267-v1.5.patch (obsolete) — Splinter Review
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.
Attachment #8539009 - Attachment is obsolete: true
Attachment #8539017 - Flags: review?(glob)
Attached patch bug-69267-v1.5.patch (obsolete) — Splinter Review
without fat-fingering a 't' while copying to my local text editor,
and with correct pod formatting.
Attachment #8539017 - Attachment is obsolete: true
Attachment #8539017 - Flags: review?(glob)
Attachment #8539022 - Flags: review?(glob)
Attached patch bug-69267-v1.5.patch (obsolete) — Splinter Review
Attachment #8539022 - Attachment is obsolete: true
Attachment #8539022 - Flags: review?(glob)
Attachment #8539025 - Flags: review?(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.
Attachment #8539025 - Flags: review?(glob) → review-
(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.
(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.
(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.
Attached patch bug-69267-v2.patch (obsolete) — Splinter Review
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.
Attachment #8539025 - Attachment is obsolete: true
Attachment #8544324 - Flags: review?(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?
Attachment #8544324 - Flags: review?(glob) → review-
(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.
(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.
Attached patch bug-69267-v3.patch (obsolete) — Splinter Review
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.
Attachment #8544324 - Attachment is obsolete: true
Attachment #8589283 - Flags: review?(glob)
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 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.
Attachment #8589283 - Flags: review?(glob) → review-
I wonder if I uploaded the wrong patch. Let's find out.
Attached patch bug-69267-v4.patch (obsolete) — Splinter Review
Should apply cleanly. The only non-essential changes left involve fixing whitespace.
Attachment #8589283 - Attachment is obsolete: true
Attachment #8590298 - Flags: review?(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
Attachment #8590298 - Flags: review?(glob) → review-
(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.
Attached patch 69267_5.patch (obsolete) — Splinter Review
re-added all_keywords, only search uses it. First patch using bz-dev-manager.
Attachment #8590298 - Attachment is obsolete: true
Attachment #8593009 - Flags: review?(glob)
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.
Attachment #8593009 - Attachment description: 69267_1.patch → 69267_5.patch
Attachment #8593009 - Attachment filename: 69267_1.patch → 69267_5.patch
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
Attachment #8593009 - Flags: review?(glob) → review-
Attached patch 69267_6.patchSplinter Review
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.
Attachment #8593009 - Attachment is obsolete: true
Attachment #8596243 - Flags: review?(glob)
Comment on attachment 8596243 [details] [diff] [review]
69267_6.patch

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

r=glob
Attachment #8596243 - Flags: review?(glob) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 6.0
Dylan, Byron, many thanks for your work. It is really appreciated and it will simplify our life as release manager!
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.
Flags: documentation?
Keywords: relnote
To ssh://gitolite3@git.mozilla.org/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.
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   c3c2ecc..7b85e3b  master -> master

+ slightly clearer documentation.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: documentation? → documentation+
Resolution: --- → FIXED
(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! :)
Blocks: 1177267
You need to log in before you can comment on or make changes to this bug.