It would be good if globals.pl defined a sub CallProcessMail which contained the system() call, so that all the separate files could just call that single sub. In a Win32 installation it's many times necessary to whip up several parameters to the system call, and it's rather cumbersome to run around the source files pasting the new code everywhere. Combining the calls into a single place would help changing the system call syntax. I currently use the following: sub CallProcessMail { system("d:\\perl\\bin\\perl.exe", "-I\\\\t1\\\\s1\\bugzilla\\", "\\\\t1\\\\s1 \\bugzilla\\processmail.pl", @_); }

Actually, processmail should be a package/sub, and then we wouldn't have to go through all these problems. I'll attach a patch which both moves it to a sub, and causes processmail to return values rather than outputting them directly. Callers of ./processmail have been changed. Once post_bug abnd process_bug are templatised then the extra template code just becomes an INCLUDE, like it is for attachments. I've kept the interface to processmail as is for this patch. mattyt, timeless: Here's the patch I was talking about. Dave: do we want this for 2.16? As well as fixing the windows issues, it makes the templatisation a bit easier, and allows us to defer calling processmail to when we display it (and can then FLUSH, too, after each one), so that you still see some results even if the mailserver is down. Note that after applying the patch, you need to |mv processmail Processmail.pm| - I've done the diff this way so that the differences can easily be seen.

I think we're too close to 2.16 for this. Moving off

I just posted bug 140782 which has a strong relationship to this one. This should be 2.16 imo, though I wholly understand why you wouldn't want it that way. The processmail stuff is really way too complicated to be made to work on Windows platforms. I say this, having just worked on it several hours with the latest CVS version. ;-I

OK, moving back to 2.16 and making a blocker. We just completely broke email altogether on Win32 (not sendmail, but processmail - see bug 140782), and this is the right way to fix it. Although we still aren't officially supporting Win32, we shouldn't break things that previously did work on Win32.

If this patch gets revived: - The current bug/process/results.html.tmpl template should be enhanced to contain the "enter bug" results, and bug/create/created.html.tmpl changed to use it. (Same with attachment/updated.html.tmpl IMO.) - let's not use StudlyCaps for CGI parameters - Should we take the opportunity to kill off data/nomail? Email prefs are the correct way to do this now. - Can we think of a better name than "Processmail" for this? I've never liked that name. Gerv

Processmail the module could be called 'BugChangeNotifier' (or something like that) to indicate that: 1) it is used to send mail about bug changes, not just any mail 2) it is essentially a notifier, not a mailer: mail is the way to do it now, but in the future other notification methods should be available and possibly incorporated into the same module.

OK, lets try this. Now that all the places which want to use this are templatised, its a bit simpler. Before applying, do |cp processmail Bugmail.pm| Gerv: > - The current bug/process/results.html.tmpl template should be enhanced to contain the "enter bug" results, and bug/create/created.html.tmpl changed to use it. (Same with attachment/updated.html.tmpl IMO.) I don't know what this means. Take a look at what I've done, and see if this is what you wanted > - let's not use StudlyCaps for CGI parameters bah. _ sucks, and InterCaps is easier to type. Fixed anyway, in most places. > - Should we take the opportunity to kill off data/nomail? Email prefs are the correct way to do this now. No. We are taking this opportunity to hoepfully fix this for win32. Nothing else. The only other thing I am fixing is that the easiest way to do this is to have it go from the template, so that if mail is slow and there are lots of dependancies, you at least see what is happening in stages, like 2.14 does. This doesn't actually really matter, since processbug uses lots of little templates, but at least you'll be able to see which bug its dying on. > - Can we think of a better name than "Processmail" for this? I've never liked that name. I used Bugmail.pm instaead of BugNotifications.pm, or something. because this is still strongly related to bugmail, and would require a rewrite to be different. Other comments: - in globals.pl, I have to dereference the hash (It took me ages to work out that thats what the problem was). Alternatives include just not passing a ref in at all (can we then get at it from within TT?) or having Bugmail::Send take a hash reference, rather than a hash. - data/nomail sucks. I read it in once per use, which should have a slight perf improivment when we go through lots of dependancies, but has the downside that if I didn't use the wrapper func in globals.pl, .we'd do that for every cgi Or will a use be pulled in always, anyway, at compile time? I'll have to test. If so I'll just read it in once per bug, keeping the current usage. - 'people' isn't such a good name. Suggestions for a better one? - Its been lightly tested - it sends mail, and appears to get most of the -force stuff right. I haven't done a detailed test, though, esp with the cc forcing code. - In the new tempalte, I don't seem to be able to take stuff out of the array into a pair of variables - I have to use .0 and .1. Is there a better way to do this with TT, or is this a TT limitation? - can someone on windows please test this, and let me know if it fixes those problems?

Ok, that patch does it. Preliminary testing shows it works fine on Windows, although you still have to spank Bugmail.pm a bit to enable mailing on Win platforms (as Windows boxes don't tend to have a '/usr/lib/sendmail' handy). One code change I had to do to get it working: + @{$force{'CClist'}} = ($people{'cc'} && scalar($people{'cc'}) > 0) ? map(trim($_), $people{'cc'}) : (); to @{$force{'CClist'}} = ($people{'cc'} && scalar($people{'cc'}) > 0) ? map(trim($_), @{$people{'cc'}}) : (); (note the added deref near the end of the line). Wonder if this change breaks anything in your patch? It worked fine with me.

Bug 140782 is the real (potential) release blocker; the fix for this bug fixes that one but is not otherwise necessary for the release. At this point in the development cycle only the simplest and least risky fix for a blocker bug should be considered. Is the fix for this bug really the simplest and least risky approach? For that matter, is it really necessary to block a release because of a Win32 issue with code that had to be customized by Win32 installers of every previous release?

There is going to be a release candidate, I think. If this is true, then this fix will necessarily be tested by everyone trying it out, since hardly anybody will play without completely without email. And we have already a report that it's working (even) on windows, so I don't see the problem with this. Also note that email handling currently is a mess which is complained about (e.g. on the newsgroups) quite often.

Oops, this patch is indeed more complicated than just centralizing the calls to sendmail.

Sendmail has nothing at all to do with this patch - on windows, we fail before even getting there. The only way arround it would be to print all of the template up to where we;d call process mail, thencall processmail, then do the test of it. The other way is to change all that code to use work explictly, ad mentioned in the docs, but that would then need testing on perl5.005, too... The diffs are reasonably trivial though, except for that type in the cc bit (I mentioned that I hadn't tested that part...) We need processmail to be able to be called directly, which means making it a package. Or we could write to a temp file, I suppose, but that will have its own issues. One other issue - why does processmail-the-script set the umask to 0? I removed that, and it seemeed to work - does processmail create files anymore, or is this a relic from teh pre-bug_activity days?

Myk, re comment 10: While it's true that hacking the source has always been necessary on Win32, this is a different issue. If the Bugzilla-Sendmail border breaks, that's a somewhat external issue which can be left for the Win32'ers to fix. But if, say, process_bug-processmail border breaks, that's a Bugzilla bug (albeit caused by a lacking feature in ActiveState Perl). If this fix isn't checked into 2.16, everyone running Win32 will have to manually apply something like bbaetz's patch AND change processmail to use another mailer. This patch removes the first step (which is a new issue), and the latter is way more trivial (and exactly what's been needed for the past releases as well).

> We need processmail to be able to be called directly, which means making it a > package. Can't we do what I did with bug_form.pl - wrap the entire thing in a subroutine, require "processmail" at the top, and call the subroutine? That's only a few lines of change. Gerv

No, because that would still print to stdout - thats the reasl problem. As I said, we could redirect to a temp file, then read in from that file afterwards. But thats really messy and ugly, and has its own security issues. I've basically done what you did, really. Making it a package involved changing the file's name, and adding |Package Bugmail;| to the top (Ok, and the AUTOLOAD, too) In fact, the 'new' bug_form.pl should have been done that way. This is not a fundamental change in how things work - just look at the changes visually. If you want me to walk though the changes to processmail/Bugmail.pm, then I will, but they're really quite simple.

Comment on attachment 81491 [details] [diff] [review] v2 >+if (exists $::FORM{'rescanallBugmail'}) { >+ use Bugmail; >+ Status("OK, now attempting to send unsent mail"); >+ SendSQL("SELECT bug_id FROM bugs WHERE lastdiffed < delta_ts AND delta_ts < date_sub(now(), INTERVAL 30 minute) ORDER BY bug_id"); >+ my @list; >+ while (my @row = FetchSQLData()) { >+ push @list, $row[0]; >+ } Isn't while (MoreSQLData()) { push (@list, FetchOneColumn()); } the right way to do this? >+ print "<br>" . scalar(@list) . " bugs found with possibly unsent mail."; Do you need "scalar" here? >+ foreach my $id (@list) { >+ if (detaint_natural($id)) { >+ Bugmail::Send($id); >+ } Why are we detainting bug ids we get from the DB? >- print("Run <code>processmail rescanall</code> to fix this<p>\n"); >+ print qq{<a href="sanitycheck.cgi?rescanallBugmail=1">Click here to send these mails</a><p>\n}; Nit: add a full stop outside the link. >+ # SendBugmail- sends mail about a bug, using Bugmail.pm >+ 'SendBugmail' => sub ($;$) { House style seems to be not to use function prototyping ($;$). >- [% mailresults %] >+ [% PROCESS "bug/process/bugmail.html.tmpl" id = bugid %] <sigh> I'm not quite sure how it got so we have four different templates for that square box you get when you change a bug or something else. The Right Thing is for all of these to be centralised, and bugmail.html.tmpl to be part of that one template. However, I'm not sure if we'll manage this for 2.16 :-( (The template which appears to be doing the unifying is bug/process/results.html.tmpl.) >+ # Contributor(s): Bradley Baetz <bbaetz@student.usyd.edu.au> >+ #%] >+ >+[%# INTERFACE: >+ # id: bug id to send mail about >+ # people: hash for processmail >+ #%] That's a bit wet :-) Try: # people: hash. People involved in this change. Hash has up to five elements: # changer: string. The login name of the user who made the change. # For bug changes: # owner: string. The login name of the bug assignee. # reporter: string. The login name of the bug reporter. # qacontact: string. The login name of the bug's QA contact. Optional. # cc: list of strings. The login names of those on the CC list. >+ # people: hash; processmail params. Optional As above. >+# This is run when we load the package >+if (open(FID, "<data/nomail")) { >+ while (<FID>) { >+ $nomail{trim($_)} = 1; >+ } >+ close FID; >+} Can't we just remove support for this entirely (cut out the code)? Impact: it wouldn't work any more if people are using it. Possible mitigation: - Get checksetup.pl to read the file, if it exists, and turn off email prefs for all the users in it. Then delete the file. Do we even need to do that? >+# This is a bit of a hack, basically keeping the old system() >+# cmd line interface. Should clean this up at some point Is this still true? You appear to be using a nicer interface. >+ # Make sure to clean up _all_ package vars here. Yuck... >+ $nametoexclude = $people{'changer'} ? $people{'changer'} : ""; $nametoexclude = $people{'changer'} || ""; >+ #die "FOO: $people{'changer'}\n"; Please remove debugging code :-) >+ >+ return ProcessOneBug($id); Can this sub not now be rolled into the above sub? It appears to do very little. Gerv

- 'people' isn't such a good name. Suggestions for a better one? conspirators? :-) Gerv

> Isn't > while (MoreSQLData()) { > push (@list, FetchOneColumn()); >} >the right way to do this? Hey, I jsut moved code ;) But yes, I'll change that. >>+ print "<br>" . scalar(@list) . " bugs found with possibly unsent mail."; >Do you need "scalar" here? I want to know the number of items in teh list - how else do I do so? >>+ foreach my $id (@list) { >>+ if (detaint_natural($id)) { >>+ Bugmail::Send($id); >>+ } >Why are we detainting bug ids we get from the DB? heh. Because processmail runs with dbi-taint mode enabled. I personally think that that is stupid, but my attempts to remove it met with opposition, so... However, that is no longer part of processmail, so I can and will fix that > House style seems to be not to use function prototyping ($;$). My intent is to change house style. Once we no longer 'require' all this stuff, we'll be able to have perl test this for us at compile time. > <remove data/nomail> Theres a bug on moving this into the db. This bug will be as minimally invasive as possible - I am not fixing this now. Lets just leave it here, and deal with it later, either by removing it entirely, or by having a per user setting. We can fix the 'vacation mode' bug at the same time. >>+# This is a bit of a hack, basically keeping the old system() >>+# cmd line interface. Should clean this up at some point >Is this still true? You appear to be using a nicer interface. Yes, this is still true. What should happen is that the activity log should be read for additions and subtractions. Then we can call ProcessOneBug, passing only the bugid in. The 'people' hash is just the command lines made nicer, since I figured that I could at least get rid of @ARGLIST. (We may pass the changer in, to make things simpler. We currently have an issue where if mail isn't sent, the changer which does kick off the larger mail is considered to ahve changed everything. We may want to fix this, although since that case should, in theory, never happen, it may not be worth it) >>+ return ProcessOneBug($id); >Can this sub not now be rolled into the above sub? It appears to do very >little. On the contrary; this sub does all the work. Its the previous sub which has the issues, since it has to clear all the pacakge vars. I want to keep the two logically separate. You didn't comment on my reference-vs-hash thing - do you know the TT rules for that sort of thing? Yes, people isn't a good name - see my comments. 'conspirators' is worse, though.

> >Do you need "scalar" here? > > I want to know the number of items in teh list - how else do I do so? print "<br>" . @list . " bugs found..." ? > > House style seems to be not to use function prototyping ($;$). > > My intent is to change house style. Once we no longer 'require' all this > stuff, we'll be able to have perl test this for us at compile time. That's as may be. But currently house style is not to do it - and I, for one, will oppose a change. Function prototypes in Perl gain nothing that well-structured code and early parameter reading into sensibly named variables doesn't get you, but just cause hassle and errors when they get out of sync with reality > You didn't comment on my reference-vs-hash thing - do you know the TT rules > for that sort of thing? Er... I didn't quite understand the situation. > Yes, people isn't a good name - see my comments. 'conspirators' is worse, > though. I was only joking :-) Gerv

I just wanted to note that making it a .pm will be a good thing. If for nothing else to save me tearing my hair out trying to figure out why process_bug.cgi's output was repeating itself. process_bug.cgi does not handle processmail having the wrong path to perl very well, and goes nuts. The regexp given in the docs: perl -pi -e 's@#!/usr/bonsaitools/bin/perl@#!/usr/bin/perl@' *cgi *pl Bug.pm doesn't quite work against processmail.

don't forget the voters in that hash...

No, voters aren't covered there. Hmm. That probably means that the pref to send mail when you're added/removed from voting probably doesn't work. I'll have to test that at some point, but ts a separate bug.

Actually, changing your votes _never_ sends mail, at all, so that option doesn't make sense. Its only manually put into the hash so that thers a valid array in there when we iterae over the types of people.

OK, here comes v3. Again, remember to |mv processmail Bugmail.pm| first.

The image_map changes in dependency-graph.html.tmpl are unrelated to this bug. > - let's not use StudlyCaps for CGI parameters There is a rescanallBugmail param. Maybe it should be rescanall_bugmail. > [% mail = SendBugmail(id, people) %] In the long run, we shouldn't rely on a template calling this function (IMO). See also the SyncAllPendingShadowChanges discussion. But for now, it's probably fine. I'm going to apply this now to my installation, and I will try to test it a bit. But if this is going to be checked in for 2.16, then it should go in asap, so that we have more time to catch any regressions it may cause. If there are any bugs introduced with this patch, we certainly won't find them by just looking at the patch.

Template->process() failed twice. First error: [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: Attempted to send tainted string 'SELECT assigned_to,bug_file_loc,bug_severity,bug_status,bug_type,cclist_accessible,component,everconfirmed,groupset,keywords,op_sys,priority,product,qa_contact,rep_platform,reporter,reporter_accessible,resolution,short_desc,status_whiteboard,target_milestone,version,votes, lastdiffed, now() FROM bugs WHERE bug_id = 1' to the database at globals.pl line 251. Second error: [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002] process_bug.cgi: Attempted to send tainted string 'select (bit & 64504) != 0 from groups where name = 'tweakparams'' to the database at globals.pl line 251. Do I need to do some additional untainting for my "bug_type" stuff, or is this a bug in the patch? This happened when I tried to comment on one of my test bugs where I am reporter, assignee and qacontact at the same time.

Oops - yeah, ignore that template. Why shouldn't we rely on the template to send mail? Thats effectivly what the old code did, and it meant that people could see what was taking all this time. (To do so here, I probably need to add some [% FLUSH %] calls, though) Those errors look really unrelated to my patch. Do you get them without it?

No, the errors went away when I backed out your patch. But this doesn't necessarily mean that it's a problem with your patch. I have an additional column bug_type in the bugs table: bug_type | enum('DEFECT','ENHANCEMENT','FEATURE','TASK','QUESTION','PATCH') | | | DEFECT | So if this patch adds some additional taint stuff I may be required to make some additional changes. But maybe someone else can test the patch first? Is it already live on landfill? Maybe you are right with the email sending stuff. But we should document somewhere all the places where we rely on templates to call some backend code.

Without looking at your patch, I don't know. Work out which value is tainted, and detaint it. There shouldn't be any extra requirements from my patch in that regard, though. In any event, this doesn't happen without your addon patch, so...

Ok, I finally found out that $id is tainted here, in ProcessOneBug: SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " . "FROM bugs WHERE bug_id = $id"); Note that your patch removes a call to detaint_natural($id) before calling ProcessOneBug: - foreach my $id (@list) { - if (detaint_natural($id)) { - ProcessOneBug($id); - } Placing a detaint_natural($id); at the beginning of ProcessOneBug seems to fix it for me. But maybe it belongs in the Send(...) sub above? I wonder why this error is not showing up in your version...

But I removed this from sanity check - is that where you are having your errors? (Actually, now that I think of it, that detainting code probably does need to stay) The difference is that now the number we pass to the bug stuff needs to be untainted. If you remove the two lines in Bugmail.pm changing $db->{'taint'}, does that fix it for you? Otherwise there probably is somewhere which needs to be detainted. However, this works for me, so it could be a perl 5.005 thing. Can you get a stacktrace, so that I can see where it is failing? That second error lookes real strange.

> I removed this from sanity check - is that where you are having your errors? No, I'm getting the error when commenting on a bug, as I said in comment 27. > remove the two lines in Bugmail.pm changing $db->{'taint'} No, commenting out these two lines does not fix it. > Otherwise there probably is somewhere which needs to be detainted. Well, it obviously doesn't work for me, and I don't understand the problem. If ProcessOneBug expects $id to be untainted, and now we are passing it through the template processing stuff which eventually calls Bugmail::Send(...), then I think it's quite understandable that it needs to be detainted before passing it to ProcessOneBug, isn't it? Well, I don't understand this stuff at all, but it sounds plausible to me. > stacktrace I sent a page with confess output in private mail. > That second error lookes real strange. I think it's a followup from the first error. It goes away when the first error is fixed. When I turned on sqllog, I was at first very confused where the last two sql queries came from, since they did not seem to match with the code. I think they were generated by an attempt to generate a nice error template or something, or even for the footer.

The thing I got in the email was just a "you didn't select any bugs to change" think. Just copy and paste the stack to the bug, here.

OK, impact on this one is a little too high to do this close to a release. The Windows folks will have to wait for 2.17.1. However, 2.17.1 WILL be released within a month of 2.16, and I plan on this being one of the first things checked in after 2.16 goes out. Maybe we'll even put this patch by itself back on top of 2.16 and make it a 2.16.1. We'll RC the 2.16 branch tonight.

I think the issue is just related to my old perl 5.005_02 installation, so it shouldn't hold up any progress on this bug, as long as you require a newer perl or put in the one-line workaround. Great news about the RC1 :-)

If this works with 5.005_03, then yes, we'll just up the perl version. OTOH, are we going to require 5.6 after 2.16, in the end?

Dave, I think it's a good idea to relnote the Windows problems and do the 2.16.1 pretty soon after 2.16 is out so that even Windows people can get on with the upgrade. There are lots of reasons to switch from 2.14 to 2.16, and not everyone is willing to use a developer version (2.17.1).

If 2.16.1 include this patch and the Bug 84875 this will remove 90% of the modifications needed to work under windows...

oups, Bug 84876 not 84875...

That's a good idea... 84876 has had a lot of work done on it, but was waived off on 2.16 because those involved (Gerv) were busy getting 2.16 RC1 out the door, and there were some details we were still trying to get nailed down on the patch for 84876. I said in 84876 (comment #72) that I'd backport the patch for 84876 if anyone wanted it, but releasing a 2.16.1 with it in there makes even more sense.

Why not making 2.16.1 the first "windows ready" version ??

Becasue it would be 2.17.1, not 2.16.1 - ie a development release, not a stable one.

This is really a semantic issue (but then again, as one of my software engineering profs says, life is nothing but semantics), but why can't it be released as 2.16.1? We released a 2.14.1 when we had security "issues"; if we can e very controlled in what we might put into 2.16.1 (maybe fixes for this and 84876, or a subset of 84876), and relnote the fact that it's a release that makes "Bugzilla do sane things on Windows, no need to upgrade if you're on a real OS," I shouldn't think it would be a development release. It's not meant to be used in development situations, or conversely: we *should* have a release that "does the sane thing on Windows," and it *should* be a stable version... we should give the impression, via the version number, that the patches applied to 2.16 are stable enough to be used in a (Windows) production environment, assuming they indeed are.

I don't think there will be "Windows support" until we have a developer who runs Bugzilla under Windows.

Depending of what you call a "devloper" I can do the job !! I am running 2 system with 2.14 and a test 2.16 and already submite some "win32" patch

Jouni's another one, see http://bugzilla.mozilla.org/show_bug.cgi?id=142744#c2 , http://bugzilla.mozilla.org/buglist.cgi?field0-0-0=product&type0-0-0=substring&value0-0-0=bugzilla&field1-0-0=reporter&type1-0-0=substring&value1-0-0=jouni . That makes two already. Matty, you don't want to introduce first and second class developers, do you? ;-)

OK, the taint this is 5.00503, too - I reproduced this on landfill. The test case is really odd. It appears that having _anything_ which is tainted in the vars hash will cause this: use Template; my $template = Template->new({ INCLUDE_PATH => '.', }) || die Template->error(); my $vars = { 'is_tainted' => sub { return not eval { my $foo = join('',@_), kill 0; 1; }; } }; $vars->{'foo'} = $ENV{'PATH'}; $vars->{'var'} = 1; print "Perl: " . is_tainted($vars->{'var'}) . "\n"; with a template of: [% is_tainted(var) %] Comment out the $vars{'foo'} line, and everything is ok. This looks really odd. The only think I can think of is a builtin function in perl5.005 somewhere which converts the hash to an array and then back, but thats a bit far fetched. Anyone got some ideas? (The test is in ~bbaetz/tainttest/ on landfill, btw)

My only idea would be to look at the compiled code of the template, and then try to find out which function is responsible for this. But I have absolutely no experience in this area, so this may be a stupid idea...

I thikn its doing this earlier. Actually, this is probably why TT only lets you use hash refs - stuff is put into an array. I'lll have to think about this.

Just a status report: I pulled a fresh CVS copy on 9th May at about 10:00 GMT, patched it with attachment 82411 [details] [diff] [review] (bbaetz's patch v3) and have been running it in production use ever since. No problems with mailing at all. Platform W2k, IIS5, ActiveState Perl 5.6.1. So the patch seems to be working quite nicely apart from the taint problems you've been discussing.

What does it take to get it on the trunk, to get broader testing? I've been running with this patch for some time, too, and I haven't heard of any problems, besides this one-liner to workaround the taint problem. (But that doesn't mean too much; most of our users wouldn't even complain if Bugzilla was down for a day or two ;-)

Mainly me fionding time to tidy this up...

I'm starting to get convinced this might make it to 2.16. Andreas, you're using Solaris, right? Hearing from a few more folks (Linux and Windows) that they've used this successfully without problems would be convincing. :)

Yes, my installation is on a heavily patched solaris 2.6 Generic_105181-16 on a sun4u sparc SUNW,Ultra-2 with with currently only one processor (though there was a time when it had two) running perl 5.005_02 and mysql Ver 9.36 Distrib 3.22.27, for sun-solaris2.6 (sparc), using sendmail. :-)

OK, this adds the detaint_natural in, and changes the interface so that we pass a hash ref, not a hash, into Bugmail::Send. Any other comments?

Note that the wait time is actually a real pain - I almost double submitted a review on a bug with lots of people ccd, because it was taking so long before I got any feedback. If this patch goes in, and I add a [% FLUSH %] call both before and after sending mail, then we get better perceived performace.

Comment on attachment 84192 [details] [diff] [review] v4 I have plans on reviewing this soon - if you're kind enough to give me a version that applies cleanly on the trunk tip. :-)

Assigned -> me, per a conversation on IRC to this effect (doing this so it shows up in my bug lists).

This version applies cleanly to the trunk as of right now (which isn't saying much).

jth/other Windows folks: Could you please apply this new version of the patch and let me know how it goes? I'm driving this patch through to checkin for bbaetz; he's busy right now. I'd like to get this checked in before it rots too much since the trunk not as static as, say, the 2.16 branch, so your help in this area would be appreciated.

As I mentioned on irc, I've changed my mind - the template should not have the job of calling code to process mail messages. Also, the .pm should be in the newly-created Bugzilla/ directory

Ok. My Win32 testing platform has almost recovered from the crash in late July, so I plan on testing this next week. Two weeks away from computers has been good, although not for Bugzilla ;-)

Comment on attachment 95210 [details] [diff] [review] v5 Ok, I've quickly tested this. There are a couple of problems I encountered: The patch doesn't apply cleanly (the first hunk of processmail fails) Whenever Bugmail::Send is about to get called, the following error pops up: undef error - Undefined subroutine &Bugmail::Send called at globals.pl line 1666. These occur on both Windows and Linux. Unfortunately I need to run now, so I can't debug this further now. :-( If you can create another version of the patch, I'll certainly test more.

Jouni: Thanks for testing that; I'll get the patch updated to HEAD, address the issues bbaetz raised in comment 62 and test it on Linux.

This would be a massive perf gain, as we then wouldn't need to reload all the modules every time we call processmail (Siunce they'd be in the same process)

In case anyone cares, I'm gonna try to get this fixed for 2.17.1, since it's a blocker and all. Everyone together now: cross your fingers.

*** Bug 125688 has been marked as a duplicate of this bug. ***

Ok, let's get this fun bus back on the road. This is an updated version of v5, which I ended up patching manually because stuff has changed so much. Some things to note: -- I removed a call to processmail in CGI.pl, having to do with voting; I don't know if I have all the correct variables necessary. -- In the new BugMail.pm, calling Taint on the $::db handle caused things to blow chunks (complain about tainting); since it was noted that that stuff's a work in progress, I commented it out and all works, but don't really know if that's the right thing(tm) to do. -- The patch is basically v5, but it's been updated to head, which involved a lot of general changes; for instance, changes to Bugzilla/Template.pm, which didn't exist when v5 was written. -- I modified Bugzilla::BugMail::Send to return a hashref instead of an array, which made the code a bit easier to understand. You can play around with a current HEAD installation + this patch at http://landfill.mozilla.org/preed/bugzilla/ I did some simple things like file bugs and make changes, and I got the emails, but please try to break it. Any Win32ers on CC: the faster you can test this, the quicker we can knock out another [needed for Win32bz] bug.

Comment on attachment 110202 [details] [diff] [review] v6 New patch coming shortly.

This patch is v6 + the fix for bug 183388, which was checked in today. It was merged manually, since it's such a small patch. I'll be cc'ing the patch author, asking him to verify the merge.

Cc'ing the patch author for bug 183388; can you please verify my manual merge of the fix for that bug on v7 of this patch? Thanks!

Comment on attachment 110301 [details] [diff] [review] v7 Since reviewers@ didn't listen to me, we'll do this this spammish way...

Initial note... In sanitycheck.cgi..... 1) s/use Bugmail/use Bugzilla::BugMail/ 2) (nit) If this is being USEd, should it be at the top? Could it be REQUIREd instead?

re: comment 74: Ignore that 'use' statement behind the curtain. ;-) It's from bbaetz's first version of the patch, and both issues are moot, since I removed the line entirely, and it works fine. I also had to change a line further down to be: Bugzilla::BugMail::Send($id); Since I'm sure there will be another version or two of this patch, look for it in v8.

re: comment 75, where's v8? :-)

re comment 76: Why it's in your local grocer's freezer, of course! :-) v8 is still in my local tree; the differences between v7 and v8 are two lines that don't really affect the core patch (just the cleanup features in sanitycheck.cgi), so I didn't see a reason to go through the process of canceling all the r= requests, posting a new page, and then re-requesting everything. I did this, in part, because I'm sure there will be a v8 that includes more than just those two-line changes after those first r='s are done.

Comment on attachment 110301 [details] [diff] [review] v7 > Index: CGI.pl > =================================================================== > + $vars->{'people'} = { 'changer' => $who }; As bbaetz commented in comment 8, people isn't such a good name. "mailaddressees"? "addressees"? "recipients"? > Index: post_bug.cgi > =================================================================== > -if (defined $::FORM{'qa_contact'}) { > - push (@ARGLIST, "-forceqacontact", DBID_to_name($::FORM{'qa_contact'})); > +if (defined $::FORM{qa_contact}) { > + $vars->{'people'}->{'qa'} = DBID_to_name($::FORM{'qa_contact'}) Nit: please retain the quotes around the hash key. > push (@{$vars->{'sentmail'}}, { type => 'created', > id => $id, > - mail => $mailresults > + #mail => $mailresults This should be removed rather than commented out - same for the two other occurrences in this file. > Index: process_bug.cgi > =================================================================== > - # Save off the removedCcString so it can be fed to processmail > - $removedCcString = join (",", @removed); > > # If any changes were found, record it in the activity log > if (scalar(@removed) || scalar(@added)) { > @@ -1396,6 +1394,7 @@ > LogActivityEntry($id,"cc",$removed,$added,$whoid,$timestamp); > $bug_changed = 1; > } > + @ccRemoved = @removed; > } This does do a copy rather than just create another reference to the same array, right? > @@ -1709,11 +1691,7 @@ > CheckFormFieldDefined(\%::FORM,'comment'); > SendSQL("INSERT INTO duplicates VALUES ($duplicate, $::FORM{'id'})"); > > - $vars->{'mail'} = ""; > - open(PMAIL, "-|") or exec('./processmail', $duplicate, $::COOKIE{'Bugzilla_login'}); > - $vars->{'mail'} .= $_ while <PMAIL>; > - close(PMAIL); > - > + $vars->{'people'} = { 'changer' => $::COOKIE{'Bugzilla_login'} }; The change appears always to be current user - do we need to specify this everywher explicitly? > Index: sanitycheck.cgi > =================================================================== > - print("Run <code>processmail rescanall</code> to fix this<p>\n"); > + print qq{<a href="sanitycheck.cgi?rescanallBugmail=1">Click here to send these mails</a>.<p>\n}; Please let's not have "Click here". Just "Send these mails" would be a fine link. Also, wouldn't this rerun all the sanity checks that you had just run _again_? On a big installation, that would be rather irritating. Perhaps we could make rescanallbugmail an exclusive action, and exit() after doing it? > Index: Bugzilla/Template.pm > =================================================================== > RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v > retrieving revision 1.1 > diff -u -r1.1 Template.pm > --- Bugzilla/Template.pm 20 Dec 2002 07:21:30 -0000 1.1 > +++ Bugzilla/Template.pm 29 Dec 2002 11:55:59 -0000 > @@ -198,6 +198,18 @@ > # UserInGroup - you probably want to cache this > 'UserInGroup' => \&::UserInGroup, > > + # SendBugmail- sends mail about a bug, using Bugzilla::BugMail.pm > + 'SendBugmail' => sub { > + my ($id, $people) = (@_); I'd prefer to call this sub either SendMail (or SendBugMail or SendMailForBug or BugMailSend), if you are using StudlyCaps. My preference would probably be SendMailForBug... > + use Bugzilla::BugMail; > + > + # perl5.005 will taint all template vars if any of them > + # are tainted > + detaint_natural($id) || die "id for SendBugmail isn't a number"; Well, we don't support 5.005 any more, so do we need to do this? > Index: Bugzilla/BugMail.pm > =================================================================== > +# This code is really ugly. It was a commandline interface, then it was moved Nit: full stop. > +# This is run when we load the package > +if (open(FID, "<data/nomail")) { > + while (<FID>) { > + $nomail{trim($_)} = 1; > + } > + close FID; > +} Should we continue to support data/nomail? The Right Way to do this these days is create an account for the user and disable their mail in preferences. > +sub ProcessOneBug($) { > + my ($id) = (@_); > + > + # Set Taint mode for the SQL > + #$::db->{Taint} = 1; > + # ^^^ Taint mode is still a work in progress... What are the implications of doing this? It looks quite scary... how does it interact with normal taint mode? > Index: template/en/default/bug/process/bugmail.html.tmpl > =================================================================== > +[%# INTERFACE: > + # people: hash. People involved in this change. Hash has up to five elements: > + # changer: string. The login name of the user who made the change. > + # For bug changes: > + # owner: string. The login name of the bug assignee. > + # reporter: string. The login name of the bug reporter. > + # qacontact: string. The login name of the bug's QA contact. Optional. > + # cc: list of strings. The login names of those on the CC list. Where do watchers fit into this scheme? > +<center> > + If you wish to tweak the kinds of mail Bugzilla sends you, you can > + <a href="userprefs.cgi?tab=email">change your preferences</a>. > +</center> Can we get this just once per batch of mails sent, rather than for every mail, as now? Gerv

> Should we continue to support data/nomail? The Right Way to do this these days > is create an account for the user and disable their mail in preferences. I saw we should still support data/nomail. 1) If somebody is using it and it suddenly is unsupported with no reasonable replacement, that's a bad thing. 2) There is no easy way for an admin to disable all of a users bugmail prefs, so that makes this not be a viable solution. > What are the implications of doing this? It looks quite scary... how does it > interact with normal taint mode? Turning on $::db->{Taint} was the eventual goal when we stated taint mode. Basically what it does is die() if you try to send a tainted string to and cause data coming from the database to be tained. We accomplished the first half of this with the is_tainted function now in Bugzilla::Util, but we still "trust" everything the database gives us. Of course this is commented out, so it doesn't really effect anything, but I am curious why it's there.

the taint stuff is there because we all agreed to turn it off, since taintout as silly - we trust our db. I have patches to use the new DBI TaintIn attribute, which tests stuff gong in, similar to what SendSQL does. Theres a separate bug on moving data/nomail into the db, which should be done for all sorts of other reasons anyway.

Comment on attachment 110301 [details] [diff] [review] v7 Canceling the r= requests. I've got a new version of the patch that addresses most of your issues, Gerv and fixes the stuff Joel brought up in sanitycheck.cgi. But now bbaetz and I are hashing out whether TT should send the mail or not... which is an issue we've both been waffling on. Look for a new patch within the week.

Just an update: I haven't dropped this on the floor, but my workstation died this weekend, so I'm currently rushing around town for a new motherboard + CPU; I don't think I lost any data, though (hard drives are intact; mobo crapped itself, though); I will post the new patch once I have my workstation back up and running.

v8: try this one on for size. This patch addresses the issues raised in the r=, plus has been updated to HEAD.

Installation of the patch went fine, but I can't test it yet. The reason I can't test it isn't related to this bug, so I'll ask on the webtools mailing list. After that is solved, I should be able to test the patch and I'll report my findings here.

OK, v8 gives me this when submitting a comment to a bug : _____quote_____ Bugzilla has suffered an internal error. Please save this page and send it to jean_seb@hybride.com with details of what you were doing at the time this message appeared. URL: http://pcrd.hybride.com/bugzilla/process_bug.cgi undef error - Template::Exception at D:/perl/lib/CGI/Carp.pm line 301. _____end_quote_____ The update went through fine though. (the comment is added to the bug) Is there a way of backing out the patch (I have the cygwin version of patch, which is supposed to be the same as the unix version), so that I can check if the error message goes away then?

Well, yee haw. My test installation is at http://landfill.mozilla.org/preed/bugzilla/; can you try and repro it there? Oh, and what version did you patch? HEAD? It should apply cleanly to earlier versions, but may not be valid with earlier versions. backing out the patch: cvs up -C. This assumes you have cvs 1.11 or better (on both the client and server... don't know if cvs-mirror.m.o is 1.11 or better, though).

patch -R

Thanks for the commands, I'll try to see what happens. JP, (can I call you JP? :-) I'll check your test installation. Is it win32 as well? You probably tested it already, which would mean that I have a problem on my installation... More info : I also ran a sanity check, which told me that there were about 40 bugs with unsent mail. When clicking the link to send it, I got a page saying this : _____quote_____ Bugzilla Sanity Check OK, now attempting to send unsent mail 25 bugs found with possibly unsent mail. _____end_quote_____ without a footer, which I interpreted to mean that a perl error occured. Checking my apache error log, I found this : [Fri Jan 24 16:00:59 2003] [error] [client 192.8.200.204] Undefined subroutine &Bugzilla::BugMail::Send called at D:/htdocs/bugzilla/sanitycheck.cgi line 164., referer: http://pcrd.hybride.com/bugzilla/sanitycheck.cgi BTW, the link is still called "Click here to send these mails", which I think Gerv objected to in comment #78...

Oh, wait, did I have to have any of the previous patches installed? Or did I have to |mv processmail BugMail.pm| first? Damn, I feel stupid :-(

My installation isn't Win32. But that's ok, since we need help making it work on Win32, so the more testers with that platform, the better. I'll change the name of the link; look for it in v9 (note to Gerv: ignore that in v8, please). You shouldn't have had to do any mv's or install any of the other patches... although, make sure Bugzilla/BugMail.pm is there; if it's not, the patch may be horked.

Yeah, BugMail.pm wasn't there, bugmail.html.tmpl either. So now, I've created those files manually from the contents of the patch file, because even when I created empty files and did patch < [patch_file], nothing would be put into them. After changing BugMail.pm to use Net::SMTP, it WORKS! :-) And for sanitycheck.cgi, it still gave me the same error, so I added this : <------- [line 161 of sanitycheck.cgi] --------> print "<br>" . @list . " bugs found with possibly unsent mail.<br>"; + use Bugzilla::BugMail; foreach my $id (@list) { Bugzilla::BugMail::Send($id); } <----------------------------------------------> and that works. (I got 25 bugmails about old bugs... :-) But the page telling me that 25 bugmails have been sent still didn't have a footer, is that normal? No error in apache's log... So I can confirm that the patch works. Now all we need is Bug #84876, and most (easily 90%) of the Windows installation headache will be gone!

v9: fixes the problems raised by Jean-Sebastien in comments 91 and 88. *This* one *might* actually go in! :-)

Comment on attachment 112564 [details] [diff] [review] v9 Let's start by trying to get an r= from Gerv... :-)

Jean-Sebastien: Can you do this for me: back out v8 locally, and try applying v9, and see if the only thing you have to change is using Net::SMTP, and let me know if *that* works. Thanks!

Oh, one last spam (sorry): this patch should work correctly this time in terms of creating the new files. Be sure you use the correct -p value, even if it's -p0.

Yep, v9 works great. No errors or anything. Great work!

Comment on attachment 112564 [details] [diff] [review] v9 This is generally pretty good (with a few nits below). But before I give it review+, I need to see the differences between processmail and (Bug)Mail.pm. These are a bit hard to see in diff - any chance you could quickly summarise the changes you made to make processmail into a module? > Index: CGI.pl > =================================================================== > + $vars->{'bugmailrcpts'} = { 'changer' => $who }; Ick. Can we call it "recipients", please? :-) > Index: attachment.cgi > =================================================================== > - $mailresults .= $_ while <PMAIL>; > - close(PMAIL); > - > # Define the variables and functions that will be passed to the UI template. > + $vars->{'bugmailrcpts'} = { 'changer' => $::COOKIE{'Bugzilla_login'} }; > @@ -791,21 +779,10 @@ > # Define the variables and functions that will be passed to the UI template. > + $vars->{'bugmailrcpts'} = { 'changer' => DBID_to_name($::userid) }; Why do you use $::COOKIE{'Bugzilla_login'} in one place, and DBID_to_name($::userid) in the other? > Index: post_bug.cgi > =================================================================== > -push (@ARGLIST, "-forceowner", DBID_to_name($::FORM{assigned_to})); > +# Tell the user all about it > +$vars->{'bugmailrcpts'} = { > + 'cc' => \@cc, > + 'owner' => DBID_to_name($::FORM{'assigned_to'}), > + 'reporter' => DBID_to_name($::userid), > + 'changer' => $::COOKIE{'Bugzilla_login'} Same question here. The reporter is the changer, so you should make that clear in the code. > foreach my $i (@all_deps) { > - my $mail = ""; > - open(PMAIL, "-|") or exec('./processmail', $i, $::COOKIE{'Bugzilla_login'}); > - $mail .= $_ while <PMAIL>; > - close(PMAIL); > +# my $mail = ""; > +# open(PMAIL, "-|") or exec('./processmail', $i, $::COOKIE{'Bugzilla_login'}); > +# $mail .= $_ while <PMAIL>; > +# close(PMAIL); Please remove this instead of commenting it out. > Index: Bugzilla/BugMail.pm > =================================================================== > +# This is run when we load the package > +if (open(FID, "<data/nomail")) { > + while (<FID>) { > + $nomail{trim($_)} = 1; > + } > + close FID; > +} While you are there, please give this filehandle a better name :-) > +# args: bug_id, and an optional hash ref which may have keys for: > +# changer, owner, qa, reporter, cc > +# Which contain values of people to be forced to their respective > +# roles. Slightly dodgy English here :-) > + # Since any email recipients must be rederived if the user has not > + # been rederived since the most recent group change, figure out when that > + # is once and determine the need to rederive users using the same DB > + # access that gets the user's email address each time a person is > + # processed. This comment is seriously cryptic. But if you didn't write it, I won't make you fix it. > + my $resid = > + > + SendSQL("SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, " . > + " removed, added " . Not sure about this formatting... > Index: Bugzilla/Template.pm > =================================================================== > + # SendBugmail- sends mail about a bug, using Bugzilla::BugMail.pm > + 'SendBugmail' => sub { > + my ($id, $bugmailrcpts) = (@_); > + require Bugzilla::BugMail; > + Bugzilla::BugMail::Send($id, $bugmailrcpts); (Same renaming thing.) Also, please have a consistent capitalisation - is it Bugmail (the function) or BugMail (the package)? Are you certain there's no chance of the BugMail package ever being used to send non-bug-mail? If there is, you should call it Bugzilla::Mail instead. > Index: template/en/default/bug/process/results.html.tmpl > =================================================================== > + # > + # bugmailtcpts: hash; BugMail recipient params. Optional. Typo. But the name's changing anyway, right? ;-) > <h2>[% title.$type %]</h2> > - [% mail %] > + [% PROCESS "bug/process/bugmail.html.tmpl" %] Does this template not need a bug_id passed to it? Gerv

> Ick. Can we call it "recipients", please? :-) Recipients is just as bad as "people," IMO. Recipients of what? Granted, Bugzilla only send email now, but... I want to be more clear on what they're recipients *of*. I'd be willing to change it to bugmailrecips if you don't like rcpts... I was in an SMTP-y mood that day... > Are you certain there's no chance of the BugMail package ever being used to > send non-bug-mail? If there is, you should call it Bugzilla::Mail instead. Yes. Bug 84876 will introduce a Bugzilla::ProcessMail package, which is what that will be. > Does this template not need a bug_id passed to it? The way that works is it uses the bug number in id; I'm assuming that's set somewhere else in the template, but maybe that's a recipe for disaster. On the other hand, I don't know where I'd pull that from (Template::GetBugID() or some such?), so maybe 'id' is guaranteed for us? > Not sure about this formatting... It's to keep the SELECT clause clear from the FROM clause... and pretty much lifted from processmail; I won't defend it, but I understand it, and see no reason to change it. > Why do you use $::COOKIE{'Bugzilla_login'} in one place, and > DBID_to_name($::userid) in the other? Old style, I suspect; I changed it, and will test. > Same question here. The reporter is the changer, so you should make that clear > in the code. Same answer here... :-) re: comments: I didn't write either of those, the first one ("Dodgy English") would be good to ask bbaetz what he meant, assuming he wrote it; the 2nd one, same thing, although I'm planning to rewrite a lot of processmail once things settle down, so... I'll post a summary of the diffs in a moment.

Summary of changes between processmail and BugMail.pm: -- Some autoload stuff/package variable initialization; also removed some global vars we don't need anymore because they're in the package now. -- BugMail::Send() replaces the command line parsing stuff, and drives the whole process (aside: I understand that "dodgy" comment now, and will try to clean it up...) BugMail::Send() now also calculates the $::last_changed value; this used to be done whenever processmail was run, but it's done here now since it's a package. -- Removed the generation of the HTML-formatted output of processmail, since that's done by TT now. -- Includes the fix for bug 183388 -- One *really* oddly formatted SQL statement was fixed. -- Finally, remove all the commandline processing stuff. These changes are in order of what diff output, so you should be able to go through them and see what I'm referencing.

Changes between v9 and v10: -- s/bugmailrcpts/bugmailrecips/g -- $::last_changed has been moved to Bugzilla::BugMail; no reason to polute the global namespace. -- The disparity between $::COOKIE{Bugzilla_login} and DBID_to_name was changed/tested -- Changed one of the comments to (hopefully) make it clearer.

v10 tested on my side, no problem.

> Recipients is just as bad as "people," IMO. Recipients of what? Granted, > Bugzilla only send email now, but... I want to be more clear on what they're > recipients *of*. A notification. Not necessarily mail, as you (or someone) pointed out in bug 84876. It could be an instant message, or whatever. This is a variable being passed to a template, which will do some sort of notification, but the CGI should neither know nor care what. So recipients is obvious as to its content, anyone knows what it means now, but it could mean something additionally in the future. But it's also aesthetically horrible, as are most word contractions. And "bugmailrecips" makes me think of recipes. :-) So, if the above doesn't convince you or your other reviewer, how about "mailrecipients"? > > Does this template not need a bug_id passed to it? > > The way that works is it uses the bug number in id; I'm assuming that's set > somewhere else in the template, but maybe that's a recipe for disaster. On the > other hand, I don't know where I'd pull that from (Template::GetBugID() or > some such?), so maybe 'id' is guaranteed for us? Even if it's set elsewhere, you should set it explicitly, to avoid the sort of confusion which confused me. If the interface is an ID, we should make it clear that we are passing it across the interface - otherwise someone might change that template, for some reason, to call it "bug_id", and mail will break because it can't find "id" any more. (Actually, in general, calling any variable "id" in Bugzilla is bad, as we have so many different sorts of id. But that's another thing.) Gerv

I'm not convinced, but mailrecipients is fine; I'll s/// that. I'll also fix that id issue you mentioned; you're absolutely right. Joel: assume I fix these, and r= the rest of v10, if you have time, please. Same with with you, Gerv. I'll carry the r='s forward to v11 if you don't find anything else.

i like 'victims' but failing that, bugmailees should suffice. oh it isn't mail. bugnotifiess bugalertees :-) bugcontacts <- best bet

Comment on attachment 112865 [details] [diff] [review] v10 > Index: post_bug.cgi > =================================================================== > -push (@ARGLIST, "-forceowner", DBID_to_name($::FORM{assigned_to})); > +# Tell the user all about it > +$vars->{'bugmailrecips'} = { > + 'cc' => \@cc, > + 'owner' => DBID_to_name($::FORM{'assigned_to'}), > + 'reporter' => $::COOKIE{'Bugzilla_login'}, > + 'changer' => $::COOKIE{'Bugzilla_login'} > + } > Index: process_bug.cgi > =================================================================== > + $vars->{'bugmailrecips'} = { 'cc' => \@ccRemoved, > + 'owner' => $origOwner, > + 'qa' => $origQaContact, > + 'changer' => $::COOKIE{'Bugzilla_login'} > + }; > + $vars->{'bugmailrcpts'} = { 'changer' => > + $::COOKIE{'Bugzilla_login'} }; Nit: inconsistent formatting between instances. > + # SendBugMail- sends mail about a bug, using Bugzilla::BugMail.pm Tiny nit: missing space. > - [% mailresults %] > + [% PROCESS "bug/process/bugmail.html.tmpl" id = bugid %] Just to be completely clear (I think you are doing this anyway) - I think you should a) always pass the ID across this interface explicitly, and b) rename the variable inside bugmail.html.tmpl to bugid or bug_id, because calling things "id" is bad. > Index: template/en/default/bug/process/bugmail.html.tmpl > =================================================================== > +[%# INTERFACE: > + # bugmailrecips: hash. People involved in this change. Hash has up to five > + # elements: > + # changer: string. The login name of the user who made the change. > + # For bug changes: > + # owner: string. The login name of the bug assignee. > + # reporter: string. The login name of the bug reporter. > + # qacontact: string. The login name of the bug's QA contact. Optional. > + # cc: list of strings. The login names of those on the CC list. > + #%] Shouldn't id/bugid/bug_id be in this list? Fix these nits, and r=gerv. Let's get this in :-) Gerv

v11; fixes Gerv's nits: -- Changes the %vars variable name to mailrecipients -- always pass the bugid to the template via a variable called mailing_bugid (bugid and bug_id were *really* common).

Comment on attachment 113937 [details] [diff] [review] v11 Mostly really small nits, but there's one potentially harmful case issue in BugMail.pm which I'd like to see fixed before checkin. ------------------ >+# Tell the user all about it This comment sucks. If it means nothing, remove it. If it means something, change it :-) (I know, it was there before, but the context of the comment has changed so much that it's a good idea to do something about this, too) >- push (@ARGLIST, "-forceqacontact", DBID_to_name($::FORM{'qa_contact'})); >+ $vars->{'mailrecipients'}->{'qa'} = DBID_to_name($::FORM{'qa_contact'}) Just for the sake of consistency (and for the future), add a semicolon there :-) >- >- my $removedCcString = ""; >+ >+ my @ccRemoved = (); If you're changing those empty lines, kill the spaces totally. Now you've removed one out of four. >+ $vars->{'mailrecipients'} = { 'changer' => >+ $::COOKIE{'Bugzilla_login'} }; This wrapping doesn't look good. >+if (exists $::FORM{'rescanallBugmail'}) { Über-nit: If you're using "BugMail" (with the capital M) everywhere else, you should probably use it in this form field as well. >+ print "<br>" . @list . " bugs found with possibly unsent mail.<br>"; Make this Status("@list bugs found with possibly unsent mail."); or something similar. Two issues: 1) avoid the .-based catenation if you're using "" anyway (or use ''), 2) use Status instead of prints with <br>s to preserve formatting. >+ @{$force{'CClist'}} = (exists $recipients->{'cc'} && >+ scalar($recipients->{'cc'}) > 0) ? map(trim($_), >+ @{$recipients->{'cc'}}) : (); >+ @{$force{'Owner'}} = $recipients->{'owner'} ? >+ (trim($recipients->{'owner'})) : (); >+ @{$force{'QAcontact'}} = $recipients->{'qacontact'} ? >+ (trim($recipients->{'qacontact'})) : (); >+ @{$force{'Reporter'}} = $recipients->{'reporter'} ? >+ (trim($recipients->{'reporter'})) : (); This is _so_ ugly. Any chance of a cuter wrapping? The scope of the ? : operator is not very clear now. But, an important issue (this is the one I'm marking review- for): In BugMail.pm, you have the $force{'QAContact'} key written as both 'QAContact' and 'QAcontact'. Please review the role names once more for case issues. How about making keys of the force hash all lowercase, just to avoid conflicts in the future? >+ [% PROCESS "bug/process/bugmail.html.tmpl" mailing_bugid= bugid %] Nit: a space before the assignment operator. >+ # mailrecipients: hash. People involved in this change. Hash has up to five >+ # elements: >+ # changer: string. The login name of the user who made the change. >+ # For bug changes: >+ # owner: string. The login name of the bug assignee. >+ # reporter: string. The login name of the bug reporter. >+ # qacontact: string. The login name of the bug's QA contact. Optional. >+ # cc: list of strings. The login names of those on the CC list. First, indent the mailrecipients comment like # mailrecipients: hash. Yadda yadda yadda # This is the second line --------------- Empty space here :-) And then, "For bug changes:" isn't too clear. How about a description like: "The following keys have values if the respective fields have changed."? Also, a diff of tip-processmail and the BugMail.pm in your patch reveals the following: >- SendSQL("SELECT userid, (refreshed_when > " . SqlQuote($::last_changed) . ") " . >- "FROM profiles WHERE login_name = " . SqlQuote($person)); >+ SendSQL("SELECT userid, (refreshed_when > " . SqlQuote($last_changed) . >+ ") " . "FROM profiles WHERE login_name = " . SqlQuote($person)); Which is fine with me, but the second line now contains an unnecessary catenation; you could just write it as ") FROM profiles...".

This should hopefully make *everyone* happy. :-) Jouni: I fixed that one instance of "QAContact," since it looks like it was missed in the fix for bug 183388, and I can't find an instance of "QAContact" anywhere else (they were all changed to "QAcontact"); admittedly, this isn't very clear at all, but fixing the ambiguity is *another* bug. I also left some of the ternary operators as is... yes, it's ugly, but... that *too* is another bug. ;-)

Comment on attachment 113944 [details] [diff] [review] v12 >I also left some of the ternary operators as is... yes, it's ugly, but... that >*too* is another bug. ;-) I disagree, but don't really care at this point. >+# Email everyone regarding the change This comment still sucks, because it's not a "change" we're talking about here. This is post_bug.cgi, so why not make it "Email everyone about the new bug"? r=jouni, but please fix that comment before checkin. Good job, by the way :-)

> so why not make it "Email everyone about the new bug"? Done. Also, the reason it's good is because bbaetz wrote it. ;-) Justdave?

hooray!!

Same thing as v12, just updated to HEAD. r='s carried forward from gerv and jth.

Checked in: Checking in CGI.pl; /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v <-- CGI.pl new revision: 1.199; previous revision: 1.198 done Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.37; previous revision: 1.36 done Checking in post_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v <-- post_bug.cgi new revision: 1.78; previous revision: 1.77 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.175; previous revision: 1.174 done Removing processmail; /cvsroot/mozilla/webtools/bugzilla/processmail,v <-- processmail new revision: delete; previous revision: 1.94 done Checking in sanitycheck.cgi; /cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v <-- sanitycheck.cgi new revision: 1.63; previous revision: 1.62 done RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v done Checking in Bugzilla/BugMail.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v <-- BugMail.pm initial revision: 1.1 done Checking in Bugzilla/Template.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v <-- Template.pm new revision: 1.3; previous revision: 1.2 done Checking in template/en/default/attachment/created.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/created.html.tmpl,v <-- created.html.tmpl new revision: 1.8; previous revision: 1.7 done Checking in template/en/default/attachment/updated.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/updated.html.tmpl,v <-- updated.html.tmpl new revision: 1.8; previous revision: 1.7 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/bugmail.html.tmpl,v done Checking in template/en/default/bug/process/bugmail.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/bugmail.html.tmpl,v <-- bugmail.html.tmpl initial revision: 1.1 done Checking in template/en/default/bug/process/results.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/results.html.tmpl,v <-- results.html.tmpl new revision: 1.4; previous revision: 1.3 done Thanks for everyone's help on this!

i have installed the patch, i thought that all open Sendmail commands will replaced to one in bugmail.pm. Just search for open sendmail and found serveral open sendmail commands. cgi.pl globals.pl importxml.pl move.pl whieneatnews.pl token.pm and so on. Am i wrong with my meaning? Sorry for my english, it's not my default. thanks

Re comment 114: You're thinking of bug 84876.