Bug 289357: Move AddFDef from checksetup into Field.pm

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=bkor, a=myk

3 files changed, 176 insertions, 136 deletions

diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index e870ec01d..87c799234 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -42,9 +42,9 @@ Bugzilla::Field - a particular piece of information about bugs
   # so both methods take the same arguments.
   print Dumper(Bugzilla::Field->match({ obsolete => 1, custom => 1 }));
 
-  # Create a custom field.
-  my $field = Bugzilla::Field::create("hilarity", "Hilarity");
-  print $field->description . " is a custom field\n";
+  # Create or update a custom field or field definition.
+  my $field = Bugzilla::Field::create_or_update(
+    {name => 'hilarity', desc => 'Hilarity', custom => 1});
 
   # Instantiate a Field object for an existing field.
   my $field = new Bugzilla::Field({name => 'qacontact_accessible'});
@@ -93,6 +93,13 @@ use constant DB_COLUMNS => (
     'enter_bug',
 );
 
+# How various field types translate into SQL data definitions.
+use constant SQL_DEFINITIONS => {
+    # Using commas because these are constants and they shouldn't
+    # be auto-quoted by the "=>" operator.
+    FIELD_TYPE_FREETEXT, { TYPE => 'varchar(255)' },
+};
+
 =pod
 
 =head2 Instance Properties
@@ -176,44 +183,61 @@ sub enter_bug { return $_[0]->{enter_bug} }
 
 =over
 
-=item C<create($name, $desc)>
+=item C<create_or_update({name => $name, desc => $desc, in_new_bugmail => 0, custom => 0})>
 
-Description: creates a new custom field.
+Description: Creates a new field, or updates one that
+             already exists with the same name.
 
-Params:      C<$name> - string - the name of the field;
-             C<$desc> - string - the field label to display in the UI.
+Params:      This function takes named parameters in a hashref: 
+             C<name> - string - The name of the field.
+             C<desc> - string - The field label to display in the UI.
+             C<in_new_bugmail> - boolean - Whether this field appears at the
+                 top of the bugmail for a newly-filed bug.
+             C<custom> - boolean - True if this is a Custom Field. The field
+                 will be added to the C<bugs> table if it does not exist.
 
-Returns:     a field object.
+Returns:     a C<Bugzilla::Field> object.
 
 =back
 
 =cut
 
-sub create {
-    my ($name, $desc, $custom) = @_;
+sub create_or_update {
+    my ($params) = @_;
     
-    # Convert the $custom argument into a DB-compatible value.
-    $custom = $custom ? 1 : 0;
-
-    my $dbh = Bugzilla->dbh;
+    my $custom         = $params->{custom} ? 1 : 0;
+    my $name           = $params->{name};
+    my $in_new_bugmail = $params->{in_new_bugmail} ? 1 : 0;
 
-    # Some day we'll allow invocants to specify the sort key.
-    my ($sortkey) =
-      $dbh->selectrow_array("SELECT MAX(sortkey) + 1 FROM fielddefs");
+    # Some day we'll allow invocants to specify the field type.
+    my $type = $custom ? FIELD_TYPE_FREETEXT : FIELD_TYPE_UNKNOWN;
 
-    # Some day we'll require invocants to specify the field type.
-    my $type = FIELD_TYPE_FREETEXT;
+    my $field = new Bugzilla::Field({name => $name});
 
-    # Create the database column that stores the data for this field.
-    $dbh->bz_add_column("bugs", $name, { TYPE => 'varchar(255)' });
+    my $dbh = Bugzilla->dbh;
+    if ($field) {
+        # Update the already-existing definition.
+        $dbh->do("UPDATE fielddefs SET description = ?, mailhead = ?
+                   WHERE id = ?",
+                 undef, $params->{desc}, $in_new_bugmail, $field->id);
+    }
+    else {
+        # Some day we'll allow invocants to specify the sort key.
+        my ($sortkey) = $dbh->selectrow_array(
+            "SELECT MAX(sortkey) + 100 FROM fielddefs") || 100;
+
+        # Add the field to the list of fields at this Bugzilla installation.
+        $dbh->do("INSERT INTO fielddefs (name, description, sortkey, type,
+                                         custom, mailhead)
+                       VALUES (?, ?, ?, ?, ?, ?)", undef,
+                 $name, $params->{desc}, $sortkey, $type, $custom, 
+                 $in_new_bugmail);
+    }
 
-    # Add the field to the list of fields at this Bugzilla installation.
-    my $sth = $dbh->prepare(
-                  "INSERT INTO fielddefs (name, description, sortkey, type,
-                                          custom, mailhead)
-                   VALUES (?, ?, ?, ?, ?, 1)"
-              );
-    $sth->execute($name, $desc, $sortkey, $type, $custom);
+    if (!$dbh->bz_column_info('bugs', $name) && $custom) {
+        # Create the database column that stores the data for this field.
+        $dbh->bz_add_column('bugs', $name, SQL_DEFINITIONS->{$type});
+    }
 
     return new Bugzilla::Field({name => $name});
 }
diff --git a/checksetup.pl b/checksetup.pl
index 9d48644ae..7fa0e65fa 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -329,6 +329,7 @@ import Bugzilla::Install::Filesystem qw(update_filesystem create_htaccess
 
 require Bugzilla::DB;
 require Bugzilla::Template;
+require Bugzilla::Field;
 require Bugzilla::Install;
 
 ###########################################################################
@@ -444,101 +445,138 @@ sub AddGroup {
 
 
 ###########################################################################
-# Populate the list of fields.
+# The list of fields.
 ###########################################################################
 
-my $headernum = 1;
+# 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_id',       desc => 'Bug #',      in_new_bugmail => 1},
+    {name => 'short_desc',   desc => 'Summary',    in_new_bugmail => 1},
+    {name => 'classification', desc => 'Classification', in_new_bugmail => 1},
+    {name => 'product',      desc => 'Product',    in_new_bugmail => 1},
+    {name => 'version',      desc => 'Version',    in_new_bugmail => 1},
+    {name => 'rep_platform', desc => 'Platform',   in_new_bugmail => 1},
+    {name => 'bug_file_loc', desc => 'URL',        in_new_bugmail => 1},
+    {name => 'op_sys',       desc => 'OS/Version', in_new_bugmail => 1},
+    {name => 'bug_status',   desc => 'Status',     in_new_bugmail => 1},
+    {name => 'status_whiteboard', desc => 'Status Whiteboard',
+     in_new_bugmail => 1},
+    {name => 'keywords',     desc => 'Keywords',   in_new_bugmail => 1},
+    {name => 'resolution',   desc => 'Resolution'},
+    {name => 'bug_severity', desc => 'Severity',   in_new_bugmail => 1},
+    {name => 'priority',     desc => 'Priority',   in_new_bugmail => 1},
+    {name => 'component',    desc => 'Component',  in_new_bugmail => 1},
+    {name => 'assigned_to',  desc => 'AssignedTo', in_new_bugmail => 1},
+    {name => 'reporter',     desc => 'ReportedBy', in_new_bugmail => 1},
+    {name => 'votes',        desc => 'Votes'},
+    {name => 'qa_contact',   desc => 'QAContact',  in_new_bugmail => 1},
+    {name => 'cc',           desc => 'CC',         in_new_bugmail => 1},
+    {name => 'dependson',    desc => 'BugsThisDependsOn', in_new_bugmail => 1},
+    {name => 'blocked',      desc => 'OtherBugsDependingOnThis',
+     in_new_bugmail => 1},
+
+    {name => 'attachments.description', desc => 'Attachment description'},
+    {name => 'attachments.filename',    desc => 'Attachment filename'},
+    {name => 'attachments.mimetype',    desc => 'Attachment mime type'},
+    {name => 'attachments.ispatch',     desc => 'Attachment is patch'},
+    {name => 'attachments.isobsolete',  desc => 'Attachment is obsolete'},
+    {name => 'attachments.isprivate',   desc => 'Attachment is private'},
+
+    {name => 'target_milestone',      desc => 'Target Milestone'},
+    {name => 'creation_ts', desc => 'Creation date', in_new_bugmail => 1},
+    {name => 'delta_ts', desc => 'Last changed date', in_new_bugmail => 1},
+    {name => 'longdesc',              desc => 'Comment'},
+    {name => 'alias',                 desc => 'Alias'},
+    {name => 'everconfirmed',         desc => 'Ever Confirmed'},
+    {name => 'reporter_accessible',   desc => 'Reporter Accessible'},
+    {name => 'cclist_accessible',     desc => 'CC Accessible'},
+    {name => 'bug_group',             desc => 'Group'},
+    {name => 'estimated_time', desc => 'Estimated Hours', in_new_bugmail => 1},
+    {name => 'remaining_time',        desc => 'Remaining Hours'},
+    {name => 'deadline',              desc => 'Deadline', in_new_bugmail => 1},
+    {name => 'commenter',             desc => 'Commenter'},
+    {name => 'flagtypes.name',        desc => 'Flag'},
+    {name => 'requestees.login_name', desc => 'Flag Requestee'},
+    {name => 'setters.login_name',    desc => 'Flag Setter'},
+    {name => 'work_time',             desc => 'Hours Worked'},
+    {name => 'percentage_complete',   desc => 'Percentage Complete'},
+    {name => 'content',               desc => 'Content'},
+    {name => 'attach_data.thedata',   desc => 'Attachment data'},
+    {name => 'attachments.isurl',     desc => 'Attachment is a URL'}
+);
+# Please see comment above before adding any new values to this constant.
 
-sub AddFDef {
-    my ($name, $description, $mailhead) = (@_);
+###########################################################################
+# Changes to the fielddefs --TABLE--
+###########################################################################
 
-    my $sth = $dbh->prepare("SELECT id FROM fielddefs " .
-                            "WHERE name = ?");
-    $sth->execute($name);
-    my ($fieldid) = ($sth->fetchrow_array());
-    if (!$fieldid) {
-        $dbh->do(q{INSERT INTO fielddefs
-                               (name, description, mailhead, sortkey)
-                   VALUES (?, ?, ?, ?)},
-                 undef, ($name, $description, $mailhead, $headernum));
-    } else {
-        $dbh->do(q{UPDATE fielddefs
-                      SET name = ?, description = ?,
-                          mailhead = ?, sortkey = ?
-                    WHERE id = ?}, undef,
-                 $name, $description, $mailhead, $headernum, $fieldid);
-    }
-    $headernum++;
+# Calling Bugzilla::Field::create_or_update depends on the
+# fielddefs table having a modern definition. So, we have to make
+# these particular schema changes before we make any other schema changes.
+
+# 2005-02-21 - LpSolit@gmail.com - Bug 279910
+# qacontact_accessible and assignee_accessible field names no longer exist
+# in the 'bugs' table. Their corresponding entries in the 'bugs_activity'
+# table should therefore be marked as obsolete, meaning that they cannot
+# be used anymore when querying the database - they are not deleted in
+# order to keep track of these fields in the activity table.
+if (!$dbh->bz_column_info('fielddefs', 'obsolete')) {
+    $dbh->bz_add_column('fielddefs', 'obsolete',
+        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
+    print "Marking qacontact_accessible and assignee_accessible as obsolete",
+          " fields...\n";
+    $dbh->do("UPDATE fielddefs SET obsolete = 1
+              WHERE name = 'qacontact_accessible'
+                 OR name = 'assignee_accessible'");
 }
 
+# 2005-08-10 Myk Melez <myk@mozilla.org> bug 287325
+# Record each field's type and whether or not it's a custom field in fielddefs.
+$dbh->bz_add_column('fielddefs', 'type',
+                    { TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0 });
+$dbh->bz_add_column('fielddefs', 'custom',
+                    { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE' });
+
+$dbh->bz_add_column('fielddefs', 'enter_bug',
+    {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
+
 # Change the name of the fieldid column to id, so that fielddefs
 # can use Bugzilla::Object easily. We have to do this up here, because
 # otherwise adding these field definitions will fail.
 $dbh->bz_rename_column('fielddefs', 'fieldid', 'id');
 
-# Note that 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.
-AddFDef("bug_id", "Bug \#", 1);
-AddFDef("short_desc", "Summary", 1);
-AddFDef("classification", "Classification", 1);
-AddFDef("product", "Product", 1);
-AddFDef("version", "Version", 1);
-AddFDef("rep_platform", "Platform", 1);
-AddFDef("bug_file_loc", "URL", 1);
-AddFDef("op_sys", "OS/Version", 1);
-AddFDef("bug_status", "Status", 1);
-AddFDef("status_whiteboard", "Status Whiteboard", 0);
-AddFDef("keywords", "Keywords", 1);
-AddFDef("resolution", "Resolution", 0);
-AddFDef("bug_severity", "Severity", 1);
-AddFDef("priority", "Priority", 1);
-AddFDef("component", "Component", 1);
-AddFDef("assigned_to", "AssignedTo", 1);
-AddFDef("reporter", "ReportedBy", 1);
-AddFDef("votes", "Votes", 0);
-AddFDef("qa_contact", "QAContact", 1);
-AddFDef("cc", "CC", 1);
-AddFDef("dependson", "BugsThisDependsOn", 1);
-AddFDef("blocked", "OtherBugsDependingOnThis", 1);
-AddFDef("attachments.description", "Attachment description", 0);
-AddFDef("attachments.filename", "Attachment filename", 0);
-AddFDef("attachments.mimetype", "Attachment mime type", 0);
-AddFDef("attachments.ispatch", "Attachment is patch", 0);
-AddFDef("attachments.isobsolete", "Attachment is obsolete", 0);
-AddFDef("attachments.isprivate", "Attachment is private", 0);
-
-AddFDef("target_milestone", "Target Milestone", 0);
-AddFDef("creation_ts", "Creation date", 0);
-AddFDef("delta_ts", "Last changed date", 0);
-AddFDef("longdesc", "Comment", 0);
-AddFDef("alias", "Alias", 0);
-AddFDef("everconfirmed", "Ever Confirmed", 0);
-AddFDef("reporter_accessible", "Reporter Accessible", 0);
-AddFDef("cclist_accessible", "CC Accessible", 0);
-AddFDef("bug_group", "Group", 0);
-AddFDef("estimated_time", "Estimated Hours", 1);
-AddFDef("remaining_time", "Remaining Hours", 0);
-AddFDef("deadline", "Deadline", 1);
-AddFDef("commenter", "Commenter", 0);
+# If the largest fielddefs sortkey is less than 100, then
+# we're using the old sorting system, and we should convert
+# it to the new one before adding any new definitions.
+if (!$dbh->selectrow_arrayref(
+        'SELECT COUNT(id) FROM fielddefs WHERE sortkey >= 100'))
+{
+    print "Updating the sortkeys for the fielddefs table...\n";
+    my $field_ids = $dbh->selectcol_arrayref(
+        'SELECT id FROM fielddefs ORDER BY sortkey');
+    my $sortkey = 100;
+    foreach my $field_id (@$field_ids) {
+        $dbh->do('UPDATE fielddefs SET sortkey = ? WHERE id = ?',
+                 undef, $sortkey, $field_id);
+        $sortkey += 100;
+    }
+}
+
+# Create field definitions
+foreach my $definition (OLD_FIELD_DEFS) {
+    Bugzilla::Field::create_or_update($definition);
+}
+
+# Delete or adjust old field definitions.
 
 # Oops. Bug 163299
 $dbh->do("DELETE FROM fielddefs WHERE name='cc_accessible'");
-
 # Oops. Bug 215319
 $dbh->do("DELETE FROM fielddefs WHERE name='requesters.login_name'");
-
-AddFDef("flagtypes.name", "Flag", 0);
-AddFDef("requestees.login_name", "Flag Requestee", 0);
-AddFDef("setters.login_name", "Flag Setter", 0);
-AddFDef("work_time", "Hours Worked", 0);
-AddFDef("percentage_complete", "Percentage Complete", 0);
-
-AddFDef("content", "Content", 0);
-
+# This field was never tracked in bugs_activity, so it's safe to delete.
 $dbh->do("DELETE FROM fielddefs WHERE name='attachments.thedata'");
-AddFDef("attach_data.thedata", "Attachment data", 0);
-AddFDef("attachments.isurl", "Attachment is a URL", 0);
 
 # 2005-11-13 LpSolit@gmail.com - Bug 302599
 # One of the field names was a fragment of SQL code, which is DB dependent.
@@ -597,7 +635,10 @@ if ($old_field_id && ($old_field_name ne $new_field_name)) {
     $dbh->do('UPDATE fielddefs SET name = ? WHERE id = ?',
               undef, ($new_field_name, $old_field_id));
 }
-AddFDef($new_field_name, $field_description, 0);
+
+
+Bugzilla::Field::create_or_update(
+    {name => $new_field_name, desc => $field_description});
 
 ###########################################################################
 # Create initial test product if there are no products present.
@@ -1778,8 +1819,6 @@ if ($dbh->bz_column_info("profiles", "groupset")) {
         }
     }
     # 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);
     $sth = $dbh->prepare("SELECT id " .
                            "FROM fielddefs " .
                           "WHERE name = " . $dbh->quote('bug_group'));
@@ -2322,7 +2361,8 @@ if (!$series_exists) {
     }
 }
 
-AddFDef("owner_idle_time", "Time Since Assignee Touched", 0);
+Bugzilla::Field::create_or_update(
+    {name => "owner_idle_time", desc => "Time Since Assignee Touched"});
 
 # 2004-04-12 - Keep regexp-based group permissions up-to-date - Bug 240325
 if ($dbh->bz_column_info("user_group_map", "isderived")) {
@@ -2413,21 +2453,6 @@ if ($dbh->bz_column_info('quips', 'userid')->{NOTNULL}) {
     $dbh->do('UPDATE quips SET userid = NULL WHERE userid = 0');
 }
 
-# 2005-02-21 - LpSolit@gmail.com - Bug 279910
-# qacontact_accessible and assignee_accessible field names no longer exist
-# in the 'bugs' table. Their corresponding entries in the 'bugs_activity'
-# table should therefore be marked as obsolete, meaning that they cannot
-# be used anymore when querying the database - they are not deleted in
-# order to keep track of these fields in the activity table.
-if (!$dbh->bz_column_info('fielddefs', 'obsolete')) {
-    $dbh->bz_add_column('fielddefs', 'obsolete', 
-        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
-    print "Marking qacontact_accessible and assignee_accessible as obsolete fields...\n";
-    $dbh->do("UPDATE fielddefs SET obsolete = 1
-              WHERE name = 'qacontact_accessible'
-                 OR name = 'assignee_accessible'");
-}
-
 # Add fulltext indexes for bug summaries and descriptions/comments.
 if (!$dbh->bz_index_info('bugs', 'bugs_short_desc_idx')) {
     print "Adding full-text index for short_desc column in bugs table...\n";
@@ -2986,13 +3011,6 @@ if (scalar(@$controlchar_bugs))
     print " done.\n" if $found;
 }
 
-# 2005-08-10 Myk Melez <myk@mozilla.org> bug 287325
-# Record each field's type and whether or not it's a custom field in fielddefs.
-$dbh->bz_add_column('fielddefs', 'type',
-                    { TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0 });
-$dbh->bz_add_column('fielddefs', 'custom',
-                    { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE' });
-
 # 2005-12-07 altlst@sonic.net -- Bug 225221
 $dbh->bz_add_column('longdescs', 'comment_id',
                     {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
@@ -3091,9 +3109,6 @@ if (!$dbh->bz_column_info('classifications', 'sortkey')) {
     }
 }
 
-$dbh->bz_add_column('fielddefs', 'enter_bug',
-    {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
-
 # 2006-07-14 karl@kornel.name - Bug 100953
 # If a nomail file exists, move its contents into the DB
 $dbh->bz_add_column('profiles', 'disable_mail',
diff --git a/customfield.pl b/customfield.pl
index 033ac517a..b5b9fd07b 100755
--- a/customfield.pl
+++ b/customfield.pl
@@ -84,5 +84,6 @@ if ( new Bugzilla::Field({name => $name}) ) {
 
 # Create the field.
 print "Creating custom field $name ...\n";
-my $field = Bugzilla::Field::create($name, $desc, 1);
+Bugzilla::Field::create_or_update(
+    {name => $name, desc => $desc, custom => 1});
 print "Custom field $name created.\n";