71798 - bugmail not sent when moving from cc to assigned fields

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Mark Olson]

timeless reassigned to me, added ducarroz to cc, and commented on the referenced
bug, and I didn't get any bugmail.  My prefs are basically set to only ignore
cc-only changes, so I should have gotten mail.

per timeless, cgi feedback was:
<timeless>  Email sent to: [e-mail], [e-mail],
            [e-mail], [e-mail]
<timeless>  Excluding: [e-mail], [e-mail],
            [e-mail]

Comment 1

[Stephan Niemz]

Fix is already checked in, but I don't know if it's active yet.

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

Comment 2

[Jacob Steenhagen]

Looking at the times associated w/this, I don't think this is really a dupe.

The fix for bug 71600 was checked in on 3/11 at 12:08 (v1.54 of processmail)
timeless made the change on 3/12 at 21:38

bugzilla.mozilla.org updated to the tip twice between those two times.

According to the activity log, timeless added [e-mail] (previous
owner) to the CC and reassigned the bug to [e-mail] (thus changing
the status from ASSIGNED to NEW).

[e-mail] was previously CC'd on this bug so at this point he was
both the owner and on the CC list... I don't know if this makes a difference.

Comment 3

[Jacob Steenhagen]

Intersting enough, when I REOPENED this, [e-mail] was the only
person to be excluded.  I added a CC, changed Status, and added a comment.
Mark's only association w/this bug is being the reporter.

Comment 4

[Jacob Steenhagen]

When all I did was add a comment, Mark got the mail (according to processmail,
at least).

Comment 5

[Mark Olson]

This must have something to do with status or cc changes, since those are the
common denominators between the two instances where I didn't get bugmail that I
should have.  Let me know if there's anything I can do to help test one or both
of these scenarios.  For now at least, I'll leave my prefs alone so that doesn't
affect things.

Comment 6

[Ben Bucksch (:BenB)]

scaring... (this is also a test).

Comment 7

[Mark Olson]

I saw this again in bug 71845 where timeless cc'd me and added comments.

Comment 8

[Stephan Niemz]

I think this bug should be reconsidered for 2.12 since it it very visible,
judging by the comments, and it makes the part of the new email filtering feature
that most people would use (filter out Cc-only changes) basically useless.

Comment 9

[Mark Olson]

Is anyone working on independantly confirming this?  If so, I would be happy to
help test.  I'd like to know if I'm just a loser or if this is a more widespread
issue.

I would doubt this is only me, but it would be nice to be sure.  If it isn't
just me, it would be nice to get a note out to the newsgroups letting people
know there might be instances of lost bugmail so that they can choose their
settings with a knowledge of any potential risk.

Comment 10

[Stephan Niemz]

Gosh, this email filtering code is really heavy stuff.  But I think I found
the bug.

The logic for the status change is slightly wrong.  If there is a status
change, the change is treated as if there was no comment made, thus no
email is sent to anybody who wants to filter out CC-only or Status-only
changes:
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/processmail#689

Patch is coming.  I have also made a few other changes which helped me
understand the code and/or should improve performance (break out of the
loop in line 804 (line 821 before patching) if the condition we're looking
for is met).  Feel free to omit them if you don't like them.

I also omitted the "catch-all default, just in case the above logic is
faulty" at
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/processmail#707
because (1) if the logic is wrong, that's a bug and should be fixed and
(2) this workaround doesn't help, as can be seen in this bug.  Maybe the
author should trust his own code more. :)  Or maybe he already stumbled
across this bug (because he tried to add the comment-change flag here in
some cases) and made this as a workaround.

I haven't tested it yet, hope I find time for that later.

Comment 11

[Stephan Niemz]

Attachment: patch (diff -upbB for improved human readability)

Comment 12

[Stephan Niemz]

Attachment: patch (diff -u, the "real thing")

Comment 13

[Dave Miller [:justdave]]

What in this patch is supposed to fix this?  All I see is a bunch of reformatted 
"if" blocks, where you changed it to list the action first and then the 
condition, instead of the other way around.  Although it *does* make it look 
prettier, it makes it really hard to find what you're actually fixing in here 
when reviewing the patch.  And it also makes the code harder to follow for people 
who aren't native to Perl.  If you think the formatting changes to the "if" 
statements are important, I'd suggest making that part a separate patch and 
filing a new bug for it.

Can we have a new patch with just what's being fixed here?

Comment 14

[Jacob Steenhagen]

Attachment: Patch v2

Comment 15

[Jacob Steenhagen]

The patch v2 I just attached is cleaner in that it doesn't have any changes
other than the fix for this bug.  It also retains the logic that a comment
doesn't count if the only thing changing is that the bug is being RESOLVED or
VERIFIED.

Comment 16

[Gervase Markham [:gerv]]

Could I just add my vote for not liking 
Foo if Bar
? I much prefer 
if (Bar) Foo
. :-)

Gerv

Comment 17

[timeless]

you just did, but if (bar) foo; #doesn't work unless foo is a {block}
so it'd have to be if (bar) {foo;} #would work

sometimes i like foo if bar; #but i try to avoid mixing style changes w/ real 
code changes.

Comment 18

[Chris Yeh]

checked in a variation of jake's patch.

Comment 19

[Dawn Endico]

updated b.m.o

Comment 20

[Dave Miller [:justdave]]

Moving closed bugs to Bugzilla product