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.

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.

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

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

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.

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

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

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 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)?

(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 on attachment 222376 [details] [diff] [review] Taking into account LpSolit's comments per previous comment.

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 on attachment 224424 [details] [diff] [review] remove "FILTER txt" r=LpSolit

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

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.

Added to the relnotes currently attached to bug 349423.