275599 - flag request email prefs not behaving correctly

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Shankar Unni]

In an upgraded (2.17->2.19) installation, no mail is sent for flag requests,
even though the Email prefs for the requestee say that they want flag mail. 
However, if the requestee visits his Prefs page *and saves his prefs*, then he
starts getting mail.

(I must also admit that I only first created a flag after said upgrade, to try
out the new features.)

The reason, from what I can see, is that the email prefs column is null after
checksetup.pl is run (after the update), and stays null until the user saves
preferences.

Comment 1

[Jacob Steenhagen]

This is a manifestation of what's being fixed in bug 108870.

*** This bug has been marked as a duplicate of 108870 ***

Comment 2

[Shane H. W. Travis]

Unless I'm missing something *very* significant, Jake, this is not a dupe, but 
a completely separate issue. I specifically asked Shankar to come here and open 
a new bug so it didn't get lost because (IMHO) it's worthy of being another 
2.18 blocker all on its own.

This is specifically about Flag emailprefs for users who already have 
emailprefs stored in their profile. According to Shankar (I have not verified 
this yet) when an existing user with an existing emailpref setup visits the 
emailpref tab of userprefs.cgi, it *looks* like they have the flag turned 
on ... but Bugzilla does not behave that way. Only after the user has visited 
his emailprefs AND SAVED THEM will it start sending him flag-related Bugmail.

(Do I have that right, Shankar?)

Comment 3

[Jacob Steenhagen]

I realized about 30 seconds after I hit the "Commit" button that you had asked
him to file a seperate bug. I was under the (apparently) mistaken impression
that the reason flag mails weren't being sent was because of a blank emailprefs
field.

Comment 4

[Shankar Unni]

(In reply to comment #2)

> (Do I have that right, Shankar?)

Right.

Comment 5

[Max Kanat-Alexander]

OK, so does this mean that checksetup needs to look for people who have
emailprefs, but lack full emailprefs? If so, it would be nice to store those
default flag emailprefs as a separate Bugzilla::Constants var for bug 108870.

Comment 6

[Shane H. W. Travis]

I'll take this one, as I'm fooling around in this part of the code anyway. (I 
need to create something just like this for my site anyway as this is going to 
affect us, so I can justify the time.)

Comment 7

[Shane H. W. Travis]

(In reply to comment #5)
> OK, so does this mean that checksetup needs to look for people who have
> emailprefs, but lack full emailprefs? 

That is correct. 

Algorithm is that I'm going to do two DB searches: 
1) select from profiles where emailprefs is empty. For each of these, add a set 
of default emailprefs (stored in Constants.pm, part of bug 108870).
2) select from profiles where there are no flag prefs. For each of these, add a 
set of default flag email prefs, also stored in Constants.pm (defined as part 
of this bug).

On completion, there should be *no* profiles with empty emailprefs, and all 
users will have default flag profiles set in addition to whatever they had set 
before.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> Algorithm is that I'm going to do two DB searches: 
> 1) select from profiles where emailprefs is empty. For each of these, add a set 
> of default emailprefs (stored in Constants.pm, part of bug 108870).
> 2) select from profiles where there are no flag prefs. For each of these, add a 
> set of default flag email prefs, also stored in Constants.pm (defined as part 
> of this bug).

  I think a better thing to do would be to grab everybody's emailprefs, and
check each one to see if it's set or not. That's a bit more difficult, though,
because of the way that DEFAULT_EMAIL_SETTINGS work.

Comment 9

[Shane H. W. Travis]

(In reply to comment #8)
>   I think a better thing to do would be to grab everybody's emailprefs, and
> check each one to see if it's set or not.

It's also much more labour-intensive, and it will take the same amount of 
labour to perform *every single time* that checksetup.pl is run. The algo above 
will cause a fair bit of work the first time it is run on a newly-upgraded 
database, but it should return empty queries every time after that and thus 
just zip past that section.

Since checksetup.pl gets run a lot, this was an important consideration in 
deciding how to do it.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> [snip]
> Since checksetup.pl gets run a lot, this was an important consideration in 
> deciding how to do it.

  Oh, OK. I totally agree with that, then, actually. :-) Scanning the entire
profiles table every time somebody runs checksetup would be bad. :-)

Comment 11

[Shane H. W. Travis]

Attachment: Code changes for 2.18 and tip

This patch relies on bug 108870 landing first, and is written on the assumption
that it already exists on the system. Review it with that in mind, and
definitely do that one first if you haven't already.

The FlagRequestee/er emailflags are added at the beginning of the string
(rather than one entry in as they're saved after a visit to userprefs.cgi) but
that doesn't seem to (and shouldn't) make a difference; order should be
irrelevant, and it was a heck of a lot easier to put them up front than to try
and shoehorn them in.

Notice that I also set all blank emailflags to be the default that was set in
108870. This did not *need* to be done there, but it needs to be done here.

I have tested this on my system (2.18rc3) and it works for both conditions.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 169889 [details] [diff] [review]
Code changes for 2.18 and tip

Hey, let's just combine this patch and bug 108870. That way we can get rid of
the code in userprefs.cgi and email_prefs that has to deal with what happens if
the emailprefs are empty, right? That is, with this patch and bug 108870
combined, emailprefs can't possibly be empty, right...?

Comment 13

[Shane H. W. Travis]

(In reply to comment #12)
> (From update of attachment 169889 [details] [diff] [review] [edit])
> Hey, let's just combine this patch and bug 108870. 

Heh, after asking Shankar to open a new bug, turns out we're going to fix both 
things in 108870 after all. :)

> That way we can get rid of
> the code in userprefs.cgi and email_prefs that has to deal with what happens
> if the emailprefs are empty, right? 

Makes sense to me. I've rolled this patch into the 108870 patches now, and 
modified appropriately. Consider this bug closed when 108870 lands.

Comment 14

[Gervase Markham [:gerv]]

Comment on attachment 169889 [details] [diff] [review]
Code changes for 2.18 and tip

Removing obsolete review - this is now being fixed in bug 108870.

Gerv

Comment 15

[Shane H. W. Travis]

Bug 108870 has been RESOLVED/FIXED, so marking this bug the same.