146261 - Email not sent to users on CC list under some Status field transitions

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Tim Taylor]

I'm using a CVS snapshot of Bugzilla updated on 22 May 2002, around 12:00 EST.

I have a user with the following email settings:
- Only email reports of changes by others                                      
          
- All relationships for 'Priority, status, severity, and/or milestone changes'
- All relationships for 'The bug is resolved or verified'
All other email settings are turned off.

This user is on the CC list for a bug.  When the bug is RESOLVED, the user gets
email.  If the bug is then CLOSED, they don't get email, but should since the
status field has changed.  If the bug is then REOPENED they also don't get
email, but should for the same reason.

I have a patch for this problem against revision 1.81 (the head at the time of
this report) of processmail which I will try to attach to this bug after
submitting it.

Comment 1

[Tim Taylor]

Attachment: Sends email on Status field changes other than RESOLVED

This is a patch against CVS revision 1.81 of the file processmail.  This was
the HEAD revision at the time this bug was posted.

Comment 2

[Bradley Baetz (:bbaetz)]

myk's being playing with the filtering code recently. This looks right to me,
though, and could explain some of the odd bugmails I occasio9nally get from
bugzilla when bugs are reopened.

Comment 3

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

Comment on attachment 84669 [details] [diff] [review]
Sends email on Status field changes other than RESOLVED

Looks good, r=myk.

It would be useful to note somewhere that 'The bug is resolved 
or verified' overrides 'Priority, status, severity, and/or 
milestone changes', since it isn't obvious why a user with 
the former unchecked and the latter checked doesn't get mail 
when the status changes to resolved or verified.

This patch doesn't conflict at all with the filter changes
I have been doing over in bug 128469.

Comment 4

[Bradley Baetz (:bbaetz)]

Hang on a sec. Why are these elsif's, then? Shouldn't they all be separate, so
that if any of the things are true, we send mail?

Comment 5

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

No, because the code containing the elsifs runs once per change, so no change
matches more than one condition.  The only exception is status changes to
resolved or verified, which match both the 'The bug is resolved or verified' and
the 'Priority, status, severity, and/or milestone changes' preferences.

Since the former preference is more specific than the latter, however, it should
override it, so the elsifs work in that situation as well (but this should be
documented).

Comment 6

[Bradley Baetz (:bbaetz)]

I realise that they do match once per condition, but I'm asking why they do that.

Comment 7

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

They do it by definition.  Every email preference matches different fields
(modulo previously discussed Status overlap).  Every iteration of the loop
examines a change to one field.  Thus by definition it will never be the case
that more than one "elsif" condition is true for each iteration of the loop. 
Thus it makes no sense to separate these conditions from each other so that all
of them get run; the first matching condition automatically excludes the
possibility that other conditions match.

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 84669 [details] [diff] [review]
Sends email on Status field changes other than RESOLVED

r=bbaetz, then

Comment 9

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

Tim, do you have check-in privileges, or do you need someone to check this in
for you?

Comment 10

[Tim Taylor]

I don't have check-in privileges, so if someone could check my patch in for me
that would be great.

Comment 11

[Andreas Franke (gone)]

If this bug is present on the 2.16 branch also, then it should be fixed there
for 2.16, too, since this is a correctness issue. If this is the case and you
agree, please adjust the target milestone.

Comment 12

[Bradley Baetz (:bbaetz)]

Only regressions should be going onto the 2.16 branch at this point. Is this a
regression?

Comment 13

[Andreas Franke (gone)]

Not sure if this is a regression. Anyway, if it's not gonna make the branch then
you need a release note like this "There is a known bug that causes you to not
get bugmail with certain email settings. To fix this, apply the patch from bug
146261." Not sure what is better: to have such a release note, or to check in
the fix. :-)

Comment 14

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

At this point, it's probably to add to the release notes, which is bug 97496.

Comment 15

[Andreas Franke (gone)]

Request for release note submitted to bug 97496. Note that I still think this is
(at least temporary) dataloss because you are not getting notified of possibly
important changes. (And way more important than html compliance and such ;-)

Comment 16

[Matthew Tuck [:CodeMachine]]

Myk please file a bug on making the wording on those prefs more specific as you
suggest.

Comment 17

[Matthew Tuck [:CodeMachine]]

I'm trying to write a release note for this, but it's not obvious what the bug
is here specifically.

Is it that 'Priority, status, severity, and/or milestone changes' is ignored? 
Or just in certain situations?  Or doesn't really apply to statuses?  Or what?

Apparently this is true for 2.14 too so I'll release note it for 2.14.2 too.

Comment 18

[Tim Taylor]

The bug is that there are two email notification categories that overlap: 
"Priority, status, severity, and/or milestone changes", and "The bug is resolved 
or verified".  Prior to my patch, if the status field of a bug changed from 
CLOSED to REOPENED, NO email notification was being sent.  With my patch, email 
will be sent in this situation.

The information that needs to go in the release notes is to explain a 
side-effect of the patch.  If I've registered to receive email when "Priority, 
status, severity, and/or milestone changes", but not for "The bug is resolved or 
verified", I won't get an email when a bug is "resolved or verified" even though 
the status field has changed.

After submitting my patch and watching the discussion that followed, I've begun 
to think that email should really go to both groups of people in the case I 
described above.  Then there wouldn't be a need for an explanation in the 
release notes.

If people concur, I'll make a new version of the patch.

Comment 19

[Matthew Tuck [:CodeMachine]]

OK, so the basic problem was that status changes were not governed by the option
that claimed to govern it, but rather the resolution/verification one.  And the
patch that is included changes it so all status changes are governed except to
RESOLVED and VERIFIED.

Comment 20

[Andreas Franke (gone)]

Matty, are you sure? Isn't it that without the patch, any status changes except
resolved and verified didn't trigger at all? And that nobody else noticed it
only because most people have other flags set as well (e.g. comments) and
usually status changes are accompanied with comments?

Comment 21

[Matthew Tuck [:CodeMachine]]

That's what I meant.

Comment 22

[Bradley Baetz (:bbaetz)]

-> patch author.

Comment 23

[Bradley Baetz (:bbaetz)]

Myk fixed this with:

revision 1.82
date: 2002/05/23 08:08:53;  author: myk%mozilla.org;  state: Exp;  lines: +2 -4
Fix for bug 146261: fixes bug preventing the sending of email to users when the
status of bugs changes in some situations.
Patch by Tim Taylor <[e-mail]>.
r=myk,bbaetz

but didn't mark the bug as fixed

Comment 24

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

D'oh!

Comment 25

[Randall M! Gee]

I concur with comment #18 from Tim Taylor.  When a bug gets RESOLVED or
CLOSED, bug mail should go out to people who want to be notified of status
changes, whether or not they also signed up to get "resolved or closed"
mail.

It's a lot more logical.  You're doing it the way people expect, instead
of doing it the way people don't expect and documenting it as a feature.

It's any easy fix, too: just change
"elsif ( $fieldName eq 'Severity' || ..." to 
"   if ( $fieldName eq 'Severity' || ..." and you're done!  (Well,
you should also change the comments above, as well.)

Comment 26

[Bradley Baetz (:bbaetz)]

I STR that the customised resolution stuff will split those sorts of status
changes up.

Comment 31

[Wisdomjobs.com]

(In reply to Andreas Franke (gone) from comment #15)
> Request for release note submitted to bug 97496. Note that I still think
> this is
> (at least temporary) dataloss because you are not getting notified of
> possibly
> important changes. (And way more important than html compliance and such ;-)

bug 1328087  also resolved