Bug 352243: Make editusers.cgi use Bugzilla::User for basic user updates

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

3 files changed, 127 insertions, 91 deletions

diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 5d80a9d0f..a2ca8ff94 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -149,17 +149,28 @@ sub update {
     my $dbh      = Bugzilla->dbh;
     my $table    = $self->DB_TABLE;
     my $id_field = $self->ID_FIELD;
+
+    my $old_self = $self->new($self->id);
     
-    my $columns = join(', ', map {"$_ = ?"} $self->UPDATE_COLUMNS);
-    my @values;
+    my (@update_columns, @values, %changes);
     foreach my $column ($self->UPDATE_COLUMNS) {
-        my $value = $self->{$column};
-        trick_taint($value) if defined $value;
-        push(@values, $value);
+        if ($old_self->{$column} ne $self->{$column}) {
+            my $value = $self->{$column};
+            trick_taint($value) if defined $value;
+            push(@values, $value);
+            push(@update_columns, $column);
+            # We don't use $value because we don't want to detaint this for
+            # the caller.
+            $changes{$column} = [$old_self->{$column}, $self->{$column}];
+        }
     }
 
+    my $columns = join(', ', map {"$_ = ?"} @update_columns);
+
     $dbh->do("UPDATE $table SET $columns WHERE $id_field = ?", undef, 
-             @values, $self->id);
+             @values, $self->id) if @values;
+
+    return \%changes;
 }
 
 ###############################
@@ -452,9 +463,27 @@ data into the database. Returns a newly created object.
 
 =item C<update>
 
+=over
+
+=item B<Description>
+
 Saves the values currently in this object to the database.
 Only the fields specified in L</UPDATE_COLUMNS> will be
-updated. Returns nothing and takes no parameters.
+updated, and they will only be updated if their values have changed.
+
+=item B<Params> (none)
+
+=item B<Returns>
+
+A hashref showing what changed during the update. The keys are the column
+names from L</UPDATE_COLUMNS>. If a field was not changed, it will not be
+in the hash at all. If the field was changed, the key will point to an arrayref.
+The first item of the arrayref will be the old value, and the second item
+will be the new value.
+
+If there were no changes, we return a reference to an empty hash.
+
+=back
 
 =back
 
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 33c8535f5..ce778c728 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -67,8 +67,8 @@ use constant MATCH_SKIP_CONFIRM  => 1;
 
 use constant DEFAULT_USER => {
     'id'             => 0,
-    'name'           => '',
-    'login'          => '',
+    'realname'       => '',
+    'login_name'     => '',
     'showmybugslink' => 0,
     'disabledtext'   => '',
     'disable_mail'   => 0,
@@ -82,8 +82,8 @@ use constant DB_TABLE => 'profiles';
 # fixed one day.
 use constant DB_COLUMNS => (
     'profiles.userid     AS id',
-    'profiles.login_name AS login',
-    'profiles.realname   AS name',
+    'profiles.login_name',
+    'profiles.realname',
     'profiles.mybugslink AS showmybugslink',
     'profiles.disabledtext',
     'profiles.disable_mail',
@@ -101,6 +101,18 @@ use constant VALIDATORS => {
     realname      => \&_check_realname,
 };
 
+sub UPDATE_COLUMNS {
+    my $self = shift;
+    my @cols = qw(
+        disable_mail
+        disabledtext
+        login_name
+        realname
+    );
+    push(@cols, 'cryptpassword') if exists $self->{cryptpassword};
+    return @cols;
+};
+
 ################################################################################
 # Functions
 ################################################################################
@@ -117,6 +129,29 @@ sub new {
     return $class->SUPER::new(@_);
 }
 
+sub update {
+    my $self = shift;
+    my $changes = $self->SUPER::update(@_);
+    my $dbh = Bugzilla->dbh;
+
+    if (exists $changes->{login_name}) {
+        # If we changed the login, silently delete any tokens.
+        $dbh->do('DELETE FROM tokens WHERE userid = ?', undef, $self->id);
+        # And rederive regex groups
+        $self->derive_regexp_groups();
+    }
+
+    # Logout the user if necessary.
+    Bugzilla->logout_user($self) 
+        if (exists $changes->{login_name} || exists $changes->{disabledtext}
+            || exists $changes->{cryptpassword});
+
+    # XXX Can update profiles_activity here as soon as it understands
+    #     field names like login_name.
+
+    return $changes;
+}
+
 ################################################################################
 # Validators
 ################################################################################
@@ -127,13 +162,18 @@ sub _check_disabledtext { return trim($_[1]) || ''; }
 # This is public since createaccount.cgi needs to use it before issuing
 # a token for account creation.
 sub check_login_name_for_creation {
-    my ($self, $name) = @_;
+    my ($invocant, $name) = @_;
     $name = trim($name);
     $name || ThrowUserError('user_login_required');
     validate_email_syntax($name)
         || ThrowUserError('illegal_email_address', { addr => $name });
-    is_available_username($name) 
-        || ThrowUserError('account_exists', { email => $name });
+
+    # Check the name if it's a new user, or if we're changing the name.
+    if (!ref($invocant) || $invocant->login ne $name) {
+        is_available_username($name) 
+            || ThrowUserError('account_exists', { email => $name });
+    }
+
     return $name;
 }
 
@@ -153,12 +193,36 @@ sub _check_password {
 sub _check_realname { return trim($_[1]) || ''; }
 
 ################################################################################
+# Mutators
+################################################################################
+
+sub set_disabledtext { $_[0]->set('disabledtext', $_[1]); }
+sub set_disable_mail { $_[0]->set('disable_mail', $_[1]); }
+
+sub set_login {
+    my ($self, $login) = @_;
+    $self->set('login_name', $login);
+    delete $self->{identity};
+    delete $self->{nick};
+}
+
+sub set_name {
+    my ($self, $name) = @_;
+    $self->set('realname', $name);
+    delete $self->{identity};
+}
+
+sub set_password { $_[0]->set('cryptpassword', $_[1]); }
+
+
+################################################################################
 # Methods
 ################################################################################
 
 # Accessors for user attributes
-sub login { $_[0]->{login}; }
-sub email { $_[0]->{login} . Bugzilla->params->{'emailsuffix'}; }
+sub name  { $_[0]->{realname};   }
+sub login { $_[0]->{login_name}; }
+sub email { $_[0]->login . Bugzilla->params->{'emailsuffix'}; }
 sub disabledtext { $_[0]->{'disabledtext'}; }
 sub is_disabled { $_[0]->disabledtext ? 1 : 0; }
 sub showmybugslink { $_[0]->{showmybugslink}; }
@@ -187,7 +251,7 @@ sub identity {
 
     if (!defined $self->{identity}) {
         $self->{identity} = 
-          $self->{name} ? "$self->{name} <$self->{login}>" : $self->{login};
+          $self->name ? $self->name . " <" . $self->login. ">" : $self->login;
     }
 
     return $self->{identity};
@@ -199,7 +263,7 @@ sub nick {
     return "" unless $self->id;
 
     if (!defined $self->{nick}) {
-        $self->{nick} = (split(/@/, $self->{login}, 2))[0];
+        $self->{nick} = (split(/@/, $self->login, 2))[0];
     }
 
     return $self->{nick};
@@ -767,7 +831,7 @@ sub derive_regexp_groups {
                                          AND isbless = 0
                                          AND grant_type = ?});
     while (my ($group, $regexp, $present) = $sth->fetchrow_array()) {
-        if (($regexp ne '') && ($self->{login} =~ m/$regexp/i)) {
+        if (($regexp ne '') && ($self->login =~ m/$regexp/i)) {
             $group_insert->execute($id, $group, GRANT_REGEXP) unless $present;
         } else {
             $group_delete->execute($id, $group, GRANT_REGEXP) if $present;
@@ -1101,10 +1165,11 @@ sub match_field {
 
             # skip confirmation for exact matches
             if ((scalar(@{$users}) == 1)
-                && (lc(@{$users}[0]->{'login'}) eq lc($query)))
+                && (lc(@{$users}[0]->login) eq lc($query)))
+
             {
                 $cgi->append(-name=>$field,
-                             -values=>[@{$users}[0]->{'login'}]);
+                             -values=>[@{$users}[0]->login]);
 
                 next;
             }
@@ -1117,7 +1182,7 @@ sub match_field {
             if (scalar(@{$users}) == 1) { # exactly one match
 
                 $cgi->append(-name=>$field,
-                             -values=>[@{$users}[0]->{'login'}]);
+                             -values=>[@{$users}[0]->login]);
 
                 $need_confirm = 1 if $params->{'confirmuniqueusermatch'};
 
@@ -1282,7 +1347,7 @@ sub wants_bug_mail {
     # 
     # We do them separately because if _any_ of them are set, we don't want
     # the mail.
-    if ($wants_mail && $changer && ($self->{'login'} eq $changer)) {
+    if ($wants_mail && $changer && ($self->login eq $changer)) {
         $wants_mail &= $self->wants_mail([EVT_CHANGED_BY_ME], $relationship);
     }    
     
diff --git a/editusers.cgi b/editusers.cgi
index 5f356fb40..a1c82db9f 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -226,9 +226,6 @@ if ($action eq 'search') {
     my $otherUser = check_user($otherUserID, $otherUserLogin);
     $otherUserID = $otherUser->id;
 
-    my $logoutNeeded = 0;
-    my @changedFields;
-
     # Lock tables during the check+update session.
     $dbh->bz_lock_tables('profiles WRITE',
                          'profiles_activity WRITE',
@@ -245,73 +242,19 @@ if ($action eq 'search') {
                                            action => "modify",
                                            object => "user"});
 
-    my $login        = $cgi->param('login');
-    my $password     = $cgi->param('password');
-    my $realname     = trim($cgi->param('name')         || '');
-    my $disabledtext = trim($cgi->param('disabledtext') || '');
-    my $disable_mail = $cgi->param('disable_mail') ? 1 : 0;
+    $vars->{'loginold'} = $otherUser->login;
 
     # Update profiles table entry; silently skip doing this if the user
     # is not authorized.
+    my %changes;
     if ($editusers) {
-        my @values;
-
-        if ($login ne $otherUser->login) {
-            # Validating untaints for us.
-            $login || ThrowUserError('user_login_required');
-            validate_email_syntax($login)
-              || ThrowUserError('illegal_email_address', {addr => $login});
-            is_available_username($login)
-              || ThrowUserError('account_exists', {email => $login});
-
-            push(@changedFields, 'login_name');
-            push(@values, $login);
-            $logoutNeeded = 1;
-
-            # Since we change the login, silently delete any tokens.
-            $dbh->do('DELETE FROM tokens WHERE userid = ?', {}, $otherUserID);
-        }
-        if ($realname ne $otherUser->name) {
-            # The real name may be anything; we use a placeholder for our
-            # INSERT, and we rely on displaying code to FILTER html.
-            trick_taint($realname);
-            push(@changedFields, 'realname');
-            push(@values, $realname);
-        }
-        if ($password) {
-            # Validating untaints for us.
-            validate_password($password) if $password;
-            push(@changedFields, 'cryptpassword');
-            push(@values, bz_crypt($password));
-            $logoutNeeded = 1;
-        }
-        if ($disabledtext ne $otherUser->disabledtext) {
-            # The disable text may be anything; we use a placeholder for our
-            # INSERT, and we rely on displaying code to FILTER html.
-            trick_taint($disabledtext);
-            push(@changedFields, 'disabledtext');
-            push(@values, $disabledtext);
-            $logoutNeeded = 1;
-        }
-        if ($disable_mail != $otherUser->email_disabled) {
-            push(@changedFields, 'disable_mail');
-            push(@values, $disable_mail);
-        }
-        if (@changedFields) {
-            push (@values, $otherUserID);
-            $logoutNeeded && Bugzilla->logout_user($otherUser);
-            $dbh->do('UPDATE profiles SET ' .
-                     join(' = ?,', @changedFields).' = ? ' .
-                     'WHERE userid = ?',
-                     undef, @values);
-            # XXX: should create profiles_activity entries.
-            #
-            # We create a new user object here because it needs to
-            # read information that may have changed since this
-            # script started.
-            my $newprofile = new Bugzilla::User($otherUserID);
-            $newprofile->derive_regexp_groups();
-        }
+        $otherUser->set_login($cgi->param('login'));
+        $otherUser->set_name($cgi->param('name'));
+        $otherUser->set_password($cgi->param('password'))
+            if $cgi->param('password');
+        $otherUser->set_disabledtext($cgi->param('disabledtext'));
+        $otherUser->set_disable_mail($cgi->param('disable_mail'));
+        %changes = %{$otherUser->update()};
     }
 
     # Update group settings.
@@ -399,8 +342,7 @@ if ($action eq 'search') {
     delete_token($token);
 
     $vars->{'message'} = 'account_updated';
-    $vars->{'loginold'} = $otherUser->login;
-    $vars->{'changed_fields'} = \@changedFields;
+    $vars->{'changed_fields'} = [keys %changes];
     $vars->{'groups_added_to'} = \@groupsAddedTo;
     $vars->{'groups_removed_from'} = \@groupsRemovedFrom;
     $vars->{'groups_granted_rights_to_bless'} = \@groupsGrantedRightsToBless;