Closed Bug 108870 Opened 21 years ago Closed 18 years ago

Bugzilla does not set email prefs for new user until user visits userprefs.cgi

Categories

(Bugzilla :: Email Notifications, defect, P4)

2.15

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: jlaska, Assigned: shane.h.w.travis)

References

Details

Attachments

(1 file, 10 obsolete files)

I have a bugzilla setup using LDAP to verify user accounts.  For a first time
login CGI::confirm_login() will call
globals.pl::InsertNewUser(username,realname) to insert the user into the
profiles table.  However, no email preferences are set for that user until
he/she visits the userprefs.cgi page.  Is this the expected behavior?  The
excerpt below is from userprefs.cgi to verify that it expects to see no
emailflags set.

sub showAdvancedEmailFilterOptions()
...
    SendSQL("SELECT emailflags FROM profiles WHERE userid = $userid");

    ($flags) = FetchSQLData();

    # if the emailflags haven't been set before, that means that this user 
    # hasn't been to (the email pane of?) userprefs.cgi since the change to 
    # use emailflags.  create a default flagset for them, based on
    # static defaults. 
    #
    if ( !$flags ) {
        $flags = $defaultEmailFlagString;
        setEmailFlags($flags);
    }
...

I believe $defaultEmailFlagString should be a definable parameter and set in the
InsertNewUser function.  I'm guessing that this isn't a problem for non-LDAP
logins because those users will most likely goto userprefs.cgi and change their
passwords (in which case the above function will set their emailflags).  Is
there any associated with setting their emailflags in
globals.pl::InsertNewUser()?  Do we want this to be an installation definable
(e.g. a parameter) setting?  Let me know you comments and I'll submit the patch.

Thanks!
The way this is all handled will change when bug 73665 gets fixed.  For the time
being, this isn't all bad... in processmail when it comes accross a mail pref
that hasn't been defined yet in the user's prefs, it simply treats it as "I want
mail for this" which is the default value.
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
reassigning to the Email folks, since they're the ones that actually implimented
those prefs originally.
Assignee: myk → preed
Component: User Accounts → Email Notifications
OS: Linux → All
Hardware: PC → All
I'll go ahead and take ownership of this.  Like I said in comment #1, no big
deal.  In fact, it's better that it doesn't store 'em as that's one long string
just to say "do the default thing".

When bug 73665 gets fixed, we won't store anything for those users who don't
specifically turn things off.
Assignee: preed → jake
Severity: normal → trivial
Depends on: emailprefs
Priority: P2 → P4
*** Bug 185013 has been marked as a duplicate of this bug. ***
According to bug 185013 which I just duped to this, the "only send me mail of
changes others make" is not using the correct default.  You get mail initially
then it stops after the first time you visit the prefs.
See proposed solution to this problem in bug 109573, comment 12.
*** Bug 213673 has been marked as a duplicate of this bug. ***
*** Bug 218892 has been marked as a duplicate of this bug. ***
reassigning to default...   Jake's Army Reserve unit has been deployed.
Assignee: jake → preed
All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Gerv: Will this be auto-fixed when bug 73665 lands?
Joel: yes. :-)

Gerv
Attached patch patch v0.9 (obsolete) — Splinter Review
Quick and dirty patch that sets the email flags to default (just as in
contrib/syncLDAP.pl) when a user is inserted. Temporarily, will be obsolete as
soon as bug 84876 is done.
Sorry, attachment (id=166946) will be obsolete with bug 73665.
Comment on attachment 166946 [details] [diff] [review]
patch v0.9

>     #XXXX - init defaultflagstring together with defaultflagstring in userprefs.cgi

defaultflagstring is mentioned twice in the comment, is that the intended
behaviour?

Besides, the code from the patch inserts it in bugzilla/globals.pl, so the
comment doesn't reflect the behaviour implemented by the patch.

The patch should be in the unified diff format (cvs diff -uN ...), that's what
we use since it's easier to read.

Also, $defaultflagstring should probably be configurable as a parameter, but
that's a nit. However, it creates redundancy if the default flag string appears
both here and in userprefs.cgi (I haven't checked if this is the case).
Attachment #166946 - Flags: review-
I have an updated patch for 73665 ready to go, so hold off on this one, OK? :-)

Gerv
Could we please get this into 2.20? 

It's not just for *new* users, it's also for users from a current 2.17+ base
after we upgrade to 2.19+ - they have non-NULL email flags, but no
"Flag"-specific email flags (because those didn't exist in 2.17+). 

And so, as things stand, they receive no Flag-related email at all (even though
the Email preferences page claims they will, until they *set* their preferences
explicitly).

As things stand, we can't start using a feature like "Flags" usefully unless
everyone who is potentially the requestor or requestee of a flag first goes and
"sets" their email preferences. The current default behavior is not to send mail..
Flags: blocking2.20?
*** Bug 273755 has been marked as a duplicate of this bug. ***
If bug 73665 makes it in before we branch for 2.20, it will solve this.  I'd
like this fixed for the 2.18 branch however, and the route described in this bug
looks like the way to go for that one.  If someone can fix Vladd's issues with
this patch and update it for the 2.18 branch I'd appreciate it.  If bug 73665
doesn't make it before we branch for 2.20, it'll get held for 2.22, and then
we'll want to port this forward to the 2.20 branch as well at that point.
Severity: trivial → minor
Flags: blocking2.20?
Flags: blocking2.20+
Flags: blocking2.18+
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Whiteboard: bug awaiting patch
So, Rudolf, any chance of updating this? :-)
I'm uploading the patch for 73665 tonight.

Gerv
(In reply to comment #17)
> It's not just for *new* users, it's also for users from a current 2.17+ base
> after we upgrade to 2.19+ - they have non-NULL email flags, but no
> "Flag"-specific email flags (because those didn't exist in 2.17+). 
> 
> And so, as things stand, they receive no Flag-related email at all (even though
> the Email preferences page claims they will, until they *set* their preferences
> explicitly).

It seems to me that this is actually a bug in the flag related email pref
parsing code (eg, it doesn't follow the rules we say it should on the editprefs
page) that is manifested by this particular issue. But, I suppose this is a
quicker/easier fix than fixing up the flags stuff.
No longer depends on: emailprefs
(In reply to comment #16)
> I have an updated patch for 73665 ready to go, so hold off on this one, OK? 
> :-)

<travis> justdave: is bug 73665 going to land before we get 2.18 shipped?
<justdave> yeesh, no way in hell. :)

Given that this bug is a 2.18 blocker, and bug 73665 has just been re-targeted 
for 2.22, I'm removing the dependson relationship between them. (This one never 
really 'depended on' that one anyway... more like that one would have rendered 
this one obsolete.)

I'll also take ownership of this bug, get Vlad's suggestions incorporated, and 
reload another patch so that it can land ASAP.
Assignee: preed → travis
Attached patch Code patch for 2.18 and tip (obsolete) — Splinter Review
This is an update of the original patch, only with the default email settings
broken out into a subroutine so that they are easier to read and modify if a
localizer wants to do so.

With Dave's permission, this patch also contains a fix for bug 109573; the
default setting for "emailXXXcontactCC" is ~~ (off), except for the Owner
(where it remains ~on~ by default).

Default setting for everything else is ~on~.
Attachment #166946 - Attachment is obsolete: true
Attachment #169236 - Flags: review?(vladd)
Blocks: 109573
Comment on attachment 169236 [details] [diff] [review]
Code patch for 2.18 and tip

>+    $defaultflagstring .= "emailReporterCC~~";

Does it have to have an empty ~~ for off or will ~off~ work?  It would be much
clearer if we could put ~off~ in there.
(In reply to comment #24)
> Created an attachment (id=169236) [edit]
> Code patch for 2.18 and tip

From what I can see, this still doesn't address the problem of existing users in
a 2.16/2.17+ installation that gets upgraded to 2.19+ (see comment #17).
Dave: looking at Bugzilla/User.pm, the $prefs hash is built up by very 
speficically looking for the word 'on' after the whole string is split on 
the '~' character, so I would assume that '~off~' would be ignored in the same 
manner as '~~' would be. I did one empirical test: I changed the 
profile.emailprefs string to '~off~' everywhere that it currently says '~~'. 
The Email tab of userprefs.cgi still showed them as being off, and changing a 
bug produced no bugmail for these categories.

You want I sould re-submit with everything set to ~off~, or should that just be 
changed on checkin?

Shankar: if I'm understanding you correctly, you've got a completely different 
(but no less valid) bug. What you mention is either an oversight in 
checksetup.pl (it should add default flag-based email parameters to everyone's 
profiles.emailprefs entry if said entry is non-empty, but has no flag 
mailprefs) *or* a bug in how the $prefs hash is set in User.pm for people in 
the above listed situation. 

Your bug sounds important, and it shouldn't be hidden by the other (also valid) 
issues that are being addressed here. Please confirm that what you say isn't 
already written up somewhere as another bug; if not, please enter a new one 
ASAP (and add me to cc: if you'd be so kind). I suspect that it will be another 
2.18 blocker.
Status: NEW → ASSIGNED
(In reply to comment #27)
> You want I sould re-submit with everything set to ~off~, or should that just
> be changed on checkin?

If it otherwise passes review, that's fine to fix on checkin.
Whiteboard: bug awaiting patch → patch awaiting review
(In reply to comment #27)

> Shankar: if I'm understanding you correctly, you've got a completely different 
> (but no less valid) bug. 

Good point. Bug 275599.
*** Bug 275599 has been marked as a duplicate of this bug. ***
Comment on attachment 169236 [details] [diff] [review]
Code patch for 2.18 and tip

>--- globals.pl	2004-11-25 23:43:07.000000000 -0600
>+++ globals.pl.emailprefs	2004-12-20 16:06:00.000000000 -0600
>@@ -403,6 +403,67 @@
>     return 1;
> }
> 
>+sub GetDefaultEmailSettings {

  I think this should be a variable inside of Bugzilla::Constants. You can
still build it the same way, for clarity, though. (You could even have a
private sub inside of Bugzilla::Constants build it the same way.)

  Other than that, this looks perfect to me if you assert that you've tested it
and it passes.
I have tested this: a minor variant of it has been running on our local system 
for about a year now.

I agree with Max's thoughts (comment #31) about putting it in Constants.pm, 
since there isn't really an interface to change it anyway... never thought of 
it in the time, but in retrospect it makes perfect sense. I'll produce a new 
patch with this change (and change ~~ to ~off~ as per comment #25) within the 
next couple of days.
Attached patch Code patch for 2.18 and tip (obsolete) — Splinter Review
This is the common code between the tip and 2.18 that needs to be changed for
this patch to work. In the last patch, any new account created by a user would
have had default email, but one created by the admin would not have. This is
now fixed here.

Also, when a user first visits userprefs, the defaults are set to the value in
Bugzilla/Constants.pm and not 'on' for everything. (This will only affect users
who have never set their e-mail before.)

Max, Vlad's busy... so you get to review this one. :)
Attachment #169236 - Attachment is obsolete: true
Attachment #169757 - Flags: review?(mkanat)
Attached patch Constants.pm changes for 2.18 (obsolete) — Splinter Review
Constants.pm changes for 2.18
Attachment #169758 - Flags: review?(mkanat)
Attached patch Constants.pm changes for tip (obsolete) — Splinter Review
2.18 patch doesn't quite apply cleanly for tip, so here's a tip patch. They are
identical in function, so one review will apply to both patches.
Attachment #169759 - Flags: review?(mkanat)
Comment on attachment 169757 [details] [diff] [review]
Code patch for 2.18 and tip

>-# The default email flags leave all email on.
>-my $defaultflagstring = "ExcludeSelf~on~";
>+# The default email flags are set in Bugzilla/Constants.pm
>+my $defaultflagstring = SqlQuote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS); 
> 
> my @roles = ("Owner", "Reporter", "QAcontact", "CClist", "Voter");
> my @reasons = ("Removeme", "Comments", "Attachments", "Status", "Resolved", 
>                "Keywords", "CC", "Other", "Unconfirmed");

  Bugzilla::User::email_flags has almost the same code in it, and also needs to
be updated.

  Other things looked good, on a basic glance, although I haven't tested it
yet.
Attachment #169757 - Flags: review?(mkanat) → review-
(In reply to comment #36)
>   Bugzilla::User::email_flags has almost the same code in it, and also needs to
> be updated.

  That should be "Bugzilla::user::email_prefs."
These patches make the same changes for User.pm as were made for userprefs.cgi,
and the same changes for checksetup.pl as were made for globals.

One thing that concerns me is that the code removed from User.pm and
userprefs.cgi both include a 'chop' to get rid of the last '~' character... and
I can't see any reason for doing this. I include this note here in hopes that
someone reading this is aware of why it was included, or can decipher the
original code better than I can to tell me.
Attachment #169788 - Flags: review?(mkanat)
Comment on attachment 169757 [details] [diff] [review]
Code patch for 2.18 and tip

In IRC Max indicated that there was nothing *wrong* with this patch per se --
merely that it wasn't complete. I've attached new patches to deal with that
objection, so I'm requesting a re-review of this patch.
Attachment #169757 - Flags: review- → review?(mkanat)
Attachment #169236 - Flags: review?(vladd)
(In reply to comment #38)
> One thing that concerns me is that the code removed from User.pm and
> userprefs.cgi both include a 'chop' to get rid of the last '~' character... and
> I can't see any reason for doing this. I include this note here in hopes that
> someone reading this is aware of why it was included, or can decipher the
> original code better than I can to tell me.

The original code for this was a mess. It functioned, for sure, but not
everything in it was neccessarily the best practice or entirely needed. The
email prefs patch went in at a time when the code scrutiney was much lower for
things going into Bugzilla. I'd say if everything seems to work fine, it's
probably not needed. You can take a peek at cvs blame to see if this chop() was
part of the original code or added to solve a problem if you're still unsure :).
Blocks: 275599
I'm going to review these now, but could you *please*, *please* make these all
one patch in the future? :-) In fact, in this case, if you add a few more lines
of context to the diff, one patch will apply on both 2.20 and 2.18, I believe.
Comment on attachment 169757 [details] [diff] [review]
Code patch for 2.18 and tip

>-    SendSQL("INSERT INTO profiles (login_name, realname, cryptpassword) 
>-             VALUES ($username, $realname, $cryptpassword)");
>+    SendSQL("INSERT INTO profiles (login_name, realname, cryptpassword, emailflags) 
>+             VALUES ($username, $realname, $cryptpassword, $defaultflagstring");

  This actually breaks account creation. :-) Note that there's a missing
close-paren at the end of the VALUES statement. (I do that all the time, when
I'm editing this stuff, too...)


>diff -u tip/userprefs.cgi tip.108870/userprefs.cgi
>--- tip/userprefs.cgi	2004-07-26 23:53:26.000000000 -0600
>+++ tip.108870/userprefs.cgi	2004-12-28 17:10:39.000000000 -0600
>@@ -35,22 +35,13 @@
> # Use global template variables.
> use vars qw($template $vars $userid);
> 
>-# The default email flags leave all email on.
>-my $defaultflagstring = "ExcludeSelf~on~";
>+# The default email flags are set in Bugzilla/Constants.pm
>+my $defaultflagstring = SqlQuote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS); 
> 
> my @roles = ("Owner", "Reporter", "QAcontact", "CClist", "Voter");
> my @reasons = ("Removeme", "Comments", "Attachments", "Status", "Resolved", 
>                "Keywords", "CC", "Other", "Unconfirmed");
> 
>-foreach my $role (@roles) {
>-    foreach my $reason (@reasons) {
>-        $defaultflagstring .= "email$role$reason~on~";
>-    }
>-}
>-
>-# Remove final "~".
>-chop $defaultflagstring;
>-


  My understanding of how this code is supposed to work is like this:

  If I have an existing user who has an empty (or NULL) emailprefs, then his
emailprefs should be set to the default as soon as he looks at the Email
settings table. (I know that this will also change in the other bug that you're
working on, so maybe this isn't relevant.) 

  When I actually try this, it looks like ExcludeSelf is set to on in the DB,
but it doesn't show up as being checked in the Email Settings pane. Is that
actually because of the other bug on this?
Attachment #169757 - Flags: review?(mkanat) → review-
Comment on attachment 169759 [details] [diff] [review]
Constants.pm changes for tip

This works as expected. However, PLEASE re-sbumit this as part of a *single*
patch when you re-work the other stuff. :-)
Attachment #169759 - Flags: review?(mkanat) → review+
Comment on attachment 169758 [details] [diff] [review]
Constants.pm changes for 2.18

I'm granting this review based on the fact that the other version looks good,
and this looks good, and travis claims that it applies cleanly.
Attachment #169758 - Flags: review?(mkanat) → review+
Also, this explains why the final tilde is chopped off:

[Thu Dec 30 14:46:23 2004] [error] [client 69.228.77.81] [Thu Dec 30 14:46:23
2004] userprefs.cgi: Odd number of elements in hash assignment at
/var/www/html/mkanat3/userprefs.cgi line 167., referer:
http://landfill.bugzilla.org/mkanat3/userprefs.cgi

You could just remove it from the DEFAULT_EMAIL_SETTINGS var.
Comment on attachment 169788 [details] [diff] [review]
Additional patches for checksetup.pl and User.pm (2.18 and tip)

>--- tip/Bugzilla/User.pm        2004-12-28 22:59:30.000000000 -0600
>+++ tip.108870/Bugzilla/User.pm 2004-12-28 22:58:19.000000000 -0600
>@@ -863,13 +863,7 @@
>     # column, so initialize that field with the default prefs.
>     if (!$flags) {
>         # Create a default prefs string that causes the user to get all email.
>-        $flags = "ExcludeSelf~on~FlagRequestee~on~FlagRequester~on~";
>-        foreach my $role (@roles) {
>-            foreach my $reason (@reasons) {
>-                $flags .= "email$role$reason~on~";
>-            }
>-        }
>-        chop $flags;
>+        $flags = SqlQuote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS);
>     }

  You know, it's funny... the checksetup changes that you're going to make to
set emailflags is going to make this code obsolete... ah well. :-)

  It works, though. (The only place this code is called right now is from
Flag.pm. This whole code needs big re-organization. It's good that Gerv is
doing it in that *other* patch that I have to review. :-) )

>     # Convert the prefs from the flags string from the database into
>--- tip/checksetup.pl   2004-12-28 22:59:30.000000000 -0600
>+++ tip.108870/checksetup.pl    2004-12-28 22:58:19.000000000 -0600

  And I tested this code, and it works, and it looks good.
Attachment #169788 - Flags: review?(mkanat) → review+
Comment on attachment 169788 [details] [diff] [review]
Additional patches for checksetup.pl and User.pm (2.18 and tip)

>+    # Set default email flags for the Admin, same as for users
>+    my $defaultflagstring = SqlQuote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS);

   Oh, but one nit: Instead of doing SqlQuote, use $dbh->quote or change the
$dbh->do statement to a prepare/execute pair and use placeholders.
Attached patch Code changes for 2.18, take3 (obsolete) — Splinter Review
Attachment #169757 - Attachment is obsolete: true
Attachment #169758 - Attachment is obsolete: true
Attachment #169759 - Attachment is obsolete: true
Attachment #169788 - Attachment is obsolete: true
Attachment #169966 - Flags: review?(mkanat)
Attached patch Code changes for tip, take3 (obsolete) — Splinter Review
K, here we go. One large patch for 2.18, and another for the tip. Try as I
might, they simply do not resolve into a single patch, so rather than break
them up into the common stuff/different stuff patches like I did above, here's
one patch for each branch... just to make Max happy. ;-)

In addition to fixing the nits and legitimate complaints pointed out above,
this latest patch also:
 * Incorporates the changes from bug 275599, so I can...
 * Remove about-to-be-redundant code that looks for empty emailflagprefs
 * Shift the ~ around in Constants.pm so that it reads more like the program
    parses it, and therefore self-explains why there is no ~ at the end.
 * Took the opportunity to properly indent a small section of checksetup.pl

For the record, this bug now incorporates bug 109573 and bug 275599; when it
lands successfully, both of those can be closed too.
Attachment #169967 - Flags: review?(mkanat)
Comment on attachment 169967 [details] [diff] [review]
Code changes for tip, take3

This patch doesn't apply cleanly to the tip, at least on my landfill
installation.

Also, it seems to have another patch inside of it -- the "admin inherited
groups" patch (that one I asked you to review for me).
Attachment #169967 - Flags: review?(mkanat) → review-
Comment on attachment 169966 [details] [diff] [review]
Code changes for 2.18, take3

Same here -- this does not apply cleanly to my 2.18-branch install.
Attachment #169966 - Flags: review?(mkanat) → review-
Guys,

I understand the following things to be true:

- We need to do a security release ASAP (because of bug 276605).
- It would be good if that release was 2.18 final as well.
- This is the major remaining bug for 2.18 final.
- If you check this patch in on the tip, it'll break my patch for 73665, which 
  would be annoying.

So here's what I suggest:

- You write and review a patch here for the 2.18 branch *only*, and check it in 
  ASAP. 
- We release 2.16.8, which has nothing to do with this patch
- We release 2.18 final, with this patch
- We release 2.19.2, without this patch
- Max and I try and get bug 73665 done before we branch for 2.20
- If we succeed, fine. If we fail, we revive this patch for the 2.20 branch 
  also, at that point.

This plan seems to me to be the quickest way to get 2.18 and the security
releases out of the door. What do you think?

Gerv
The security bug reference in my last comment should be bug 272620.

Gerv
(In reply to comment #52)
> - If you check this patch in on the tip, it'll break my patch for 73665
...
> So here's what I suggest:
> - Max and I try and get bug 73665 done before we branch for 2.20
...
> What do you think?
> Gerv

I think that when I spoke to Dave about 73665 (see comment #23), there was no 
way on God's Green Earth that it was making it in for 2.18. After that 
discussion Dave immediately went and re-targeted bug 73665 for 2.22, which 
tells me that he doesn't plan on it going into 2.20 either. This makes sense to 
me, since the cut-off for 2.20 enhancements passed several months back.

If Dave comes in and says that there is even a chance that 73665 will make it 
into 2.20, I'll go with your plan... but based on current information, it makes 
sense to go on the assumption that both 2.18 and 2.20 are going to need this 
patch. I'm good with his decision either way, though, so if you want to launch 
a campaign to convince him, I won't get in your way. :)

On the issue of the patch itself... I don't know why it didn't apply cleanly. 
:( I know that this is the last big 2.18 blocker, so it gets top priority from 
me. I'll have a new patch for both 2.18 and tip (the latter of which can be 
ignored If Dave Says So) within 24 hours of this writing... sooner if I can 
manage it.
Shane: Dave said in bug 73665, comment 56:

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

Gerv
I agree that this patch should only be checked-in on the 2.18 branch unless we
don't get bug 73665 in before we *release* 2.20.

We will be *branching* for 2.20 *immediately* after we release 2.18, as far as I
understand. We have *lots* of cool stuff that we want to check in to the trunk
that we can't until we branch.
(In reply to comment #56)
> I agree that this patch should only be checked-in on the 2.18 branch unless we
> don't get bug 73665 in before we *release* 2.20.

I don't want 73665 on the 2.20 branch.  It's frozen.
(In reply to comment #50)
> (From update of attachment 169967 [details] [diff] [review] [edit])
> This patch doesn't apply cleanly to the tip, at least on my landfill
> installation.

Well, I dunno what to say. I just checked out a fresh, clean copy of the tip, 
and it applied perfectly. (Two 'offset 1 line' comments, but that's it.) Maybe 
your landfill copy is the issue?

> Also, it seems to have another patch inside of it -- the "admin inherited
> groups" patch (that one I asked you to review for me).

I believe what you're seeing is part of what I wrote in comment #49:

In addition to fixing the nits and legitimate complaints pointed out above,
this latest patch also:
 * Took the opportunity to properly indent a small section of checksetup.pl

It wasn't done properly (and hadn't been long before you ever touched it), so 
since I was fixing stuff right in that area I fixed that up too.

No point me uploading a new patch (for the tip) since there's nothing wrong 
with this one. Please re-review.

Attached patch Code changes for 2.18, take4 (obsolete) — Splinter Review
kay, sorry, buggered up the 2.18 patch. Not sure what happened with it, but I
just checked out a clean version of 2.18 and this one definitely applies.

Fixes more than just nits on the original; see comment #49 for full
description.
Attachment #169966 - Attachment is obsolete: true
Attachment #170213 - Flags: review?(mkanat)
(In reply to comment #57)
> I don't want 73665 on the 2.20 branch.  It's frozen.

... and it was already frozen (for about two months) when you made bug 73665, 
comment 56 too, so I can see where the confusion is coming from; you do seem to 
be saying one thing on this bug and another thing on that one.

Anyway, there's a tip patch here that can be applied or not applied; no skin 
off my ego either way. It's effectively identical to the 2.18 patch, so not 
like it needs extra reviewing either.


Comment on attachment 169967 [details] [diff] [review]
Code changes for tip, take3

OK, then I'll set it back to be reviewed again, and try it one more time.

It could just be some confusion caused by the fact that you don't use "cvs
diff" (which would make life a lot easier, particularly because I'd then be
able to use the "File" mode of PatchReader on b.m.o, which makes reviews much
easier sometimes.)
Attachment #169967 - Flags: review- → review?(mkanat)
Comment on attachment 169967 [details] [diff] [review]
Code changes for tip, take3

  OK, this patch itself does not apply, but travis sent me a nearly-identical
one through email which *does* apply. So I'm going to review this and then he
can attach the actually working patch.
>         # If the user prefers to be included in mail about this change,
>-        # or they haven't specified a preference for it (because they
>-        # haven't visited the email preferences page since the preference
>-        # was added, in which case we include them by default), add them
>-        # to the list of recipients.
>+        # add them to the list of recipients.
>         foreach my $reason (@$reasons) {
>             my $pref = "email$role$reason";
>             if (!exists($prefs{$pref}) || $prefs{$pref} eq 'on') {

  I just noticed  that !exists($prefs{$pref}) -- is that going to cause a
problem with our setting it to "off"?

  I think that maybe we shouldn't do the "off" thing -- it's different from how
we used to work, and this is going into the frozen 2.18 branch.

>+      "~emailReporterResolved~on" .
>+      "~emailReporterKeywords~on" .
>+      "~emailReporterCC~off" .

  That is, this is what I'm talking about, in case it isn't clear. The original
coder expected, I believe, that non-existent prefs would just be undefined, not
"off."

>-    $dbh->do("INSERT INTO profiles (login_name, realname, cryptpassword)" .
>-            " VALUES ($login, $realname, $cryptedpassword)");
>+    # Set default email flags for the Admin, same as for users
>+    my $defaultflagstring = $dbh->quote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS);
>+
>+    $dbh->do("INSERT INTO profiles (login_name, realname, cryptpassword, emailflags) " .
>+             "VALUES ($login, $realname, $cryptedpassword, $defaultflagstring)");
>+  }

  OK, brief explanation of why unrelated whitespace changes should not go into
a patch:

  When we check something in to CVS, it goes with a message stating the bug
number. When I look at CVS Blame in Bonsai, I can see this message attached to
each and every line. It will become really confusing who wrote this code and
what it was for if we check-in a big whitespace change along with this patch.

  So you can file another bug just for that whitespace change, and I'll
definitely give you a review+ on it very quickly.

>+# 2004-12-29 - Flag email code is broke somewhere, and doesn't treat a lack
>+# of FlagRequestee/er emailflags as 'on' like it's supposed to. Easiest way
>+# to fix this is to make sure that everyone has these set. (bug 275599).
>+# While we're at it, let's make sure everyone has some emailprefs set,
>+# whether or not they've ever visited userprefs.cgi (bug 108870). In fact,
>+# do this first so that the second check gets fewer hits.
>+$sth = $dbh->prepare("SELECT userid FROM profiles " .
>+                     "WHERE emailflags like \"\" ");

  Why LIKE "" instead of = ''? Also, you can't use "" for quoting text in a
cross-db, you need to use ''. (Unless I'm misunderstanding something here.)

>+$sth->execute();
>+while (my ($userid) = $sth->fetchrow_array()) {
>+    $dbh->do("UPDATE profiles SET emailflags = " .
>+             $dbh->quote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS) .
>+             "WHERE userid = $userid");
>+}

  I think you'll want to output a message before this, just like the rest of
checksetup does when it outputs something. (Note that you also have to check if
the quiet flag has been passed to checksetup -- just look at how it outputs
other messages.)

>+$sth = $dbh->prepare("SELECT userid, emailflags FROM profiles " .
>+                     "WHERE emailflags not like \"%Flagrequeste%\" ");

  Same there -- use '', not "". Also, capitalize NOT LIKE and LIKE in both of
these statements.

>+$sth->execute();
>+while (my ($userid, $emailflags) = $sth->fetchrow_array()) {
>+    $emailflags .= Bugzilla::Constants::DEFAULT_FLAG_EMAIL_SETTINGS;
>+    $emailflags = $dbh->quote($emailflags);
>+    $dbh->do("UPDATE profiles SET emailflags = $emailflags " .
>+             "WHERE userid = $userid");
>+}

  You'll want to find some clever way to output a message there, too. You'll
want to only output the message (this is true for both of them) if something
actually has to be changed.

  I looked over all the other code, and it looks correct. I'll also do some
testing, but in the mean time put up a new patch to address the points above.
Attachment #169967 - Flags: review?(mkanat) → review-
Attachment #170213 - Flags: review?(mkanat)
Comment on attachment 169967 [details] [diff] [review]
Code changes for tip, take3

>  I think that maybe we shouldn't do the "off" thing -- it's different 
> from how we used to work, and this is going into the frozen 2.18 branch.

I would bet significant portions of my anatomy that it isn't going to cause any
trouble... but your point is nonetheless a valid one. We know that "~" works,
because it *has* been working. Given that we're trying to get this in quickly,
let's change as few things as possible.

To that end, I've added a comment to Constants.pm for customization
explanation.


> OK, brief explanation of why unrelated whitespace changes should not go
> into a patch:

Whitespace changes removed.


> Why LIKE "" instead of = ''? 

Force of habit; cut-n-paste from somewhere else that I knew worked. (I think in
that place it needed to be careful of ' characters.)


> You'll want to find some clever way to output a message there, too.

Messages displayed for both, but only if needed.


And, joy of joys... this patch applies to both the tip and 2.18, so I can just
do one patch again! Yay!
Attachment #169967 - Attachment is obsolete: true
Attachment #170213 - Attachment is obsolete: true
Attachment #170299 - Flags: review?(mkanat)
Comment on attachment 170299 [details] [diff] [review]
Code patch for 2.18 and tip, take 5

  OK, now all the code looks good, time for testing.

>+# 2004-12-29 - Flag email code is broke somewhere, and doesn't treat a lack
>+# of FlagRequestee/er emailflags as 'on' like it's supposed to. Easiest way
>+# to fix this is to make sure that everyone has these set. (bug 275599).
>+# While we're at it, let's make sure everyone has some emailprefs set,
>+# whether or not they've ever visited userprefs.cgi (bug 108870). In fact,
>+# do this first so that the second check gets fewer hits.
>+# 
>+my $emailflags_count = 0;
>+$sth = $dbh->prepare("SELECT userid FROM profiles " .
>+                     "WHERE emailflags LIKE '' ");

  I just discovered that this needs a "WHERE emailflags = '' OR emailflags IS
NULL". Most of them are NULL in my DB.


  Everything else works properly. Here's what I tested:

  1) Creating an admin user with checksetup -- default flags are properly set
in DB and on Email Settings of userprefs.cgi.
  2) Creating a new user with the admin interface -- default flags are properly
set in DB and on Email Settings of userprefs.cgi.
  3) Creating a user through the 'New Account' interface -- default flags are
properly set in DB and on Email Settings of userprefs.cgi.

  I believe that user creation through LDAP works the same way, so there should
be no problem there.
Attachment #170299 - Flags: review?(mkanat) → review-
Attachment #169758 - Flags: review+
Attachment #169759 - Flags: review+
Attachment #169788 - Flags: review+
(In reply to comment #64)
>   Everything else works properly. Here's what I tested:

I don't see anything explicit about testing checksetup.pl itself to ensure that 
it's properly modifying existing users (both those with empty and those who are 
just missing flag prefs) ... but your addition of "IS NULL" makes me think that 
you did. Care to confirm?

(Final?) Patch, with the above issue fixed, is ready to go if that all you say 
is needed. Give the word.
(In reply to comment #65)

> I don't see anything explicit about testing checksetup.pl itself to ensure that 
> it's properly modifying existing users (both those with empty and those who are 
> just missing flag prefs) ... but your addition of "IS NULL" makes me think that 
> you did. Care to confirm?

  Confirmed. I did test checksetup's adding of emailflags. I just forgot to add
it to the test list, since it failed.
Only modification is to add the IS NULL check from comment #64.
Attachment #170370 - Flags: review?(mkanat)
Attachment #170299 - Attachment is obsolete: true
Comment on attachment 170370 [details] [diff] [review]
Code patch for 2.18 and tip, take 6

MUCH better:

Added default email prefs to 2402 users who had none.
Added default Flagrequester/ee email prefs to 568 users who had none.
Attachment #170370 - Flags: review?(mkanat) → review+
Flags: approval?
Flags: approval2.18?
Whiteboard: patch awaiting review → patch awaiting approval
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Whiteboard: patch awaiting approval → patch awaiting checkin
fwiw, I ran this on a copy of bmo's database earlier today...

the checksetup.pl run took 1 minute 20 seconds to run:

Added default email prefs to 142849 users who had none.
Added default Flagrequester/ee email prefs to 24068 users who had none.

I spot-checked a few accounts at random that were previously missing settings,
and things look sane.

It added approximately 200M to the size of the database.  Considering it was
already 5.2 GB that's probably not a big deal. :)
2.18:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.19; previous revision: 1.289.2.18
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.61.2.5; previous revision: 1.61.2.4
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.270.2.5; previous revision: 1.270.2.4
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.58.2.4; previous revision: 1.58.2.3
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.13.2.3; previous revision: 1.13.2.2
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.10.2.2; previous revision: 1.10.2.1
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.20.2.2; previous revision: 1.20.2.1
done


Tip:
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.321; previous revision: 1.320
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.70; previous revision: 1.69
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.280; previous revision: 1.279
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.62; previous revision: 1.61
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.19; previous revision: 1.18
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.32; previous revision: 1.31
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: patch awaiting checkin
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.