100953 - Move data/nomail into the DB and implement a UI to edit it

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P4)

Description

[Matthew Tuck [:CodeMachine]]

We should have a UI for data/nomail.

This would basically be adding and deleting from a list of things, therefore it
could share code with bug #95684.

Comment 1

[GavinS]

I think the summary should say 'data/nomail'

(The 'no' bit is missing at the moment)

Comment 2

[GavinS]

Attachment: Patch for gui support for data/nomail

Comment 3

[GavinS]

The patch I just attached adds a checkbox to the 2.14 editusers.cgi for totally
disabling email for this user.

If selected, then the user is added to the data/nomail file. (And removed if the
checkbox is unselected.)

This works at the moment, here, but there are still some things to consider:

1) What permissions should the nomail file be? And are these set in checksetup?

2) Is the data/nomail functionality better moved to the DB, either in the
profiles table, or a new email_prefs or user_prefs table?

3) Are there any security issues in doing this? 

4) The processmail script is also updated to use the new routines
5) There is no feedback that the mail status has changed on an update

Comment 4

[GavinS]

Comment on attachment 52706 [details] [diff] [review]
Patch for gui support for data/nomail

Minor problem with this one, so I'll attach a fixed on in a bit.

Comment 5

[GavinS]

Attachment: Fixed a small prob with previous patch

Comment 6

[Matthew Tuck [:CodeMachine]]

I can't help thinking if we do this nomail should be in the database.

Comment 7

[Gervase Markham [:gerv]]

Yeah - every time we store data outside the DB we regret it later.

Gerv

Comment 8

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 9

[GavinS]

I'd agree that this should go in the DB -- this patch is just for those
impatient enough to want it sooner :)

The question is: Is this an email pref (bug#73665), or a user pref (bug#98123)?

Comment 10

[Jouni Heikniemi]

Comment on attachment 53082 [details] [diff] [review]
Fixed a small prob with previous patch

The patch is against rather old code. It still applies, 
but only barely (stuff like "fuzz 2, offset 490 lines").

General comments: while nomail sucks big time, I think it's
quite ok to have a UI for it until it gets into the db. 
MattyT & Gerv: what do you think? Should this be 
implemented?

Well, since I did check out the patch while working on 
this kind of issue earlier, I'll give some review comments
anyway. By no means complete.

>Index: editusers.cgi
>===================================================================
>+        # Defualt to mail sending enabled

Nit: "Default" instead of "Defualt".

>+          $nomail_status = 'checked=yes';

Please quote all html attribute values (there are several other
instances of this, too.

>+sub read_nomail_file {

This is pretty complex stuff. How about having a cache hash which
would store the file's contents, and the hash were filled automatically
when needed (on the first nomail-related call per page load)? This
would also give better performance esp. in processmail, where you currently
load nomail quite a many times...

Comment 11

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 12

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 13

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 14

[timeless]

*** Bug 148069 has been marked as a duplicate of this bug. ***

Comment 15

[Kevin Benton]

I think we should do this just once.  I think it makes a lot more sense to do a little more work by updating checksetup.pl to change the profiles table so it includes a new field "allow_email bool default 1", then update the other files.  I think users should be able to turn this value on or off themselves, but more importantly, admins should be able to modify it from their browser.  By adding this to the profiles table, it removes the need for admins to have access to the file system.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
Yes, Kevin's suggestion is the right way to go. This idea was already suggested by joel in bug 123454 comment 13. Updating the bug summary to indicate that we no longer want data/nomail.

Comment 17

[Susan Cassidy]

Don't forget the users with disabledtext set, as in bug 148069.  I see that this is still ignored in 2.22.

Comment 18

[A. Karl Kornel]

Attachment: Patch v2

I've been looking for something like this, so I guess I might as well write it!

First, one big note.  It looks like nomail only affected mail generated by bug changes and by whining.  That will not change, so if a person has mail disabled then it will only mean that bug-changed mail and whining-generated mail will not be sent to that user.  That is why I called the setting "Bugmail Disabled" instead of "Email Disabled" in the editusers page, and why I've included a note to that affect next to the checkbox.  Now, on to the patch!

Checksetup is one of the first things to be affected by this patch.  The code that checks to see if the datadir needs to be created looks for the existence of the nomail file.  I changed that to look for the mail file instead.  I also added a block in checksetup to migrate data from nomail into the DB.  If nomail exists, all of the entries within are pulled out.  Users in the DB whose emails were in nomail will have their bugmail disabled, while unmatched entries are put into $datadir/nomail.bad.

Since this is a change in the profiles table, I modify Bugzilla/User.pm to fetch the setting when looking up users (and when creating blank user objects).  Accessors are also created.

global/messages.html.tmpl was also edited, changing the post-modify-user message to say if bugmail was enabled or disabled.  Of course, edituser.cgi itself was changed to recognize a new field that could be changed.  The template at admin/users/userdata.html.tmpl was also modified to show a new checkbox: "Bugmail Disabled".  The box is only shown to people with editusers access.  It's also shown when a new user is created.

Finally, I changed the code that actually checks the lists.  Whine.pl and Bugzilla/BugMail.pm both read the nomail file; those reads are now gone.  Instead, both check for $user->email_enabled or $user->email_disabled.  Whine.pl silently refuses to send mail, while Bugzilla::BugMail::ProcessOneBug puts all bugmail-disabled users in the list of users who didn't receive email when something changed.

Oh, and I also edited whineatnews so that it does not whine at assignees with untouched NEW bugs, if they have bugmail disabled.  You never know who still uses whineatnews.pl!

Comment 19

[A. Karl Kornel]

Attachment: Patch v2.01

Modification of attachment 228505 [details] [diff] [review].  A change in checksetup caused a bitrot, this patch corrects the rot.

Comment 20

[Frédéric Buclin]

Comment on attachment 229171 [details] [diff] [review]
Patch v2.01

Looks like this patch has bitrotten, at least in BugMail.pm.

Comment 21

[A. Karl Kornel]

Attachment: Patch v2.02

(grumble grumble)

Modification of attachment 229171 [details] [diff] [review] to correct additional bitrotting.

Comment 22

[Marc Schumann [:Wurblzap]]

Comment on attachment 230659 [details] [diff] [review]
Patch v2.02

Much-needed patch, cool.

Works well, and nothing much to complain code-wise either :)

>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>@@ -4274,6 +4273,52 @@
>+    while (<NOMAIL>) {
>+        $nomail{trim($_)} = 1;
>+    }
>+    close NOMAIL;
>+    unlink "$datadir/nomail";

I think you should unlink at the end of your new block so that people may stop checksetup.pl during the conversion and may let checksetup.pl finish on its next run. Unless I miss something, your database commands allow this (UPDATEs only).

>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v
>@@ -156,6 +158,8 @@
>+sub email_enabled { $_[0]->{disable_mail} ? 0 : 1; }

Nit: ! $_[0]->{disable_mail} reads easier to my old eyes.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v
>@@ -70,6 +70,26 @@
>+    <th><label for="disable_mail">Bugmail Disabled:</label></th>
>+    <td>
>+      [% IF otheruser.email_disabled %]
>+        <input type="checkbox" name="disable_mail" value="1" 

<label>s work for id only (not for name). You need to put id="disable_mail" into your <input>s.

>+      [% ELSE %]
>+        <input type="checkbox" name="disable_mail" value="1" /> (This affects 
>+        bugmail and whinemail, not password-reset or other non-bug-related 
>+        emails)

Hmm, can you reduce code duplication a little in this [% ELSE %] part here, perhaps? It seems the only things different are the "checked"-thing and 0/1 on disable_mail_old.

Comment 23

[A. Karl Kornel]

Attachment: Patch v2.1

Modification of attachment 230659 [details] [diff] [review] with respect to comment 22:

> >RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
> >@@ -4274,6 +4273,52 @@
> >+    while (<NOMAIL>) {
> >+        $nomail{trim($_)} = 1;
> >+    }
> >+    close NOMAIL;
> >+    unlink "$datadir/nomail";
> 
> I think you should unlink at the end of your new block so that people may stop
> checksetup.pl during the conversion and may let checksetup.pl finish on its
> next run. Unless I miss something, your database commands allow this (UPDATEs
> only).

Correct, it's just UPDATEs.  Moved.

> >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v
> >@@ -156,6 +158,8 @@
> >+sub email_enabled { $_[0]->{disable_mail} ? 0 : 1; }
> 
> Nit: ! $_[0]->{disable_mail} reads easier to my old eyes.

Changed (with a set of parens to be safe).

> >RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v
> >@@ -70,6 +70,26 @@
> >+    <th><label for="disable_mail">Bugmail Disabled:</label></th>
> >+    <td>
> >+      [% IF otheruser.email_disabled %]
> >+        <input type="checkbox" name="disable_mail" value="1" 
> 
> <label>s work for id only (not for name). You need to put id="disable_mail"
> into your <input>s.

OK.

> >+      [% ELSE %]
> >+        <input type="checkbox" name="disable_mail" value="1" /> (This affects 
> >+        bugmail and whinemail, not password-reset or other non-bug-related 
> >+        emails)
> 
> Hmm, can you reduce code duplication a little in this [% ELSE %] part here,
> perhaps? It seems the only things different are the "checked"-thing and 0/1 on
> disable_mail_old.

I had originally thought of doing that.  I went with my original method because it looked cleaner and less un-complicated, but OK.

Comment 24

[Marc Schumann [:Wurblzap]]

Comment on attachment 231209 [details] [diff] [review]
Patch v2.1

r=Wurblzap by interdiff and having tested patch 2.02.

>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v
>@@ -70,6 +70,24 @@
>+      [% IF otheruser.email_disabled %] checked="checked" [% END %] />

On checkin, you can change this to [% " checked=\"checked\"" IF otheruser.email_disabled %], if you want, if you feel that it's cleaner or less complicated then.

Comment 25

[A. Karl Kornel]

Attachment: Patch v2.1.1

Bug 346414 moves maintenance of the data/nomail file out of checksetup.pl and into a module.  As compromise with mkanat, his patch won't mention data/nomail, and the first checksetup.pl hunk is removed.

Comment 26

[A. Karl Kornel]

Checked in to tip:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.525; previous revision: 1.524
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.128; previous revision: 1.127
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v  <--  whine.pl
new revision: 1.30; previous revision: 1.29
done
Checking in whineatnews.pl;
/cvsroot/mozilla/webtools/bugzilla/whineatnews.pl,v  <--  whineatnews.pl
new revision: 1.27; previous revision: 1.26
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.86; previous revision: 1.85
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.122; previous revision: 1.121
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.57; previous revision: 1.56
done
Checking in template/en/default/admin/users/userdata.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v  <--  userdata.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.37; previous revision: 1.36
done

Comment 27

[Frédéric Buclin]

Comment on attachment 231274 [details] [diff] [review]
Patch v2.1.1

>Index: editusers.cgi

> } elsif ($action eq 'update') {
>     my $otherUser = check_user($otherUserID, $otherUserLogin);
>     $otherUserID = $otherUser->id;
>+    my $oldprofile = new Bugzilla::User($otherUserID);

Why creating this user object twice? $otherUser is already the user object you want. This is a useless SQL query/object creation.