Bug 1186788 - disabling an account should always disable bugmail

4 files changed, 48 insertions, 16 deletions

diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index 92015f22f..0892d3293 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -732,6 +732,8 @@ sub update_table_definitions {
 
     _add_attach_size();
 
+    _fix_disable_mail();
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################
@@ -3847,6 +3849,11 @@ sub _add_attach_size {
     ");
 }
 
+sub _fix_disable_mail {
+    # you can no longer have disabled accounts with enabled mail
+    Bugzilla->dbh->do("UPDATE profiles SET disable_mail = 1 WHERE is_enabled = 0");
+}
+
 1;
 
 __END__
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index cf366d6d9..a749aaaf2 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -276,7 +276,11 @@ sub update {
 # Validators
 ################################################################################
 
-sub _check_disable_mail { return $_[1] ? 1 : 0; }
+sub _check_disable_mail {
+    return 1 unless $_[0]->is_enabled;
+    return $_[1] ? 1 : 0;
+}
+
 sub _check_disabledtext { return trim($_[1]) || ''; }
 
 # Check whether the extern_id is unique.
@@ -362,8 +366,10 @@ sub set_name {
 sub set_password { $_[0]->set('cryptpassword', $_[1]); }
 
 sub set_disabledtext {
-    $_[0]->set('disabledtext', $_[1]);
-    $_[0]->set('is_enabled', $_[1] ? 0 : 1);
+    my ($self, $text) = @_;
+    $self->set('disabledtext', $text);
+    $self->set('is_enabled', trim($text) eq '' ? 0 : 1);
+    $self->set('disable_mail', 1) if !$self->is_enabled;
 }
 
 sub set_groups {
@@ -504,8 +510,8 @@ sub email { $_[0]->login . Bugzilla->params->{'emailsuffix'}; }
 sub disabledtext { $_[0]->{'disabledtext'}; }
 sub is_enabled { $_[0]->{'is_enabled'} ? 1 : 0; }
 sub showmybugslink { $_[0]->{showmybugslink}; }
-sub email_disabled { $_[0]->{disable_mail}; }
-sub email_enabled { !($_[0]->{disable_mail}); }
+sub email_disabled { $_[0]->{disable_mail} || !$_[0]->{is_enabled}; }
+sub email_enabled { !$_[0]->email_disabled; }
 sub last_seen_date { $_[0]->{last_seen_date}; }
 sub cryptpassword {
     my $self = shift;
@@ -2973,11 +2979,12 @@ Params: login_name - B<Required> The login name for the new user.
             Even though the name says "crypt", you should just specify
             a plain-text password. If you specify '*', the user will not
             be able to log in using DB authentication.
-        disabledtext - The disable-text for the new user. If given, the user 
+        disabledtext - The disable-text for the new user. If given, the user
             will be disabled, meaning he cannot log in. Defaults to an
             empty string.
-        disable_mail - If 1, bug-related mail will not be  sent to this user; 
+        disable_mail - If 1, bug-related mail will not be  sent to this user;
             if 0, mail will be sent depending on the user's  email preferences.
+            disable_mail is always 1 for disabled users.
 
 =item C<check>
 
diff --git a/template/en/default/admin/users/edit.html.tmpl b/template/en/default/admin/users/edit.html.tmpl
index 8eced20f7..a5ab5cf03 100644
--- a/template/en/default/admin/users/edit.html.tmpl
+++ b/template/en/default/admin/users/edit.html.tmpl
@@ -26,9 +26,25 @@
 
 [% title = BLOCK %]Edit user [% otheruser.identity FILTER html %][% END %]
 
+[% js = BLOCK %]
+$(function() {
+  $('#disabledtext')
+    .keyup(function() {
+      if ($(this).val().trim() !== '') {
+        $('#disable_mail').prop('checked', true).prop('disabled', true);
+      }
+      else {
+        $('#disable_mail').prop('disabled', false);
+      }
+    })
+    .keyup();
+});
+[% END %]
+
 [% PROCESS global/header.html.tmpl
   title = title
   message = message
+  javascript = js
   style_urls = ['skins/standard/admin.css', 'skins/standard/editusers.css']
   doc_section = "useradmin.html#modifyusers"
 %]
diff --git a/template/en/default/admin/users/userdata.html.tmpl b/template/en/default/admin/users/userdata.html.tmpl
index f3f0e5aa9..16d720a4a 100644
--- a/template/en/default/admin/users/userdata.html.tmpl
+++ b/template/en/default/admin/users/userdata.html.tmpl
@@ -25,10 +25,10 @@
   <td>
     [% IF editusers %]
       <input size="64" maxlength="255" name="login" 
-             id="login" value="[% otheruser.login FILTER html %]" />
+             id="login" value="[% otheruser.login FILTER html %]">
       [% IF editform %]
         [% IF !otheruser.in_group('bz_sudo_protect') %]
-          <br />
+          <br>
           <a href="relogin.cgi?action=prepare-sudo&amp;target_login=
           [%- otheruser.login FILTER uri %]">Impersonate this user</a>
         [% END %]
@@ -57,7 +57,7 @@
     [% IF editusers %]
       <input size="64" maxlength="255" name="name"
              autocomplete="off"
-             id="name" value="[% otheruser.name FILTER html %]" />
+             id="name" value="[% otheruser.name FILTER html %]">
     [% ELSE %]
       [% otheruser.name FILTER html %]
     [% END %]
@@ -75,8 +75,8 @@
     <th><label for="password">Password:</label></th>
     <td>
       <input type="password" size="16" name="password" id="password"
-             value="" autocomplete="off" />
-      [% IF editform %]<br />
+             value="" autocomplete="off">
+      [% IF editform %]<br>
         (Enter new password to change.)
       [% END %]
     </td>
@@ -85,10 +85,12 @@
   <tr>
     <th><label for="disable_mail">[% terms.Bug %]mail Disabled:</label></th>
     <td>
-      <input type="checkbox" name="disable_mail" id="disable_mail" value="1" 
-      [% IF otheruser.email_disabled %] checked="checked" [% END %] />
-      (This affects [% terms.bug %]mail and whinemail, not password-reset or other 
-      non-[% terms.bug %]-related emails)
+      <input type="checkbox" name="disable_mail" id="disable_mail" value="1"
+      [% IF otheruser.email_disabled %] checked="checked" [% END %]>
+      <label for="disable_mail">
+        (This affects [% terms.bug %]mail and whinemail, not password-reset or other
+        non-[% terms.bug %]-related emails)
+      </label>
     </td>
   </tr>
   <tr>