73665 - (emailprefs) Create an "emailprefs" table

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Jacob Steenhagen]

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).

Comment 1

[Jacob Steenhagen]

This can't be done until at the very least "processmail regenerate" is removed
from ./checksetup.pl

Comment 2

[Matthew Tuck [:CodeMachine]]

See bug #14137 ... it would require a specific sort of functionality from an
email prefs table.

Comment 3

[Jacob Steenhagen]

-> New Bugzilla Product

Comment 4

[Jacob Steenhagen]

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).

Comment 5

[Jacob Steenhagen]

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).

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

What's the status on this?  I'm adding an extra SQL query to mailprefs in bug
34488...

Comment 7

[Jacob Steenhagen]

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.

Comment 8

[Dave Miller [:justdave]]

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.

Comment 9

[Dave Miller [:justdave]]

This obviously isn't going to make it....   -> 2.18

Comment 10

[Jacob Steenhagen]

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.

Comment 11

[Jacob Steenhagen]

Attachment: The checksetup.pl portion for comment

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).

Comment 12

[Jacob Steenhagen]

Attachment: Some more work for comment

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).

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 72656 [details] [diff] [review]
Some more work for comment

needs-work - this is incomplete, and I doubt it still applies.

Comment 14

[Jacob Steenhagen]

Attachment: Catching up to 4mos of CVS changes

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.

Comment 15

[Jacob Steenhagen]

Attachment: patch v0.9

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 :)

Comment 16

[Bradley Baetz (:bbaetz)]

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 @@
>-<!-- [e-mail] -->
>+[%# -*- mode: html -*- %]   <!-- [e-mail] -->

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

Comment 17

[Jacob Steenhagen]

Attachment: Patch v1

>>+	       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.

Comment 18

[Joel Peshkin]

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.

Comment 19

[Jacob Steenhagen]

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.

Comment 20

[Bradley Baetz (:bbaetz)]

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.

Comment 21

[Jacob Steenhagen]

Attachment: Patch v2

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.

Comment 22

[Joel Peshkin]

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??

Comment 23

[J. Paul Reed [:preed]]

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

[Simon Spiegel]

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.

Comment 25

[Dave Miller [:justdave]]

reassigning to default owner/QA... Jake's Army Reserve unit has been deployed.

Comment 26

[Gervase Markham [:gerv]]

I'm currently working on this, and a lot of related bugs.

Gerv

Comment 27

[Gervase Markham [:gerv]]

Attachment: Patch v.1

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

Comment 28

[Gervase Markham [:gerv]]

Comment on attachment 139553 [details] [diff] [review]
Patch v.1

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

Gerv

Comment 29

[J. Paul Reed [:preed]]

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.)

Comment 30

[Gervase Markham [:gerv]]

> 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

Comment 31

[J. Paul Reed [:preed]]

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.

Comment 32

[Gervase Markham [:gerv]]

> 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

Comment 33

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 34

[Dave Miller [:justdave]]

(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.

Comment 35

[Dave Miller [:justdave]]

(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)

Comment 36

[Dave Miller [:justdave]]

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? ;)

Comment 37

[Gervase Markham [:gerv]]

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

Comment 38

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Unbitrotted version - hopefully still working ;-)

Gerv

Comment 39

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Fixes issues found by Myk in testing.

Gerv

Comment 40

[Joel Peshkin]

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

[Joel Peshkin]

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

Bitrotted - wont apply to tip

Comment 42

[Gervase Markham [:gerv]]

Attachment: Patch v.4

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

Comment 43

[Diego Biurrun]

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

bitrotted

Comment 44

[Diego Biurrun]

Attachment: patch v.5

Same as before minus some trivial bitrot, the wording in one message had been
changed...

Comment 45

[Diego Biurrun]

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.

Comment 46

[Gervase Markham [:gerv]]

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

Comment 47

[Gervase Markham [:gerv]]

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

Gerv

Comment 48

[Diego Biurrun]

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 :-)

Comment 49

[Joel Peshkin]

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

Comment 50

[Myk Melez [:myk] [@mykmelez]]

>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.

Comment 51

[Gervase Markham [:gerv]]

> 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

Comment 52

[Bradley Baetz (:bbaetz)]

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

Comment 53

[Joel Peshkin]

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.

Comment 54

[Gervase Markham [:gerv]]

Sadly, we're just not there yet on this, if we want 2.18 to happen any time soon.

Gerv

Comment 55

[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 56

[Dave Miller [:justdave]]

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)

Comment 57

[Gervase Markham [:gerv]]

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

Comment 58

[Gervase Markham [:gerv]]

Attachment: Patch v.6

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

Comment 59

[Max Kanat-Alexander]

(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.)

Comment 60

[Marc Schumann [:Wurblzap]]

This is marked blocking2.18- and at the same time blocking a blocking2.18+ bug
(bug 108870) -- Dave, please re-decide blocking2.18.

Comment 61

[Dave Miller [:justdave]]

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.

Comment 62

[Gervase Markham [:gerv]]

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

Comment 63

[Dave Miller [:justdave]]

(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 64

[:glob ✱]

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 65

[Max Kanat-Alexander]

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.

Comment 66

[Gervase Markham [:gerv]]

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

Comment 67

[Gervase Markham [:gerv]]

Attachment: Patch v.7

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

Comment 68

[Max Kanat-Alexander]

(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

Comment 69

[Gervase Markham [:gerv]]

(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

Comment 70

[Gervase Markham [:gerv]]

Attachment: Patch v.8

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

Comment 71

[Vlad Dascalu]

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

[Vlad Dascalu]

Ok, I read comment #56 now, but I still think 2.20 is pretty optimistic :-) Good
luck with that, anyway. :)

Comment 73

[Max Kanat-Alexander]

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.

Comment 74

[Max Kanat-Alexander]

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.

Comment 75

[Jacob Steenhagen]

(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 76

[Dave Miller [:justdave]]

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.

Comment 77

[Gervase Markham [:gerv]]

Attachment: Patch v.9

(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

Comment 78

[Max Kanat-Alexander]

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.

Comment 79

[Max Kanat-Alexander]

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?

Comment 80

[Jacob Steenhagen]

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.

Comment 81

[Shane H. W. Travis]

(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?

Comment 82

[Gervase Markham [:gerv]]

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

[Joel Peshkin]

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.

Comment 84

[Gervase Markham [:gerv]]

Attachment: Patch v.10

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

Comment 85

[Gervase Markham [:gerv]]

Attachment: Patch v.11

This merges the most recent set of checksetup.pl changes and the Bug.pm
whackage.

Gerv

Comment 86

[Joel Peshkin]

+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.

Comment 87

[Joel Peshkin]

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

Comment 88

[Joel Peshkin]

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.

Comment 89

[Gervase Markham [:gerv]]

Attachment: Patch v.12

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

Comment 90

[Joel Peshkin]

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 :-)

Comment 91

[Max Kanat-Alexander]

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.

Comment 92

[Max Kanat-Alexander]

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.

Comment 93

[Gervase Markham [:gerv]]

> 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

Comment 94

[Gervase Markham [:gerv]]

Attachment: Patch v.13

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

Comment 95

[Max Kanat-Alexander]

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.

Comment 96

[Dave Miller [:justdave]]

(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.

Comment 97

[Gervase Markham [:gerv]]

Attachment: Patch v.14

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

Comment 98

[Gervase Markham [:gerv]]

Attachment: Patch v.15

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

Comment 99

[Dave Miller [:justdave]]

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.

Comment 100

[Gervase Markham [:gerv]]

Attachment: Patch v.16

Unrotted at Jake's request.

Gerv

Comment 101

[Jacob Steenhagen]

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.

Comment 102

[Gervase Markham [:gerv]]

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

Comment 103

[Colin Ogilvie [:cso]]

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.

Comment 104

[Jacob Steenhagen]

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).

Comment 105

[Gervase Markham [:gerv]]

Attachment: Patch v.17

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

Comment 106

[Colin Ogilvie [:cso]]

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.

Comment 107

[Gervase Markham [:gerv]]

Attachment: Patch v.18

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

Comment 108

[Jacob Steenhagen]

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.

Comment 109

[Gervase Markham [:gerv]]

Attachment: Patch v.19

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

Comment 110

[Jacob Steenhagen]

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 :).

Comment 111

[Gervase Markham [:gerv]]

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

Comment 112

[Max Kanat-Alexander]

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.

Comment 113

[[e-mail]]

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