Closed Bug 289357 Opened 19 years ago Closed 18 years ago

Move AddFDef from checksetup into Field.pm

Categories

(Bugzilla :: Installation & Upgrading, enhancement)

2.19.2
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 3 obsolete files)

AddFDef would be useful for custom fields as well as being useful for
checksetup, and so should be moved into the upcoming "Field.pm" module.

We should also rename the function, probably something like add_fielddef or
add_field_def.
Target Milestone: --- → Bugzilla 2.22
Target Milestone: Bugzilla 2.22 → ---
Assignee: installation → mkanat
Target Milestone: --- → Bugzilla 2.24
Attached patch v1 (obsolete) — Splinter Review
Okay, here we go. Note that this changes how Bugzilla generates the "sortkey" field for fielddefs.

Before, it used to generate it by the order of the AddFDef statements, and regenerate it if you added a new AddFDef statement. Now, it generates it by adding 100 to the highest sortkey currently in existence, and never regenerates it.

I don't think this matters much, and it's much simpler. I don't think it matters because fielddefs.sortkey is not used anywhere extremely important, that I know of.
Attachment #229641 - Flags: review?(colin.ogilvie)
Attachment #229641 - Flags: review?(bugzilla-mozilla)
Status: NEW → ASSIGNED
Blocks: 345107
No longer blocks: 287324
Comment on attachment 229641 [details] [diff] [review]
v1

bitrotten. Apologies for waiting so long.
Attachment #229641 - Flags: review?(colin.ogilvie)
Attachment #229641 - Flags: review?(bugzilla-mozilla)
Attachment #229641 - Flags: review-
Attached patch Broken patch (obsolete) — Splinter Review
Okay, I fixed the bitrot, but this patch is broken.

The problem is that create_or_update won't work before the database is upgraded, because it depends on fielddefs having its modern structure. But ideally I'd like to be able to call create_or_update throughout checksetup, before the table is ugpraded fully. So I have to figure that out, somehow.
Attachment #229641 - Attachment is obsolete: true
Attached patch v2 (obsolete) — Splinter Review
Okay, this version works! I just moved all of the fielddefs schema changes to the top of checksetup.

This is fine, I tested it and it works now.

I'll say as module owner that the checksetup changes are okay, and you don't have to review those if you don't want to. The only thing I'd like review on is any of the Bugzilla::Field changes that I made, and anything outside of checksetup that I touched.
Attachment #231263 - Attachment is obsolete: true
Attachment #231560 - Flags: review?(bugzilla-mozilla)
Blocks: 346815
Comment on attachment 231560 [details] [diff] [review]
v2

>Index: checksetup.pl

Assumed this was ok, only glanced over it.

>@@ -329,6 +329,8 @@
> 
> require Bugzilla::DB;
> require Bugzilla::Template;
>+# For create_or_update
>+require Bugzilla::Field;

Nit: bitrot.

>@@ -443,101 +445,138 @@
>+# NOTE: All of these entries are unconditional, from when get_field_id
>+# used to create an entry if it wasn't found. New fielddef columns should
>+# be created with their associated schema change.
>+use constant OLD_FIELD_DEFS => (

>+    {name => 'bug_group',             desc => 'Group'},

>@@ -1778,7 +1820,7 @@
>     }
>     # Replace old activity log groupset records with lists of names of groups.
>     # Start by defining the bug_group field and getting its id.
>-    AddFDef("bug_group", "Group", 0);
>+    Bugzilla::Field::create_or_update({name => "bug_group", desc => "Group"});

Isn't this already done?

>Index: customfield.pl

In another bug: Might be nice to allow someone to update the description now it is possible.

>Index: Bugzilla/Field.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v
>retrieving revision 1.14
>diff -u -r1.14 Field.pm
>--- Bugzilla/Field.pm	25 Jul 2006 23:20:22 -0000	1.14
>+++ Bugzilla/Field.pm	1 Aug 2006 08:34:44 -0000
>@@ -176,44 +183,61 @@
> 
> =over
> 
>-=item C<create($name, $desc)>
>+=item C<create_or_update({name => $name, desc => $desc, in_new_bugmail => 1, custom => 0})>

If this is meant to show the defaults, then in_new_bugmail should be 0.

+             C<in_new_bugmail> - boolean - Whether this field appears at the
+                 top of the bugmail for a newly-filed bug.

That currently only works for custom fields.
Attachment #231560 - Flags: review?(bugzilla-mozilla) → review+
Flags: approval?
Flags: approval? → approval+
Okay, I fixed most of your comments on checkin. I'll attach a new patch showing what was checked in.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.532; previous revision: 1.531
done
Checking in customfield.pl;
/cvsroot/mozilla/webtools/bugzilla/customfield.pl,v  <--  customfield.pl
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.15; previous revision: 1.14
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Okay, here's what I checked in.
Attachment #231560 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: