Closed Bug 946565 Opened 11 years ago Closed 10 years ago

Bug Mail does not correct notify when two people have made changes

Categories

(Bugzilla :: Email Notifications, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: mail, Assigned: mail)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch mail-v1.patch (obsolete) — Splinter Review
There are cases where Bugzilla sometimes does not send out notifications (but not under normal operation). When the next change is made, the previous change and current change is attributed to the user making the most recent change.

(The publicly available BMO snapshot had 30 cases of this, brc a lot more)
Attachment #8342849 - Flags: review?(glob)
Comment on attachment 8342849 [details] [diff] [review]
mail-v1.patch

>=== modified file 'template/en/default/email/bugmail.txt.tmpl'

>+[%+ IF isnew %]
>+[%+ END %]

Huh?? An empty block?
(In reply to Frédéric Buclin from comment #1)
> Comment on attachment 8342849 [details] [diff] [review]
> mail-v1.patch
> 
> >=== modified file 'template/en/default/email/bugmail.txt.tmpl'
> 
> >+[%+ IF isnew %]
> >+[%+ END %]
> 
> Huh?? An empty block?

No. It prints a new line. I couldn't think of a better way of doing it.
(In reply to Simon Green from comment #2)
> No. It prints a new line. I couldn't think of a better way of doing it.

Simply leave an empty line in the template. That's how we do it currently.
Attached patch mail-v2.patch (obsolete) — Splinter Review
Attachment #8342849 - Attachment is obsolete: true
Attachment #8342849 - Flags: review?(glob)
Attachment #8343393 - Flags: review?(glob)
Comment on attachment 8343393 [details] [diff] [review]
mail-v2.patch

>=== modified file 'template/en/default/email/bugmail.txt.tmpl'

>   [% urlbase %]show_bug.cgi?id=[% bug.id %]
>-
> [%+ last_changer = 0 %]
>+[% IF isnew %]
>+
>+[% END %]

No. I meant to leave the original blank line above alone. There is no need to enclose it between [% IF isnew %] ... [% END %] and it's unrelated to this bug, AFAIK.
(In reply to Frédéric Buclin from comment #5)
> Comment on attachment 8343393 [details] [diff] [review]
> mail-v2.patch

> No. I meant to leave the original blank line above alone. There is no need
> to enclose it between [% IF isnew %] ... [% END %] and it's unrelated to
> this bug, AFAIK.

I disagree. In order for the plain text e-mails to look good, it was necessary to add a line between the different blocks in the diff part of the e-mail (line 80 of the patch). But this meant that Template would create two blank lines between the URL at the top (line 62 in patch file) and the person making the change (line 79/81). That is why I only want to print a blank line for new e-mails.
(In reply to Frédéric Buclin from comment #3)
> (In reply to Simon Green from comment #2)
> > No. It prints a new line. I couldn't think of a better way of doing it.
> 
> Simply leave an empty line in the template. That's how we do it currently.

[%+ +%]
Blocks: 821070
No longer blocks: 821070
Comment on attachment 8343393 [details] [diff] [review]
mail-v2.patch

r=glob
Attachment #8343393 - Flags: review?(glob) → review+
Flags: approval?
Flags: approval4.4?
Target Milestone: --- → Bugzilla 4.4
(In reply to David Lawrence [:dkl] from comment #7)
> [%+ +%]

I *much* prefer dkl's solution above than the empty block. It doesn't make sense and will disappear the next time someone edits this template (how are you supposed to know why there is an empty block in a template??). Could you attach a new patch, please?
Attached patch mail-v3.patchSplinter Review
Attachment #8343393 - Attachment is obsolete: true
Attachment #8346381 - Flags: review?(glob)
Comment on attachment 8346381 [details] [diff] [review]
mail-v3.patch

redirecting to lpsolit as he raised objections to the last patch.
Attachment #8346381 - Flags: review?(glob) → review?(LpSolit)
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Wait, I haven't reviewed your patch yet.
Flags: approval4.4+
Flags: approval+
Comment on attachment 8346381 [details] [diff] [review]
mail-v3.patch

>=== modified file 'template/en/default/email/bugmail.txt.tmpl'

>   [% urlbase %]show_bug.cgi?id=[% bug.id %]
>-
> [%+ last_changer = 0 %]
>+[% IF isnew %]
>+[%+ +%]
>+[% END %]

Revert these changes and the UI will be correct. There was no empty line before [%+ last_changer = 0 %] on purpose, to avoid your empty block.
Attachment #8346381 - Flags: review?(LpSolit) → review-
Comment on attachment 8346381 [details] [diff] [review]
mail-v3.patch

Hum ok, while doing more testing, I saw the problem you described. I cannot find a better solution either. So r=LpSolit
Attachment #8346381 - Flags: review- → review+
Flags: approval?
Flags: approval4.4?
Will commit change next week, unless someone wants to do it earlier.
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Blocks: 821070
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.4/                         
modified Bugzilla/BugMail.pm
modified template/en/default/email/bugmail.html.tmpl
modified template/en/default/email/bugmail.txt.tmpl
Committed revision 8645.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/BugMail.pm
modified template/en/default/email/bugmail.html.tmpl
modified template/en/default/email/bugmail.txt.tmpl
Committed revision 8845.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: