User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0 The whining interface allows you to schedule email reminders with no queries. Yet it doesn't seem to send out any emails. Reproducible: Always Steps to Reproduce:

Suggested patch.

I think the idea behind whining of sending mails only if there are bugs in the result set is good. A whine event having no query is, the way I see it, just an intermediate stage when building a full whine event. I propose WONTFIX.

(In reply to comment #2) > I think the idea behind whining of sending mails only if there are bugs in the > result set is good. A whine event having no query is, the way I see it, just an > intermediate stage when building a full whine event. > > I propose WONTFIX. I disagree. With the patch, whine.pl allows me to broadcast various messages, based on a user group, not on a query. In the past, I had to use my own custom script (bug #248587), but it's no longer needed with this patch. The only "weakness" is that whine.pl doesn't cleanly support one-time-only broadcasts. I don't think it would take much work to fully support broadcasting, but as it stands, the patch is more than sufficient for me.

Suggest confirming. At first I wanted to WONTFIX this, but after a couple of hours I changed my mind. I could easily see how the regular mailing of annoying(?) Bugzilla messages could be classified as "whining", and if the current whining infrastructure can support it, I see no reason why it should not take on this task!

After a brief chat on IRC, I think the correct way to solve this is a per-event checkbox for something to the effect of "send mail for this even even if there are no search results" or something to that effect. I can imagine several situations (in my own whines) where some reports would be nice to only happen when they have matches, and others I'd like to be told that there's no matches, and those are both situations where I have searches defined on that event. If this happens to support sending a mail on an event with no searches defined, so be it. :)

Comment on attachment 190779 [details] [diff] [review] v1 per justdave's comment

(In reply to comment #5) I was also thinking maybe another button that says whine-only-once or possibly a whine-until-this-date field. This would then make the whine feature handle all broadcast related features.

(In reply to comment #7) Those sound like additional functionality that should be filed under different bugs.

I'd love to see a checkbox "Whine if no query/results". The whine-only-once, or whine-until-date, would be great enhancements too. This way the admin could send a broadcast message to the users of Bugzilla. This hasn't been touched in 2 1/2 years, but would be a great enhancement.

Here is a patch that provides the user with a checkbox, Send mail with empty results, defaulted to off. I think it provides enough of a solution for all, with the sole exception of running the whine only once. Please review and let me know how it looks. As always, I don't have access to check stuff in, so I'll need help with that. I applied this patch to my 3.0.3 install and checked it out. Works nicely. This would be great to get into 3.2 if Max would agree so I don't have to apply the patch myself when it comes out.

I think Mike's proposed fix sounds good, but it's still an enhancement and would have to wait for 4.0. I haven't looked at the actual patch.

Needed to 'my' the $there_are_bugs outside of the if. oops.

wicked, can you review? or should someone else take a look? This is a small enough enhancement that would provide a great deal of functionality. I think many people would like this.

Comment on attachment 304219 [details] [diff] [review] Code Patch Ver 2 Sorry that my review took this long (about 7 months!) but better late than never. :) >diff -Nru old/Bugzilla/DB/Schema/Mysql.pm new/Bugzilla/DB/Schema/Mysql.pm >@@ -69,6 +69,7 @@ >+ whine_events => {mailifnobugs => 1}, No need to add this line because BOOLEAN_MAP is only used for upto version 2.19.3 and doesn't need to be accurate after that. Mkanat? >diff -Nru old/Bugzilla/Install/DB.pm new/Bugzilla/Install/DB.pm >@@ -287,6 +287,11 @@ >+ # 2008-02-15 MikeTosh >+ # Add ability to send mail if no queries or no results >+ $dbh->bz_add_column('whine_events', 'mailifnobugs', >+ {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'}); >+ I'm pretty sure these should be in time order so this should be added at the bottom of sub (but before the comment). Mkanat? >diff -Nru old/template/en/default/whine/schedule.html.tmpl new/template/en/default/whine/schedule.html.tmpl >@@ -128,6 +128,16 @@ >+ <td valign="top" align="right"> >+ Send Mail with empty results: This should be reworded. Something like "Send a message even if there are no bugs in the search result" and remember to use $term where necessary. >+ <td> Would this need a colspan="2" definition like in the other td-tags? >diff -Nru old/whine.pl new/whine.pl >@@ -281,6 +282,7 @@ >+ 'mailifnobugs' => $mailifnobugs, Add this new hashref entry to the comment just before this sub. >@@ -295,6 +297,7 @@ >+# mailifnobugs (send mail if no queries or query results?) Nit: "send message even if there is no queries or query results" >@@ -316,9 +319,14 @@ >+ my $there_are_bugs; >+ if($mailifnobugs) { This line failes to compile due with error: Global symbol "$mailifnobugs" requires explicit package name at ./whine.pl line 323. You need to read the value from event hashref using $event->{'mailifnobugs'} syntax. Also, I think it would be better to rewrite this so that the mailifnobugs if is around the logic, including next line. Bonus points for fixing the comment to say about the new logic.

Yep, wicked is correct about both things he asked me about. I'm not entirely certain that this feature is worth this much code. Perhaps whining needs some refactoring first?

Sorry for the long delay, I've finally updated this patch with all your comments. Should be good to go now! please re-review.

Perhaps someone could also assign this to me? I would greatly appreciate it.

Comment on attachment 357372 [details] [diff] [review] Code Patch Ver 3 >Index: bugzilla/whine.pl >+# 2009-01-16 oreomike@gmail.com - Bug 302420: >+# add mailifnobugs: send message even if there is no query or query results Don't add this comment in the code. We have Bonsai for that.

Removed change comment.

Comment on attachment 357674 [details] [diff] [review] Code Patch Ver 4 Sorry, there are some strange errors in this patch. Did you test it at all? >Index: bugzilla/editwhines.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v Fails to compile with error "Global symbol "$mno" requires explicit package name at editwhines.cgi line 451." You probably misspelled this variable in line 447. >@@ -143,20 +143,22 @@ >+ # check the subject, body, and mailifnobugs for changes Nit: No comma before "and". >Index: bugzilla/whine.pl >=================================================================== >@@ -64,12 +64,13 @@ For some reason this hunk fails to apply cleanly. It also misses a '" .' after the body line so if it would apply, whine.pl would most likely fail to compile. >@@ -199,6 +200,7 @@ >+# mailifnobugs - send message even if there is no query or query results Nit: That "is" needs to be plural ("are"). @@ -281,6 +282,7 @@ 'mailto' => \@users, 'subject' => $subject, 'body' => $body, + 'mailifnobugs' => $mailifnobugs, }; } } You forgot to include this hunk from the previous patch to your new patch so whining fails. >@@ -295,6 +297,7 @@ >+# mailifnobugs (send message even if there is no query or query results) Nit: Same plurality change for this comment too. >@@ -315,12 +318,14 @@ >+ if(!$event->{'mailifnobugs'}) { Nit: Add space after if.

I've addressed your comments, but I can't seem to get DateTime::TimeZone updated for now, so I can't even check it right now. At least I can apply the patch with no errors and all the changes seem logical.

> >@@ -143,20 +143,22 @@ > >+ # check the subject, body, and mailifnobugs for changes > > Nit: No comma before "and". According to the APA style guide, "Use commas to separate three or more items in a series.Use a comma after each item, including the one before the conjunction (and or or)." So, your "nit" is not based on the same grammer rules that I follow.

(In reply to comment #22) > So, your "nit" is not based on the same grammer rules that I follow. (Please also know that I work as a sub to the Government, and for the work we deliver to them, it is mandatory. So I'm trained to use it. I can make exceptions if it speeds up the implementation! ;-) Thanks) Ref: http://www.gpoaccess.gov/stylemanual/2000/chapter_txt-8.html See 8.42.

The comma is fine.

(In reply to comment #22) > > Nit: No comma before "and". > So, your "nit" is not based on the same grammer rules that I follow. It's perfectly fine that you simply ignore my nit for that code comment. Especially since I could easily be wrong because grammar is not my strong point. I may have picked this rule from Finnish but even there I'm only 44% certain such rule exists. :) Usually all my nits are ignorable and I wouldn't deny a review only for nits. I apologize for any confusion my review comment raised. I'll review your patch ASAP.

(In reply to comment #25) > It's perfectly fine that you simply ignore my nit for that code comment. > Especially since I could easily be wrong because grammar is not my strong > point. I may have picked this rule from Finnish but even there I'm only 44% > certain such rule exists. :) In English I believe it can go either way.

Comment on attachment 360637 [details] [diff] [review] Code Patch Ver 5 Apologies for taking so long to get to this review request (again!). It might be nice addition to say "Zarro boogs found" message when there are no results. Might also say "no queries defined" for that case. These can be filed as new bugs and might need discussion before implementing. Currently description needs to include something like "if any" to indicate to whine recipient that it's normal that there are no results for this whine. (There no query header either, only description and link to edit whines when there are no query results to whine about.) This seems to work as expected now. Following small style issues can be fixed before commit. >Index: bugzilla/whine.pl >=================================================================== >+# mailifnobugs (send message even if there is no query or query results) Change "is" to plural "are" to conform with other comments. >+ if(!$event->{'mailifnobugs'}) { One space after "if" to conform with style rules.

Miketosh, do you have a Bugzilla CVS write access to commit this yourself? I can commit this for you if you don't, just let me know.

(In reply to comment #28) > Miketosh, do you have a Bugzilla CVS write access to commit this yourself? I > can commit this for you if you don't, just let me know. No I don't. Please submit for me, thank you!

Here we go, thanks for the patch! Checking in editwhines.cgi; /cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v <-- editwhines.cgi new revision: 1.24; previous revision: 1.23 done Checking in whine.pl; /cvsroot/mozilla/webtools/bugzilla/whine.pl,v <-- whine.pl new revision: 1.39; previous revision: 1.38 done Checking in Bugzilla/DB/Schema.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v <-- Schema.pm new revision: 1.116; previous revision: 1.115 done Checking in Bugzilla/Install/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v <-- DB.pm new revision: 1.63; previous revision: 1.62 done Checking in template/en/default/whine/schedule.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/whine/schedule.html.tmpl,v <-- schedule.html.tmpl new revision: 1.14; previous revision: 1.13 done

Added to the release notes in bug 547466.