Bug 73665 (emailprefs)

Create an "emailprefs" table

RESOLVED FIXED in Bugzilla 2.20

Status

()

enhancement
P1
normal
RESOLVED FIXED
19 years ago
5 years ago

People

(Reporter: jacob, Assigned: gerv)

Tracking

(Blocks 2 bugs)

2.11
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +
blocking2.20 +
blocking2.18 -

Details

Attachments

(1 attachment, 24 obsolete attachments)

86.40 KB, patch
jacob
: review+
Details | Diff | Splinter Review
Reporter

Description

19 years ago
Currently, the e-mail filtering prefs are all stored in a field in the profiles
table called "emailflags" and seperated by tildes (~).  As discussed in IRC,
this should really be stored in a table of it own.  

My basic thinking is that this should gave two columns, event and value (generic
names, nothing in stone).  Event would be what we currently have
w/emailOwnerRemoveme and value would be "1" for filtered.  Notice that having
the value 1 makes it filtered (nomail).  If there is now emailOwnerRemoveme for
a particular user, it should default to sending mail (the checkbox is checked in
editprefs.cgi).

While this logic may seem backwards, it allows the amount of data stored to be
much smaller (an entry in the table is only created if a user doesn't want mail,
if the later decide they do want mail on that type of change, simply remove the
row from the table).
Reporter

Comment 1

19 years ago
This can't be done until at the very least "processmail regenerate" is removed
from ./checksetup.pl
Depends on: 71552
Reporter

Updated

19 years ago
Blocks: 71808
Reporter

Updated

19 years ago
Blocks: 74996
No longer depends on: 71552
Blocks: 53044
No longer blocks: 71808
See bug #14137 ... it would require a specific sort of functionality from an
email prefs table.
Blocks: 14137
Reporter

Updated

18 years ago
Blocks: 83245
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Reporter

Comment 3

18 years ago
-> New Bugzilla Product
Assignee: tara → justdave
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.11 → 2.11
Reporter

Comment 4

18 years ago
What I'm think of for this is a schema along the lines of:

userid - integer - The user's ID that's doing the watching :)
watchtype - string - one of several hard-coded strings for type
 - watchtype would be things like 'user', 'product', 'component', 'direct'
   - direct would be our current categories (reporter, cc, voter, assignee, qa)
watchdata - integer - a number that maps to it's data using watchtype
 - eq (watchtype => 'user', watchdata => 1) would mean to watch user 1*
Attachments
Comments
Status
(the above 3 are just samples of current filtering categories).

A schema such as this should allow each watch to have it's own filtering prefs.
 eg, I could watch the Bugzilla product and choose to get everything
Webtools/LXR and only get mail when somebody fixes a bug.  This would, of
course, also require some UI changes as the number of relationships could
quickly make the page quite long :)

* This means that we'd have to fix 43600 and impliment some way to map reporter,
cc, etc. to numeric values.  For that latter, I think simply hardcoding those
values would be better than a table in the db (they are hard-coded
relationships, after all).
Depends on: 43600
Reporter

Comment 5

18 years ago
BTW, this could also be expanded to watch certain keywords, attachmentdefs, etc.
(in fact, keywords would be easier than products/comp as they already have an
integer primary key, IIRC).
Reporter

Updated

18 years ago
Blocks: 69253
Reporter

Updated

18 years ago
No longer blocks: 53044
Reporter

Updated

18 years ago
Blocks: 34787
Reporter

Updated

18 years ago
Blocks: 74258
Reporter

Updated

18 years ago
Blocks: 103845
What's the status on this?  I'm adding an extra SQL query to mailprefs in bug
34488...
Reporter

Comment 7

18 years ago
I'm working on bug 43600 ATM... it should be ready for review shortly and once
it's in I'll start working on this one.
This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.
This obviously isn't going to make it....   -> 2.18
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Reporter

Comment 10

18 years ago
I started working on this one in the airport/on the plane during my recent trip
to Texas.  I have the conversion in checksetup.pl done to go from the
'emailflags' field to the 'emailprefs' table.  It does not change the 'watches'
table, but that is what bug 74996 is for :)

I'll try to get the checksetup.pl portion up soon for comments.
Assignee: justdave → jake
Reporter

Updated

18 years ago
No longer blocks: 103845
Status: NEW → ASSIGNED
Reporter

Comment 11

18 years ago
Still needs everything else, but I'd like to gather any comments.  As you can
see, this patch also adds two extra filters that are requested in bug 125538
(this seemed like the easiest time to do it as we can apply whatever their
current pref is regarding RESOLVED or VERIFIED to each of the new prefs).
Reporter

Updated

18 years ago
Blocks: 125538
Reporter

Comment 12

18 years ago
Posted patch Some more work for comment (obsolete) — Splinter Review
Even though I never heard any comments on the checksetup.pl portion, I went
ahead and did some more.  This makes some changes inside userprefs.cgi as well
as the email templates found inside default/prefs/.  I haven't written the code
to save the new prefs (and, in fact, crippled the code to change the old prefs
:) nor have I done anything inside processmail yet.  I also haven't done the
code in checksetup.pl to remove the old prefs, so that you can compare the
pre-conversion and post-conversion prefs very easily on the
userprefs.cgi?tab=email&email_tab=email-relationship (hmmm... a lot of
redundancy there, I think I'll change that a little, later) page.

I'm looking for comments on just about everything (page layout, code,
implimentation, the meaning of life).
Attachment #70332 - Attachment is obsolete: true
Comment on attachment 72656 [details] [diff] [review]
Some more work for comment

needs-work - this is incomplete, and I doubt it still applies.
Attachment #72656 - Flags: review-
Reporter

Updated

17 years ago
Alias: emailprefs
Reporter

Comment 14

17 years ago
Nothing new in this patch, it's still far from done... but I'm catching it up
to 4 months of CVS changes and posting it for comment.	Now that this is done,
I can start in with making it actually do something other than convert and look
pretty.
Reporter

Updated

17 years ago
Attachment #72656 - Attachment is obsolete: true
Reporter

Updated

17 years ago
Reporter

Comment 15

17 years ago
Posted patch patch v0.9 (obsolete) — Splinter Review
This is really close to complete... bad things that I know about in this patch:

* I still have to global arrays declared in globals.pl and used elsewhere
* There's user viewable data in the aforementioned arrays
* checksetup.pl doesn't yet delete the old prefs field in the profiles table

Everything else is fair game :)

Also, if anybody has a suggestion as to how I should fix the first two issues,
I'd be willing to listen... if not, I'm sure I'll come up w/something, but I
haven't followed recent discussion on what's up w/global variables and where
exactly all the template stuff lands for some obvious reasons (and some not so
obvious :)
Attachment #96958 - Attachment is obsolete: true
Comment on attachment 102641 [details] [diff] [review]
patch v0.9


>+            if ($found_off) {

What is this doing?

>+                # Assemble an array of what values we need to set.
>+                my @pref_names;
>+                my @pref_values;
>+                foreach my $key(keys %new_prefs) {
>+                    push (@pref_names, $key);
>+                    push (@pref_values, $new_prefs{$key});
>+                }
>+                # watch_type for hard coded values is "0"

I don't know what a 'hard coded value' is, but you should put the 0 in there
anyway, even if it is implicit, mainly for dicumentation purposes.

>+                $dbh->do("INSERT INTO emailprefs (userid, watch_id, " .
>+                         join(", ", @pref_names) . ") VALUES ($userid, " .
>+                         $relationships{$relationship} . ", " .
>+                         join(", ", @pref_values). ")\n");

>Index: globals.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
>retrieving revision 1.211
>diff -u -r1.211 globals.pl
>--- globals.pl	10 Oct 2002 19:02:22 -0000	1.211
>+++ globals.pl	12 Oct 2002 00:25:13 -0000
>@@ -98,6 +98,58 @@
> $::unconfirmedstate = "UNCONFIRMED";
> $::dbwritesallowed = 1;
> 
>+# @::relationships and @::email_prefs are related to the e-mail filtering
>+# code, they really should be put into a module of some-sort, but for this
>+# stage in development, it's really not important.  It should be fairly easy
>+# to fix before the "final" patch is submitted (once I know for sure how
>+# we're dividing up globals.pl and CGI.pl).
>+@::email_relationships = ( { name        => 'reporter',
>+                             value       => 0,
>+                             description => 'Reporter' },

What you should do with these, is in Bugzilla::Constants (new file), do:

use constant RELATIONSHIP_REPORTER => 0;

or some better name

>+@::email_reasons = ( { name        => 'add_remove',
>+                       description => 'I\'m added to or removed from this capacity' },

You should only use the 'name' internally, and have the name->description
mapping in a template, which can then be used from the userprefs/mail
templates. You may need to (finally!) templatise the processmail stuff for this
to work

>+        elsif ( $fieldName eq 'Status' && $new eq 'CLOSED' ) {
>+            push (@flags, 'closed');
>+            $status = 1;
>+        }
>+        elsif ( $fieldName eq 'Status' ) {
>+            push (@flags, 'status');
>+            $status = 1;
>+        }
>+        elsif ( $fieldName eq 'Severity' || $fieldName eq 'Target Milestone'
>+                || $fieldName eq 'Priority') {
>+            push (@flags, 'severity');

why does TM get counted as severity?

>+        # Get this user's prefs out of the emailprefs table and put
>+        # them in the %pref hash

Actually, you can do better than that, now - you know what role you're looking
for, so just select WHERE type IN (whatever teh valid changes are) LIMIT 1.

That may need to wait for teh rewrite, though.

>+        if (lc($user) eq $nametoexclude) {
>+            SendSQL("SELECT excludeself FROM profiles WHERE userid = $userid");
>+            # If this user doesn't has checked the "ExcludeSelf" box, then
>+            # FetchOneColumn() will return "0" after this query.

Tht doesn't really need commenting

> ###############################################################################
> # Each panel has two functions - panel Foo has a DoFoo, to get the data 
> # necessary for displaying the panel, and a SaveFoo, to save the panel's 
>@@ -149,6 +134,28 @@
> 
> 
> sub DoEmail {
>+    my $current_email_tab = $::FORM{'email_tab'} || 'global';
>+
>+    my @email_tabs = ( { name        => 'global',
>+                         description => 'Global Options',
>+                         saveable    => 1 },
>+                       { name        => 'relationship',
>+                         description => 'Direct Relationships',
>+                         saveable    => 1 },
>+                     );

again, move this to the template, and similarly elsewhere.


>--- template/en/default/account/prefs/email.html.tmpl	28 Sep 2002 18:42:38 -0000	1.7
>+++ template/en/default/account/prefs/email.html.tmpl	12 Oct 2002 00:25:19 -0000
>@@ -1,4 +1,4 @@
>-<!-- 1.0@bugzilla.org -->
>+[%# -*- mode: html -*- %]   <!-- 2.0@bugzilla.org -->

I don;t know what the final outcome was on bumping the template version

>+<input type="hidden" name="email_tab" value="[% email.current_tab.name %]">
>+ 
>+[% INCLUDE "account/prefs/email-${email.current_tab.name}.html.tmpl"
>+   IF email.current_tab.name.defined %] 

That will break on perl 5.005_03, but we're dropping that next week, so don't
worry about it

> 
>-  [% FOREACH reason = [ 
>-      { name = 'Removeme',    
>-        description = 'I\'m added to or removed from this capacity' },

As I mentioned above, to share this among templates, just put it into a
different one, and include it here
Attachment #102641 - Flags: review-

Updated

17 years ago
Severity: normal → enhancement
Summary: Need an "emailprefs" table → Create an "emailprefs" table
Reporter

Comment 17

17 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
>>+	       if ($found_off) {
>
>What is this doing?

This new table is set up to only store when prefs are turned off, therefore if
we didn't find that this user turned anything off, then we don't need to store
anything about them here.

>>+		   # watch_type for hard coded values is "0"
>
>I don't know what a 'hard coded value' is, but you should put the 0 in there
>anyway, even if it is implicit, mainly for dicumentation purposes.

Done and clarified

>>+@::email_relationships = ( { name	    => 'reporter',
>>+				value	    => 0,
>>+				description => 'Reporter' },
>
>What you should do with these, is in Bugzilla::Constants (new file), do:
>
>use constant RELATIONSHIP_REPORTER => 0;

Put in Bugzilla::Constants, but using the names I'd been using.  Would this be
better served as Bugzilla::Constants::Mail ? Currently I only put the "use"
line for this into the two files I actually use it in.

>You should only use the 'name' internally, and have the name->description
>mapping in a template, which can then be used from the userprefs/mail
>templates. You may need to (finally!) templatise the processmail stuff for
this
>to work

Put it into the appropriate templates, but there wasn't any cause to rewrite
processmail to do it (one thing at a time, right :)

>why does TM get counted as severity?

Good question.	In the current code, it's being counted as Status.  I wanted to
create a seperate pref for "Other status changes", but didn't want to have to
also create one for tm, priority, and severity.  Couldn't really come up w/a
better name than 'severity'.

>>+	   # Get this user's prefs out of the emailprefs table and put
>>+	   # them in the %pref hash
>
>Actually, you can do better than that, now - you know what role you're looking

>for, so just select WHERE type IN (whatever teh valid changes are) LIMIT 1.
>
>That may need to wait for teh rewrite, though.

That probably would be better, but it's probably also easier to wait for the
rewrite.

>>+	       # If this user doesn't has checked the "ExcludeSelf" box, then
>>+	       # FetchOneColumn() will return "0" after this query.
>
>Tht doesn't really need commenting

OK

>again, move this to the template, and similarly elsewhere.

Done.

>I don;t know what the final outcome was on bumping the template version

I'm not sure, either... but I've gotta believe that ripping out 90% of the file
qualifies.

>>+[% INCLUDE "account/prefs/email-${email.current_tab.name}.html.tmpl"
>>+   IF email.current_tab.name.defined %] 
>
>That will break on perl 5.005_03, but we're dropping that next week, so don't
>worry about it

I assume it's the ${some.name} thing that's broke?  If so, ditching 5.005_03 is
probably a good thing, 'cause I used that construct a few more times :)

>As I mentioned above, to share this among templates, just put it into a
>different one, and include it here

For now, at least, it's just setting in email-relationships.html.tmpl being
that that's currently the only place it's used.

OK, I think that this patch is ready to roll.  I addressed (in some form or
another) all review comments and pinned up the last couple things I said made
v0.9 not ready.
Attachment #102641 - Attachment is obsolete: true

Comment 18

17 years ago
The conventions for Bugzilla::Constants should be consistent with bug 147275.

Why not create a table like....
userid mediumint,
change_type tinyint,
relation_to_bug tinyint

So that you don't have to invent a new column for every new type of change?

This would also save you having to harvest all the columns into a hash.



Reporter

Comment 19

17 years ago
So that's where Bugzilla::Constants came from... unfortunately, the way my code
works I can't use a true constant because I need my arrays... unless I do a lot
of restructuring.  So is there a suggestestion as to a better place to put them?

As for a different table format, I suppose I could make all the "reason" columns
into one generic column and do another "constant" for what value in that column
corresponds to what type of change.  I just used the structure I outlined in
comment #4. Please note that even with the current table structure I don't
/have/ to store the hash, it just required the least amount of processmail
restructuring.

One advantage I can think of to doing it the way patch v1 has it would be that
in the event a "reason" were added that required some setup/initialization code
(such as spliting a current reason) then it would be easy in checksetup.pl to
check if that field exists and if it doesn't add it and do the setup routine.
Why is can't you |use constant| an array?

Although I do thing a name->value hash would be better, for the relationships,
mind you.
Reporter

Comment 21

17 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Renamed my file for the relationships to Mail.pm to avoid conflicts with what's
in Constants.pm.  Also, changed to using a simple hash for the relationships
and fixed a bug where Param('useqacontact') wasn't being respected for the
userprefs page.
Attachment #103476 - Attachment is obsolete: true
Reporter

Updated

17 years ago
Attachment #106460 - Flags: review?
Reporter

Updated

17 years ago
Blocks: 108870

Comment 22

17 years ago
Comment on attachment 106460 [details] [diff] [review]
Patch v2

This patch is missing its checksetup code. (makes it very hard to review)

You really can't find a way to get your constants to be constants??
Attachment #106460 - Flags: review? → review-
Looks like this patch will have to be massaged a bit to account for the changes
in bug 124174 anyway, but I'd recommend putting everything you currently have in
Bugzilla::Mail into Bugzilla::BugMail since that all has to do with bugmail.

(In case you're wondering, these were split out because Bugzilla::Mail wasn't
specific enough; after 124174 and bug 84876 go in, we'll have a
Bugzilla::BugMail module and a Bugzilla::ProcessMail module, where BugMail is
the old processmail and B::ProcessMail is the new SMTP handling stuff.)

Comment 24

16 years ago
Sorry for the spam, but does anyone actually work on this? This seems like
trivial but extremely useful feature to me. It would reduce my amount of daily
mail tremendously. I'm sure that most people subscribed to bugs are not
interested in dupes.
reassigning to default owner/QA... Jake's Army Reserve unit has been deployed.
Assignee: jake → justdave
Status: ASSIGNED → NEW
I'm currently working on this, and a lot of related bugs.

Gerv
Assignee: justdave → gerv
Posted patch Patch v.1 (obsolete) — Splinter Review
Myk asked me to split up the patch in bug 76794. This is the first aprt, being
the profiles_nomail table migration, which fits best under this bug.

Gerv
Attachment #106460 - Attachment is obsolete: true
Comment on attachment 139553 [details] [diff] [review]
Patch v.1

Myk - this is the email prefs table patch, ready for review.

Gerv
Attachment #139553 - Flags: review?(myk)
I just glanced at the patch, but I noticed that the table you're creating kind
of... shoehorns preferences to mean a certain thing.

It would be nice if the "email prefs" table were more generic than just trying
to intelligently (as opposed to the way it's done now) enumerate the types of
preferences we expose now (i.e. event, relationship, etc.)
> shoehorns preferences to mean a certain thing.

I'm not quite sure what you mean by that. The table is unashamedly for email
preferences; it's not designed or supposed to deal with any other sort.

The patch is designed so it's quite possible to add new preferences, or split
existing ones in two. Was that your question?

Gerv
What I mean is that your patch builds a table based upon the *types* of email
preferences we have now, calls the columns certain things, and makes the types
general very inflexible.

When I had originally looked at this bug, I had envisioned making the
preferences table more generic (yes, to hold just email preferences), but to
hold all types of preferences related to email... not just "event" and
"relationship" preferences (although, with the original schema I had designed
months ago, you could easily fit those preferences into it.)

Even though you didn't ask me for a review, I just wanted to make it known that
I think something more flexible that will serve us better is appropriate.

If you want examples, I'll have to go back and read your original proposal
again... there were a couple when I originally scanned it that popped into my head.
> If you want examples, I'll have to go back and read your original proposal
> again... there were a couple when I originally scanned it that popped into my 
> head.

That might help; I'm afraid I still don't quite follow.

Gerv
Ok, I think I've figured out how to test this thing.  I'm going to take a copy
of the b.m.o database, roll back lastdiffed on all bugs to "yesterday", and run
Dave's sendunsentbugmail.pl script, capturing its output.  Then I'm going to
apply the patch and do it again.  If the code works right, the output should be
the same.
(In reply to comment #31)
> When I had originally looked at this bug, I had envisioned making the
> preferences table more generic (yes, to hold just email preferences), but to
> hold all types of preferences related to email... not just "event" and
> "relationship" preferences (although, with the original schema I had designed
> months ago, you could easily fit those preferences into it.)

For the record, I wouldn't be too upset if this table did only hold event and
relationship preferences.  Most other types of preferences you might have for
email would fit in well with the generic user preferences table that's being
dealt with on some other bug.
(In reply to comment #33)
> Ok, I think I've figured out how to test this thing.  I'm going to take a copy
> of the b.m.o database, roll back lastdiffed on all bugs to "yesterday", and run
> Dave's sendunsentbugmail.pl script, capturing its output.  Then I'm going to
> apply the patch and do it again.  If the code works right, the output should be
> the same.

Unless the existing code is broken and this stuff does it right...  If any bugs
have different sent vs excluded totals we could audit those bugs and the
involved users' preferences to see.

The data it's using to generate those counts does include the email addresses. 
You might want to have the script output the included and excluded addresses,
too, just in case, to help assist in any such auditing (so we know which user
changed between the two if something's different)
Oh, and if you plan to use "diff" on the output to tell if they match or not,
you'll probably want to remove the "time taken" line, since that depends on
external forces and will likely be different between the two.

Also, you're crippling the actual sending of mail on that instance somehow,
right? ;)
Note: there are complex occasions (perhaps when someone has two or more roles on
the same bug) where this code may do something different. (The idea is that the
new behaviour is just as "right" as the old, and easier to explain.) But that
sounds like a good plan.

Hmm. It seems like the patch has rotted. I'll try and update.

Gerv
Posted patch Patch v.2 (obsolete) — Splinter Review
Unbitrotted version - hopefully still working ;-)

Gerv
Attachment #139553 - Attachment is obsolete: true
Posted patch Patch v.3 (obsolete) — Splinter Review
Fixes issues found by Myk in testing.

Gerv
Attachment #142421 - Attachment is obsolete: true
Attachment #143234 - Flags: review?(myk)
Flags: blocking2.18?

Comment 40

16 years ago
Comment on attachment 143234 [details] [diff] [review]
Patch v.3

A few comments from a first looks....

1) This changes behavior so disabled accounts never get mail.  I believe this
is the right thing to do, but it needs to be publicized.

2) If sites start to add their own events or relationships, is there some
guidance we can give to make future merges smooth?

Comment 41

16 years ago
Comment on attachment 143234 [details] [diff] [review]
Patch v.3

Bitrotted - wont apply to tip
Attachment #143234 - Flags: review-
Posted patch Patch v.4 (obsolete) — Splinter Review
This patch applies cleanly to the tip. It addresses some issues Myk raised in a
private email. 

Joel: yes, we should document that.

I hadn't thought about sites adding their own events and relationships. They
can't really add more relationships - there aren't any. As for events, I'm not
sure we'd want to support that - after all, if we do, and people succumb to the
temptation whenever anyone complains about bugmail, you'd end up with 50
events, which would be a usability nightmare... 

Gerv
Attachment #143234 - Attachment is obsolete: true
Attachment #144450 - Flags: review?(myk)

Comment 43

16 years ago
Comment on attachment 144450 [details] [diff] [review]
Patch v.4

bitrotted
Attachment #144450 - Attachment is obsolete: true

Comment 44

16 years ago
Posted patch patch v.5 (obsolete) — Splinter Review
Same as before minus some trivial bitrot, the wording in one message had been
changed...

Comment 45

16 years ago
OK, I've found a very serious problem in this patch.  After applying it Bugzilla
stops sending out email.  I doublechecked by installing a CVS version,
configuring it in the same way I did the patched version and it does send out email.

I welcome any hints and will test any further iterations of this patch that are
thrown at me.
It stops sending out mail because my temporary fix to redirect all mail to
"/tmp/mail.txt" is part of it ;-) I didn't want to spam everyone who'd signed up
for a Landfill account, so I changed mail to redirect to a file. See the end of
BugMail.pm.

Perhaps that part shouldn't be in the patch, but it's a bit tricky to extract,
and it's probably what you want for testing anyway.

Of course, bug 84876 would allow me to turn mail off in the params mumble mumble...

Gerv
And yes, I probably should have mentioned this when I posted the patch... Sorry
about that.

Gerv

Comment 48

16 years ago
OK, thanks for the hint, works like a charm now.  BTW, the "watch components"
feature you're coming up with in bug 76794 also works fine for me now (it had
the same hack).  Unfortunately the patch there has rotted quite a bit and does
not apply to CVS anymore.  It does apply to 2.17.7, though.

Nice work, thanks :-)

Updated

16 years ago
Attachment #144504 - Flags: review?

Updated

16 years ago
Blocks: 123454

Comment 49

16 years ago
Comment on attachment 144504 [details] [diff] [review]
patch v.5


A few questions before I do a final r=...
> 
> # 2000-12-18.  Added an 'emailflags' field for storing preferences about
> # when email gets sent on a per-user basis.
>-if (!GetFieldDef('profiles', 'emailflags')) {
>+if (!GetFieldDef('profiles', 'emailflags') && 
>+    !GetFieldDef('profiles_nomail', 'userid')) {
>     AddField('profiles', 'emailflags', 'mediumtext');
> }
>-

Do you really have to add this field just to remove it a bit farther down?
Why not just check below for both?

>+      <input type="checkbox" name="email-100-101" id="email-100-101" 
>+        value="1"
>+        [% " checked" IF NOT nomail.100.101 %]>
>+      <label for="email-100-101">Email me when someone sets a flag I asked for</label>

There must be a better way to show this than using hard-coded constants in the
template.

Also, I would like to have someone test this out reasonably thoroughly.

There is some very minor bitrot in userprefs.cgi also
>Also, I would like to have someone test this out reasonably thoroughly.

Indeed, my tests on an earlier version of the patch show a 4.5x slowdown.  Gerv
says the latest patch has cut it to 2x, but that's still a significant loss
given the problems we've been having lately with email performance.
> Do you really have to add this field just to remove it a bit farther down?

I believe it's much easier, conceptually and in implementation, if the database
is in a known state when approaching some migration code. So the field is added
in all cases, and then removed - but we have to make sure it's not re-added
after the migration, hence the change you reference.

> There must be a better way to show this than using hard-coded constants in the
> template.

There may well be. :-) I haven't yet found the magic incantations for using
constants in templates; please enlighten me, anyone who knows.

Is anyone able to help me by profiling this? DProf crashes on me :-( bbaetz?

Gerv
Stick them into Template.pm, in VARIABLES. If they're compile-time constants,
put them into CONSTANTS. See Template::Namespace::Constants

Comment 53

16 years ago
On the profiling question, I was wondering how much of the time is spent in
retrieving the emaiprefs from the DB during processmail versus the rest of the
processing.  Since the prefs express a desire not to get mail, it might shed
light on the question if you were to simply bypass the code that fetches the
prefs from the DB and see if that is the big time sink.
Attachment #139553 - Flags: review?(myk)
Attachment #143234 - Flags: review?(myk)
Attachment #144450 - Flags: review?(myk)
Sadly, we're just not there yet on this, if we want 2.18 to happen any time soon.

Gerv
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20

Updated

15 years ago
Attachment #144504 - Flags: review?
Flags: blocking2.18? → blocking2.18-
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Gerv, you mentioned about 2 weeks ago over on bug 108870 that you had an updated
patch for this ready to go.  Can we see it yet?  ;)  I'll take this for 2.20 if
it makes it in before we branch (hint: that's in about a week most likely)
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Sorry for the delay. I've taken half a day off work today to catch up on stuff.
I'll upload it tonight, for definite.

Gerv
Posted patch Patch v.6 (obsolete) — Splinter Review
Here it is at long last. It's very nearly ready - there are a few outstanding
questions, as outlined below.

I've tested it by rolling back the lastchangeddate on my test databases (one
before and one after):
  update bugs set lastdiffed = '2002-01-01 00:00:00';
  update bugs set delta_ts = '2004-11-19 00:00:00';
and using a hacked version of sendunsentbugmail.pl to send the mail. I then
diffed the two outputs to see what had changed, and fixed bugs accordingly. The
hacked sendunsentbugmail.pl isn't in this patch - I'll track it down and attach
it later or check it in separately.

I've made two "compatibility" fixes to the code to make it work initially as
much like the old code as possible:

1) The code still sends mail to disabled accounts
2) The code still only sends mail based on the preferences of the "catch-all"
state ("Any field not mentioned above changes") when _only_ "catch-all" fields
have changed, rather than if any "catch-all" fields have changed (the correct
behaviour).

However, there are still currently some edge cases where it's different, when a
person currently has two roles on the same bug. That leads to Q1:

Q1) Should the code prioritise one role over another, or should it send you
mail
when _any_ of the roles you have dictates that you should get mail? The code
currently does the former, with the order being Assignee, QA, Reporter,
CC, Voter. 

Q1a) If we allow multiple roles, how does watching work? Inherits all roles?

Note also I had to modify the fix to bug 267494 - it now just hides the columns
instead of removing them. If you remove them, because of the way it works,
Bugzilla will forget all your email preferences (resetting them to "no mail"!)
for e.g. QA if QA is turned off, you edit your prefs, and QA gets turned back
on again.

I still haven't figured out how to use constants in templates, despite bbaetz's
hints.

Oh, and having made a performance fix or two, the new code is approximately 10%
faster than the old rather than 2x slower :-) (Creating Bug objects is _slow_ -
we need to do something about that by making it more lazy.)

Gerv
Attachment #144504 - Attachment is obsolete: true
(I'm trying to make release notes for future versions less of a nightmare. This
will need a release note other than what will be in the bug summary, so I'm
adding the keyword and that little thing on the status whiteboard.)
Keywords: relnote
Whiteboard: [relnote: comment 40]
This is marked blocking2.18- and at the same time blocking a blocking2.18+ bug
(bug 108870) -- Dave, please re-decide blocking2.18.
Flags: blocking2.18- → blocking2.18?
This is too invasive to land on the branch.  Breaking the dependency on bug
108870, although this would make it easier, it's not actually necessary.  That
can be fixed without this.
No longer blocks: 108870
Flags: blocking2.18? → blocking2.18-
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Dave: did you mean to change the target on this from 2.20 to 2.22? I thought the
point was that it was too late to make the 2.18 train, but we wanted it in 2.20
(i.e. the current trunk)?

I'm hoping this will get reviewed and applied soon, as I want to work on some of
the patches which build on it over Christmas... (You may remember I wrote the
first version of this patch _last_ Christmas ;-)

Gerv
(In reply to comment #62)
> Dave: did you mean to change the target on this from 2.20 to 2.22? I thought
> the point was that it was too late to make the 2.18 train, but we wanted it
> in 2.20 (i.e. the current trunk)?

It's probably too late for 2.20 now, too (we're just about to branch).  However,
if you do make it in before we branch, I'll probably reconsider.

> I'm hoping this will get reviewed and applied soon

Perhaps it would help if you request review? :)
Comment on attachment 168445 [details] [diff] [review]
Patch v.6

>- open(SENDMAIL, "|/usr/lib/sendmail $sendmailparam -t -i") ||
>+ open(SENDMAIL, ">>/tmp/after.mail.txt") ||

oops.
Comment on attachment 168445 [details] [diff] [review]
Patch v.6

>+        # Important note: we store the instances where a user does _not_
>+        # want mail
>+        foreach my $relationship (keys %relationships) {
>+            foreach my $event (keys %events) {
>+                my $key = "email$relationship$event";
>+                if (exists($emailflags{$key}) && $emailflags{$key} eq '') {
>+                    $dbh->do("INSERT into profiles_nomail VALUES (" . 
>+                             $userid . ", " .
>+                             $relationships{$relationship}. ", " .
>+                             $events{$event} . ")");

  Wouldn't it be better to use placeholders and prepare() here? I don't see any
point where these are re-quoted before they're put back into the DB, otherwise.
It's possible that we don't ever need to quote our built-in values, but maybe
somebody locally did something crazy to emailflags. And it's pretty easy to use
prepare.

  *nit* Also, for those of us who don't know the schema by heart, it might be
nice to put the names of the fields being inserted into in the INSERT
statement.

>+        # Note that the value of "excludeself" is assumed to be off if the
>+        # preference does not exist in the user's list, unlike other 
>+        # preferences whose value is assumed to be on if they do not exist.

  *nit* I take it that you mean "in the old system, this is true"? I'm pretty
sure that's what you mean, but for folks who aren't totally familiar with the
emailflags system, it might be good to clarify it.


> sub ProcessOneBug($) {
>-    my ($id) = (@_);
>+    my ($id, $forced) = (@_);

  Probably should also change the prototype, if you're adding a new argument?



>-}
>+    my $userwatchers = 
>+        $dbh->selectall_arrayref("SELECT watcher, watched FROM watch 
>+                                  WHERE watched IN ($involved)
>+                                  AND watcher NOT IN ($involved)");

  Shouldn't this (and other watchers things) depend on
Param("supportwatchers")?

>@@ -868,16 +608,16 @@ sub NewProcessOnePerson ($$$$$$$$$$$$$) 
>     if (Param("sendmailnow")) {
>        $sendmailparam = "";
>     }
> 
>     if ($enableSendMail == 1) {
>-        open(SENDMAIL, "|/usr/lib/sendmail $sendmailparam -t -i") ||
>+        open(SENDMAIL, ">>/tmp/after.mail.txt") ||
>           die "Can't open sendmail";
>     
>         print SENDMAIL trim($msg) . "\n";
>         close SENDMAIL;
>     }
>-    push(@sentlist, $person);
>+

  Is the @sentlist push only removed for the purposes of testing the patch, or
was that actually done for some reason?

>+    my $count = 
>+        $dbh->selectrow_array("SELECT COUNT(event) 
>+                              FROM profiles_nomail
>+                              WHERE userid = $self->{'id'}
>+                              AND relationship = $relationship 
>+                              AND event IN (" . join(",", @$events) . ")");
>+    
>+    # If the returned count is the same as the length of the input list, the
>+    # user has turned mail off for all the given events. Otherwise, they want
>+    # this mail.
>+    my $wants_mail = ($count < scalar(@$events));

  I just wanted to say that that's really clever. :-)


>+[% events = [
>+    { index = 1,
>+      description = "I'm added to or removed from this capacity" },
>+    { index = 6,
>+      description = "The $terms.bug is resolved or reopened" },
>+    { index = 5,
> [snip]

  Forgive me if I'm missing something that somebody said about this earier, but
shouldn't these be using the constants, somehow, instead of manually using the
magic numbers?


  Overall, I have to say that I really like this patch. It's much cleaner and
more elegant than the way we do things now.
byron: indeed; I keep doing that with patches. Apologies. Hopefully another
patch will soon make the way of doing mail a param, so we don't have this
problem any more.

I'll upload a new version with Max's comments addressed ASAP.

Gerv
Posted patch Patch v.7 (obsolete) — Splinter Review
This patch addresses all Max's comments except the following. It also includes
my changes to sendunsentbugmail.pl which allow it to report on what it's doing,
using the --report flag.

Note that we also need to answer Q1 in comment #58 as soon as possible. There
is at least one more necessary change which depends on the answer.

> Is the @sentlist push only removed for the purposes of testing the patch, or
> was that actually done for some reason?

The creation of the sent list is now done one level up, in ProcessOneBug().

> I just wanted to say that that's really clever. :-)

If only it were my idea :-) I believe it was Joel's.

> shouldn't these be using the constants, somehow, instead of manually using
the
> magic numbers?

I'm still waiting for someone to tell me how to do it. But it could be fixed in

a follow-up patch.

Gerv
Attachment #168445 - Attachment is obsolete: true
Attachment #169445 - Flags: review?(mkanat)
(In reply to comment #67)
> Note that we also need to answer Q1 in comment #58 as soon as possible. There
> is at least one more necessary change which depends on the answer.

  I'm not totally sure what the difference is between prioritizing a role and
not. As a user, I would *expect* Bugzilla to send me mail when I'm in *any* of
the roles, as I could remove myself from the role or change my preference otherwise.

  The change would require a relnote.

> I'm still waiting for someone to tell me how to do it. But it could
> be fixed in a follow-up patch.

  If I had to take a guess... can you pass either a reference to the module, or
an instance of the module itself into the template? Bradley mentioned something
about this, though, which seems better:

http://search.cpan.org/~abw/Template-Toolkit-2.14/lib/Template/Namespace/Constants.pm
(In reply to comment #68)
> I'm not totally sure what the difference is between prioritizing a role and
> not. 

Role prioritisation means that the mail code thinks that you have exactly one
role in relationship to a particular bug - the one with the highest priority.

> As a user, I would *expect* Bugzilla to send me mail when I'm in *any* of
> the roles, as I could remove myself from the role or change my preference
otherwise.

Right - so the opposite of what it does now, then :-)

I'll see if I can change the patch.

Gerv
Status: NEW → ASSIGNED
Posted patch Patch v.8 (obsolete) — Splinter Review
OK, I think this is the real deal. 

I've changed the behaviour so that it emails you if _any_ of the roles you are
in dictate that you should get mail, and it also now correctly extracts
ex-office-holders from the diffs and mails them too if it needs to. So that is,
as far as I am aware, complete - apart from the follow-up task of tidying up
constant use in templates.

Let's get this on the 2.20 train :-)

Gerv
Attachment #169445 - Attachment is obsolete: true
Attachment #169863 - Flags: review?(mkanat)
Attachment #169445 - Flags: review?(mkanat)

Comment 71

15 years ago
You mean 2.22 train (2.22 could be released at the end of March 2005 if all goes
well).

2.20 is feature-frozen since 15th of September, 2004 (that's more than 3 months
ago).

Comment 72

15 years ago
Ok, I read comment #56 now, but I still think 2.20 is pretty optimistic :-) Good
luck with that, anyway. :)
Comment on attachment 169863 [details] [diff] [review]
Patch v.8

  OK, now I'll do a more in-depth review. I actually understand how the
emailflags system works, now.

checksetup.pl:
>+        foreach my $relationship (keys %relationships) {
>+            foreach my $event (keys %events) {
>+                my $key = "email$relationship$event";
>+                if (exists($emailflags{$key}) && $emailflags{$key} eq '') {

  That should be neq 'on' -- That's the way the current code actually works,
anything that's not 'on' is considered to be off.

>+        # Note that in the old system, the value of "excludeself" is assumed to
>+        # be off if the preference does not exist in the user's list, unlike 
>+        # other preferences whose value is assumed to be on if they do not 
>+        # exist.

  Yay, this comment is way better. :-)

>+        #
>+        # This preference has changed from global to per-relationship.

  I know PHBs, they'll say, "Why do I have to do so much clicking to change
that preference, now?" :-)

  But that's mostly just me talking to myself, there. This can stay, and is
probably cooler than how it used to work. :-)

>+        # Request preferences
>+        my %requestprefs = ("FlagRequestee" => EVT_FLAG_REQUESTED,
>+                            "FlagRequester" => EVT_REQUESTED_FLAG);

  This doesn't need to be defined down here, it can be defined up with the
events and relationships.

>+        foreach my $key (keys %requestprefs) {
>+            if (exists($emailflags{$key}) && $emailflags{$key} eq '') {

  Also should be neq 'on'.

>+        # We do not need to set the defaults for the new events, because in
>+        # absence of an entry in the table, mail will be sent.

  That does change the defaults that we wanted (or had?), which was that some
CC events shouldn't cause people to be sent mail.


userprefs.cgi:
> # Use global template variables.
>-use vars qw($template $vars $userid);
>+use vars qw($cgi $template $vars $userid);
> 
> # The default email flags leave all email on.
> my $defaultflagstring = "ExcludeSelf~on~";
> 
> my @roles = ("Owner", "Reporter", "QAcontact", "CClist", "Voter");

  Hey, shouldn't @roles and @relationships now be inside of
Bugzilla::Constants... or actually just deleted?

>@@ -147,101 +148,86 @@ sub SaveAccount {
>             " WHERE userid = $userid");
> }
> 
> 
> sub DoEmail {
>+    my $dbh = Bugzilla->dbh;
>+    
>+    ###########################################################################

  *nit* I think these should be a little shorter (70 chars?) to account for
being able to quote them like I'm doing here. :-)

>+sub SaveEmail {
>+    my $dbh = Bugzilla->dbh;
>+    my $cgi = Bugzilla->cgi;

  Won't $cgi here hide the one from "use vars($cgi...)" that you added above,
and cause a warning?

>+    for (my $i = 0; $i < REL_COUNT; $i++) {
>+        # Positive events are those for which a ticked box means "send me mail."
>+        for (my $j = POS_EVT_START; $j < POS_EVT_END; $j++) {

  *nit* It seems to me like it might be easier to have the Relationships and
Events in some kind of arrays, maybe, and we could just do a foreach here and
in other places that we need to do something like this? It makes me
uncomfortable to have to maintain POS_EVT_START and POS_EVT_END -- it's one of
those things that developers have to magically know to do.

>+    # Global positive events (not ticked => entry in nomail database)
>+    foreach my $j (EVT_FLAG_REQUESTED, EVT_REQUESTED_FLAG) {

  These should probably also be in an array, or some such thing.

  And actually, this points out an architectural problem -- we can't
programmatically know that certain EVT flags are "only global."


BugMail.pm:
>+use constant REL_COUNT              => 5;

  This could use a comment -- it's not immediately clear to me what REL_COUNT
is.

  Also, as I said above, I think that the need for all of these variables
should go away and BugMail.pm should export a few arrays or hashes that just
have these values in them, in addition to the values themselves.

>+# There are two sorts of event - positive and negative. Positive events are
>+# those for which the user says "I want mail if this happens." Negative events
>+# are those for which the user says "I don't want mail if this happens."
>+use constant POS_EVT_START      => 0;
>+use constant EVT_OTHER              => 0;
>+use constant EVT_ADDED_REMOVED      => 1;
>+use constant EVT_COMMENT            => 2;
>+use constant EVT_ATTACHMENT         => 3;
>+use constant EVT_ATTACHMENT_DATA    => 4;
>+use constant EVT_PROJ_MANAGEMENT    => 5;
>+use constant EVT_OPENED_CLOSED      => 6;
>+use constant EVT_KEYWORD            => 7;
>+use constant EVT_CC                 => 8;
>+use constant POS_EVT_END        => 9;

  I think almost all of these need a one-line comment that explains what the
event is. Also, I'm unclear on why POS_EVT_END is different than the actual
number of the last event, while POS_EVT_START is exactly the same as the number
of the first event.

>+use constant NEG_EVT_START      => 50;
>+use constant EVT_UNCONFIRMED        => 50;
>+use constant EVT_CHANGED_BY_ME      => 51;
>+use constant NEG_EVT_END        => 52;

  And I think that these need simple comments, too.

>+# These are the "global" flags, which aren't tied to a particular relationship.
>+# and so use REL_ANY.
>+use constant EVT_FLAG_REQUESTED     => 100;
>+use constant EVT_REQUESTED_FLAG     => 101;

  These probably just need comments, because I'm unclear on which one indicates
what directon of flag requests.


  I need somebody else to review the BugMail.pm part of this patch, starting
from "# We now have a complete set of all the users, and their relationships
to"

>     if ($enableSendMail == 1) {
>-        open(SENDMAIL, "|/usr/lib/sendmail $sendmailparam -t -i") ||
>+        open(SENDMAIL, ">>/tmp/after.mail.txt") ||

  Although this part does need to go before we can actually check in this
patch.


  Flag.pm looks fine.

  I'll need to finish this review, starting at User.pm, later. However, just
the little bits that I noted above do need to change.
Attachment #169863 - Flags: review?(mkanat) → review-
Comment on attachment 169863 [details] [diff] [review]
Patch v.8

Hey Jake. If you get a chance to do so before Gerv posts another patch, could
you look over the BugMail.pm part of this patch, starting from "# We now have a
complete set of all the users, and their relationships to"?

Also, anything below that is also fair game for a review, as I haven't had the
chance to look at it yet.
Attachment #169863 - Flags: review?(jake)
Reporter

Comment 75

15 years ago
(In reply to comment #74)
> Hey Jake. If you get a chance to do so before Gerv posts another patch

I'd actually love to look at this patch, I just don't know when I'm gonna get
the opprotunity :(.
Comment on attachment 169863 [details] [diff] [review]
Patch v.8

(In reply to comment #65)
> >+                    $dbh->do("INSERT into profiles_nomail VALUES (" . 

>   *nit* Also, for those of us who don't know the schema by heart, it might be
> nice to put the names of the fields being inserted into in the INSERT
> statement.

That's not a nit.  That's damn important.  We should NEVER depend on the order
of the columns in the table.  If there's ever schema changes to a table, the
order of the columns could very well be dependent on whether the user had a new
install or upgraded with checksetup.pl.

This appears to be still present in the current patch, so consider that a
review- from me, too.
Attachment #169863 - Flags: review?(jake) → review-
Posted patch Patch v.9 (obsolete) — Splinter Review
(In reply to comment #73)

I've addressed all comments except:

> *nit* I think these should be a little shorter (70 chars?) to account for
> being able to quote them like I'm doing here. :-)

I'm being consistent with the other ones in the file...

If you could finish the review, that would be great.

Gerv
Attachment #169863 - Attachment is obsolete: true
Attachment #172878 - Flags: review?(mkanat)
Comment on attachment 172878 [details] [diff] [review]
Patch v.9


  OK, I understand Bugzilla much better now than I did last time I reviewed
this, so I do have a few new comments on the code, and I should also be able to
make it through all the parts, this time.

>Index: userprefs.cgi
>===================================================================
>
>+sub SaveEmail {
>+    my $dbh = Bugzilla->dbh;
>+    my $cgi = Bugzilla->cgi;

  You know, since we're re-writing this whole sub anyway... it kind of looks
like it could be useful in Bugzilla::User, instead of in here. Not necessary,
just could be nice.


>+        foreach my $event (POS_EVENTS) {
>+            if (!defined($cgi->param("email-$rel-$event"))) {

  Normally we also check if it's defined and false, in case somebody wants to
do strange things, or if some browser is acting weird, or if we want to change
the templates somehow. :-)

  Also, another note is that the email-$rel-$event method of handling this in
the UI makes it very difficult to change the UI to, say, multi-select boxes
(which I've wanted to do for a while).

  Also, this whole POS_EVENTS thing makes us required to have a UI where all
the events are always showing, and they are always submitted. So basically,
what I'm getting at is that we can't ever change the UI, with this backend. :-)

  Those are just nits, though.

>Index: Bugzilla/BugMail.pm
>[snip]
>+@Bugzilla::BugMail::EXPORT = qw(

  Just so you know -- we do have an EXPORT statement in BugMail, now, since I
moved PerformSubsts there. That's really recent, though.

>+    RELATIONSHIPS

  I think we should call it EMAIL_RELATIONSHIPS so that it's clearer when it's
in the code.

>+    GLOB_EVENTS

  Since a "glob" is actually something in perl, I think it'd be better to call
it GLOBAL_EVENTS.

>+use constant EVT_UNCONFIRMED        => 50;
>+use constant EVT_CHANGED_BY_ME      => 51;

  You know, these aren't really "events" so much as they are "states." Perhaps
we could name them differently.

>+use constant NEG_EVENTS => EVT_UNCONFIRMED, EVT_CHANGED_BY_ME;

  I think brackets would be nice for clarity, if they don't break anything.

>-my %seen;
>-my @sentlist;
>+my %nomail;

  Does %nomail still need to be out here globally? (I haven't looked yet, maybe
I'll figure this out as I read.)

>@@ -81,10 +133,16 @@ if (open(NOMAIL, '<', "$datadir/nomail")
>         $nomail{trim($_)} = 1;
>     }
>     close(NOMAIL);
> }
> 
>+my $sitespec = '@' . Param('urlbase');
>+$sitespec =~ s/:\/\//\./; # Make the protocol look like part of the domain
>+$sitespec =~ s/^([^:\/]+):(\d+)/$1/; # Remove a port number, to relocate
>+if ($2) {
>+    $sitespec = "-$2$sitespec"; # Put the port number back in, before the '@'
>+}

  Why is this code here, outside of a subroutine?


>+            if ($what eq "CC") {
>+                push(@{$recipients{DBNameToIdAndCheck($old)}}, REL_CC);
>+            }

  I'm confused about the keys of $recipients. Sometimes it looks like you're
using the userid, sometimes it looks like you're using $user->login. Which one
is it supposed to be?

>+            if ((!$nomail{$user->login}) &&

  Why is it that %nomail uses $user->login, but %recipients uses the userid?
Was it just like that before you got here?


>+    $dbh->do("UPDATE bugs SET lastdiffed = '$end', delta_ts = delta_ts " .
>+             "WHERE bug_id = $id");

  Use placeholders, here.

>+sub sendMail($$$$$$$$$$$$) {

  Hrm... don't use mixedCaps. And this should probably be called
SendMailToUser, to make it more clear what it does.

>     my %values = %$valueRef;
>     my @headerlist = @$hlRef;
>-    my @reasons = @$reasonsRef;
>-    my %defmailhead = %$dmhRef;
>+    my %mailhead = %$dmhRef;

  Why pass this in as a reference, if we're just going to recast and reassign
it anyway?

>-    $substs{"reasonsheader"} = join(" ", @reasons);
>+    $substs{"reasonsheader"} = join(" ", map { $rel_names{$_} } @$relRef);

  Are we (and should we be) removing things from the reasonsheader if you
actually *didn't* get the email for that reason, even if you have that
relationship to the bug?

>@@ -886,14 +683,16 @@ sub MessageToMTA ($) {
>     unless (Param("sendmailnow")) {
>        $sendmailparam = "-ODeliveryMode=deferred";
>     }
> 
>     if ($enableSendMail == 1) {
>-        open(SENDMAIL, "|/usr/lib/sendmail $sendmailparam -t -i") ||
>+        open(SENDMAIL, ">>/tmp/after.mail.txt") ||
>           die "Can't open sendmail";
> 
>         print SENDMAIL trim($msg) . "\n";
>         close SENDMAIL;
>     }

  Please remove this part from the next patch, so that we can actually check it
in. :-)

>+
>+    return 1;
> }

  Why are you making MessageToMTA return something? That's not really a part of
this patch.

>Index: Bugzilla/User.pm
>[snip]
>
>+# Changes in some fields automatically trigger events. The 'field names' are
>+# from the fielddefs table. We really should be using proper field names 
>+# throughout.
>+my %names_to_events = (

  If this is intended to be a package-global, it should be "our". If it's a
constant, it should be a "use constant," and all-caps, too (just not exported).
If it's not intended to be a package-global, it should be inside of a
subroutine. :-)


>+    foreach my $ref (@$fieldDiffs) {
>+        my ($who, $fieldName, $when, $old, $new) = @$ref;

  Couldn't @$fieldDefs be an array of hashes? That would make this code much
easier to understand and write, I'd think.

>+            # $events{+EVT_OTHER} = 1; 

  This must be something I don't know about perl -- why is that + there?

>+        if (($fieldName eq "AssignedTo" && $relationship == REL_ASSIGNEE) ||
>+            ($fieldName eq "QAContact" && $relationship == REL_QA)) 
>+        {
>+            $events{+EVT_ADDED_REMOVED} = 1;
>+        }

  Could you add a comment above this explaining what it's doing? It's not
totally simple for somebody who doesn't understand emailprefs. Also explain how
we know that we can definitely say "this person was added/removed from this
capacity" by the logic here.

>+    my $wants_mail = $self->wants_mail(\@event_list, $relationship);
>+
>+    # The negative events are handled separately - they can't be incorporated
>+    # into the first wants_mail call, because they are of the opposite sense.
>+    # 
>+    # 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)) {
>+        $wants_mail &= $self->wants_mail([EVT_CHANGED_BY_ME], $relationship);

  I'd appreciate it if the comment also explained why we check if $changer
exists at all. I'm not clear on when $changer would not exist.

>+        my $bug_status = $dbh->selectrow_array("SELECT bug_status 
>+                                                FROM bugs 
>+                                                WHERE bug_id = $bug_id"); 

  Use a placeholder.

>+# Returns true if the user wants mail for a given set of events.

  *nit* This comment is already in the pod docs, so doesn't need to also be
here.

>+sub wants_mail {

  Hrm... I think wants_mail_for_events would be clearer. Otherwise the
distinction between wants_bug_mail and wants_mail was confusing me.

>+    if (!defined($relationship)) {
>+        $relationship = REL_ANY;
>+    }

  Maybe I'm just getting tired, here, but I'd also like a short comment above
this. :-)

>+    my $count = 
>+        $dbh->selectrow_array("SELECT COUNT(event) 

  *nit* COUNT(*) is always guaranteed to pick the fastest way of counting.

>+                              FROM profiles_nomail
>+                              WHERE userid = $self->{'id'}
>+                              AND relationship = $relationship 
>+                              AND event IN (" . join(",", @$events) . ")");

  And use placeholders, here, where possible. It will be nice for the DB to be
able to cache the parsing of at least some of this statement, since it may
happen a lot.

>+      <input type="checkbox" name="email-100-100" id="email-100-100" 

  Myk just today checked-in a bug that makes Bugzilla::Constants available to
templates. :-)

  Which probably means that the constants will have to be moved back into
Bugzilla::Constants. But I'm OK with that.


  OK, so that's it! :-) As far as the technical *accuracy* of the patch, I
trust your tests above, so far.
Attachment #172878 - Flags: review?(mkanat) → review-
Also, I wanted to note that the "negative boolean" aspect of this patch makes it
really confusing, code-wise. That is, the fact that we store times when people
*don't* want mail is confusing.

Is the performance and space difference really significant enough to justify the
fact that it's generally hard for people to wrap their minds around negative
booleans? Or does it perhaps make the code much more elegant somehow, and the
positive version would be confusing?
Reporter

Comment 80

15 years ago
I think the negative boolean thing you're refering to is my fault. My reason for
picking that method was space/table size. If a person never changes their
prefences (which is probably the vast majority of Bugzilla's users), there won't
be any entries for them in this table. In the positive boolean method, each user
would have a row for every relationship, which, as I recall, would be about 5
rows per user. In all honesty, either way would probably result in less storage
space than what's currently there.
(In reply to comment #80)
> If a person never changes their
> prefences (which is probably the vast majority of Bugzilla's users), there
> won't be any entries for them in this table.

FYI, since 2.18 and bug 108870 this is no longer the case. Does that then give 
rise to good reason to do this with positive boolean logic rather than negative?


The reason for the "nomail" part is indeed table space. By default, users get
all mail, and most leave it that way. So it makes sense to only store exceptions
to that.

On b.m.o., if we stored every state, we would store approximately 5
relationships * 13 events * 184969 users = 12 million rows. Somewhat unnecessary.

Bug 108870 makes no difference - it's just added a lot of information (that
everyone wants all mail) which this patch 'throws away' - or rather, assumes and
doesn't store.

I'll try and update the patch soon.

Gerv

Comment 83

15 years ago
12 million rows is hardly something I would lose sleep over.  If that reverse
logic makes things messy, I'd suggest we rethink the rationale.

Posted patch Patch v.10 (obsolete) — Splinter Review
Here's an un-rotted version of the patch, where I've also tried to address some
of Max's concerns. Some of the things I haven't changed, for a variety of
reasons - it's 11.46pm, I've been at this for hours and I'm not going to
provide a laundry list.

I emailed Dave about the "sense" of the pref storage, and received deafening
silence in reply, so I've left it the way it is. I spent a lot of November
(literally) testing this patch exhaustively to make sure there were no
regressions, and changing the sense would basically mean having to repeat all
of that work.

When reviewing, please bear in mind that one form or another of this change has
been around now for a year and a half, being kicked around by a wide variety of
reviewers and their various requirements, and I'm getting a little bit
frustrated at trying to keep a very nasty-to-merge patch up to date. It's
particularly annoying as default-cc and component-watching are the _target_
things here, and this is just an infrastructure fix.

So, review away, but please bear the above in mind.

Gerv
Attachment #172878 - Attachment is obsolete: true
Attachment #176795 - Flags: review?(mkanat)
Posted patch Patch v.11 (obsolete) — Splinter Review
This merges the most recent set of checksetup.pl changes and the Bug.pm
whackage.

Gerv
Attachment #176795 - Attachment is obsolete: true
Attachment #177254 - Flags: review?(mkanat)
Attachment #176795 - Flags: review?(mkanat)

Comment 86

15 years ago
+my $sitespec = '@' . Param('urlbase');
+$sitespec =~ s/:\/\//\./; # Make the protocol look like part of the domain
+$sitespec =~ s/^([^:\/]+):(\d+)/$1/; # Remove a port number, to relocate
+if ($2) {
+    $sitespec = "-$2$sitespec"; # Put the port number back in, before the '@'
+}

What is this?  A bugfix for threading?

+        # If !$user, the person doesn't exist - they probably changed email.
+        next if !$user;

For this, the userid would have to be gone rather than the email.  I think it
deserves a warning at least.




Blocks: 23924

Comment 87

15 years ago
Comment on attachment 177254 [details] [diff] [review]
Patch v.11

Very close...  

The only problem I see (only a little testing so far) is that it makes the
emailprefs edit page depend on CSS.  This is because it uses CSS to disable
columns while computing the span of the headers using colspan.

OOPS - Broke editusers, add new user

DBD::mysql::db do failed: Unknown column 'emailflags' in 'field list' at
Bugzilla/User.pm line 1092
	Bugzilla::User::insert_new_user('xxx') called at /xxx/editusers.cgi
line 174
Attachment #177254 - Flags: review-

Comment 88

15 years ago
Also, in preparing a new patch...  there was another problem seen with creating
a new account (createaccount.cgi).  It is not certain that it resulted from the
patch, so please try account creation with the new candidate patch.
Posted patch Patch v.12 (obsolete) — Splinter Review
This fixes the issues raised by Joel, including making editing users work. The
prefs page does indeed depend on CSS to look right. Do we think this is a
problem?

The createaccount.cgi thing was a regression unrelated to this patch - see bug
286008.

Gerv
Attachment #177254 - Attachment is obsolete: true
Attachment #177324 - Flags: review?(mkanat)

Comment 90

15 years ago

17:21	joel 	Anybody know if we care if things become pretty much unusable
without CSS???
17:22	LpSolit 	ask myk :)
17:23	justdave 	yes, we care.
17:23	justdave 	it should degrade gracefully

I'll let you query justdave about the definition of degrade gracefully, but I
think the patch degraded clumsily :-)

Updated

15 years ago
Attachment #177254 - Flags: review?(mkanat)
Comment on attachment 177324 [details] [diff] [review]
Patch v.12

>--- checksetup.pl	11 Mar 2005 19:45:01 -0000	1.371
>+++ checksetup.pl	13 Mar 2005 23:39:35 -0000
>@@ -132,10 +132,11 @@ BEGIN {
> 
> use lib ".";
> 
> use vars qw( $db_name %answer );
> use Bugzilla::Constants;
>+use Bugzilla::BugMail;

  checksetup may never "use" anything. And it may only "require" much lower
than this, around where we require Bugzilla::Auth.

> # 2000-12-18.  Added an 'emailflags' field for storing preferences about
> # when email gets sent on a per-user basis.
>-if (!$dbh->bz_get_field_def('profiles', 'emailflags')) {
>+if (!$dbh->bz_get_field_def('profiles', 'emailflags') && 
>+    !$dbh->bz_get_field_def('profiles_nomail', 'userid')) {

  You can safely just remove this entirely. Just check if the emailflags field
is defined inside of your migration code.

> [snip]
>+} # END LEGACY CHECKS

  We seem to be missing a bz_add_field... am I imagining that?

>Index: Bugzilla/BugMail.pm
[snip]
>+@Bugzilla::BugMail::EXPORT = qw(
>+    RELATIONSHIPS
>+    REL_ASSIGNEE REL_QA REL_REPORTER REL_CC REL_VOTER 

  These constants must be moved back into Bugzilla::Constants, so that they
will be accessible from the templates.


  Other than that, I think:

  (1) The code is much too complicated.
  (2) It's still better than what we have now.
  (3) It looks like we depend on JavaScript for the checkboxes to function
properly in userprefs? (Unless I'm missing something.)
  (4) My first instinct once this code is checked in will be to start planning
the re-write of it.
  (5) It's still better than what we have now.
Attachment #177324 - Flags: review?(mkanat) → review-

Updated

15 years ago
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
I also wanted to point out that it's unnecessary to have stored the data as
rows. It could just as easily have been in columns, since we don't expect
customizers to add their own Roles and Relationships, as you have mentioned.
That would have made it easily possible to simply transition the code from using
emailflags to using the table, without a significant re-write.

This bug was just about a table, not about a re-write.
> We seem to be missing a bz_add_field... am I imagining that?

What field are you expecting?

> These constants must be moved back into Bugzilla::Constants, so that they
> will be accessible from the templates.

Sure :-) (Bugzilla::Constants didn't exist when this patch was first written.)

> (3) It looks like we depend on JavaScript for the checkboxes to function
> properly in userprefs? (Unless I'm missing something.)

You are; it doesn't. :-) There are convenience buttons to enable and disable all
the checkboxes, which depend on JS, and are only present if JS is enabled. This
is not a new feature - these exist today.

> (4) My first instinct once this code is checked in will be to start planning
> the re-write of it.

Thank you for your constructive criticism. Do you have anything more specific?
None of the many people who have looked at this code in its previous
incarnations have suggested architectural problems.

> This bug was just about a table, not about a re-write.

You may not be aware of the history here, which is this: over Christmas 2003, I
wrote a reasonably large patch which updated the email preferences
infrastructure, and implemented component watching and default CC on top. You
can see it attached to bug 76794. Myk asked for it to be broken into pieces, so
I broke out the email preferences part, and have been trying to get that section
reviewed on and off for the past year. 

Therefore, the code is designed to be the supporting infrastructure for those
larger changes. This bug just happened to be the most appropriate one to attach
the patch to.

> I also wanted to point out that it's unnecessary to have stored the data as
> rows. It could just as easily have been in columns, since we don't expect
> customizers to add their own Roles and Relationships, as you have mentioned.
> That would have made it easily possible to simply transition the code from 
> using emailflags to using the table, without a significant re-write.

I'm planning to add extra roles and relationships; as things stand, this is
really quite easy. Which I will appreciate, even if no-one else does. :-)

The "significant rewrite" has also fixed a whole clutch of email issues,
including several latent bugs I discovered along the way and some odd
behaviours, and has been very carefully QAed and side-by-side regression tested
with the existing implementation.

I'll fix the issues you've raised and upload another patch.

Gerv
Posted patch Patch v.13 (obsolete) — Splinter Review
Patch v.13 - hopefully not unlucky. All comments addressed (including removal
of the dependence on CSS) apart from the one about "safely delete" - which I
didn't quite understand.

I'm becoming more tempted by the idea of reversing the sense of the entire
thing, but I'm only going to do it if a) no-one then comes along and tells me
to switch it back, and b) it doesn't mean that this patch misses yet another
release.

Gerv
Attachment #177324 - Attachment is obsolete: true
Comment on attachment 177439 [details] [diff] [review]
Patch v.13

Thanks for responding to my comments. I think that my architectural
difficulties are with BugMail.pm in general, and perhaps not with your code in
particular.

Thinking about it now, I think that the best solution (eventually) would be to
move the email filtering code to the inside of Bug.pm, and then just ask Bug.pm
for a list of email addresses who should get notified of a particular change.
After all, Bug.pm already knows all the possible recipients.

However, the code as it is looks good to me.

I know that you have extensively tested it for accuracy in sending email.

I would agree with reversing the sense, if you'd like to do that. I believe
that we will still get it into 2.20 if you could have it done within a week and
a half, with the same extensive testing that the negative sense currently has.

You might also want to sketch out some pseudocode for yourself of what the
positive sense would look like, and see how much simpler it would actually be
to implement.
Attachment #177439 - Flags: review+
(In reply to comment #83)
> 12 million rows is hardly something I would lose sleep over.  If that reverse
> logic makes things messy, I'd suggest we rethink the rationale.

Disk space is cheap, and MySQL indexing is quite fast these days.  I'd rather do
it whichever way gets the best performance, and not worry about which one
consumes disk space.
Posted patch Patch v.14 (obsolete) — Splinter Review
Typical - I finally get r+ and I've gone and rewritten part of it :-) Here's
the positive sense version. It's no simpler, but it is more future-proof and
gives more flexibility in changing the UI later. I've compared the mail-sending
of old and new, and they are identical for my test run.

Looking at the initial description of this bug, I think we can blame Jake for
the "reverse sense" business ;-)

Gerv
Attachment #177439 - Attachment is obsolete: true
Attachment #177467 - Flags: review?(mkanat)
Posted patch Patch v.15 (obsolete) — Splinter Review
And here's a more polished version, with the symbolic constants used in the
templates and stuff.

Go ahead and review - I promise not to rev it any more :-)

Gerv
Attachment #177467 - Attachment is obsolete: true
Attachment #177557 - Flags: review?(mkanat)
I'm setting blocking to keep this on the radar since it's being actively worked
on and has already passed review once.  If it doesn't land within the next
couple days I might change my mind.
Flags: blocking2.20+
Posted patch Patch v.16 (obsolete) — Splinter Review
Unrotted at Jake's request.

Gerv
Attachment #177557 - Attachment is obsolete: true
Attachment #178301 - Flags: review?(mkanat)
Reporter

Comment 101

15 years ago
Comment on attachment 178301 [details] [diff] [review]
Patch v.16

Unfortunately, I was unable to get into any intensive testing because the email
settings page in the user prefs section was definately not working as intended.
There were no relationships (like reporter, cc, etc) listed  nor were there any
checkboxes other than the two at the top related to flags.
Attachment #178301 - Flags: review?(mkanat) → review-
Jake: I've just carefully tested that page, both with usevotes on and off and
usewatchers on and off, and haven't seen any issue like the one you describe.
Are you able to take a moment to diagnose the problem a little bit?

The email.html.tmpl file was one of the conflicted ones - are you sure you
cleaned up properly after applying the version of the patch that conflicted?

Gerv
I tried with a current CVS version of Bugzilla as of about 20 minutes ago, and I
see the same as Jake. There was amanually resolved conflict in checksetup.pl
though, but I doubt that caused it. However, it looks like its all dependant on
the magic values of "useqacontact" and "usevotes" - both need to be on for it to
work here.

Incidently, Around line 178 in template/en/default/account/prefs/email.html.tmpl
you seem to be missing a  > on a td tag.
Reporter

Comment 104

15 years ago
The error does seem to be related to weather or not the useqacontact Param() is
on. With it off, I see that error, with it on, I don't. I also seem to be having
other problems with the userprefs page. Even when all the checkboxes are there,
they don't show up properly checked. The "I want it when" boxes are all
unchecked and the "not when" boxes are all checked. At first I thought this was
a problem with the migration code, but it seems to be the prefs page. When I
clicked the button for all mail and then submit, I'm still seeing the same
results (even though all the top boxes were checked before I hit submit).
Posted patch Patch v.17 (obsolete) — Splinter Review
Thanks to Colin for pointing out the problem - a missing >. Probably an editing
mistake on my part. Apologies for that. The userprefs page should now be
working OK.

Gerv
Attachment #178301 - Attachment is obsolete: true
Attachment #178414 - Flags: review?(jake)
Gerv

I'm not convinced that was the entire fix - it still fails for me with your new
patch (although it did apply cleanly :))

I still think its to do with the Use of QA and Votes preferences.
Posted patch Patch v.18 (obsolete) — Splinter Review
I just did a clean pull of CVS tip Bugzilla and my latest patch applies to it
without any fuzziness, conflicts or anything like that. I'm attaching it again
to be certain, but it should be identical to v17. 

The first time I loaded the email prefs tab, I saw what you saw - no
checkboxes. It was as if the "relationships" defined in the template just
weren't there. However, having fiddled a bit and restored it to what I thought
was the same text, it started working and I couldn't break it.

So, I checked out a completely new tree. Again, it didn't work first time. I
added a single space character to email.html.tmpl and now it does. I have no
idea why that could be - perhaps a TT caching issue of some sort?

Gerv
Attachment #178414 - Attachment is obsolete: true
Reporter

Comment 108

15 years ago
Comment on attachment 178595 [details] [diff] [review]
Patch v.18

This is just killing me. I'm not sure what's going on with the userprefs page,
but it seems that adding a space does do the trick... it's just weird that all
the other changes don't seem to matter, but that one space kicks it off.

Anyway, the r- this time is because of the following:
* When new users create an account, their emailprefs page shows all blank
checkboxes. (They still receive email, but the prefs page makes it look like
they'll get none).
* Users that currently have prefs but are converted end up having none of the
"New attachments are added" boxes check.
* The checksetup.pl portion puts back what was backed out in bug 286625.
Attachment #178595 - Flags: review-
Reporter

Updated

15 years ago
Attachment #178414 - Flags: review?(jake)
Posted patch Patch v.19Splinter Review
This addresses Jake's three issues. Of course, switching from
"no-entry-means-send-mail" to "no-entry-means-dont-send-mail" means you have to
add a number of entries at account creation time.

Lesson: don't change the way a patch works fundamentally so late in
development.

Gerv
Attachment #178595 - Attachment is obsolete: true
Attachment #178669 - Flags: review?(jake)

Updated

14 years ago
Attachment #177557 - Flags: review?(mkanat)
Reporter

Comment 110

14 years ago
Comment on attachment 178669 [details] [diff] [review]
Patch v.19

template/en/default/account/prefs/email.html.tmpl has an unfiltered directive
(prefname), but that can be fixed on checkin. All other t/* tests pass as well
as my funcality testing. This looks like it's finally ready :).
Attachment #178669 - Flags: review?(jake) → review+
Reporter

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
Fixed! Wahey!

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.379; previous revision: 1.378
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.72; previous revision: 1.71
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.109; previous revision: 1.108
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.38; previous revision: 1.37
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.34; previous revision: 1.33
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.50; previous revision: 1.49
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/request/email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v
 <--  email.txt.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.39; previous revision: 1.38
done
Checking in contrib/sendunsentbugmail.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/sendunsentbugmail.pl,v  <-- 
sendunsentbugmail.pl
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.17; previous revision: 1.16
done

Gerv
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Blocks: 289593

Updated

14 years ago
Blocks: 219377
Added to the release notes in bug 286274. I didn't actually relnote comment 40,
since my understanding is that before checking in the patch, we set the code to
behave more like the current code, which is that disabled accounts still get mail.

If I'm wrong, please let me know.
Keywords: relnote
Whiteboard: [relnote: comment 40]

Updated

14 years ago
Blocks: 262318
*** Bug 309907 has been marked as a duplicate of this bug. ***
Blocks: 369648
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.