Templatise 'whinemail' email

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Email Notifications
P3
enhancement
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Emmanuel Seyman)

Tracking

2.19.1
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

13 years ago
The 'whinemail' param should be a template instead of a param.

I'm not sure how important this is with the new whine re-write, but we still
have that param, so something's got to be done about it.

See the patches on bug 84876 for hints and possible implementations.
Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.
Assignee: justdave → email-notifications
QA Contact: mattyt-bugzilla → default-qa
(Reporter)

Updated

13 years ago
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.22
(Reporter)

Updated

13 years ago
Assignee: email-notifications → eseyman
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Depends on: 215148
(Assignee)

Comment 2

13 years ago
Created attachment 183746 [details] [diff] [review]
remove whinemail from params and use the file whine/news.txt.tmpl instead
Attachment #183746 - Flags: review?(erik)
(Assignee)

Comment 3

13 years ago
Created attachment 183747 [details]
template to be used by whineatnews.pl
Attachment #183747 - Flags: review?(jouni)

Comment 4

13 years ago
Comment on attachment 183747 [details]
template to be used by whineatnews.pl

I can't review this separately from the cgi change in general, so you should
get Erik's comments when he takes a look at the code itself.

Here are some suggestions:

1. Add template interface documentation (see
template/en/default/admin/table.html.tmpl for an example, we have plenty of
these)

>To: [% email %][%Param("emailsuffix") %]

Nit: Space before Param.

>Subject: Your [% terms.Bugzilla %] buglist needs attention.

Hmm... In case many separate whines are set up, should we have different
subjects for them? Should we generate certain headers for filtering? Well
actually, should we support separate templates for each whine anyway?

>You have one or more [% terms.bugs %] assigned to you in the [% terms.Bugzilla %]
>bugsystem ([% Param("urlbase") %]) that require
>attention.

Suggestion: Rephrase to avoid "bugsystem".

>(2) You decide the [% terms.bug %] doesn't belong to you, and you reassign it to someone

Avoid lines this long. Although the standard substitution of "bug" keeps it at
76 chars, even a bit longer word will make the line wrap.
Attachment #183747 - Flags: review?(jouni) → review?(erik)
(Assignee)

Comment 5

13 years ago
(In reply to comment #4)
> 
> Hmm... In case many separate whines are set up, should we have different
> subjects for them? Should we generate certain headers for filtering? Well
> actually, should we support separate templates for each whine anyway?

Moving the mail templates from the parameters to the /templates directory is the
first step towards making them user-configurable. It also makes sense from an
i18n point of view.

See comments in bug 268577 for détails.

> Suggestion: Rephrase to avoid "bugsystem".

"bug tracking system"?

I'll submit a new patch soon.
(Assignee)

Updated

12 years ago
Attachment #183746 - Flags: review?(erik)

Updated

12 years ago
Whiteboard: [patch awaiting review]
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24

Comment 6

12 years ago
Comment on attachment 183746 [details] [diff] [review]
remove whinemail from params and use the file whine/news.txt.tmpl instead

just to make things clear: defparams.pl no longer exists.
Attachment #183746 - Flags: review-

Comment 7

12 years ago
Comment on attachment 183747 [details]
template to be used by whineatnews.pl

manu, could you update this patch? And write a single one instead of splitting it into two pieces?
Attachment #183747 - Flags: review?(erik)

Updated

12 years ago
Whiteboard: [patch awaiting review]
(Assignee)

Comment 8

12 years ago
The new version is almost done.
I should be able to make it public by the end of the week.
(Assignee)

Comment 9

12 years ago
Created attachment 218016 [details] [diff] [review]
Move the contents of the whinemail param to a template file and update code
Attachment #183746 - Attachment is obsolete: true
Attachment #183747 - Attachment is obsolete: true
Attachment #218016 - Flags: review?
Related note (not this bug, though): we need to make sure that we remember the last Accept-Language HTTP header with a user, so that we may send whine mails in the user's language.
(Assignee)

Comment 11

12 years ago
(In reply to comment #10)
> Related note (not this bug, though): we need to make sure that we remember the
> last Accept-Language HTTP header with a user, so that we may send whine mails
> in the user's language.

https://bugzilla.mozilla.org/show_bug.cgi?id=275637 (comments 2 and onwards)
https://bugzilla.mozilla.org/show_bug.cgi?id=297186

Comment 12

12 years ago
Comment on attachment 218016 [details] [diff] [review]
Move the contents of the whinemail param to a template file and update code

Bugzilla/Config/MTA.pm must be fixed too.
Attachment #218016 - Flags: review? → review-
(Assignee)

Comment 13

12 years ago
Created attachment 218277 [details] [diff] [review]
Include Bugzilla/Config/MTA.pm in the files affected

(In reply to comment #12)
>
> (From update of attachment 218016 [details] [diff] [review] [edit])
> Bugzilla/Config/MTA.pm must be fixed too.

Taking into account, which allowed me to fix a few cosmetic errors I found.
Asking for review again.
Attachment #218016 - Attachment is obsolete: true
Attachment #218277 - Flags: review?(LpSolit)

Comment 14

12 years ago
Comment on attachment 218277 [details] [diff] [review]
Include Bugzilla/Config/MTA.pm in the files affected

>+++ ./template/en/default/email/whine.txt.tmpl	2006-04-11 11:00:45.000000000 +0200

>+[% PROCESS global/variables.none.tmpl %]
>+[% PROCESS "global/field-descs.none.tmpl" %]

Nit: field-descs.none.tmpl already calls variables.none.tmpl. Remove the first line on checkin.


>+All of these [% terms.bugs %] are in the NEW or REOPENED state, and have not

Marc, shouldn't we write these statuses as [% status_descs.NEW FILTER html %] and [% status_descs.REOPENED FILTER html %]? That's the reason I ask you for review. Else the patch looks good and works fine. r=LpSolit
Attachment #218277 - Flags: review?(wurblzap)
Attachment #218277 - Flags: review?(LpSolit)
Attachment #218277 - Flags: review+
(Assignee)

Comment 15

12 years ago
Comment on attachment 218277 [details] [diff] [review]
Include Bugzilla/Config/MTA.pm in the files affected

(In reply to comment #14)
>
> >+All of these [% terms.bugs %] are in the NEW or REOPENED state, and have not
> 
> Marc, shouldn't we write these statuses as [% status_descs.NEW FILTER html %]
> and [% status_descs.REOPENED FILTER html %]? That's the reason I ask you for
> review. Else the patch looks good and works fine. r=LpSolit

Add to that that INVALID should be [% resolution_descs.INVALID FILTER html %]
and that the other references to NEW and REOPENED (except for the ones in the URL) should be changed. Cancelling and submitting a new patch.
Attachment #218277 - Flags: review?(wurblzap)
(Assignee)

Comment 16

12 years ago
Created attachment 222376 [details] [diff] [review]
Taking into account LpSolit's comments

Taking into account LpSolit's review of the previous patch,
using status_descs and resolutions_descs where appropriate,
change line length to be < 76 chars.
Attachment #218277 - Attachment is obsolete: true
Attachment #222376 - Flags: review?(LpSolit)

Comment 17

12 years ago
Comment on attachment 222376 [details] [diff] [review]
Taking into account LpSolit's comments

>--- /dev/null	2006-05-17 13:24:52.593056250 +0200
>+++ ./template/en/default/email/whine.txt.tmpl	2006-05-17 18:40:04.000000000 +0200
[..]
>+(1) You decide this [% terms.bug %] is really quick to deal with (like, it's [% resolution_descs.INVALID FILTER html %]),
>+    and so you get rid of it immediately.

Why is this html filtered when this is for a text template (just wondering)?

Comment 18

12 years ago
(In reply to comment #17)

> Why is this html filtered when this is for a text template (just wondering)?

You are right! The file has a .txt extension and shouldn't use 'FILTER html'. Nice catch! manu, could you update the patch accordingly?

Comment 19

12 years ago
Comment on attachment 222376 [details] [diff] [review]
Taking into account LpSolit's comments

per previous comment.
Attachment #222376 - Flags: review?(LpSolit)
(Assignee)

Comment 20

12 years ago
Created attachment 224409 [details] [diff] [review]
Once more, with feeling
Attachment #222376 - Attachment is obsolete: true
Attachment #224409 - Flags: review?(LpSolit)

Comment 21

12 years ago
Comment on attachment 224409 [details] [diff] [review]
Once more, with feeling

>+++ ./whineatnews.pl	2006-05-17 11:56:14.000000000 +0200

>     Bugzilla::BugMail::MessageToMTA($msg);

The patch doesn't apply cleanly because MessageToMTA() has been moved into Bugzilla::Mailer.



>+++ ./template/en/default/email/whine.txt.tmpl	2006-05-17 18:40:04.000000000 +0200

>+All of these [% terms.bugs %] are in the [% status_descs.NEW FILTER txt %] or 
>+[% status_descs.REOPENED FILTER txt %] state, and have not been

FILTER txt doesn't exist. Do not filter these directives at all.
Attachment #224409 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 22

12 years ago
Created attachment 224424 [details] [diff] [review]
remove "FILTER txt"
Attachment #224409 - Attachment is obsolete: true
Attachment #224424 - Flags: review?(LpSolit)

Comment 23

12 years ago
Comment on attachment 224424 [details] [diff] [review]
remove "FILTER txt"

r=LpSolit
Attachment #224424 - Flags: review?(LpSolit) → review+

Updated

12 years ago
Flags: approval?
Flags: approval? → approval+

Comment 24

12 years ago
Checking in whineatnews.pl;
/cvsroot/mozilla/webtools/bugzilla/whineatnews.pl,v  <--  whineatnews.pl
new revision: 1.23; previous revision: 1.22
done
Checking in Bugzilla/Config/MTA.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/MTA.pm,v  <--  MTA.pm
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/admin/params/mta.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/mta.html.tmpl,v  <--  mta.html.tmpl
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/email/whine.txt.tmpl,v
done
Checking in template/en/default/email/whine.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/whine.txt.tmpl,v  <--  whine.txt.tmpl
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Probably worthy of a followup, but the new (and old) mail text and described whine functionality is nowhere near what I experience in whineing with the perm set on by Bugzilla.mozilla.org account.

This may actually be possible to be removed; (or cause a regression?)

I don't claim to know bugzilla code, so possibly filing a spinoff bug would have been better than commenting here of course.
(Reporter)

Updated

12 years ago
Keywords: relnote
(Reporter)

Comment 26

11 years ago
Added to the relnotes currently attached to bug 349423.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.