77193 - Add the ability to retire (disable) old versions, components and milestones

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Matthew Tuck [:CodeMachine]]

It might be nice to be able to set up Bugzilla so if the user enters a bug on a 
sufficiently old version of the software, we reject the report telling them we 
don't accept bug reports for that version anymore.

We can't just delete versions as they still should stay in the database for 
current reports.

Comment 1

[Jacob Steenhagen]

My thoughts are that it'd be nice to "retire" versions.  This really goes for
milestones and components, too.  That way they wouldn't show up on the entry
pages and/or bug forms, but could still be used in queries (possibly have them
hidden from the query page by default w/a link "Show retired stuff" [better
verbage, of course]).

Comment 2

[Matthew Tuck [:CodeMachine]]

Yes, because versions and milestones are chronological, we could have a "current
version", and use the existing "current milestone".

Comment 3

[Dave Miller [:justdave]]

the existing "current milestone" is being toasted in bug 82497, because 
milestones are per-product now, and that is a global parameter.  I don't think 
that's the way to go anyway.  Not all milestones are cronological...  "---" for 
example, usually sorts to the earliest position, however, it's still perfectly 
legal for untargetted bugs, and we shouldn't have to special-case it.  Some sites 
may use versions to cover branches in the code, etc, which wouldn't necessarily 
go away cronologically.  Better to set up some way to retire individual versions/
milestones.

Comment 4

[Matthew Tuck [:CodeMachine]]

By "current milestone" I was referring to our current per-product current
milestone, "default milestone".  At least that's what the most doomed reports
think it is.

Generally, I believe '---' should be special cased.  It _is_ a special case.

However you're right that versions might be non-linear so I'll concede it on
that point.  What about milestones though?

Comment 5

[Dave Miller [:justdave]]

moving this to 2.16.  We have precedent with existing code that could be 
"borrowed" :)  for retiring groups.  The same logic would apply to versions, 
components, and milestones.

Comment 6

[Andreas Franke (gone)]

-> Bugzilla product, Administration component, reassigning.

Comment 7

[Matthew Tuck [:CodeMachine]]

Taking this as a part of bug #97706.

Comment 8

[Dave Miller [:justdave]]

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

Comment 9

[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 10

[xyzzy]

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

Comment 11

[Arne Schirmacher]

I need a way to retire products and components so that they don't show up on the
query or any other page at all. We have lots of small products which are no
longer needed and their product/component/version strings clutter our menus. 

This requirement is similar to bug #62551 . If I ever need access to any old bug
again, I would simply turn off that "retire" flag temporarily.

Comment 12

[Dave Miller [:justdave]]

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

Comment 13

[Eric Hanson]

Posting content of original bug 62551 since it seems relevent.  Dave, please note 
that although there is more 'discussion' not all of it is relevent to the 
implementation of bug 62551 (note the milestone argument), marking an older bug 
as a duplicate of a new one is incorrect (but since bugzilla functionality is 
often missued, I am not surprised). 
---
In my company our product changes quiet abit over just 6 months, as such
components are created and dropped.  The reason for dropping are
aesthetic and functional, we have alot (bold) of components and if we
didn't drop the dead ones people might misassign bugs, wastes time, etc.
Since we have deleted components the bugs still stick around, but cause
sanity check errors because they have components that no longer exist.
I am not sure if this is because I updated our bugzilla from 2.4 to 2.10
or what.  Before we had 800 bugs, with the bug id at 1700, after the
upgrade a bunch of "missing" bugs appeared, we now have 1700 bugs, but
some of the bugs are really wierd, have to manually close through mysql
commands, bugzilla is sorta wierded out by the missing component deal.

So, if this happens normally, I was thinking it would be a good idea to
be able to "Retire" components.  Basically they would still exist in
some master list, but they would _not_ show up as choices when submitting a
bug or even querying a bug (this last might be another option).  Perhaps
a "Buried" component doesn't show in query or submit, but a "Retired"
component would show in a query screen.

Comment 14

[Dave Miller [:justdave]]

Taking.  We have this working on Zippy.  I'll try to port it out.

Comment 15

[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 16

[Dave Miller [:justdave]]

Moving to nobody because I'm not actually working on this, and can't...   I have
not managed to get an IPA release for my Zippy work (yet).  It might still
happen (there's folks on the inside still pushing to get it for me), but I'm not
holding my breath at this point.

Comment 17

[Mike Schrag]

Attachment: adds a new "enabled" field to milestones

The attached patch adds a new "enabled" field to milestones.  A milestone that
is not enabled will appear to an admin, but will not appear as a valid option
in either the search screen or the new/update bug milestone lists.

If bugs were left in the disabled milestone, the value WILL appear in the
pulldown for those bugs only so that edits on the bug don't "accidentally"
change the milestone.  If the milestone for a "leftover" bug is changed from
the disabled milestone value, when the form comes back after the edit the
disabled milestone will then be gone from the list.

The only slightly weird thing for disabling milestones is that you can no
longer query on disabled milestones.  I can see that it might be desirable to
only hide disabled milestones from the new bug/update bug screens and not from
the query screen.

Comment 18

[GavinS]

(In reply to comment #17)
> Created an attachment (id=158792)
> adds a new "enabled" field to milestones

editmilestones.cgi has just been templatised in CVS, so the patch will not
apply, sorry :(

Any chance of an update to the templatised version?

Comment 19

[Mike Schrag]

Attachment: proper patch gainst HEAD

This new patch does a couple much nicer things:
1) it's actually against HEAD instead of 2.18 (sorry about that)
2) it uses the term isactive instead of enabled (to be more consistent with
other such fields other places in bugzilla)
3) enables querying on inactive milestones, but disallows editing bugs and
setting the milestone to an inactive field or change-many-bugs from the list
view

To make the milestone appear some places but not others, this become a more
invasive change, but it's more what I think people expect the feature to do. 
So it didn't become really nasty, I made it a little bit hacky (I didn't want
to redefine what was in the target_milestone list, which really needs to become
objects now rather that just names), so I created an active_target_milestone
list that is filled along side target_milestone.  target_milestone works just
like it always did, active_target_milestone only contains the active ones.  I
also modified the select function on the edit bug screen to allow an
alternative list of choices (by default the field name is the same as the
choices name, now you can pass in "choicesname" into the select function).

By the way way, anyone know why target milestone is only a field on the edit
page and not on the create page?

Comment 20

[Mike Schrag]

Attachment: fix problem with missing value on create form

bugfix for isactive field on create milestone form

Comment 21

[Mike Schrag]

Attachment: fix for choicesname not being blank as expected

Comment 22

[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 23

[Mike Schrag]

This bug actually covers three different things -- versions, milestones, and
components.  My patch only deals with the milestones aspect.  As written, the
bug is definitely not able to be closed with just the patch I provided.  I don't
know the timeline for the 2.2 release, but I don't believe there has been much
testing on the milestone part of the patch yet, so if it should probably wait
until a later point release.

Comment 24

[GavinS]

Bug#261860 is a dupe of the milestones bit of this bug, so maybe that is a good
place to move the patch to?

Comment 25

[Dave Miller [:justdave]]

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

Comment 26

[Dave Miller [:justdave]]

Comment on attachment 159201 [details] [diff] [review]
fix for choicesname not being blank as expected

Obsoleting the current patch on this bug and making it depend on bug 17453. 
The current round of patches on that bug puts the schema in place to make this
bug possible, but doesn't actually add any code to make it happen.  Once that
schema is in place, we can start adding new patches to this bug to add said
code.

Comment 27

[Dave Miller [:justdave]]

(In reply to comment #23)
> This bug actually covers three different things -- versions, milestones, and
> components.  My patch only deals with the milestones aspect.  As written, the
> bug is definitely not able to be closed with just the patch I provided.  I don't
> know the timeline for the 2.2 release, but I don't believe there has been much
> testing on the milestone part of the patch yet, so if it should probably wait
> until a later point release.

Having a quick look at this patch before I marked it obsolete, this looks like a
really good start.  Part of what you did here is duplicated in the patches on
bug 17453, but that's just doing the schema end of it.  If you want to tackle
the UI for it once that lands, you're welcome to go for it :)  Also, it'll make
our lives easier if you use "cvs diff -u" to create the patch.  It's easier to
follow, and provides a bit of context so we can tell where the code is going
(and survives bitrot better if the surrounding code moves, since it can match on
the surrounding code when trying to apply the patch instead of depending on the
line numbers)

Comment 28

[Paul]

Why isn't the Bugzilla version list super long?  What is today's solution? Are 
the versions being deleted?  Does everything work/look okay when a version is 
deleted?  My version list is becomming quite long and I need to start retiring 
versions or maybe just deleting them...

Comment 29

[timeless]

my proposed rules:
1. people without editbugs are not allowed to select a disabled target milestone
2. you can always leave a bug at its current target milestone
3. people with edit bugs can manually (w/o ui) select a disabled target milestone
4. disabled target milestones should not be visible in showbug unless trumped by another rule
5. [bonus points] users with editbugs can edit their preferences to enable them to  select individual 
target milestones.
6. [bonus points] when an admin disables a target milestone, all users with editbugs and all previously 
disabled target milestones listed in 5 will automatically have the newly disabled target milestone added 
to their inclusion override list.

Comment 30

[Shane H. W. Travis]

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

Comment 31

[Dave Miller [:justdave]]

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

Comment 32

[Greg Hendricks]

We have had a similar request on our installation. Now that the evil enums are
gone this should be pretty straight forward. This will run into issues with
versioncache as I have already found out, but with the new object files we can
possibly make versioncache disappear now as well.

Comment 33

[Greg Hendricks]

Attachment: 2.21v1

This patch allows admins to set a disallownew flag on versions components and milestones much like is done on products. Entities with this flag set will prevent new bugs being entered against them and prevent existing bugs from being changed to them but will still allow searching.

If a product has no active components, it is treated as if it has no components (meaning it will not show up on the enter_bug page).

Comment 34

[Greg Hendricks]

Attachment: 2.21v2

fixed whitespace issues pointed out

Comment 35

[Greg Hendricks]

Attachment: 2.21v3

more whitespace

Comment 36

[Max Kanat-Alexander]

Comment on attachment 201203 [details] [diff] [review]
2.21v3

Actually, we already have a column called is_active, which was designed for exactly this purpose. Use that column.

Comment 37

[Greg Hendricks]

(In reply to comment #36)
> (From update of attachment 201203 [details] [diff] [review] [edit])
> Actually, we already have a column called is_active, which was designed for
> exactly this purpose. Use that column.
> 

Actually, we originally thought to use is_active but determined that it should behave more like the disallownew of the products table rather than the is_active of the priorities etc.
I know it is really just a difference in vocabulary, but it makes more sense if you think about it to mimic the terms of the products.

Also changing the term to is_active is the negative of disallownew which complicates things in the code.

Comment 38

[Max Kanat-Alexander]

> Also changing the term to is_active is the negative of disallownew which
> complicates things in the code.

  Yes, but booleans should generally be positive. Disallownew is negative. The fact that we have any negative booleans hanging around is just a historical accident. 

  Also, we already have is_active, and so that's really the one that should be used. You can make it behave like disallownew, just call it is_active.

Comment 39

[Dave Miller [:justdave]]

I'd rather rename disallownew to is_active and fix the existing values in the database to have the proper context.  "disallownew" is a headache for people who customize Bugzilla because the semantics are backwards.

Comment 40

[Greg Hendricks]

Attachment: 2.21v4

Changed disallownew to is_active

Comment 41

[Frédéric Buclin]

Comment on attachment 201564 [details] [diff] [review]
2.21v4

many things to change, per my "Live" review on IRC

Comment 42

[Max Kanat-Alexander]

When we do live reviews, I was thinking it would be nice to paste the log into the bug, or attach it as an attachment. The reason is that a lot of the time I go back and look at reviews far, far in the past, to determine why the code works a certain way. I've done this a lot, actually. So it's very useful to have the reviews in the bug, if at all possible.

Comment 43

[Greg Hendricks]

<ghendricks> The biggest one I am worried about by way of bit rot is bug 77193 though
<ssdbot> ghendricks: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=77193 enh, P2, Bugzilla 2.22, [e-mail], ASSI, Retire old versions, components and milestones.
<ghendricks> it touches a lot of files
<ghendricks> given how old the bug is, and the size of it's CC list, I think it should be a priority
<ghendricks> LpSolit: do you have time to take a look at it?
<LpSolit> ghendricks: probably not this week
<LpSolit> ghendricks: but we are frozen anyway, your patch will be delayed till 2.24
<LpSolit> and will probably bitrot when we unfreeze
<ghendricks> LpSolit: yeah, that is what frustrates me
<ghendricks> spent hours trying to get it in beofre the freeze
<LpSolit> ghendricks: importxml.pl or another patch?
<ghendricks> all of them really
<ghendricks> but 77193 especially
<LpSolit> this one won't be taken for 2.22
<LpSolit> ghendricks: you can create a product object using its name
<LpSolit> ghendricks: my $product = new Bugzilla::Product({name => $product_name});
<ghendricks> hmmm
<ghendricks> I was doing it that way but ran into some problems it seemed
<ghendricks> is this in importxml?
<LpSolit> ghendricks: and get_active_foo sounds better than get_selectable_foo
<LpSolit> ghendricks: selectable is related to groups
<LpSolit> such as get_selectable_products
<LpSolit> we must not confuse them
<LpSolit> ghendricks: no, bug 77193
<ghendricks> LpSolit: ok
<LpSolit> ghendricks: pass objects to templates, not foo->is_active separately
<ghendricks> LpSolit: The reason I pass is_active separately is since the templates sometimes use a code block and other times they don't
<LpSolit> ghendricks: right, only editproducts.cgi has been rewritten to pass objects only
<LpSolit> ghendricks: objects will be introduced in bug 311258
<ssdbot> LpSolit: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=311258 enh, --, ---, [e-mail], NEW, use objects methods in templates of edit[product,components,milestones,versions]
<ghendricks> ah
<ghendricks> LpSolit: so I can look at editproduct and it's templates as an example then
<LpSolit> ghendricks: let bug 311258 introduce objects consistently in templates. Only pass is_active separately if other fields of the same object are passed separately too; else let the full object being passed and retrieve is_active from templates using foo.is_active
<ghendricks> gotcha
<LpSolit> ghendricks: move $cgi->param('is_active') == 1 ? 1 : 0 this check before the SQL query
<LpSolit> ghendricks: my @enterable_products = @{&Bugzilla::Product::get_selectable_products()}; get_selectable_products != get_enterable_products
<LpSolit> ghendricks: and please don't write these useless & before the function name ;)
<ghendricks> ok
<ghendricks> LpSolit: so how do you mean move the check before the query?
<LpSolit> my $is_active = $cgi->param('is_active') == 1 ? 1 : 0;
<LpSolit> $dbh->do('fooooo', undef, $is_active ....)
<LpSolit> ghendricks: even better: my $is_active = $cgi->param('is_active') ? 1 : 0;
<LpSolit> ghendricks: it's a boolean, you don't care if it's 1, 2, 86
<LpSolit> ghendricks: rewrite your methods in Product.pm
<LpSolit> ghendricks: you should not do new SQL queries as you already have $product->versions available
<LpSolit> ghendricks: my @active_versions = grep {$_->{'is_active'} == 1} @$self->versions; should work
<LpSolit> ghendricks: remove get_selectable_products from Product.pm. This method already exist in User.pm and has a different meaning
<LpSolit> ghendricks: in DB/Schema.pm, please align arrows vertically
<ghendricks> I thought it was get_enterable_products there
<LpSolit> ghendricks: both methods exist in User.pm. They have different meaning
<LpSolit> ghendricks: you can for instance view a product even if the product doesn't accept new bugs
<LpSolit> ghendricks: your changes in admin/components/updated.html.tmpl are wrong
<LpSolit> ghendricks: in all updated.html.tmpl btw
<LpSolit> else it's look good :-D
<ghendricks> LpSolit: hmm
<LpSolit> it looks even
<ghendricks> ok, I will look at them
<ssdbot> [e-mail] denied review for attachment 201564 [details] [diff] [review] on bug 77193.
<ssdbot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=77193 enh, P2, Bugzilla 2.22, [e-mail], ASSI, Retire old versions, components and milestones.
<LpSolit> justdave_: I guess you won't take bug 77193 for 2.22?
<ssdbot> LpSolit: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=77193 enh, P2, Bugzilla 2.22, [e-mail], ASSI, Retire old versions, components and milestones.
<LpSolit> just to make ghendricks angry
<ghendricks>  ; ;

Comment 44

[Frédéric Buclin]

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

Comment 45

[Dave Miller [:justdave]]

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

Comment 46

[Frédéric Buclin]

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

Comment 47

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 48

[Frédéric Buclin]

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

Comment 50

[timeless]

Comment on attachment 201564 [details] [diff] [review]
2.21v4

>Index: template/en/default/admin/components/updated.html.tmpl
>-  <p>Nothing changed for component '[% name FILTER html %]'.
>+  <p>Updated component '[% name FILTER html %]'.

this change seems wrong

>Index: template/en/default/admin/milestones/updated.html.tmpl
>-  <p>Nothing changed for milestone '[% name FILTER html %]'.
>+  <p>Updated milestone '[% name FILTER html %]'.

this change seems wrong

>Index: template/en/default/admin/versions/edit.html.tmpl
>+      <th valign="top" align="right"><label for="version">Version:</label></th>
>+      <th valign="top" align="right"><label for="is_active">Active:</label></th>
>+      <td><input type="checkbox" name="is_active" value="1" 
>+           [% 'checked="checked"' IF is_active %]></td>

this is amazingly painful. sure it's easy to disable one version. but try disabling 20.

and I'm speaking from experience having seen what is probably this patch.

>Index: template/en/default/admin/versions/updated.html.tmpl
>-  <p>Nothing changed for version '[% name FILTER html %]'.
>+  <p>Updated version '[% name FILTER html %]'.

this change seems wrong

Comment 52

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 54

[Denis Roy]

We've had a request for this over at Eclipse.  See https://bugs.eclipse.org/bugs/show_bug.cgi?id=237821 for the use case.

Comment 55

[Prasad Wani]

I had done this changes for bugzilla version 2.18

I am having the milestone defined in follwoing fashion 

"M000-10MAR2008"
"M001-11MAR2008"
"M002-12MAR2008"
"M003-13MAR2008"
"M004-14MAR2008"
"M005-15MAR2008"

I want to hide the milestone which are less than today's date. For this i created the BANNED_MILESTONE array and then displaying the milestone which are not match with this arrary. Same you can do for Versions and Components. (dont forget to delte version cache before you proceed)

I am not able to this same thing in Bugzilla version 3.0. I don't know which files to edit :(

You need to edit the Four files mention below.

1. globals.pl

Search for line " sub GenerateVersionTable {" and add the following line just above it.

my %BANNED_MILESTONES = ();

Search for line " if ($dotargetmilestone) { " (mostly line no. 251)

change this full "if" block to following
	  

      if ($dotargetmilestone) {
          # reading target milestones in from the database - [e-mail]
          SendSQL("SELECT milestones.value, products.name " .
                  "FROM milestones, products " .
                  "WHERE products.id = milestones.product_id " .
                  "ORDER BY milestones.sortkey, milestones.value");
          my @line;
          my %tmarray;
          my $extracted_date;
          my $date;
          my $current_date;
          @::legal_target_milestone = ();
          while(@line = FetchSQLData()) {
              my ($tm, $pr) = (@line);
              if (!defined $::target_milestone{$pr}) {
                  $::target_milestone{$pr} = [];
              }
              push @{$::target_milestone{$pr}}, $tm;
              if (!exists $tmarray{$tm}) {
                  $tmarray{$tm} = 1;
                  push(@::legal_target_milestone, $tm);
                  $extracted_date = "$1 $2 $3 01:01" if ($tm =~ /M\d+-(\d+)(...)(\d+)/);
                  next if($extracted_date eq ""); # Keep "---", "M-old" and "M-future"

		  $date = str2time($extracted_date);
                  if(defined $date) {
                          $BANNED_MILESTONES{$tm} = 1 if ($date < time);
                  }
                  $date = "";
              }

              my ($tmbanned, $prbanned) = (@line);
              if(defined $BANNED_MILESTONES{$tmbanned})
              {
                    next;
              }
              if (!defined $::target_milestone_banned{$prbanned}) {
                  $::target_milestone_banned{$prbanned} = [];
              }
              push @{$::target_milestone_banned{$prbanned}}, $tmbanned;
              if (!exists $tmarray{$tmbanned}) {
                  $tmarray{$tmbanned} = 1;
                  push(@::legal_target_milestone, $tmbanned);
              }
          }

          print $fh (Data::Dumper->Dump([\%::target_milestone,
                                         \@::legal_target_milestone,
                                         \%::milestoneurl],
                                        ['*::target_milestone',
                                         '*::legal_target_milestone',
                                         '*::milestoneurl']));

          print $fh (Data::Dumper->Dump([\%::target_milestone_banned],
                                        ['*::target_milestone_banned']));
      }


2. Bugzilla/Bug.pm

Search for line " 'target_milestone' => $::target_milestone{$self->{product}}," and comment it. Add the following line just below it.
       my $present = 0;
       foreach my $version (@{$::versions_mod{$self->{product}}}) {
               $present = 1 if $version eq $self->{version};
       }

       if($present) {
               $self->{'choices'}->{'version'} = $::versions_mod{$self->{product}};
       } else {
               $self->{'choices'}->{'version'} = [@{$::versions_mod{$self->{product}}}, $self->{version}];
       }

       my $present_milestone = 0;
       foreach my $target_milestone (@{$::target_milestone_banned{$self->{product}}}) {
               $present_milestone = 1 if $target_milestone eq $self->{target_milestone};
       }

       if($present_milestone) {
               $self->{'choices'}->{'target_milestone'} = $::target_milestone_banned{$self-->{product}};
       }
 else {
    $self->{'choices'}->{'target_milestone'} = [@{$::target_milestone_banned{$self->{product}}}, $self->{target_milestone}];
       }


3. buglist.cgi

Searh for line "  $vars->{'targetmilestones'} = $::target_milestone{$product} if Param('usetargetmilestone'); " and comment it. Add the following line just below it.

 $vars->{'targetmilestones'} = $::target_milestone_banned{$product} if Param('usetargetmilestone');

4. enter_bug.cgi

Search for line " $vars->{'target_milestone'} = $::target_milestone{$product}; " and comment it. Add the following line just below it.

$vars->{'target_milestone'} = $::target_milestone_banned{$product};

Comment 57

[Greg Hendricks]

Attachment: 3.2 V1

I don't really think that this necessarily is blocked by bug 456743. I will look into doing that one next.

Comment 58

[Frédéric Buclin]

ghendricks, do you have an updated patch based on 3.5? I doubt this patch still applies cleanly.

Comment 59

[Max Kanat-Alexander]

Comment on attachment 376782 [details] [diff] [review]
3.2 V1

Instead of having ACTIVEONLY, you should always return only active components/versions/milestones unless otherwise specified. Also, you can't cache the result like you're doing, because people might want both active-only and all objects in the same request.

(The solution is to grep the result when we only want active objects.)

Otherwise this looks pretty decent, actually, though you may want to change the accessors to be is_active and the mutators to be set_is_active.

Comment 61

[David Lawrence [:dkl]]

Attachment: Updated patch that allows retiring of components/versions/milestones (v1)

I have taken the last patch and ported it to current trunk. I also made some changes to allow for the current value for a bug to still be visible when even if set inactive. When the bug's value is changed to something else, then it is no longer visible. All values remain visible in the query.cgi search UI.

Dave

Comment 62

[David Lawrence [:dkl]]

(In reply to comment #59)
> Comment on attachment 376782 [details] [diff] [review]
> 3.2 V1
> 
> Instead of having ACTIVEONLY, you should always return only active
> components/versions/milestones unless otherwise specified. Also, you can't
> cache the result like you're doing, because people might want both active-only
> and all objects in the same request.

So to clarify, we should always return all values and filter out the inactive values before display for show_bug.cgi/enter_bug.cgi or should we only return active values by default but allow a param that adds back in the inactive ones?

> (The solution is to grep the result when we only want active objects.)
> 
> Otherwise this looks pretty decent, actually, though you may want to change the
> accessors to be is_active and the mutators to be set_is_active.

Sorry. Missed these comments when working on an updated patch. I will address these and submit a new patch.

Dave

Comment 63

[Max Kanat-Alexander]

Comment on attachment 468216 [details] [diff] [review]
Updated patch that allows retiring of components/versions/milestones (v1)

You know, in general, I'm wondering why we have ACTIVE_ONLY at all. Why not just always filter things in the UI? That's how we do things now, for disabled values in global fields.

As much as possible, the code for setting, checking, and getting is_active should go into ChoiceInterface.

I don't see any reason to allow creating things inactive. Might as well keep the UI simple, there.

The "If unchecked" text isn't exactly true. Also, in the UI, it should be called "Enabled for Bugs". Then the "If unchecked" text isn't necessary. (See how it looks currently in editvalues.cgi.)

"Component/Milestone active updated to" is not very good text. We should use whatever text that editvalues.cgi is currently using.

Comment 64

[David Lawrence [:dkl]]

Attachment: Updated patch that allows retiring of components/versions/milestones (v2)

New patch that does away with ACTIVE_ONLY and simply filters the values in the UI being careful to include values that are currently set for the bug. Also added code to throw error in validators if a user tries to set the bug to a non-active value unless the bug is already set to the value.

Please review.
Dave

Comment 65

[Max Kanat-Alexander]

Comment on attachment 469355 [details] [diff] [review]
Updated patch that allows retiring of components/versions/milestones (v2)

>=== modified file 'Bugzilla/Bug.pm'
>--- Bugzilla/Bug.pm	2010-08-24 20:25:49 +0000
>+++ Bugzilla/Bug.pm	2010-08-26 05:45:40 +0000
>@@ -1434,6 +1434,16 @@
>     my $product = blessed($invocant) ? $invocant->product_obj 
>                                      : $params->{product};
>     my $obj = Bugzilla::Component->check({ product => $product, name => $name });
>+    # Do not allow a value to be set if value is not active unless
>+    # the value is already set to the non active value.
>+    if (!$obj->is_active
>+        && (!blessed($invocant) 
>+            || $invocant->component ne $obj->name))
>+    {
>+        ThrowUserError('value_is_not_active', 
>+                       { field => 'component',
>+                         value => $obj->name });
>+    }

  Don't implement this validation yet--we don't have it for the global fields, so it's something we should do in a separate bug. Right now, retiring is just a UI convenience.

>=== modified file 'Bugzilla/Install/DB.pm'
>+    # 2009-05-07 [e-mail] - Bug 77193
>+    _retire_old_values();

  Let's call it _add_isactive_to_product_fields()

>+sub _retire_old_values {
>+    # If we add the isactive column all values should start off as active
>+    if (!$dbh->bz_column_info('components', 'isactive')) {
>+        $dbh->bz_add_column('components', 'isactive', 
>+            {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'});
>+        $dbh->do("UPDATE components SET isactive = 1");

  You don't need that UPDATE--the column will default to 1 when you add it, because of the DEFAULT.

  (Same for milestone and version too.)

>=== modified file 'editcomponents.cgi'
>+    my $isactive           = $cgi->param('isactive') ? 1 : 0;

  No need for the ? 1 : 0 -- the validator will do that.

>+    my $isactive              = $cgi->param('isactive') ? 1 : 0;
>+  

  Same there.

>=== modified file 'editmilestones.cgi'
>+my $isactive       = $cgi->param('isactive') ? 1 : 0;

  And there (etc.)

>=== modified file 'editversions.cgi'
> if ($action eq 'new') {
>     check_token_data($token, 'add_version');
>     my $version = Bugzilla::Version->create(
>-        { value => $version_name, product => $product });
>+        { value => $version_name, product => $product, isactive => $isactive });

 I don't think we need or want to allow setting things inactive when creating new values. We don't currently allow it for the global fields, so we should be consistent here.

>=== modified file 'template/en/default/admin/components/edit-common.html.tmpl'
>+  <td><label for="isactive">Active:</label></td>

  Should be "Enabled For Bugs", which is what the global fields currently use.

  (This should be changed everywhere in this patch.)

>+       <label for="isactive"><em>If unchecked, this component will not  
>+       appear for new [% terms.bugs %].</em></label></td>

  And then this text can go away.

Comment 66

[David Lawrence [:dkl]]

Attachment: Updated patch that allows retiring of components/versions/milestones (v3)

Thanks for the review Max. Here is a new patch with your suggested changes.

Dave

Comment 67

[Max Kanat-Alexander]

Comment on attachment 470329 [details] [diff] [review]
Updated patch that allows retiring of components/versions/milestones (v3)

Looks great!

By the way, I just noticed that is_active is generally not respected on the Change Several Bugs At Once page--we should probably fix that, no? (But in a separate bug.)

Comment 68

[David Lawrence [:dkl]]

Attachment: Updated patch that allows retiring of components/versions/milestones (v4)

I mistakenly left out filtering of version/components/milestones in buglist.cgi when editing multiple bugs. Here is an updated patch that adds that, otherwise unchanged.

Please review.
Dave

Comment 69

[Max Kanat-Alexander]

Comment on attachment 470683 [details] [diff] [review]
Updated patch that allows retiring of components/versions/milestones (v4)

Oh, I just noticed: the update pages say "Active changed to" instead of a more appropriate message about enabling or disabling the value for bugs.

Comment 70

[David Lawrence [:dkl]]

Attachment: Updated patch that allows retiring of components/versions/milestones (v5)

When updating the value it now says "Enabled/Disabled for bugs".

Please review
Dave

Comment 71

[Max Kanat-Alexander]

Comment on attachment 470688 [details] [diff] [review]
Updated patch that allows retiring of components/versions/milestones (v5)

You are the man! :-) (As usual.)

Comment 72

[David Lawrence [:dkl]]

modified buglist.cgi
modified editcomponents.cgi
modified editmilestones.cgi
modified editversions.cgi
modified enter_bug.cgi
modified Bugzilla/Component.pm
modified Bugzilla/Milestone.pm
modified Bugzilla/Version.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/admin/components/confirm-delete.html.tmpl
modified template/en/default/admin/components/edit.html.tmpl
modified template/en/default/admin/components/list.html.tmpl
modified template/en/default/admin/milestones/edit.html.tmpl
modified template/en/default/admin/milestones/list.html.tmpl
modified template/en/default/admin/versions/edit.html.tmpl
modified template/en/default/admin/versions/list.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/global/messages.html.tmpl
Committed revision 7456

Comment 73

[Gervase Markham [:gerv]]

Where was this checked in? I don't see this UI in 4.0...

Gerv

Comment 74

[David Lawrence [:dkl]]

(In reply to comment #73)
> Where was this checked in? I don't see this UI in 4.0...
> 
> Gerv

It made it into trunk but not 4.0

https://bzr.mozilla.org/bugzilla/trunk/revision/7456

Dave

Comment 75

[Frédéric Buclin]

(In reply to comment #74)
> It made it into trunk but not 4.0

... as indicated by the target milestone. ;)

/me waves gerv

Comment 76

[miketosh]

Comment on attachment 470688 [details] [diff] [review]
Updated patch that allows retiring of components/versions/milestones (v5)

>=== modified file 'editmilestones.cgi'
>--- editmilestones.cgi	2010-06-22 02:38:34 +0000
>+++ editmilestones.cgi	2010-08-29 20:17:56 +0000
>@@ -115,9 +116,11 @@
> 
> if ($action eq 'new') {
>     check_token_data($token, 'add_milestone');
>-    my $milestone = Bugzilla::Milestone->create({ value   => $milestone_name,
>-                                                  product => $product,
>-                                                  sortkey => $sortkey });
>+
>+    my $milestone = Bugzilla::Milestone->create({ value    => $milestone_name,
>+                                                  product  => $product,
>+                                                  sortkey  => $sortkey });
>+
>     delete_token($token);

Uh, looking at this patch, nothing changed here.
Should 'isactive' have gotten passed in here?  Could be a simple oversight? Or is there a bug in the Milestone->create that doesn't like setting isactive on create?

(Oh, and a million thanks to ghendricks for doing the leg work here!  Porting this back to 4.0 is a cinch)

Comment 77

[miketosh]

(In reply to comment #76)
> (Oh, and a million thanks to ghendricks for doing the leg work here!  Porting
> this back to 4.0 is a cinch)

Doh! Thanks goes to David L too.

Comment 78

[Gervase Markham [:gerv]]

miketosh: if you think there's a bug in the code, best to open another bug (and give symptoms :-).

Gerv