Bug 17453 (bz-enums)

Enumerators in Bugzilla are not cross-DB compatible

RESOLVED FIXED in Bugzilla 2.20

Status

()

P1
normal
RESOLVED FIXED
20 years ago
5 years ago

People

(Reporter: andrejohn.mas, Assigned: mkanat)

Tracking

(Blocks: 1 bug)

2.10
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(3 attachments, 9 obsolete attachments)

14.32 KB, text/plain
Details
1.23 KB, patch
bbaetz
: review-
Details | Diff | Splinter Review
21.82 KB, patch
bugreport
: review+
Details | Diff | Splinter Review
(Reporter)

Description

20 years ago
Although this has not been an issue yet it might become one. The operating
system types and platforms are define as values of enumerators in the bugs
database. If there is a new OS that comes out, either by name or version
I would have to modify the value of the definition of the enumerator. This
is not very clean, in the sense that attributes should be set once at the
creation of the database, any other value should really be in a table where
it is easy to update. Sure a table with one column is not very elegant, but
it does provide the flexibility that time would require.

Updated

20 years ago
Status: NEW → ASSIGNED

Comment 1

19 years ago
One issue with changing enum columns to text columns (am assuming that
the proposed new table would have some type of text column) will be
to make sure that the values continue to sort in proper order.
Especially if fields such as Status and Resolution are changed,
as well as Platform and OS.

Comment 2

19 years ago
tara@tequilarista.org is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)
Assignee: terry → tara
Status: ASSIGNED → NEW

Comment 3

19 years ago
enums are bad because they are specific to mysql. this will be fixed when we 
re-do the database schema
Assignee: tara → cyeh
Whiteboard: 3.0

Comment 4

18 years ago
Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.
QA Contact: matty
Moving to real milestones...
Whiteboard: 3.0
Target Milestone: --- → Bugzilla 3.0
Taking all Bugzilla 3.0 bugs -- congratulations to MattyT for the triage, it
really was spot on. Note: I may end up pushing some of these bugs back to 3.2,
we'll see. However, I believe all these bugs should just fall out of the 
redesign. Let's hope I'm right!

(Note: I'm also resetting the priority field to "---" so that I can retriage any
that I consider important or likely to be dropped.)
Assignee: cyeh → ian
Component: Bugzilla → Bugzilla 3
Priority: P3 → --
Stealing this back for Bugzilla 2.16.  I've found numerous comments in other 
places by both cyeh and tara indicating that this was going to be one of the 
first priorities after 2.14 shiped.  The status of the bug about it needs to 
reflect that.
Assignee: ian → tara
Component: Bugzilla 3 → Bugzilla
Priority: -- → P1
Target Milestone: Bugzilla 3.0 → Bugzilla 2.16
*** Bug 88188 has been marked as a duplicate of this bug. ***
*** Bug 92597 has been marked as a duplicate of this bug. ***
moving
Assignee: tara → justdave
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → 2.10
I have made preliminary changes to do away with enum types in Mysql. I am
uploading a enum patch against 2.14 for review that alleviates enum types
in the database and adds the necessary tables to replace them. Only two files so
far seem to be affected, globals.pl and checksetup.pl. Please let me know what
you think. I am running it on a test bugzilla install with not noticable problems.

Comment 13

18 years ago
Comment on attachment 48106 [details] [diff] [review]
Patch to alleviate enum data types

> # Contributor(s): Terry Weissman <terry@mozilla.org>
> #                 Dan Mosedale <dmose@mozilla.org>
> #                 Jake <jake@acutex.net>
>+# 				  David Lawrence <dkl@redhat.com>

Why is your name so much farther over than everyone elses?  :)

>+# Set driver to desired database, ex. mysql, Pg, Oracle, etc.
>+$::driver  = "mysql";
>+

If we're gonna have this, it need to be in localconfig, not globals.pl.  I'd personally hope
to aviod having this for the time being.

>+    if ( $::driver eq 'mysql' ) {
>+        SendSQL("show columns from $table");
>+        while ( my @row = FetchSQLData() ) {
>+            my ($name,$type) = (@row);
>+            $a{"$name,type"} = $type;
>+            push (@list, $name);
>+        }
>+    } elsif ( $::driver eq 'Pg' ) {
>+        my $ref = $::db->func($table, "table_attributes");
>+        for my $index ( 0..@{$ref} ) {
>+            next if !$ref->[$index]->{'NAME'};
>+            my $name = $ref->[$index]->{'NAME'};
>+            my $type = $ref->[$index]->{'TYPE'};
>+            $a{"$name,type"} = $type;
>+            push (@list, $name);
>+        }
>+    }   

There almost has to be a good way to make DBD fake this for us.  It can't be that uncommon
to want to know what all columns are in a table.

> sub SplitEnumType {

Hmmm... I think if we keep the name 'SplitEnumType()' it might not be a bad idea to explain
that it's a legacy name.

>Index: checksetup.pl
...
>+    bug_severity varchar(255) not null,
>+    bug_status varchar(255) not null,
...
>+    op_sys varchar(255) not null,
>+    priority varchar(255) not null,
...
>+    rep_platform varchar(255) not null,
...
>+    resolution varchar(255) not null,
Isn't 255 a bit big for these fields?

>+        if ( $tabname =~ /$enum/ ) {

I know matching the table in a regexp is done just a few lines before this, but this
really isn't a good idea (see bug 95890)

> # Final checks...
>+
>+# 2001-09-03 dkl@redhat.com bug17453
>+# Moved enum types to separate tables so we need change the old enum types to 
>+# standard varchars in the bugs table.
>+ChangeFieldType ('bugs', 'bug_status', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'resolution', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'priority', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'bug_severity', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'rep_platform', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'op_sys', 'varchar(255) not null');

These really should be done before the # Final checks comment (you should start at line
2530 in the current CVS, not 2542 where this one does.


Also, we'll need to add some checks to sanitycheck.cgi to make sure that any
os/resolution/etc that exists in the bugs table also exists in it's associated 'enum' table.
Attachment #48106 - Flags: review-

Comment 14

18 years ago
-> Patch Writer
Assignee: justdave → dkl
I made suggested changes by Jake and also added a section to sanitycheck.cgi
that reports number of bugs that have values not present in the separate enum
tables.
I don't really support because it contains schema changes that checksetup.pl
will need to convert later.  This is because I'm intending on doing all this and
more for 2.16, including using IDs for everything, allowing records to be set
inactive, sortkeys, etc.

The start of this will be bug #94534 for resolutions, which I will shortly
upload a patch to.
Blocks: 98304
Also, the sanity checks you added are referential checks, and as such, you
should use the CrossCheck subroutine.  See the other uses of CrossCheck in the
file.
Why VARCHAR?  The size of everyone's database will explode depending on how many
bugs they have in there.  Going from 1-2 bytes per field per bug for an enum to
N+1 bytes for varchar?  Why not create a separate table each for OS, platform,
etc.  Have each table's keyID field be whatever is the least common denominator
of an unsigned int.  (tinyint, smallint, whatever).  Then in the bug's table,
store the OS ID or Platform ID or whatever.  We can do a sub-select (or 2nd
query for dbs that don't do subselects) to get what to substitute.  Or since
those don't change much at all, we can do something similar to what we do with
localconfig.js.  But instead of generating a js file, just generate a perl file
with pre-made variables.  Does this make any sense at all?
right in with the rest of the crap in versioncache :)
This is likely to change.  We are already moving to IDs for resolutions (bug
#94534) and products/components (bug #43600).  I'll want to move more over as a
part of bug #97706.  I also suggested to dkl we make statuses an integer with
hardcoded meaning and eschew a statuses table until we're ready to do customised
statuses (bug #101179), since it will make that easier.

We may not get all that done for 2.16, but we're unlikely to get all the
PostgreSQL stuff done for 2.16 either.
This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.
Moving off, custres at least won't make 2.16.
Depends on: 43600, 94534
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18

Comment 24

17 years ago
Following dkl's patch, here is a script that will allow you to edit/add/delete
the values in the tables used to replace the "enum" data types.  Works with
2.15

Comment 25

17 years ago
Patch to include a link to edittable.cgi for 2.16rc1

Updated

16 years ago
Attachment #84253 - Flags: review?

Updated

16 years ago
Attachment #48114 - Attachment is obsolete: true

Updated

16 years ago
Attachment #48106 - Attachment is obsolete: true
Attachment #48114 - Attachment is obsolete: false
Attachment #48114 - Flags: review?(bbaetz)
Comment on attachment 48114 [details] [diff] [review]
Second version with Jake suggested changes

This all needs to be done with ids.

For the moment, theres nothing wrong with a CHECK constraint, and stuff can get
moved along  to lookup tabes as customfields/custres/etc happen
Attachment #48114 - Flags: review?(bbaetz) → review-
Comment on attachment 84253 [details] [diff] [review]
Patch to include link to edittable.cgi

This is just an addon to the other patch
Attachment #84253 - Flags: review? → review-
(Assignee)

Updated

15 years ago
No longer blocks: 98304
(Assignee)

Updated

15 years ago
Blocks: 146679
enhancements without current patches are being pushed to 2.20
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
(Assignee)

Comment 30

15 years ago
Posted patch Un-bitrotted varchar patch (obsolete) — Splinter Review
This patch is basically just an un-bit-rotted version of what dkl posted quite
some time ago. I've done some basic testing on it, and it still works just as
well as it did back then.

I talked to justdave a bit about this in IRC.

At first, I wanted to go to using ids and foreign keys and all that. It turns
out, though, that that would involve major and invasive changes to Bugzilla,
because it would change the type of all the affected fields from a string to an
integer, as far as the perl was concerned. That is, if we have to both
eliminate enumerators and convert to a foreign-key/id-based system, we'll never
get this badly-needed patch in.

The nice thing about this patch is that it only involves a very small change
for a very important architectural benefit.

Red Hat uses varchars now; I've been running their code fork for over a year
locally. It doesn't cause a speed problem, and the database doesn't seem
significantly bloated here.

I've given all the tables a primary key that's a smallint, though. The plan is
to:

(1) Move enums to varchars.
(2) Move these varchars to integers.

That breaks up the patch much more nicely.

There ARE some problems with this patch which have to be resolved:

(1) The sort order of statuses and resolutions becomes alphabetical. This can
be fixed by a hack that I have locally. Basically, every time you see something
like "resolution" in the sort order, you replace it with "resolution.id". You
also have to add the appropriate table to the JOIN, and make sure that you also
modify the GROUP BY clause as necessary.
  I have code for this that works; I can clean it up and post it here soon.

(2) The actual resolutions that are allowed to be inserted into the DB becomes
unbounded. This is *nice* because it means that I can delete resolutions, and
clean up afterward if I want. The only place this might cause a problem is if
there's some security hole that we don't know about in the perl side of things,
and then bugs can be made to have invalid statuses or resolutions. Not too big
of a deal, and sanitycheck catches it easily.
(Assignee)

Updated

15 years ago
Attachment #48114 - Attachment is obsolete: true
(Assignee)

Comment 31

15 years ago
Whoops, the previous patch accidentally also had a very old docs patch of mine
inside it.
(Assignee)

Updated

15 years ago
Attachment #163454 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Depends on: 179451

Comment 32

15 years ago
(In reply to comment #30)
> At first, I wanted to go to using ids and foreign keys and all that. It turns
> out, though, that that would involve major and invasive changes to Bugzilla,
> because it would change the type of all the affected fields from a string to an
> integer, as far as the perl was concerned. That is, if we have to both
> eliminate enumerators and convert to a foreign-key/id-based system, we'll never
> get this badly-needed patch in.
What about using views to hide the IDs?

> (1) The sort order of statuses and resolutions becomes alphabetical. This can
> be fixed by a hack that I have locally. Basically, every time you see something
> like "resolution" in the sort order, you replace it with "resolution.id". You
> also have to add the appropriate table to the JOIN, and make sure that you also
> modify the GROUP BY clause as necessary.
>   I have code for this that works; I can clean it up and post it here soon.
I would suggest using a seperate field for sort order; that would allow for
arbitrary changes without the uglyness of deleting and inserting or changing
your primary key.

> (2) The actual resolutions that are allowed to be inserted into the DB becomes
> unbounded. This is *nice* because it means that I can delete resolutions, and
> clean up afterward if I want. The only place this might cause a problem is if
> there's some security hole that we don't know about in the perl side of things,
> and then bugs can be made to have invalid statuses or resolutions. Not too big
> of a deal, and sanitycheck catches it easily.
Wouldn't referrential integrity be a much better way to ensure there are no
invalid statuses or resolutions?
(Assignee)

Comment 33

15 years ago
(In reply to comment #32)
> What about using views to hide the IDs?

  Eh? In MySQL 3? Could you explain that a bit more? :-) Maybe I'm just missing
something.

> I would suggest using a seperate field for sort order; that would allow for
> arbitrary changes without the uglyness of deleting and inserting or changing
> your primary key.

  I totally agree, I'll add that.

> Wouldn't referrential integrity be a much better way to ensure there are no
> invalid statuses or resolutions?

  Yes, if we were using/requiring a database that provided referential
integrity. You'll notice that that is almost the entire purpose of sanitycheck
at this point, to assure referential integrity.

  Also, seeing as how dkl hasn't yet commented on this, and I've emailed him and
he hasn't responded, I'm going to take this since I'm actively working on it and
I really care about it. DKL, you can totally feel free to take it back from me,
if you want.
Assignee: dkl → mkanat

Comment 34

15 years ago
(In reply to comment #33)
> (In reply to comment #32)
> > What about using views to hide the IDs?
> 
>   Eh? In MySQL 3? Could you explain that a bit more? :-) Maybe I'm just missing
> something.

No, guess not (well, except for a real database ;P)

>   Yes, if we were using/requiring a database that provided referential
> integrity. You'll notice that that is almost the entire purpose of sanitycheck
> at this point, to assure referential integrity.

I think it would be worthwhile to look at supporting real RI once real databases
are supported. Faking RI in the application not only performs worse, it's prone
to breaking. Of course that's beyond the scope of this bug.

Comment 35

15 years ago
I'm glad to see someone doing this because I need it as does anyone who makes
hardware and doesn't want to have to upadte the schema every time a new product
ships.

I am a little concerned about using a varchar instead of the id value.  I'm torn
between being a purist and being expedient here.  

Comment 36

15 years ago
Comment on attachment 163456 [details] [diff] [review]
v3.1 (Without the extra patch in it)

OK, vladd and I debated this in IRC a bit.  Using the Varchar() is not as much
of a shortcut as it would appear to be because you still have to handle renames
properly etc...

So, we should really do this "right" and reference the ID.  I suggest the
following...

Add ONE new table with columns...
ID - primary key, used for the references
field - the fieldid of the former enum field for which this row applies
name - the text
sortkey - the sort order to put these enume types in order.  In case of a tie,
we go alphabetical, so we always sort by sortkey, name
status - either ENUM_NORMAL, ENUM_DEFAULT, or ENUM_DELETED

Then, the code in checksetup becomes strictly migration code and you need a UI
to create, edit, delete, and rename the types.

The added difficulty of the patch is not so bad once you take into account the
way that fixes renames.

It looks like this is already a P1, so I can't raise it any further.  I wish I
could.
Attachment #163456 - Flags: review-

Comment 37

15 years ago
(In reply to comment #36)
> status - either ENUM_NORMAL, ENUM_DEFAULT, or ENUM_DELETED

Per comment #3, we should use something else because this is MySQL specific and
maybe we do it right this time from the beginning :)

Comment 38

15 years ago
Actually, that is what I meant.  Those would be constants in Constants.pm

I'm open to suggestions on better names.

Comment 39

15 years ago
Some brainstorming:

Maybe we should create a single table, and an additional column called "Field ID"?

We could have 1 for "Hardware" and 2 for "OS". This would make really really
easy to convert future fields to such said system, and would drive us right into
the implementation of custom fields (or maybe ten kilometers closer :) ).

When implementing the said UI, we could have a param, field, that would specify
that field to edit, something like: editfields.cgi?fieldid=1.
Adding/removing/editing values for drop-down fields would have the advantage to
be made using one centralized UI form.


So, even if we never implement custom fields, having only one table with an
additional FIELD_ID column could have its benifits.

Comment 40

15 years ago
There is already a table of field ids... fielddefs

Do we really need a new one?

Comment 41

15 years ago
joel: This would be a table with the options that drop-down fields can take. We
only specify the fieldid so that it associates a possible field value with the
said fieldid.

Comment 42

15 years ago
This sounds like your field column:

field - the fieldid of the former enum field for which this row applies

only that I would say:

field - the fieldid where this drop-down option is possible.

Comment 43

15 years ago
Yes, I think we are both saying the same thing.

Comment 44

15 years ago
I'd also like to make sure that myk agrees with the schema change.  All the
commonly used Search.pm criteria would look up the numeric values before
starting the search.  The boolean charts might just do the extra join.  This
would add a simple join to fetch displayed fields, but that is relatively minor
since it effects only the results returned.  In general, I think this is all
goodness jut like moving product and components to used ids.

Comment 45

15 years ago
(In reply to comment #39)
> Some brainstorming:
> 
> Maybe we should create a single table, and an additional column called "Field ID"?

There is a downside to this approach: it makes doing refferential integrity a
pain. You'd have to emulate it using triggers.
(Assignee)

Comment 46

15 years ago
I'm cautious about adding too much "it would be cool if" to this bug -- it's
already been open much too long. :-)

When I first took over this bug, I looked at the code changes required to move
to ids, and it's non-trivial. I don't recall the exact details, but when I got
into it, it was a major modification to the behavior of Bugzilla in certain
places. The basic details were that the enum types were considerably different
than the product/component stuff, particularly as the Bugzilla process flow is
centered around them, touching almost everywhere in the code. (Of course, thanks
to SQL, we should be able to hide some complexity by always returning the
values, and not the ids, to perl.)

Actually, even a hack to do the rename would be a smaller code change than
moving to ids. However, if moving to ids is the "right way" of doing it, then
that's what we'll do.
Getting rid of enumerators is the right thing to do.

We should use unique tables for each bug attribute, rather than a single table
for all of them, because that models their distinct real-world meanings.  After
all, status and severity may both be attributes of bugs, but they are
fundamentally different attributes (as are version, milestone, and component)
whose values bear no real-world relationship to each other (unlike the reporter
and assignee attributes, whose values are all Bugzilla users and may even be the
same user).

As for IDs versus values in the bugs table, IDs are the way to go in the long
run, but there's nothing inherently unacceptable about using names temporarily
if they move us forward.  There's a development cost in having to write code to
update the names everywhere they're used when they change as well as to fix the
increased number of bugs in such code; but if switching to names first is much
easier, and it doesn't make switching to IDs in the future much harder, then it
may be a logical first step.

In general, several small changes usually gets us there faster and better than
one big one, although I haven't looked at the code enough to determine if that's
the case here.
bbaetz' opinion on the schema issues would also be welcome here.

Comment 49

15 years ago
OK, then attachment 163456 [details] [diff] [review] is a lot closer to the right thing than I previously
said.

Let's try...

Make sure that we handle renames properly (I think this means that we use an
administrative GUI rather than localconfig)

Make sure that, when we delete a name from the list, we just deactivate it from
being selected rather than forgetting it was ever valid.  Either that or we
require anything using that value to be changed before deleting a name.

I suggest that the tables holding the legal values follow a clear naming
convention like legal_op_sys and legal_priority, etc....

I think that providing attributes (sortkey and status { NORMAL, DEFAULT, DELETED
} ) in the legal_* table is a good way to go.

I have a hidden agenda here (especially in suggesting the strong naming
convention).  The standard bugzilla will use the same mechanism to manage
op_sys, priority, status, etc...  That should make it very easy to hack in a few
more using the same mechanism.

> Make sure that, when we delete a name from the list, we just deactivate it
> from being selected rather than forgetting it was ever valid.  Either that
> or we require anything using that value to be changed before deleting a name.

We do both of these with some other attributes, and I think it would make sense
to do so with these attributes as well.

> I suggest that the tables holding the legal values follow a clear naming
> convention like legal_op_sys and legal_priority, etc....

I agree about the clear naming conventien, but I think we should be consistent
with other tables holding bug attributes like the components, milestones, and
versions tables and just name these after their related bug table columns, i.e.
op_syses and priorities.

In addition to being consistent, those names are simpler and don't imply that
all values in the table are legal (which, if some values can be disabled, seems
misleading and may be inconsistent with how we use the concept of legality in
other parts of Bugzilla).

> I think that providing attributes (sortkey and status { NORMAL, DEFAULT, DELETED
> } ) in the legal_* table is a good way to go.

Note that other Bugzilla tables use a boolean "isactive" column to specify
whether or not a given value can now be associated with bugs.  We should
probably do that here as well.

Comment 51

15 years ago
(In reply to comment #47)
> if they move us forward.  There's a development cost in having to write code to
> update the names everywhere they're used when they change as well as to fix the
> increased number of bugs in such code; but if switching to names first is much
> easier, and it doesn't make switching to IDs in the future much harder, then it
> may be a logical first step.

Doesn't mysql support refferential integrity that propagates updates? I know
PostgreSQL, Sybase, and the 'big 3' all do. That would remove the need for any
extra code to handle renames.
MySQL's InnoDB table type supports referential integrity, although I don't know
if it automatically propagates updates.  We don't use that table type at the
moment, although we could and probably should.  That's perhaps a larger change,
however, and it might require more work than we want to do at this stage.
(Assignee)

Comment 53

15 years ago
OK, then here's a summary of the changes that need to be made to attachment 163456 [details] [diff] [review]:

General
-------

+ We want to add an "isactive" column to each table. isactive=0 values will
still be valid if they show up, but they will not be in lists otherwise. That's
actually going to be a bit complex in editbugs. 

+ The table names must change. In one way, I prefer the legal_* option, as that
makes it easier to look at the schema in a GUI tool, and it groups all our
"field value" tables together in a logical fashion. I know that right now we use
"products" and "components", but we could say that's a legacy issue and go to
legal_* from here on out for enumerated types. On the other hand, there's a
chance that at some point these tables may evolve to be complex like the
"products" and "components" table, and then I'd feel sort of silly having called
them legal_*.

  I'd really like a comment from bbaetz or justdave on the above. Without that,
I'll follow Myk's authority and use the "op_syses" method, which I think also
makes a lot of sense.

+ Add a "sortkey" field that's a smallint.

Renames
-------

+ Renaming a value must be handled gracefully. This will require an interface
much like versions has for products/components. As a side effect, we will
finally be able to set their sort value! Hooray! Unfortunately, there's no way
to conceptualize a "rename" in localconfig, so this interface MUST be added. It
should be easy, mostly copy/paste from the Target Milestone interface.

+ The above change has a side-effect: Dealing with the enum types in
checksetup.pl and localconfig will go away. This means that the first time
checksetup.pl is run, it will create the "default" values (the standard BZ
values) for Resolution, Status, Severity, and Priority. Otherwise, the default
values will be:

Platform: --- (isactive=0), PC, Macintosh, Other, All
Op_sys: --- (isactive=0), Windows, Mac OS, Linux, Other, All

  checksetup.pl will not re-create these values later, as after the first
install they are dealt with by the admin.

  As far as I know, defaults for the values don't really matter, and so don't
need to be dealt with at this point.

  This patch will not allow you to *truly* gracefully rename
resolutions/statuses, as that would require major changes to templates, and
would also require ids. However, the patch will deal with everything but the
templates, keeping the functionality that we have now. A future bug can handle
the rest.
(In reply to comment #53)
> On the other hand, there's a
> chance that at some point these tables may evolve to be complex like the
> "products" and "components" table, and then I'd feel sort of silly having
> called them legal_*.
> 
>   I'd really like a comment from bbaetz or justdave on the above. Without
> that, I'll follow Myk's authority and use the "op_syses" method, which I
> think also makes a lot of sense.

Yeah, I think legal_* sounds funny, too.  Go with the basics :)

>   As far as I know, defaults for the values don't really matter, and so don't
> need to be dealt with at this point.

The only potential issue I can think of is with the browser/OS detection in
enter_bug.cgi...  As long as it gracefully deals with missing options, it should
be okay.  We should probably get good documentation of what the detection stuff
looks for, so if people want those detected, they can add the values that it can
find.

Comment 55

15 years ago
iirc it doesn't fail very gracefully, i don't have time to check atm.

Comment 56

15 years ago
We need to find a better suffix than "es." A lot of code will need to get from
the field name to the table name so we need to be 100% consistent about it.
op_syses is fine, but then priority becomes priorityes.  How about "_vals" so we
have "op_sys_vals" and "priority_vals".

I'm open to alternatives, but the consistency is very important.
> We need to find a better suffix than "es."

This is usually solved by using singular names, so that the tables would be
op_sys and priority.  Along with making the table names consistent (although not
consistent with existing pluralized names), this makes certain logic dealing
with the tables and their values easier to code.

Singular names are a popular and accepted way of naming tables (and OO classes,
which tables in a relational DB are roughly equivalent to).  In fact, using
singular names has been proposed before by Bugzilla developers, and we use it in
module (Bug.pm, Attachment.pm) and template (template/en/default/bug|attachment)
names.  We should do the same here.
The biggest issue with this is query performance. Mysql won't use more than one
index per table. You'd have to have query.pm do a lookup first, rather than
doing this as a join. (On other dbs, it should instead be a simple subselect)

Comment 59

15 years ago
(In reply to comment #58)
> The biggest issue with this is query performance. Mysql won't use more than one
> index per table. You'd have to have query.pm do a lookup first, rather than
> doing this as a join. (On other dbs, it should instead be a simple subselect)

I'm not sure what comment this is a reply to, but most databases wouldn't use an
index to scan a tiny lookup table because it would actually be more expensive
than just doing a table-scan.
(Assignee)

Comment 60

15 years ago
(In reply to comment #58)
> The biggest issue with this is query performance. Mysql won't use more than one
> index per table. You'd have to have query.pm do a lookup first, rather than
> doing this as a join. (On other dbs, it should instead be a simple subselect)

I'm also not entirely sure what this is in reply to, since we'll only be using
one index. When we're using varchars, it will be the varchar index, and when
we're using ids, it will be the id index.
Create a dummy 100,000 row bus table and compare the perf with a query selecting
on platform and os and something else which already does a join.

In theory you're right, but in practice there are issues.

Comment 62

15 years ago
Corrct me if I'm wrong, but using a varchar or an ID (assuming we look up the ID
first rather than doing a join) should be no better and no worse
performance-wise than an ENUM.  Am I wrong?
(Assignee)

Comment 63

15 years ago
OK, here are the other changes, then, and the final decisions:

+ We will use singular names for the tables.
+ Fix browser/OS detection in enter_bug.cgi (which should be done anyway) to
deal with missing options.

- Query Performance should be identical to sorting against target_milestone as
it works currently, and so should not matter.

There are no other joins that are relevant with the varchar method, other than
the above. Since the current column type in the bugs table stays the same, the
only issue is referential integrity, which does not need to be checked in the
Search.pm query, only in sanitycheck.cgi.
(Assignee)

Comment 64

15 years ago
Update: I've completed the checksetup changes, on to more.

Notes to self:
  + An enum table cannot be allowed to be emptied. Otherwise, checksetup will
re-create it with the default values. The GUI will just have to disallow
deleting the final value, or always make sure that a "---" isactive=0 value is
in there.

  Also, will I need a new permission bit for the "fieldvalues" edit screen, or
should I just use the "admin" bit?
Status: NEW → ASSIGNED

Comment 65

15 years ago
I'll vote for using the existing "admin" group.
I'm coming into this bug late as I'm trying to catch up on bugmail... :)

(In reply to comment #64)
>   + An enum table cannot be allowed to be emptied. Otherwise, checksetup will
> re-create it with the default values. The GUI will just have to disallow
> deleting the final value, or always make sure that a "---" isactive=0 value is
> in there.

I'd go with the "never allow the last value to be deleted" option, personally. 
Need to always require at least one enabled option as well.  If they're all
disabled, then the enter_bug page breaks.

>   Also, will I need a new permission bit for the "fieldvalues" edit screen, or
> should I just use the "admin" bit?

It can probably go under editcomponents for now.  That'll get broken apart more
later when we revamp the admin permissions.
>   + An enum table cannot be allowed to be emptied. Otherwise, checksetup will
> re-create it with the default values.

Can't checksetup just check for table existence?


>I'd go with the "never allow the last value to be deleted" option, personally. 
>Need to always require at least one enabled option as well.  If they're all
>disabled, then the enter_bug page breaks.

Seems like fixing the bug in enter_bug that causes it to break on empty lists
would be easier, more robust, and make more sense to an installation admin than
forbidding them from deleting the (randomly selected, based on the pattern of
deletions) last remaining item in the list.
(Assignee)

Comment 68

15 years ago
(In reply to comment #67)
> Can't checksetup just check for table existence?

  That would be nice. :-)

  The structure of checksetup is such that the table exists before I am allowed
to populate it with data. In fact, "create the table" and "populate it with
data" happen in very different places.

  I have to populate it with the default data if "the user has never populated
it." The only way that I can guess that is if checksetup has just created an
empty table. I suppose I could add some sort of method to checksetup that would
note if it just created the table, but I think that's a complexity beyond the
scope of this patch.

  Anyhow, as justdave pointed out, the tables always need to have one value
anyhow, for enter_bug.

> Seems like fixing the bug in enter_bug that causes it to break on empty lists
> would be easier, more robust, and make more sense to an installation admin 
> than forbidding them from deleting the (randomly selected, based on the 
> pattern of deletions) last remaining item in the list.

  I'm not sure it would make sense to have an empty Version or Operating
System... although maybe I could make them NULL.

  Anyhow, if we'd like, I think we can file another bug for that later. I'm
already changing enough things that I'd like to not change enter_bug if I don't
have to. :-)
(Assignee)

Comment 69

15 years ago
Here's my work in progress. This is actually a patch against my work on bug
179451, since this work requires that work.

This is here to get comments on what I've done so far. This is a working patch,
you can see the installation currently at http://landfill.bugzilla.org/mkanat/

Basically, this patch has everything but the renaming facility and the actual
use of isactive (which are significant bits in and of themselves). Mostly, it's
just LARGE modifications to checksetup.

Feel free to give me any comments.
Attachment #163456 - Attachment is obsolete: true
>   I'm not sure it would make sense to have an empty Version or Operating
> System... although maybe I could make them NULL.

I think it probably would, actually, for some uses of Bugzilla.  Take the
mozilla.org product on b.m.o, for example.

>   Anyhow, if we'd like, I think we can file another bug for that later. I'm
> already changing enough things that I'd like to not change enter_bug if I
don't have to. :-)

Ok, sounds good.
Comment on attachment 167706 [details] [diff] [review]
v4.0 Work In Progress (Requires bug 179451) (use -p1)

>+if (LocalVarExists('severities')) {
>+    print "\nThe \@severities setting in your localconfig file ",
>+          "is no longer used.\nThis data is now stored in the ",
>+          "database, instead.\nWe recommend you remove this ",
>+          "setting from localconfig after\nchecksetup.pl is finished ",
>+          "running.\n";
>+}
>+if (LocalVarExists('priorities')) {
   etc
>+if (LocalVarExists('opsys')) {
   etc
>+if (LocalVarExists('platforms')) {
   etc

Would it make sense here to do the print statement once, inside a routine that
checks if any of those exist?  "The following settings in your localconfig file
are no longer used:" then list them or something.  Since all four are going
away at once, that's going to fill up the whole screen if it prints the same
thing 4 times.

>+my @my_severities;
>+@my_severities = @{*{$main::{'severities'}}{ARRAY}} 
>+    if defined($main::{'severities'});

Do we want to use exists instead of defined here?  It is a hash we're looking
at after all, and the reason this whole "examining the symbol table" thing was
concocted was so we could tell if an empty array existed.  Empty arrays are
considered undefined, but they exist.

>+# Set default values for what used to be the enum types.
>+# These values are no longer stored in localconfig.
>+# However, if we are upgrading from a Bugzilla without enums to a 
>+# Bugzilla with enums, we use the localconfig values one more time.

This comment is backwards.  We're upgrading from a Bugzilla with enums to one
without.

>+my @resolutions = ("","FIXED","INVALID","WONTFIX","LATER","REMIND",
>+                   "DUPLICATE","WORKSFORME","MOVED");
>+PopulateEnumTable('resolution', @resolutions);

>+my @states = ("UNCONFIRMED","NEW","ASSIGNED","REOPENED","RESOLVED",
>+              "VERIFIED","CLOSED");
>+PopulateEnumTable('bug_status', @states);

We should put these in variables up with the others just for consistency.  We
should also make the comments above it (and the other enum values, too) crystal
clear that it's defaults only, and they won't affect the database once you have
a running bugzilla, and use the UI to change them.  We just had someone on the
newsgroup in the last day or two who edited checksetup.pl trying to fix his
enums instead of editing localconfig.

We also need to be careful here...  while I'm thinking about it...  I believe
the MOVED resolution was added later, as was the UNCONFIRMED status (i.e. they
didn't always exist).  We need to test on whether we're importing and removing
the other enums from localconfig or not, and if we are moving from localconfig
to the database, and those values are not present for status and resolution, to
add them, in order to ensure smooth upgrades from early Bugzilla versions.  It
would probably be a really good idea to examine the version history of
checksetup.pl to make sure of this.
Attachment #167706 - Flags: review-
Since this is adding the schema necessary to make things retirable (but not
actually adding code to deal with it yet), making the bug for retiring things
depend on this.
Blocks: 77193
No longer blocks: 92763
(Assignee)

Comment 73

15 years ago
> We need to test on whether we're importing and removing
> the other enums from localconfig or not, and if we are moving from localconfig
> to the database, and those values are not present for status and resolution, to
> add them, in order to ensure smooth upgrades from early Bugzilla versions.

  I can just add an optional parameter to PopulateEnumTable() that says "If
true, all the values must be in the table," so it will behave a bit more like
the old CheckEnumField. But then people wouldn't be able to delete resolutions
or statuses... is that OK? Can't they not delete them now, anyway?
(Assignee)

Comment 74

15 years ago
> [snip]
> those values are not present for status and resolution, to
> add them, in order to ensure smooth upgrades from early Bugzilla versions.

  Oh wait, I just realized that the current patch actually already does this.
The user can't specify any preference for what he wants status and resolution to
be, they're just always set to that array. I believe the current checksetup
actually also works this way.
(Assignee)

Comment 75

15 years ago
Status update: I've incorporated Dave's comments, and I'm about 1/2-way through
working on the interface for updating field values. You can see my work in
progress at: http://landfill.bugzilla.org/mkanat/editvalues.cgi

If you're not a landfill admin, just email me directly with your landfill
account name and I'll grant you editcomponents on my installation.

By the way, thinking about it now, even people with editcomponents should *not*
be able to change the global field values, really... at least, this is a
security change from before, where only somebody with shell access to the
machine ("admin") could do it...
(In reply to comment #74)
>   Oh wait, I just realized that the current patch actually already does this.
> The user can't specify any preference for what he wants status and resolution
> to be, they're just always set to that array. I believe the current checksetup
> actually also works this way.

OK, you're right.  Since we know lots of people *have* customized them though,
we need to make sure it's relnoted that they need to modify that line to contain
the ones they customized before running checksetup.pl or they'll get errors in
the database.

(In reply to comment #75)
> By the way, thinking about it now, even people with editcomponents should
> *not* be able to change the global field values, really... at least, this is
> a security change from before, where only somebody with shell access to the
> machine ("admin") could do it...

But it's something that most folks had wanted to let people do, they just
couldn't.  maybe a 'bz_editfieldvalues' priv?
(Assignee)

Updated

15 years ago
Keywords: relnote
Whiteboard: [relnote comment 76]
(Assignee)

Comment 77

15 years ago
OK, I believe that this version is ready for review.

After careful consideration, I decided that this patch does NOT do certain
things. It does NOT:

+ Create retireable enumeration items
+ Fix up old chart data when you rename something
+ Create any new permissions
+ Fix enter_bug.cgi's broken browser detection

This is because Bugzilla ALREADY does NOT do these things. This patch changes
ONLY what it changes, it does not add any other new functionality to Bugzilla
(except it adds sortkeys to the enums, because that was really easy and
required).

The intention of this patch is to keep Bugzilla functionality the SAME as it is
now, basically, but get us the large architectural benefit of moving away from
enums.

A few questions:

(1) Am I doing something silly or practical with those indexes on the tables?
(2) Did I need to make that modification to the admin/table.html.tmpl template?


Note that this patch is a patch AGAINST the patch on bug 179451. That means I
used diff -Nru to get it, not "cvs diff". So use -p1 when you patch.
Attachment #167706 - Attachment is obsolete: true
Attachment #168741 - Flags: review?(bugreport)
(Assignee)

Updated

15 years ago
Alias: bz-enums
Blocks: 94534
No longer depends on: 94534
(Assignee)

Updated

15 years ago
Blocks: 248175

Comment 78

15 years ago
Comment on attachment 168741 [details] [diff] [review]
v5.0 Ready For Review (Requires bug 179451) (use -p1)

Just took a quick glance...  One thing jumped out.  Do we want to have any
default contents of the former enum fields?  I'm all for getting it out of
localconfig, but should checksetup put in something when it creates a new DB?
(Assignee)

Comment 79

15 years ago
(In reply to comment #78)
> (From update of attachment 168741 [details] [diff] [review] [edit])
> Just took a quick glance...  One thing jumped out.  Do we want to have any
> default contents of the former enum fields?  I'm all for getting it out of
> localconfig, but should checksetup put in something when it creates a new DB?

  Oh, it does put in defaults. Look around where we call PopulateEnumTable.
(Assignee)

Comment 80

15 years ago
Comment on attachment 168741 [details] [diff] [review]
v5.0 Ready For Review (Requires bug 179451) (use -p1)

I hear that Joel's pretty busy nowadays. :-) Joel, if you review this before
Dave does, and you feel confident about your review, you can clear my request
to Dave. Same to Dave, vice-versa.
Attachment #168741 - Flags: review?(justdave)
(In reply to comment #80)
> (From update of attachment 168741 [details] [diff] [review] [edit])
> I hear that Joel's pretty busy nowadays. :-) Joel, if you review this before
> Dave does, and you feel confident about your review, you can clear my request
> to Dave. Same to Dave, vice-versa.
> 

I do want to look at this. We have been very busy here at RH as well. Good news
is that we will be on a mandatory shutdown from Dec 24th to Jan 3rd so I will
try to get a look at it during that time if Joel doesn't beat me to it. Even if
so, I will still look at it since two heads are better sometimes.
(In reply to comment #81)
> 
> I do want to look at this. We have been very busy here at RH as well. Good news
> is that we will be on a mandatory shutdown from Dec 24th to Jan 3rd so I will
> try to get a look at it during that time if Joel doesn't beat me to it. Even if
> so, I will still look at it since two heads are better sometimes.

Ok, I have been working too hard. I think I just realized that you meant the
other  Dave. Oh well, time to sleep ;)
(Assignee)

Comment 83

15 years ago
Oh, I could mean you, too, dkl! :-) If you'll review it for me, I'd accept your
review. A lot of it's your code, though. :-)

I'm pretty sure that this patch is good, though, except for the few questions I
posted when I posted it.
(Assignee)

Updated

14 years ago
Blocks: 106592
(Assignee)

Comment 84

14 years ago
Comment on attachment 168741 [details] [diff] [review]
v5.0 Ready For Review (Requires bug 179451) (use -p1)

Anybody can review the patch.
Attachment #168741 - Flags: review?(justdave)
Attachment #168741 - Flags: review?(bugreport)
Attachment #168741 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #168741 - Flags: review?
(Assignee)

Comment 85

14 years ago
Posted patch v5.1 Ready for Review (obsolete) — Splinter Review
I fixed some bitrot, here.

Also, to make review easier, there's an installation with this patch installed
on landfill, where everybody has the right to modify fields [editcomponents]
(so you can see the admin templates):

http://landfill.bugzilla.org/bz17453/
Attachment #168741 - Attachment is obsolete: true
Attachment #172631 - Flags: review?
(Assignee)

Updated

14 years ago
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
(Assignee)

Updated

14 years ago
Attachment #168741 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #172631 - Flags: review?(jouni)
Attachment #172631 - Flags: review?(dkl)
Attachment #172631 - Flags: review?

Comment 86

14 years ago
Comment on attachment 172631 [details] [diff] [review]
v5.1 Ready for Review

Note: I went *very* quickly through this patch, so I may misunderstand some
points. Moreover, I did not read any comment before looking at this patch, so
again, I may say exactly the same (or even the opposite) of what has already
been said. Sorry if it is the case. ;)


>+ChangeFieldType ('bugs', 'bug_status', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'resolution', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'priority', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'bug_severity', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'rep_platform', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'op_sys', 'varchar(64) not null');

Why don't we store the ID of the different fields here instead of there name?
This way, you don't need to change values in the 'bugs' table everytime you
rename one of this field. Moreover, IIRC we generally prefer to point to field
IDs rather than field names.



>+    unless ($field) {
>+        &::ThrowUserError('fieldname_not_specified');
>+        exit;
>+    }

Is &:: really necessary here?? Moreover, ThrowUserError() already calls 'exit'
so you don't need to repeat it here.



>+unless (UserInGroup("editcomponents")) {
>+    ThrowUserError('auth_cant_edit_fieldvalues');
>+    exit;
>+}

This has been "deprecated" in favor of (see bug 265898):

UserInGroup("editcomponents")
  || ThrowUserError("auth_failure", {group  => "editcomponents",
				     action => "edit",
				     object => "field values"});



>+    # Cleanups and validity checks
>+    unless ($value) {
>+        ThrowUserError('fieldvalue_undefined');
>+        exit;
>+    }

Why not using this synthax:

ThrowUserError() unless $value;

Same remarks for the following checks. Remember that you don't need to write
'exit'.



>+        $dbh->do("UPDATE bugs
>+                  SET $field = ?,
>+                  delta_ts = delta_ts
>+                  WHERE $field = ?",
>+                 undef,
>+                 $value,
>+                 $valueold);

Bah... I really don't like the identation. Why not:

$dbh->do("UPDATE bugs SET $field = ?, delta_ts = delta_ts
	  WHERE $field = ?", undef, $value, $valueold);

Personally, I find it much easier to read. Moreover, delta_ts will *very* soon
change its type, see bug 257315. Just for your information. ;)


>+  [% ELSIF error == "auth_cant_edit_fieldvalues" %]
>+    [% title = "Access Denied" %]
>+    Sorry, you aren't a member of the 'editcomponents' group, and so
>+    you aren't allowed to add, modify or delete field values.

As said above, you should use the existing "auth_failure" error message.
(Assignee)

Comment 87

14 years ago
(In reply to comment #86)
> Why don't we store the ID of the different fields here instead of there name?

  Because that's an ENOURMOUSLY complex change. We decided to do it this way. We
had a long conversation about this, if you look back in the bug.

> >+    unless ($field) {
> >+        &::ThrowUserError('fieldname_not_specified');
> >+        exit;
> >+    }
> 
> Is &:: really necessary here?? Moreover, ThrowUserError() already calls 'exit'
> so you don't need to repeat it here.

  You're probably right about that. I copied this code straight out of
editmilestones.cgi. :-) This was my first real Bugzilla patch. :-)

> This has been "deprecated" in favor of (see bug 265898):
> 
> UserInGroup("editcomponents")
>   || ThrowUserError("auth_failure", {group  => "editcomponents",
> 				     action => "edit",
> 				     object => "field values"});

  Oh, that's much better, anyway! :-) OK, I'll use that. :-)

> Why not using this synthax:
> 
> ThrowUserError() unless $value;

  Also a much better idea. :-) Or even $value || ThrowUserError, like we do
everywhere else.

> Bah... I really don't like the identation. Why not:
> 
> $dbh->do("UPDATE bugs SET $field = ?, delta_ts = delta_ts
> 	  WHERE $field = ?", undef, $value, $valueold);

  Yeah. I also agree the indentation should be better.
(Assignee)

Comment 88

14 years ago
Comment on attachment 172631 [details] [diff] [review]
v5.1 Ready for Review

OK, I've decided to split off the admin interface into a different bug.
Attachment #172631 - Attachment is obsolete: true
Attachment #172631 - Flags: review?(jouni)
Attachment #172631 - Flags: review?(dkl)
(Assignee)

Comment 89

14 years ago
Posted patch v6.1 (Only Remove Enums) (obsolete) — Splinter Review
OK, this version only removes the enum fields. The admin interface will be
moved to a separate bug.
Attachment #174004 - Flags: review?
(Assignee)

Updated

14 years ago
Severity: enhancement → normal
Summary: Enumerators and Long-term use of Bugzilla → Enumerators in Bugzilla are not cross-DB compatible
(Assignee)

Updated

14 years ago
Blocks: 281876

Comment 90

14 years ago
Just some nits by inspection, I will test it during the weekend. Otherwise it
looks pretty good :-).

> # mkanat@kerio.com - bug 17453
> # Set default values for what used to be the enum types.
> # These values are no longer stored in localconfig.
> # However, if we are upgrading from a Bugzilla without enums to a 
> # Bugzilla with enums, we use the localconfig values one more time.

"without enums to a BZ with enums" is reversed :-).

> my $isactive = 1;
> $isactive = 0 if ($defaultinactive{$value});

can be simplyfied to "my $isactive = $defaultinactive{$value};".

> # The resolution and bug_status lists are absolute. On an upgrade from
> # a Bugzilla with enums, whatever is in the enum will be replaced with
> # this. However, after the upgrade, they can be changed at will in the DB.

Hmm, I think we still have some of them hardcoded, so you should mention that
changing some of them later can break bugzilla (what will happen if you e.g.
remove FIXED state? ;-) ).

> # 2000-02-12 Added a new state to bugs, UNCONFIRMED.  Added ability to confirm
> # a vote via bugs.  Added user bits to control which users can confirm bugs
> # by themselves, and which users can edit bugs without their names on them.
> # Added a user field which controls which groups a user can put other users
> # into.

This comment does not seem to correspond to the resolutions enum, so you
probably should not move it with it?

>     print "\nThe following settings in your localconfig file",
>           " are no longer used:\n  " . join(", ", @deprecatedvars) .
>           "\nThis data is now controlled through the Bugzilla",
>           " administrative interface.\nWe reccomend you remove",
>           " these settings from localconfig.\n"

Shouldn't you add that they should be removed after successful run of the
upgrade code? (e.g. something like "... you remove these settings once they are
imported to DB" or something in that sense). If someone gets some error later
and thinks it's because of this, they could remove it too early.

> # If the above returned a enum type, take that type and parse it into the
> # list of values.  Assumes that enums don't ever contain an apostrophe!
> # Note: This function no longer works directly on enum types but instead 
> # accesses separate tables containing enum-like values. This was done to
> # accomplish better compatibility with databases that do not support enum
> # types.

You want to rephrase the first two lines. E.g. apostrophes are fine now.
(Assignee)

Comment 91

14 years ago
Posted patch v7 (obsolete) — Splinter Review
OK, here is Version 7, based on Tomas's feedback. I also noticed that
SplitEnumTypes was *only* being used in globals.pl anyway, so I renamed it.

Eventually it will get moved out of globals.pl, but that's a part of bug
110503.
(Assignee)

Updated

14 years ago
Attachment #174004 - Attachment is obsolete: true
Attachment #174110 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #174004 - Flags: review?

Comment 92

14 years ago
Comment on attachment 174110 [details] [diff] [review]
v7

sanitycheck reports invalid values for all the fields that were formerly enums.
 It looks like you just forgot to yank the onld enum type checking code.


Aside from that, it seems to work and it looks clean.

This can be reviewed independently of the admin interface, but this must not
land until both are ready to land since it does eliminate the simple way to
update enum types without adding the new one.
Attachment #174110 - Flags: review? → review-
(Assignee)

Comment 93

14 years ago
Ahh, nice catch. :-) I fixed sanitycheck. There should be no other differences
in this patch.
Attachment #174110 - Attachment is obsolete: true
Attachment #174179 - Flags: review?(bugreport)

Comment 94

14 years ago
Comment on attachment 174179 [details] [diff] [review]
v8 (Fixed sanitycheck)

r=joel with the condition that this not land until its administration interface
does.
Attachment #174179 - Flags: review?(bugreport) → review+

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 95

14 years ago
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.351; previous revision: 1.350
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.304; previous revision: 1.303
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.82; previous revision: 1.81
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Flags: documentation?(documentation)

Updated

14 years ago
Blocks: 146104
(Assignee)

Comment 96

14 years ago
Added to the release notes in bug 286274.
Keywords: relnote
Whiteboard: [relnote comment 76]

Updated

14 years ago
Blocks: 299848
(Assignee)

Comment 97

13 years ago
*** Bug 346838 has been marked as a duplicate of this bug. ***

Comment 98

13 years ago
Documentation about legal values (editvalues.cgi) has already been updated in bug 281876.
Flags: documentation?(documentation)
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.