Bug 347291: Make Bugzilla::User use Bugzilla::Object

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

6 files changed, 122 insertions, 111 deletions

diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index dd92cd3b4..98a8e92bf 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -168,7 +168,7 @@ sub ProcessOneBug {
     
     # Convert to names, for later display
     $values{'changer'} = $changer;
-    $values{'changername'} = Bugzilla::User->new_from_login($changer)->name;
+    $values{'changername'} = Bugzilla::User->new({name => $changer})->name;
     $values{'assigned_to'} = user_id_to_login($values{'assigned_to'});
     $values{'reporter'} = user_id_to_login($values{'reporter'});
     if ($values{'qa_contact'}) {
diff --git a/Bugzilla/DB/Pg.pm b/Bugzilla/DB/Pg.pm
index 7bf64ab9d..98bfc5903 100644
--- a/Bugzilla/DB/Pg.pm
+++ b/Bugzilla/DB/Pg.pm
@@ -224,6 +224,49 @@ sub bz_setup_database {
     # login_name, which we do with sql_istrcmp all over the place.
     $self->bz_add_index('profiles', 'profiles_login_name_lower_idx', 
         {FIELDS => ['LOWER(login_name)'], TYPE => 'UNIQUE'});
+
+    # Now that Bugzilla::Object uses sql_istrcmp, other tables
+    # also need a LOWER() index.
+    _fix_case_differences('fielddefs', 'name');
+    $self->bz_add_index('fielddefs', 'fielddefs_name_lower_idx',
+        {FIELDS => ['LOWER(name)'], TYPE => 'UNIQUE'});
+    _fix_case_differences('keyworddefs', 'name');
+    $self->bz_add_index('keyworddefs', 'keyworddefs_name_lower_idx',
+        {FIELDS => ['LOWER(name)'], TYPE => 'UNIQUE'});
+    _fix_case_differences('products', 'name');
+    $self->bz_add_index('products', 'products_name_lower_idx',
+        {FIELDS => ['LOWER(name)'], TYPE => 'UNIQUE'});
+}
+
+# Renames things that differ only in case.
+sub _fix_case_differences {
+    my ($table, $field) = @_;
+    my $dbh = Bugzilla->dbh;
+
+    my $duplicates = $dbh->selectcol_arrayref(
+          "SELECT DISTINCT LOWER($field) FROM $table 
+        GROUP BY LOWER($field) HAVING COUNT(LOWER($field)) > 1");
+
+    foreach my $name (@$duplicates) {
+        my $dups = $dbh->selectcol_arrayref(
+            "SELECT $field FROM $table WHERE LOWER($field) = ?",
+            undef, $name);
+        my $primary = shift @$dups;
+        foreach my $dup (@$dups) {
+            my $new_name = "${dup}_";
+            # Make sure the new name isn't *also* a duplicate.
+            while (1) {
+                last if (!$dbh->selectrow_array(
+                             "SELECT 1 FROM $table WHERE LOWER($field) = ?",
+                              undef, lc($new_name)));
+                $new_name .= "_";
+            }
+            print "$table '$primary' and '$dup' have names that differ",
+                  " only in case.\nRenaming '$dup' to '$new_name'...\n";
+            $dbh->do("UPDATE $table SET $field = ? WHERE $field = ?",
+                     undef, $new_name, $dup);
+        }
+    }
 }
 
 #####################################################################
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 42d747d60..04623524e 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -330,7 +330,7 @@ sub validate {
 
                 # We know the requestee exists because we ran
                 # Bugzilla::User::match_field before getting here.
-                my $requestee = Bugzilla::User->new_from_login($login);
+                my $requestee = new Bugzilla::User({ name => $login });
                 
                 # Throw an error if the user can't see the bug.
                 # Note that if permissions on this bug are changed,
@@ -593,7 +593,7 @@ sub modify {
                 create({ type      => $flag->type,
                          setter    => $setter, 
                          status    => "?",
-                         requestee => Bugzilla::User->new_from_login($login) },
+                         requestee => new Bugzilla::User({ name => $login }) },
                        $bug, $attachment, $timestamp);
             }
         }
@@ -796,7 +796,8 @@ sub FormToNewFlags {
                 push (@flags, { type      => $flag_type ,
                                 setter    => $setter , 
                                 status    => $status ,
-                                requestee => Bugzilla::User->new_from_login($login) });
+                                requestee => 
+                                    new Bugzilla::User({ name => $login }) });
                 last unless $flag_type->is_multiplicable;
             }
         }
@@ -843,7 +844,7 @@ sub notify {
     if (scalar(@bug_in_groups) || $attachment_is_private) {
         my @new_cc_list;
         foreach my $cc (split(/[, ]+/, $flag->type->cc_list)) {
-            my $ccuser = Bugzilla::User->new_from_login($cc) || next;
+            my $ccuser = new Bugzilla::User({ name => $cc }) || next;
 
             next if (scalar(@bug_in_groups) && !$ccuser->can_see_bug($bug->bug_id));
             next if $attachment_is_private
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index 9beeaf28e..47efbd68a 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -451,7 +451,7 @@ sub validate {
             foreach my $login (@requestees) {
                 # We know the requestee exists because we ran
                 # Bugzilla::User::match_field before getting here.
-                my $requestee = Bugzilla::User->new_from_login($login);
+                my $requestee = new Bugzilla::User({ name => $login });
     
                 # Throw an error if the user can't see the bug.
                 if (!$requestee->can_see_bug($bug_id)) {
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index c250f80fd..fa6c4e9cb 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -21,7 +21,9 @@ package Bugzilla::Object;
 use Bugzilla::Util;
 use Bugzilla::Error;
 
-use constant LIST_ORDER => 'name';
+use constant NAME_FIELD => 'name';
+use constant ID_FIELD   => 'id';
+use constant LIST_ORDER => NAME_FIELD;
 
 ###############################
 ####    Initialization     ####
@@ -35,12 +37,19 @@ sub new {
     return $object;
 }
 
+
+# Note: Because this uses sql_istrcmp, if you make a new object use
+# Bugzilla::Object, make sure that you modify bz_setup_database
+# in Bugzilla::DB::Pg appropriately, to add the right LOWER
+# index. You can see examples already there.
 sub _init {
     my $class = shift;
     my ($param) = @_;
     my $dbh = Bugzilla->dbh;
     my $columns = join(',', $class->DB_COLUMNS);
     my $table   = $class->DB_TABLE;
+    my $name_field = $class->NAME_FIELD;
+    my $id_field   = $class->ID_FIELD;
 
     my $id = $param unless (ref $param eq 'HASH');
     my $object;
@@ -52,12 +61,13 @@ sub _init {
 
         $object = $dbh->selectrow_hashref(qq{
             SELECT $columns FROM $table
-             WHERE id = ?}, undef, $id);
+             WHERE $id_field = ?}, undef, $id);
     } elsif (defined $param->{'name'}) {
         trick_taint($param->{'name'});
         $object = $dbh->selectrow_hashref(qq{
             SELECT $columns FROM $table
-             WHERE name = ?}, undef, $param->{'name'});
+             WHERE } . $dbh->sql_istrcmp($name_field, '?'), 
+            undef, $param->{'name'});
     } else {
         ThrowCodeError('bad_arg',
             {argument => 'param',
@@ -74,6 +84,7 @@ sub new_from_list {
     my $columns = join(',', $class->DB_COLUMNS);
     my $table   = $class->DB_TABLE;
     my $order   = $class->LIST_ORDER;
+    my $id_field = $class->ID_FIELD;
 
     my $objects;
     if (@$id_list) {
@@ -85,7 +96,7 @@ sub new_from_list {
             push(@detainted_ids, $id);
         }
         $objects = $dbh->selectall_arrayref(
-            "SELECT $columns FROM $table WHERE id IN (" 
+            "SELECT $columns FROM $table WHERE $id_field IN (" 
             . join(',', @detainted_ids) . ") ORDER BY $order", {Slice=>{}});
     } else {
         return [];
@@ -152,9 +163,10 @@ sub get_all {
     my $dbh = Bugzilla->dbh;
     my $table = $class->DB_TABLE;
     my $order = $class->LIST_ORDER;
+    my $id_field = $class->ID_FIELD;
 
     my $ids = $dbh->selectcol_arrayref(qq{
-        SELECT id FROM $table ORDER BY $order});
+        SELECT $id_field FROM $table ORDER BY $order});
 
     my $objects = $class->new_from_list($ids);
     return @$objects;
@@ -206,11 +218,24 @@ for C<Bugzilla::Keyword> this would be C<keyworddefs>.
 The names of the columns that you want to read out of the database
 and into this object. This should be an array.
 
+=item C<NAME_FIELD>
+
+The name of the column that should be considered to be the unique
+"name" of this object. The 'name' is a B<string> that uniquely identifies
+this Object in the database. Defaults to 'name'. When you specify 
+C<{name => $name}> to C<new()>, this is the column that will be 
+matched against in the DB.
+
+=item C<ID_FIELD>
+
+The name of the column that represents the unique B<integer> ID
+of this object in the database. Defaults to 'id'.
+
 =item C<LIST_ORDER>
 
 The order that C<new_from_list> and C<get_all> should return objects
 in. This should be the name of a database column. Defaults to
-C<name>.
+L</NAME_FIELD>.
 
 =item C<REQUIRED_CREATE_FIELDS>
 
@@ -240,7 +265,8 @@ They must return the validated value.
                        id of the object, from the database, that we 
                        want to read in. If you pass in a hash with 
                        C<name> key, then the value of the name key 
-                       is the name of the object from the DB.
+                       is the case-insensitive name of the object from 
+                       the DB.
 
  Returns:     A fully-initialized object.
 
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 6039e3fe6..01f2db4d9 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -48,7 +48,7 @@ use Bugzilla::Product;
 use Bugzilla::Classification;
 use Bugzilla::Field;
 
-use base qw(Exporter);
+use base qw(Bugzilla::Object Exporter);
 @Bugzilla::User::EXPORT = qw(insert_new_user is_available_username
     login_to_id user_id_to_login validate_password
     UserInGroup
@@ -66,95 +66,51 @@ use constant USER_MATCH_SUCCESS  => 1;
 
 use constant MATCH_SKIP_CONFIRM  => 1;
 
+use constant DEFAULT_USER => {
+    'id'             => 0,
+    'name'           => '',
+    'login'          => '',
+    'showmybugslink' => 0,
+    'disabledtext'   => '',
+    'disable_mail'   => 0,
+};
+
+use constant DB_TABLE => 'profiles';
+
+# XXX Note that Bugzilla::User->name does not return the same thing
+# that you passed in for "name" to new(). That's because historically
+# Bugzilla::User used "name" for the realname field. This should be
+# fixed one day.
+use constant DB_COLUMNS => (
+    'profiles.userid     AS id',
+    'profiles.login_name AS login',
+    'profiles.realname   AS name',
+    'profiles.mybugslink AS showmybugslink',
+    'profiles.disabledtext',
+    'profiles.disable_mail',
+);
+use constant NAME_FIELD => 'login_name';
+use constant ID_FIELD   => 'userid';
+
 ################################################################################
 # Functions
 ################################################################################
 
 sub new {
     my $invocant = shift;
-    my $user_id = shift;
-
-    if ($user_id) {
-        my $uid = $user_id;
-        detaint_natural($user_id)
-          || ThrowCodeError('invalid_numeric_argument',
-                            {argument => 'userID',
-                             value    => $uid,
-                             function => 'Bugzilla::User::new'});
-        return $invocant->_create("userid=?", $user_id);
-    }
-    else {
-        return $invocant->_create;
-    }
-}
-
-# This routine is sort of evil. Nothing except the login stuff should
-# be dealing with addresses as an input, and they can get the id as a
-# side effect of the other sql they have to do anyway.
-# Bugzilla::BugMail still does this, probably as a left over from the
-# pre-id days. Provide this as a helper, but don't document it, and hope
-# that it can go away.
-# The request flag stuff also does this, but it really should be passing
-# in the id it already had to validate (or the User.pm object, of course)
-sub new_from_login {
-    my $invocant = shift;
-    my $login = shift;
-
-    my $dbh = Bugzilla->dbh;
-    return $invocant->_create($dbh->sql_istrcmp('login_name', '?'), $login);
-}
-
-# Internal helper for the above |new| methods
-# $cond is a string (including a placeholder ?) for the search
-# requirement for the profiles table
-sub _create {
-    my $invocant = shift;
     my $class = ref($invocant) || $invocant;
+    my ($param) = @_;
 
-    my $cond = shift;
-    my $val = shift;
-
-    # Allow invocation with no parameters to create a blank object
-    my $self = {
-        'id'             => 0,
-        'name'           => '',
-        'login'          => '',
-        'showmybugslink' => 0,
-        'disabledtext'   => '',
-        'flags'          => {},
-        'disable_mail'   => 0,
-    };
-    bless ($self, $class);
-    return $self unless $cond && $val;
-
-    # We're checking for validity here, so any value is OK
-    trick_taint($val);
+    my $user = DEFAULT_USER;
+    bless ($user, $class);
+    return $user unless $param;
 
-    my $dbh = Bugzilla->dbh;
-
-    my ($id, $login, $name, $disabledtext, $mybugslink, $disable_mail) =
-        $dbh->selectrow_array(qq{SELECT userid, login_name, realname,
-                                        disabledtext, mybugslink, disable_mail 
-                                 FROM profiles WHERE $cond},
-                                 undef, $val);
-
-    return undef unless defined $id;
-
-    $self->{'id'}             = $id;
-    $self->{'name'}           = $name;
-    $self->{'login'}          = $login;
-    $self->{'disabledtext'}   = $disabledtext;
-    $self->{'showmybugslink'} = $mybugslink;
-    $self->{'disable_mail'}   = $disable_mail;
-
-    return $self;
+    return $class->SUPER::new(@_);
 }
 
 # Accessors for user attributes
-sub id { $_[0]->{id}; }
 sub login { $_[0]->{login}; }
 sub email { $_[0]->{login} . Bugzilla->params->{'emailsuffix'}; }
-sub name { $_[0]->{name}; }
 sub disabledtext { $_[0]->{'disabledtext'}; }
 sub is_disabled { $_[0]->disabledtext ? 1 : 0; }
 sub showmybugslink { $_[0]->{showmybugslink}; }
@@ -844,7 +800,7 @@ sub match {
         # User objects.
         my $user_logins = $dbh->selectcol_arrayref($query, undef, ($wildstr, $wildstr));
         foreach my $login_name (@$user_logins) {
-            push(@users, Bugzilla::User->new_from_login($login_name));
+            push(@users, new Bugzilla::User({ name => $login_name }));
         }
     }
     else {    # try an exact match
@@ -884,7 +840,7 @@ sub match {
 
         my $user_logins = $dbh->selectcol_arrayref($query, undef, ($str, $str));
         foreach my $login_name (@$user_logins) {
-            push(@users, Bugzilla::User->new_from_login($login_name));
+            push(@users, new Bugzilla::User({ name => $login_name }));
         }
     }
     return \@users;
@@ -1513,6 +1469,10 @@ there is currently no way to modify a user from this package.
 Note that the currently logged in user (if any) is available via
 L<Bugzilla-E<gt>user|Bugzilla/"user">.
 
+C<Bugzilla::User> is an implementation of L<Bugzilla::Object>, and thus
+provides all the methods of L<Bugzilla::Object> in addition to the
+methods listed below.
+
 =head1 CONSTANTS
 
 =over
@@ -1541,26 +1501,7 @@ confirmation screen.
 
 =head1 METHODS
 
-=over 4
-
-=item C<new($userid)>
-
-Creates a new C<Bugzilla::User> object for the given user id.  If no user
-id was given, a blank object is created with no user attributes.
-
-If an id was given but there was no matching user found, undef is returned.
-
-=begin undocumented
-
-=item C<new_from_login($login)>
-
-Creates a new C<Bugzilla::User> object given the provided login. Returns
-C<undef> if no matching user is found.
-
-This routine should not be required in general; most scripts should be using
-userids instead.
-
-=end undocumented
+=over
 
 =item C<id>