Bug 320197: Watcher editing in userprefs should use a list box instead of text entry

Patch by Colin Ogilvie <colin.ogilvie@gmail.com>; r=wicked; a=myk

2 files changed, 43 insertions, 19 deletions

diff --git a/template/en/default/account/prefs/email.html.tmpl b/template/en/default/account/prefs/email.html.tmpl
index cfb34a893..1cfc4f18f 100644
--- a/template/en/default/account/prefs/email.html.tmpl
+++ b/template/en/default/account/prefs/email.html.tmpl
@@ -261,12 +261,25 @@ document.write('<input type="button" value="Disable All Mail" onclick="SetCheckb
 If you watch a user, it is as if you are standing in their shoes for the 
 purposes of getting email. Email is sent or not according to <u>your</u>
 preferences for <u>their</u> relationship to the [% terms.bug %] 
-(e.g. Assignee). You are watching anyone on the following comma-separated list:
+(e.g. Assignee).
+[% IF watchedusers.size %]You are watching anyone on the following list:
+  </p>
+
+  <p>
+    <select id="watched_by_you" name="watched_by_you" multiple="multiple" size="5">
+        [% FOREACH w = watchedusers %]
+           <option value="[% w FILTER html %]">[% w FILTER html %]</option>
+        [% END %]
+    </select> <br />
+    <input type="checkbox" id="remove_watched_users" name="remove_watched_users">
+    <label for="remove_watched_users">Remove selected users from my watch list</label>
+[% ELSE %]
+You are currently not watching any users.
+[% END %]
 </p>
 
-<p><a name="watched_by_you" id="watched_by_you">Users to watch</a>:
-  <input size="60" name="watchedusers" 
-         value="[% watchedusers FILTER html %]">
+<p><a name="new_watched_by_you" id="new_watched_by_you">Add users to my watch list (comma separated list)</a>:
+  <input size="60" name="new_watchedusers" value="">
 </p>            
 
 <p><a name="watching_you" id="watching_you">Users watching you</a>:<br>
diff --git a/userprefs.cgi b/userprefs.cgi
index 688a437fd..2a890f19d 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -205,7 +205,7 @@ sub DoEmail {
             " ON watch.watched = profiles.userid" .
             " WHERE watcher = ?",
             undef, $user->id);
-        $vars->{'watchedusers'} = join(',', @$watched_ref);
+        $vars->{'watchedusers'} = $watched_ref;
 
         my $watcher_ids = $dbh->selectcol_arrayref(
             "SELECT watcher FROM watch WHERE watched = ?",
@@ -298,7 +298,8 @@ sub SaveEmail {
     # User watching
     ###########################################################################
     if (Bugzilla->params->{"supportwatchers"} 
-        && defined $cgi->param('watchedusers')) 
+        && (defined $cgi->param('new_watchedusers')
+            || defined $cgi->param('remove_watched_users'))) 
     {
         # Just in case.  Note that this much locking is actually overkill:
         # we don't really care if anyone reads the watch table.  So 
@@ -306,34 +307,44 @@ sub SaveEmail {
         # using user-defined locks rather than table locking.
         $dbh->bz_lock_tables('watch WRITE', 'profiles READ');
 
-        # what the db looks like now
+        # Use this to protect error messages on duplicate submissions
         my $old_watch_ids =
             $dbh->selectcol_arrayref("SELECT watched FROM watch"
                                    . " WHERE watcher = ?", undef, $user->id);
- 
-       # The new information given to us by the user.
-        my @new_watch_names = split(/[,\s]+/, $cgi->param('watchedusers'));
+
+        # The new information given to us by the user.
+        my @new_watch_names = split(/[,\s]+/, $cgi->param('new_watchedusers'));
         my %new_watch_ids;
+
         foreach my $username (@new_watch_names) {
             my $watched_userid = login_to_id(trim($username), THROW_ERROR);
             $new_watch_ids{$watched_userid} = 1;
         }
-        my ($removed, $added) = diff_arrays($old_watch_ids, [keys %new_watch_ids]);
-
-        # Remove people who were removed.
-        my $delete_sth = $dbh->prepare('DELETE FROM watch WHERE watched = ?'
-                                     . ' AND watcher = ?');
-        foreach my $remove_me (@$removed) {
-            $delete_sth->execute($remove_me, $user->id);
-        }
 
         # Add people who were added.
         my $insert_sth = $dbh->prepare('INSERT INTO watch (watched, watcher)'
                                      . ' VALUES (?, ?)');
-        foreach my $add_me (@$added) {
+        foreach my $add_me (keys(%new_watch_ids)) {
+            next if grep($_ == $add_me, @$old_watch_ids);
             $insert_sth->execute($add_me, $user->id);
         }
 
+        if (defined $cgi->param('remove_watched_users')) {
+            my @removed = $cgi->param('watched_by_you');
+            # Remove people who were removed.
+            my $delete_sth = $dbh->prepare('DELETE FROM watch WHERE watched = ?'
+                                         . ' AND watcher = ?');
+            
+            my %remove_watch_ids;
+            foreach my $username (@removed) {
+                my $watched_userid = login_to_id(trim($username), THROW_ERROR);
+                $remove_watch_ids{$watched_userid} = 1;
+            }
+            foreach my $remove_me (keys(%remove_watch_ids)) {
+                $delete_sth->execute($remove_me, $user->id);
+            }
+        }
+
         $dbh->bz_unlock_tables();
     }
 }