275638 - Templatise 'whinemail' email

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P3)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Dave Miller [:justdave]]

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.

Comment 2

[Emmanuel Seyman]

Attachment: remove whinemail from params and use the file whine/news.txt.tmpl instead

Comment 3

[Emmanuel Seyman]

Attachment: template to be used by whineatnews.pl

Comment 4

[Jouni Heikniemi]

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.

Comment 5

[Emmanuel Seyman]

(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.

Comment 6

[Frédéric Buclin]

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.

Comment 7

[Frédéric Buclin]

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?

Comment 8

[Emmanuel Seyman]

The new version is almost done.
I should be able to make it public by the end of the week.

Comment 9

[Emmanuel Seyman]

Attachment: Move the contents of the whinemail param to a template file and update code

Comment 10

[Marc Schumann [:Wurblzap]]

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.

Comment 11

[Emmanuel Seyman]

(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

[Frédéric Buclin]

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.

Comment 13

[Emmanuel Seyman]

Attachment: 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.

Comment 14

[Frédéric Buclin]

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

Comment 15

[Emmanuel Seyman]

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.

Comment 16

[Emmanuel Seyman]

Attachment: 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.

Comment 17

[Olav Vitters]

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

[Frédéric Buclin]

(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

[Frédéric Buclin]

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

per previous comment.

Comment 20

[Emmanuel Seyman]

Attachment: Once more, with feeling

Comment 21

[Frédéric Buclin]

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.

Comment 22

[Emmanuel Seyman]

Attachment: remove "FILTER txt"

Comment 23

[Frédéric Buclin]

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

r=LpSolit

Comment 24

[Frédéric Buclin]

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

Comment 25

[Justin Wood (:Callek)]

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.

Comment 26

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.