The current whining system could use some major updates. When it does whine, it should send more than just a list of bug numbers. It should send a table with all of a users actionable bugs in one email. The decisions on what information to send and what should be whined about should be easily customizable to a site. Should the user have any prefs involved?

->me

This is an early version of fine_while.pl It works for my own purposes, but probably wants to be made more feature-rich. Hopefully it will provoke some discussion. It should not interfere with anything. It adds a new fine_whine.pl script (use instead of whineatnews) and a new template. It will whine at NEW untouched for 7 days bugs as well as at any open untouched for 7 days bugs. Please comment.

Some opinions: You should split the whinedays into two different params: one for setting the number of days before new bugs are whined at, and another for setting the number before any open bugs are whined at. It should be possible to disable either of them. On our Bugzilla many developers have hundreds of open (usually assigned) bugs on them, but they're scheduled to be done a lot later. Whining at new bugs could still be useful, though. You should also make the styling-related "10" configurable in the template; that won't look good with Param('whinedays') set to, say, 14. I could be satisfied with a 1.5*Param('whinedays'), though. >+Content-Transfer-Encoding: 7bit You know, this is going to suck with scandinavian characters... Other than that, it looks good. Not that I would've tested the code, though. :-)

Oh, and no, I don't think there's a particular reason why watchers should get whinemail. The only way they could get rid of this is by assigning the bugs to their owner (sounds bad) or reassigning them to somebody else (which may or may not be ok - but in a case of vacation watching it's most likely not what's wanted). I think you can push this concept very far by creating different sorts of reports for managers and so on. However, I think that you should aim at getting your version checked in without a horrendous bulk of features. If somebody needs something, they're probably going to ask, right?

How does this relate to bug 6679? Is it a dupe (doesn't sound like it)? A blocker? Can 6679 be rolled into this bug?

Only one quick comment - fine_whine.pl is an amusing name, but will cease to be so in a year or two, when people have forgotten the older version. whineatnews.pl is also a very bad name; I keep things of news as in mail and news. How about "whine.pl"? :-) Gerv

having just read both bugs, I'd say it's a dupe. This one has newer and better patches, so I'll leave this one.

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

restoring relevant dependencies picked up from the dupe...

Having installation-wide whines is fine, but I think user and group whines would be just as useful. You should be able to store whines in a similar way that you store queries. I think any other implementation is just going to be obsolete as soon as it is written. Given you can reuse the query controls, couldn't you set up an interface similar to custom charting? For the moment, have installation and user whines, and maybe add groups later (along with group stored queries and reports). I haven't actually had time to look at the patch, apologies if this is already the case, but it seemingly isn't, based on the comments here.

WRT comment 10: I agree. However, much of the value of the whining feature is that they system whines AT people (even if they don't like it), so it should probably be controlled by the administrator rather than the user. (Or at least have that capability)

Also, the system should probably either send multipart/alternative email, or just send plain text, to avoid irritating all of the ur-hackers using pine. -M

WRT multipart/alternative... agreed. Updated thoughts on this.... I probably need to add some derived fields to the query interface to support this. Somehow, I need to search on.... time since assigned to current owner. time current owner has done nothing about it, etc... If people could create stored queries that whine at them and/or other people, it would do most of this.

I'm trying to use the patch. It's coming up with a generic template error that I don't know how to trace I'm running Bugzilla 2.16.4 , and I applied another patch from bug 225474 that is in 2.16.5, which fixes a SQL error. bash-2.03$ perl -W fine_whine.pl Subroutine Cwd::fastcwd redefined at /opt/lib/perl5/5.8.3/sun4-solaris/XSLoader.pm line 91 (#1) (W redefine) You redefined a subroutine. To suppress this warning, say { no warnings 'redefine'; eval "sub name { ... }"; } [Mon Mar 8 12:06:39 2004] XSLoader.pm: Subroutine Cwd::fastcwd redefined at /opt/lib/perl5/5.8.3/sun4-solaris/XSLoader.pm line 91. <h1>Software error:</h1> <pre>template error at fine_whine.pl line 79.</pre> <p> For help, please send mail to this site's webmaster, giving this error message and the time and date of the error. </p> [Mon Mar 8 12:06:39 2004] fine_whine.pl: template error at fine_whine.pl line 79. I run checksetup as apache:web (the same user/group my web server runs as) instead of root. Would that be a problem? bash-2.03$ find . -name '*whine*' ./template/en/default/whine ./template/en/default/whine/whine.html.tmpl ./whineatnews.pl ./data/template/en/default/whine ./data/template/en/default/whine/whine.html.tmpl ./fine_whine.pl BTW, running checksetup.pl turns off x bit for fine_whine.pl

Here's the new plan... Query (boolean charts) and Search.pm get 1) some new pronouns for the RHS... "self" to match the person either running the query or for whom the query is being run. "assignee" to match the person to whom the bug is currently assigned. "group.groupname" to match a group of people 2) New verbs "exceeded" works like "is greater than" except it represents an interval in periodically scheduled searches. 3) New nouns for the LHS "Time Since Owner Changed" "Time Since Owner Commented" A new interface permits a user to create scheduled events. When a scheduled event occurs, one or more named queries is run. Ordinary users can schedule events only on their own behalf. Users with appropriate privileges can schedule events on behalf of invidual users or groups of users. Names queries are run in a specified order. Each query can be assigned to a formatting template to select the presentation style of that section of the report. (normal templates are called twice, once for text/plain and once for text/html. Certain templates may be designed to send email notifications [resembling bugmail] on a per-bug basis) This makes it possible to implement the following examples... 1) whineatnews If an administrator creates a query with criteria... (state is "NEW") AND (assigned_to is "+self") and saves it as "news" Then schedules that query to be run every Monday at 6am on behalf of every user... --> users will each get a whining report 2) whine about NEW bugs as well as OPEN P1/P2 bugs that the owner has not touched in 5 days. In addition to the "news" report above, the administrator creates.... state is ("NEW"|"ASSIGNED"|"REOPENED") AND (assigned_to is "+self") AND ("days_since_owner_changed" is_greater_than "5d") Then, schedules both reports to be run every Tuesday and Thursday at 9am. 3) A user want to know quickly about P1 bugs in product "Bugzilla". The user saves a query ... state is ("UNCONFIRMED"|"NEW") AND ("time_since_changed" exceeded "30 minutes") AND product = "Bugzilla" and schedules it to be run every 30 minutes. However, the cron is set up to run every hour. Each hour, this query would get run and return all bugs meeting that have been idle between 30 and 90 minutes.

(In reply to comment #15) > A new interface permits a user to create scheduled events. When a scheduled > event occurs, one or more named queries is run. I think would be nice to have a way for an admin to temporarily halt all events so for example when a site is being updated that it isn't trying to do a query when the db is being upgraded. Some users may accidently setup time consuming queries or they may have left the project and not stopped their scheduled queries. It would be handy for the admin to be able to easily list, stop, delete and modify any user's events.

The temporary halt should probably be just a matter of skipping STARTING the whining when there is a shutdown message in place. We should not let that cause the whining to be missed, just deferred. It is an interesting question how an admin SHOULD be able to control a user's events. Possibly a special administrative UI??

What do you think? It's everything but the pronouns.

Comment on attachment 148780 [details] [diff] [review] Whining as a saved query run on a schedule >Index: checksetup.pl >=================================================================== >@@ -3906,6 +3928,8 @@ > # BugZilla uses --GROUPS-- to assign various rights to its users. > # > >+AddGroup('whineatothers', 'Can send whine mail to other users'); >+AddGroup('whiners', 'Can generate whine reports'); > AddGroup('tweakparams', 'Can tweak operating parameters'); > AddGroup('editusers', 'Can edit or disable users'); > AddGroup('creategroups', 'Can create and destroy groups.'); Any particular reason this was added at the begining instead of the end? I suppose it doesn't really matter, but this would result in those being groups 1 and 2 on the back end in a new install. Should whiners automatically inherit membership from whineatothers? Since it seems silly to have whineatothers if you don't have whiners... on the other hand, if you're wanting to take away someone's ability to whine entirely, it's one more place to go to remove it :) >Index: defparams.pl >=================================================================== >@@ -1176,6 +1176,8 @@ > type => 'b', > default => 0, > }, >+ >+ > > ); > 1; whitespace-only change to defparams? :) That's all for now, I have yet to try it out outside of Joel's test install.

There was a problem with the reset_timer function not being called correctly. Please don't ask how that got past me. I also took care of the new groups in checksetup being in the wrong spot (no, there was no good reason for it), and the whitespace in defparams that didn't need to be in the patch.

There was a cvsdo snafu on the previous version of this patch. This one is fixed and has been tested.

I got time to try it out and it worked for me. I scheduled a query, ran whine.pl and it sent me the bug list. A few thoughts that came to mind: 1. there is no way to specify the columns you want - comments in code seem to suggest that is a known issue 2. the names of the columns in the e-mail are not the same as in a buglist on the web - they are hardcoded in the whine templates 3. how about if a list of names could be specified for the 'Mail to' or perhaps a group name(assuming person was in the group they are sending to). It looks like currently you can setup multiple schedules in order to send to more then one person, but that would be cumbersome if it was very many. 4. some might like to include additional text beyond the title in the e-mail to provide the recipients with eg. a description of how the bug list was generated. This perhaps could be done in the title depending on how verbose people get.

Erik can't do the group item until bug 240325 lands The return address on this should not be the maintainer. It should be a "whining-daemon" and probably just specified in the template like the other functions.

There was probably more whitespace sillyness in the previous patch. I have been generating my diffs with the wrong args. There was also a problem where a midnight schedule was being entered as an empty string, and then being interpreted as an every-15-minutes schedule. That should be fixed now. Regarding comment 22: > 1. there is no way to specify the columns you want - comments in code seem > to suggest that is a known issue > 2. the names of the columns in the e-mail are not the same as in a buglist > on the web - they are hardcoded in the whine templates It is a known issue. I'm trying to land a simple version of it for right now. Having a custom list of columns might get a little complicated. What I'd like to do is land the new whine system with at least the functionality that whineatnews has, then add things like custom columns afterward. The current version shows more information than whineatnews does, so I consider it sufficient as a first phase. I'll add a bug for custom columns. > 3. how about if a list of names could be specified for the 'Mail to' or > perhaps a group name(assuming person was in the group they are sending to). > It looks like currently you can setup multiple schedules in order to send to > more then one person, but that would be cumbersome if it was very many. Groups are definitely the way to go with these. Anyone sending whines to large numbers of people should be able to create or ask someone to create a group. > 4. some might like to include additional text beyond the title in the e-mail > to provide the recipients with eg. a description of how the bug list was > generated. > This perhaps could be done in the title depending on how verbose people get. I talked myself out of doing this, but after using the whine schedule for a couple of weeks, I really want more information to go out in these messages too. I'll write up a new version of the patch to include a per-message block of text and a new Subject line to the email. It means adding yet another table to the schema, if anyone's squeamish about that.

This one adds a configurable mail subject and body-blurb per scheduled event.

Comment on attachment 150029 [details] [diff] [review] Scheduled query whining Initial comments..... >+$table{whine_queries} = >+ 'id int auto_increment primary key, Is there a reason for the use of int instead of mediumint? mediumint seems to be the local convention. >+AddGroup('whiners', 'Can generate whine reports'); >+AddGroup('whineatothers', 'Can send whine mail to other users'); > Let's make the system groups use a prefix. We already had name collisions when I added "admin" How about making it "bz_whiners" and "bz_whineatothers" ?? > >+$sth = $dbh->prepare( "SELECT " >+ . "DATE_FORMAT( NOW(), '\%m'), " >+ . "DATE_FORMAT( NOW(), '\%e'), " >+ . "DATE_FORMAT( NOW(), '\%w'), " >+ . "DATE_FORMAT( NOW(), '\%k'), " >+ . "DATE_FORMAT( NOW(), '\%i')"); >+$sth->execute; >+ >+ >+my ($now_month, $now_day, $now_weekday, $now_hour, $now_minute) = >$sth->fetchrow_array; >+ optional: you don't like split() ?? >+ [% IF val == '29' %] >+ <option value="29" selected>on the 29th of the month</option> >+ [% ELSE %] >+ <option value="29">on the 29th of the month</option> >+ [% END %] >+ [% IF val == '30' %] >+ <option value="30" selected>on the 30th of the month</option> >+ [% ELSE %] >+ <option value="30">on the 30th of the month</option> >+ [% END %] >+ optional: It sure seems like the nnxx day of the month could be nn=the day and xx= either st, nd, rd, ir th depending on the last digit.

(In reply to comment #26) > >+$table{whine_queries} = > >+ 'id int auto_increment primary key, > > Is there a reason for the use of int instead of mediumint? mediumint seems to > be the local convention. Not a good reason, I guess. I took it one step up from profiles.id because, in theory, each profile can have its own queries. I guess maxing out a medioumint on these isn't that realistic of a situation, though. > >+AddGroup('whiners', 'Can generate whine reports'); > >+AddGroup('whineatothers', 'Can send whine mail to other users'); > > > Let's make the system groups use a prefix. We already had name collisions when > I added "admin" How about making it "bz_whiners" and "bz_whineatothers" ?? I guess I thought that was an axe better ground in another bug, so I was using the same (lack of) naming convention as the rest of the internal groups. Also, I considered it a safe bet that there wouldn't be a group called 'whiners,' for example. I figured if a Bugzilla admin thinks certain users are whiners, they'd keep it to themselves. > >+my ($now_month, $now_day, $now_weekday, $now_hour, $now_minute) = >$sth->fetchrow_array; > >+ > > optional: you don't like split() ?? Correct. This is easier to read and keep track of than a bunch of integers. > >+ <option value="30">on the 30th of the month</option> > >+ [% END %] > >+ > optional: It sure seems like the nnxx day of the month could be nn=the day and > xx= either st, nd, rd, ir th depending on the last digit. I'm not sure. I'll have to get back to you on that one.

Comment on attachment 150029 [details] [diff] [review] Scheduled query whining ./runtests.pl must pass before this can go in. (you probably need a filterexceptions.pl file in the template directory) If you really want tabs in the email output, use template code to define a variable within the template that emits a tab. Consensus among developers is that all new system groups should be prefixed with a "_" On the split nit, I meant you could use a single call to DATE_FORMAT and split the result instead of 5 calls to it. Use routines in globals to look up userids from name. DBname_to_ID() gets the userid from the name. When people start getting long lists of whines defined, they are going to want to be able to collapse/expand the whining UI. That can be an enhancment later though.

Also, whine scheduling page is missing a title

I am very tired right now, but this patch should address everything but the javascript collapse.

Hmm.... it looks like templates don't like to use group names with an initial underscore. Looks like that means we should go with bz_groupname Aside from that, this is ready to go.

This one has bz_* instead of _*

Oops. The other patch still didn't have a title on the schedule page.

Comment on attachment 151721 [details] [diff] [review] Scheduled query whining Ok, I've started reviewing this. Overall, this is rather nice... My review isn't very thorough yet, but here are quite a few comments and questions. You should focus on writing good comments in the right spots; this code isn't very readable for someone who doesn't understand your scheduling well. In "whine_schedule.cgi" -- should it be "editwhineschedule", by the way? -- the interface is really hard to understand when you have zero whines. You have a "Update / Commit" button first on the page, but what is it related to? That page _seriously_ needs a short introduction to the whining system. Also, when adding a query, it gets really strange: "Sort"? "Title"? Wazzuup? I was also surprised by the fact that you need a stored query to add the whine. What happens when you delete a query that's related to a whine? (I see that get_query has skipping logic for this, but shouldn't the user get warned when deleting?) Deleting whine events doesn't work: DBD::mysql::st execute failed: Column: 'id' in field list is ambiguous [for Statement "SELECT id FROM whine_schedules LEFT JOIN whine_events ON whine_events.id I really cannot understand the administration interface; how do I make queries that get run for a group of users and so on... Some code, then: >Index: checksetup.pl >+$table{whine_queries} = >+ 'id mediumint auto_increment primary key, >+ eventid mediumint not null, >+ query_name varchar(64) not null default \'\', So umm, we have a query_name, but not the userid whose query this is? How does this work? >+ sortkey smallint not null, "default 0"? >+ oneperbug tinyint not null default 0, "onemailperbug"? (now that "one" is really unclear reference) >+$table{whine_schedules} = >+ 'id mediumint auto_increment primary key, >+ eventid mediumint not null, >+ run_day varchar(32), >+ run_time varchar(32), >+ run_next datetime, The semantics of these run_day and run_time fields are a bit vague. How is data coded into them? (we really should document this somewhere...) >+AddGroup('bz_whiners', 'Can generate whine reports'); I thought the system generated the reports. I suggest bz_canusewhines and "Can configure whine reports for himself." >+AddGroup('bz_whineatothers', 'Can send whine mail to other users'); My suggestions: "bz_canusewhineatothers" and "Can configure whine reports for other users." Shouldn't admins automatically belong to these? I also tend to agree with Dave's earlier thought on whineatothers automatically implying being able to whine at yourself. >Index: whine.pl >=================================================================== Come up with a clear separation of phases and use heavy visual indicators to separate them. That'll help people to understand what we're doing at each point of code. >+# days in each month >+my @cal = qw(0 31 28 31 30 31 30 31 31 30 31 30 31); -- How do we handle leap years? I'd prefer "daysInMonth" or something else more descriptive. >+# - check through the database for schedules that have NULL for next-time-run >+# - set to now if the time setting is multiple times per day What's the logic here? When do schedules have NULL for next-time-run? >+my ($now_month, $now_day, $now_weekday, $now_hour, $now_minute) = split(',', $sth->fetchrow_array); Nit: Exceeds 80 chars >+$sth = $dbh->prepare( "SELECT id FROM whine_schedules " >+ . "WHERE run_next IS NULL"); Nit: The dot belongs to the end of the first line, according to our normal coding style :-) >+for my $id (@ids) { >+ # run the null checks >+ my $sth = $dbh->prepare( "SELECT run_day, run_time FROM whine_schedules " >+ . "WHERE id=?" ); What do the "null checks" do? Our source material is already filtered to contain only null run_nexts. Should the comment talk about something else? Also, why don't we get all these three fields in one query and then just go through this stuff in a single loop? Now we have two loops (first iterate through fetchall_arrayref and then through @ids) and quite a few queries instead of just one. >+ $this_day = &check_today($day); >+ >+ # Now that we know if it's this day, we see if it's supposed to be run at >+ # a particular hour. If so, we set it for that hour, and if not, it's set >+ # to now >+ >+ if ($this_day == 1) { You could just use "if (&check_today($day))". When can the statement "It's this day" be false? ;-) Write the comment so that the point comes across. "If the query is scheduled to run today" or something like that. At the end of the comment, don't use two different forms for saying the same thing; I guess "we set it" and "it's set to" are intended to mean the same thing, but the latter sounds like it's already externally set to some value (when it's contrasted against "we set"). >+ # non-numeric times are regularly scheduled Even numeric ones are, but their regularity is different. I guess you mean "If a time specification is non-numeric, it's a time interval like 15min; for all these, we schedule them to run now." (note that I still don't understand why, but that's asked above ;)) Also, add a comment describing that "$time is the hour the whine is scheduled to run on"; "time" could also quite well be a string representation like '23:00:00'. >+ # set it to today + number of hours >+ $sth = $dbh->prepare( "UPDATE whine_schedules " >+ . "SET run_next=DATE_ADD(CURRENT_DATE(), INTERVAL ? HOUR) " >+ . "WHERE id=?"); >+ $sth->execute($time, $id); >+ } >+ else { # set it for tomorrow >+ $sth = $dbh->prepare( "UPDATE whine_schedules " >+ . "SET run_next=" >+ . "DATE_ADD(DATE_ADD(CURRENT_DATE(), INTERVAL ? HOUR), " >+ . "INTERVAL 1 DAY) " >+ . "WHERE id=?"); >+ $sth->execute($time, $id); >+ } It would probably be cleaner to write logic like # If we're already past the running hour, schedule for tomorrow $time += 24 if ($time < $now_hour); and then do the SQL just once. >+ else { >+ >+ my $target_date = &get_next_date($day, ); Please describe this block in a short comment; which case are we handling here? Also, what's that extra comma in get_next_date params? >+ # OK, we now have a target date. Set the time. >+ my $target_time = 0; >+ if ($time =~ /^\d+$/) { >+ $target_time = $time; >+ } my $target_time = ($time =~ /^\d+$/) ? $time : 0; (and kill the comment or make it something understandable ;-)) Also add a comment describing when the $time can be non-numeric and why is it meaningful to make the runtime 0:00 hours then. >+ # and set the next run time >+ $sth = $dbh->prepare( "UPDATE whine_schedules " >+ . "SET run_next=DATE_ADD(?, INTERVAL ? HOUR) " >+ . "WHERE id=?"); Nit: I still prefer the dots in at the end of the previous line. >+# get a list of userids What list of userids? >+for my $uid (@uids) { >+ my $whiner = Bugzilla::User->new($uid); There's no sense in first creating the user objects (loop above), discarding them immediately and then creating them again. Why not just have a @users array with the User objects thrown in? >+ # set the base URL >+ $vars->{'urlbase'} = Param('urlbase'); >+ # add a slash if it needs it >+ if ($vars->{'urlbase'} !~ /\/$/) { >+ $vars->{'urlbase'} .= '/'; >+ } You could quite well do this code far above where you set vars->{'fromaddress'}; there's no need to do this once per user. Actually, it would be best to move the fromaddress initialization quite a bit lower in the file; handling template variables so early isn't necessary. Best to group the initialization here where the templates are actually used. >+ $sth = $dbh->prepare( "SELECT whine_schedules.id, mailto_userid " >+ . "FROM whine_schedules " >+ . "LEFT JOIN whine_events " >+ . "ON eventid = whine_events.id " >+ . "LEFT JOIN profiles " >+ . "ON userid = mailto_userid " >+ . "WHERE run_next <= NOW() " >+ . "AND disabledtext='' " >+ . "AND owner_userid = ?"); Hmm... I don't think you have to recheck disabledtext here; you did it already earlier when filtering into @userids. >+ # unauthorized mailings to others are silently ignored >+ if ( ($uid == $mailto) || >+ ($whiner->in_group('bz_whineatothers')) ){ >+ push @schedules, { >+ 'id' => $id, >+ 'mailto' => $mailto, >+ }; >+ } Note that you should check for data/nomail here (at some point, not necessarily this loop). >+ $sth = $dbh->prepare( "SELECT subject, body FROM whine_events " >+ . "LEFT JOIN whine_schedules " >+ . "ON whine_events.id = whine_schedules.eventid " >+ . "WHERE whine_schedules.id=?"); Hmm. Couldn't we pick the stuff we're querying here earlier and push it into schedule hashes? >+ $vars->{'subject'} =~ s/\s/ /sg; # fix newlines etc. fix == remove, right? >+ # run the query >+ &run_queries($id, $uid, $mailto); Eww, this is complex. run_queries is a monster method that does all sorts of things. And instead of returning values, it assigns them to vars hash. And... And. It's just hard to understand. Can we split this code so that the result is a bit more readable? Like first process oneperbug whinemails and then go on with the normal ones? >+ $template->process("whine/mail_html.html.tmpl", >+ $vars, \$msg) >+ || die($template->error()); Why is it that we want to repeat "html" in the template file names? >+ $vars->{'boundary'} = "-----=====-----" . $$ . "--" . time() . "-----"; I haven't thought this out thoroughly, but wouldn't it make more sense to construct the boundary in the template? >+ my $userobj = new Bugzilla::User($uid); Yet another user object construction! >+ my $sth = $dbh->prepare("SELECT " >+ . "whine_queries.id " >+ . "FROM whine_queries " Again, can we get more information with the previous queries? >+ my @ids = (); Please, name these vars so that it's possible to tell what ids we're talking about. >+ my $searchparams = new Bugzilla::CGI($savedquery); Document this line, please... >+ # swap out queries >+ my $temp_queries = $vars->{'queries'}; Ok, but wouldn't it be much cleaner to construct the original queries hash somewhere else than $vars, so that you didn't have to do this kind of tricks? (not that I wouldn't want the vars stuff to go away anyway; see comments above ;-)). >+ $vars->{'alternatives'} = [{'content' => $msg, >+ 'type' => 'text/plain', >+ }]; Why do you need to use vars hash as a temp variable? >+# check_today gets a run day from the schedule and checks to see if it matches >+# today Document possible formats here. >+sub check_today { >+ >+ my $run_day = shift; >+ >+ >+ my $run_today = 0; Nit: Kill those spurious empty lines. Rather than use $run_today, why not just return 1 whenever you find a matching row (and return 0 at the end)? >+ elsif ( >+ ($run_day eq 'All') >+ || (($run_day eq 'Sun') && ($now_weekday == 0 )) >+ || (($run_day eq 'Mon') && ($now_weekday == 1 )) >+ || (($run_day eq 'Tue') && ($now_weekday == 2 )) >+ || (($run_day eq 'Wed') && ($now_weekday == 3 )) >+ || (($run_day eq 'Thu') && ($now_weekday == 4 )) >+ || (($run_day eq 'Fri') && ($now_weekday == 5 )) >+ || (($run_day eq 'Sat') && ($now_weekday == 6 )) >+ || (($run_day eq 'last') && ($now_day == $cal[$now_month] )) >+ || ($run_day eq $now_day) >+ ) { >+ $run_today = 1; >+ } "last" will work wrong for leap year Februaries. How about rewriting the DOW mapping as an index? Like: if (length($run_day) == 3 && index("SunMonTueWedThuFriSat", $run_day)/3 == $now_weekday) { ... } (at least clean up the alignment of the &&:s) >+ return $run_today; >+ >+} >+ >+ >+ Nit: Empty lines... >+# reset_timer sets the next time a whine is supposed to run, assuming it just >+# ran moments ago. Document the parameters, and DON'T use $id -- you have so many ids already... >+ # OK, if it's a many-times-per-day event, and one that runs today, >+ # schedule it for the next event Nit: You say "OK" in quite a few comments; that's not really necessary unless you've just passed a really gigantic validating condition ;-) >+ my $minute_interval = 60; Why this default? Which case does it catch? >+ if ($run_time eq '30min') { >+ $minute_interval = 30; >+ } >+ elsif ($run_time eq '15min') { >+ $minute_interval = 15; >+ } How about a generic regexp approach like /^(\d+)min$/? >+ # This is the easy way to do it. I promise. What are you talking about? >+ # Whis would be a really simple one except we're using DATE_SUB() to drop >+ # the seconds from current time. Nit: s/Whis/This/ >+ >+ >+} >+ >+ >+ >+ >+ >+ >+ >+ >+ >+ Nit: Empty lines! AA! ;-) >+sub get_next_date { >+ >+ my $day = shift; Again, describe what this $day is and what sort of values it can contain. >+ # OK, what we need to do here is determine what the difference in days >+ # should be Nit: Difference between what and what? (I know the answer, but I worked for it! ;-)) >+ # this one is a mouthful. What it does is take the current date, "A mouthful"? It's a pretty normal algorithm for this sort of thing. Except that you can't implement it like that for it to work, because the month lengths can then cause tasks to be executed on the second-last day of the month, which is wrong. Try |SELECT DATE_SUB(DATE_ADD(CAST('2004-08-31 0:00' as date), INTERVAL 1 MONTH), INTERVAL 31 DAY);| for a demonstration. >+ elsif ($day !~ /^\d+$/) { >+ my $day_num = 0; Why this default? What do we want to achieve? >+ if ($day eq 'Sun') { >+ $day_num = 0; >+ } Again, I suggest using the appropriate index/3; that's so much cleaner. >+ else { >+ $add_days = $day - $now_day; >+ if ($add_days >= 0) { Please, comment on these: what are we doing here? Which case is this else block handling? The conditional blocks are so long that it's fairly hard to figure out what's going on. >Index: whine_schedule.cgi >=================================================================== >+# Check whether or not the user is logged in and, if so, set the $::userid This comment is out of place. >+my $whines = {}; What's this? You have pretty many global variables. If you can't get rid of them, try to give them descriptive names and use comments to describe their contents. >+my $mail_others = 0; >+if (UserInGroup('bz_whineatothers')) { >+ $mail_others = 1; >+} Err, how about just |$mail_others = UserInGroup('...');|? Preferably $can_mail_others or something similar. >+ for my $id (@eventids) { Note that parsing the input should be based on what's coming in on the form, not what's in the DB already. I can't think of a situation where this pose a problem (as all the table ids are auto_increments), but we recently had a slightly similar issue with flags (bug 223878). >+ $sth->execute($id, $userid); Again, please kill this extensive use of "id" and tell us which id's you're talking about :) >+ # check the subject and body for changes >+ $sth = $dbh->prepare("SELECT subject, body FROM whine_events " >+ . "WHERE id=?"); Hmm, why didn't we select this stuff earlier when getting the ids? >+ trick_taint($subject) if $subject; >+ trick_taint($body) if $body; Why do you trick_taint these? Does our dbh placeholder system care about taints? I don't think it should... Anyway, put comments above the trick_taints -- undocumented tricks should always sound an alarm :-) >+ my @schedules = (); >+ for (@{$sth->fetchall_arrayref}) { >+ push @schedules, $_->[0]; >+ }; scheduleIds, really... >+ # we need to double-check all of the user IDs in mailto Double check? For what? (a better comment will do suffice) >+ $arglist->{"mailto_$sid"} = { >+ 'type' => 'single', >+ }; This leads into pretty wild notations like: |mailto_1: jt matched Jouni Heikniemi <jth@foo.invalid>| -- can you have anything more readable than "mailto_$sid"? Changing the mailto targets doesn't seem to work -- when I make a change and click commit, it doesn't get saved? >+ else { >+ my $o_day = $cgi->param("orig_day_$sid"); What's the function of this else block? (can we have a comment here?) >+ $mail_uid = DBname_to_id($mailto); What if it doesn't match? >+ if ( ($o_day ne $day) >+ || ($o_time ne $time) ){ Nit: Strange spacing. >+ trick_taint($day) if length($day); >+ trick_taint($time) if length($time); Why not just "if ($day)"? >+ # queries Queries what? (the point: whole this form handler code is really hard to read, since there are so many levels of it and so many functions intertwined; please use more descriptive comments) >+ my $o_sort = $cgi->param("orig_query_sort_$qid"); >+ my $sort = $cgi->param("query_sort_$qid"); >+ my $o_queryname = $cgi->param("orig_query_name_$qid"); >+ my $queryname = $cgi->param("query_name_$qid"); >+ my $o_title = $cgi->param("orig_query_title_$qid"); >+ my $title = $cgi->param("query_title_$qid"); >+ my $o_oneperbug = $cgi->param("orig_query_oneperbug_$qid"); >+ my $oneperbug = $cgi->param("query_oneperbug_$qid"); Wouldn't it make more sense to compare these to DB values (since you're essentially doing an id query above there anyway)? (now multiple forms changing the same values can cause some edits to disappear, even though it's a rare case) If you think this is irrelevant, I'm fine with just ignoring it. >+ $o_sort = '' unless length($o_sort); How about writing these as "|| ''" after the $cgi->param("xxx")? (above) >+ if ($oneperbug eq 'on') { >+ $oneperbug = 1; >+ } >+ elsif ($oneperbug eq 'off') { >+ $oneperbug = 0; >+ } |$oneperbug = ($oneperbug eq 'on')|? >+ if ( ($o_sort ne $sort) >+ || ($o_queryname ne $queryname) >+ || ($o_oneperbug xor $oneperbug) >+ || ($o_title ne $title) ){ Nit: Odd spacing... >+ trick_taint($sort) if length $sort; >+ trick_taint($queryname) if length $queryname; >+ trick_taint($title) if length $title; >+ trick_taint($oneperbug) if length $oneperbug; Well, how about even detaint_natural for numeric values, huh? I don't think you're checking for mail_others permission when saving possible changes to the mail recipient field? >+# Here is the data layout: What data layout? What are you talking about? (I'm guessing it's the params hash) >+ >+ >+ >+ >+ >+ >+ Please use some sort of separators instead of these huge empty line blocks (if you must separate) >+ # a list of event IDs is useful. For what? ;-) (don't use comments that have no information) >Index: template/en/default/global/user-error.html.tmpl >=================================================================== >+ Sorry, you aren't a member of the 'whiners' group, and so >+ you aren't allowed to schedule whine reports. "The Whiners Group" doesn't exactly sound that good. How about "Sorry, but you are not allowed to schedule whine reports." >Index: template/en/default/whine/mail_html.html.tmpl >=================================================================== >+ <p align=left> Quote "left". >+ <p align=left> Here too. >+ <a href="[%+ urlbase FILTER html %]whine_schedule.cgi">Click here to >+ edit your whine schedule</a> Is urlbase guaranteed to end in /? >+ <td align="left"><a href="[%+ urlbase FILTER html %]show_bug.cgi?id=[% bug.bug_id >+ FILTER html +%]">[% bug.bug_id FILTER html %]</a></td> You're exceeding the line length of 80 chars. Feel free to wrap on TT tags; chomp will kill the lf. Make bug.bug_id a filterexception rather than filter it everywhere, please (it's more readable that way). >Index: template/en/default/whine/mail_plaintext.txt.tmpl >=================================================================== Some of the previous comments apply here as well. >+Query title: [%+ query.title %] How about: [% query.title %] [% "-" FILTER repeat(query.title.length) %] >+ Asignee: [% bug.$loginname %] s/Asignee/Assignee/ >Index: template/en/default/whine/schedule.html.tmpl >=================================================================== I tend to think it would be better to call the "whines" field "events" to match your DB terminology. >+ # sort: integer that sets execution order on named queries, >+ # in ascending order Since the "ascending order" is not a relevant fact here, I suggest you remove it from the interface documentation. >+[% title = "Set up Whining" %] The system is respectable, but "whining" should suffice ;-) >+<p align=left> Quote the attribute... >+ name="remove_event_[% whine.key FILTER html %]"> whine.key is a candidate for filterexceptions? >+ <input type="text" name="event_[% whine.key FILTER html %]_subject" >+ size="60" value="[% whine.value.subject FILTER html %]"> Set maxlength to help the user not write over 64 chars (that's pretty easy to exceed by the way). >+ <td valign="top" align="right"> >+ Message Body: >+ </td> That's not really the message body; it's more like "Description text (sent along with the whine mail)" >+ name="orig_day_[% schedule.id FILTER html %]"> Of course, schedule.id should also be a filterexception... >+ <input type="hidden" name="orig_mailto_[% schedule.id FILTER html %]" >+ value="[% schedule.mailto FILTER html %]"> Bizarre indentation; align 'value' with 'type'. Repeat for several other instances. >+ No queries <br /> We don't use xhtml here; <br> will suffice. >+ <input type="text" name="query_sort_[% query.id FILTER html %]" Filterexception as well. >+ [% IF query.oneperbug == 1 %] >+ <input checked type="checkbox" >+ name="query_oneperbug_[% query.id FILTER html %]"> >+ [% ELSE %] >+ <input type="checkbox" >+ name="query_oneperbug_[% query.id FILTER html %]"> >+ [% END %] Just make the "checked" come out of the conditional and avoid copypasting rest of the checkbox code. >+<p align="left"> Now that it crossed my mind, _why_ do you align these paragraphs to left; they tend to do it by themselves, right? >+ [% IF q == thisquery %] >+ <option selected value="[% q FILTER html %]">[% q FILTER html %]</option> >+ [% ELSE %] >+ <option value="[% q FILTER html %]">[% q FILTER html %]</option> >+ [% END %] As with the checkbox above. >+[% BLOCK day_field %] >+ <select name="day_[% schedule.id FILTER html %]"> >+ [% IF val == 'All' %] >+ <option value="All" selected>Each day</option> >+ [% ELSE %] >+ <option value="All">Each day</option> ... And here with _really_ many options repeated totally unnecessarily... >+ [% IF val == '1' %] >+ <option value="1" selected>on the 1st of the month</option> >+ [% ELSE %] >+ <option value="1">on the 1st of the month</option> >+ [% END %] >+ [% IF val == '2' %] >+ <option value="2" selected>on the 2nd of the month</option> >+ [% ELSE %] >+ <option value="2">on the 2nd of the month</option> >+ [% END %] Get this stuff into a loop of some sorts, this is really ugly. >+[% BLOCK time_field %] >+<select name="time_[% schedule.id FILTER html %]"> >+ [% IF val == '0' %] >+ <option value='0' selected>at midnight</option> >+ [% ELSE %] >+ <option value='0'>at midnight</option> >+ [% END %] Here too; a loop will do better. Also, please use 24 hour clock; Bugzilla doesn't really use 12 hour anywhere else.

Comment on attachment 151644 [details] [diff] [review] Scheduled query whining Removing review request on an old patch.

back to you erik

My turn. First off, thank you for your review. I really appreciate the detailed feedback. I went over everything in comment 34. For brevity's sake, if I don't respond to something in the comment, that means I addressed the problem in this patch. I need to know of the code flow makes any more sense. Hopefully it does. I did a lot of comment and loop repair. (In reply to comment #34) > (From update of attachment 151721 [details] [diff] [review]) > I really cannot understand the administration interface; how do I make > queries that get run for a group of users and so on... I hope the new instructions in the template fix that. > >Index: checksetup.pl > >+$table{whine_queries} = > >+ 'id mediumint auto_increment primary key, > >+ eventid mediumint not null, > >+ query_name varchar(64) not null default \'\', > > So umm, we have a query_name, but not the userid whose query this is? How > does this work? whine_schedules and whine_queries both link, via eventid, to the primary key of whine_events, which contains a userid. > Shouldn't admins automatically belong to these? I also tend to agree with > Dave's earlier thought on whineatothers automatically implying being able to > whine at yourself. Well, admins can grant membership in the group. No one is automatically granted access except the admin account. I think this is better as a default-off option, since it's not required for Bugzilla to work. In the new patch, whineatothers (now bz_canusewhineatothers) is inside of the other group. > >Index: whine.pl > >=================================================================== > >+ # set it to today + number of hours > >+ $sth = $dbh->prepare( "UPDATE whine_schedules " > >+ . "SET run_next=DATE_ADD(CURRENT_DATE(), INTERVAL ? HOUR) " > >+ . "WHERE id=?"); > >+ $sth->execute($time, $id); > >+ } > >+ else { # set it for tomorrow > >+ $sth = $dbh->prepare( "UPDATE whine_schedules " > >+ . "SET run_next=" > >+ . "DATE_ADD(DATE_ADD(CURRENT_DATE(), INTERVAL ? HOUR), " > >+ . "INTERVAL 1 DAY) " > >+ . "WHERE id=?"); > >+ $sth->execute($time, $id); > >+ } > > It would probably be cleaner to write logic like > > # If we're already past the running hour, schedule for tomorrow > $time += 24 if ($time < $now_hour); > > and then do the SQL just once. Can't actually do that now because I had to alter the code there to fix a bug I found. > my $target_time = ($time =~ /^\d+$/) ? $time : 0; I've been avoiding ?: notation because explicit if/else makes the logic more obvious, even if it clutters things up. I've been dinged on older patches for trying to save space like that, and if it's all the same, I'd rather keep the explicit if/else so someone else doesn't get angry with my ?:'s. If it's not all the same, though, I'll go back and change them. > Eww, this is complex. run_queries is a monster method that does all sorts of > things. And instead of returning values, it assigns them to vars hash. And... > And. It's just hard to understand. Can we split this code so that the result is > a bit more readable? Like first process oneperbug whinemails and then go on > with the normal ones? Actually, I pulled the mail delivery code out into its own function and it seems a lot easier to follow run_queries now, at least to me. I think processing oneperbug mails first might not really help at this point. > I haven't thought this out thoroughly, but wouldn't it make more sense to > construct the boundary in the template? I'm uncertain. I've been constructing the boundary based on the date and the process ID, in order to keep the boundaries unique from message to message, and I'm not sure I can find a way that I'm happy with to have the template determine stuff like that. I might be able to be talked out of it, but if it's not a big deal... > Note that parsing the input should be based on what's coming in on the form, > not what's in the DB already. I can't think of a situation where this pose a > problem (as all the table ids are auto_increments), but we recently had a > slightly similar issue with flags (bug 223878). I didn't do this and I'll tell you why. Since whine schedules each belong to an individual, there is no way two different people are going to be editing the same list of whines, unlss they're doing something weird. Also, the items that it's indexing from don't appear in the form until after a submit, which means they're in the database before they ever make it to the form. > Why do you trick_taint these? Does our dbh placeholder system care about > taints? I don't think it should... Anyway, put comments above the trick_taints > -- undocumented tricks should always sound an alarm :-) The new DB functions do not auto-detaint, so this is standard procedure. > This leads into pretty wild notations like: > > |mailto_1: jt matched Jouni Heikniemi <jth@foo.invalid>| > > -- can you have anything more readable than "mailto_$sid"? Easily? No. That would require some major-ish changes to the user matching code, which I think is a good idea, but I don't think it should get rolled into this bug. > Changing the mailto targets doesn't seem to work -- when I make a change and > click commit, it doesn't get saved? Worksforme. Try the new patch and see if it helps. > >+ trick_taint($day) if length($day); > >+ trick_taint($time) if length($time); > > Why not just "if ($day)"? Because $day can contain "0", which will fail, but is a legitimate value. Same with $time. > >+ if ($oneperbug eq 'on') { > >+ $oneperbug = 1; > >+ } > >+ elsif ($oneperbug eq 'off') { > >+ $oneperbug = 0; > >+ } > > |$oneperbug = ($oneperbug eq 'on')|? Again, if it's not a show-stopper, I prefer putting my logic in longhand. > I don't think you're checking for mail_others permission when saving possible > changes to the mail recipient field? No. There's no way for the user to do that unless they were trying to trick the form, and mail_others is being checked at mail transmission time, so the net effect is no mail being sent. > >+<p align="left"> > > Now that it crossed my mind, _why_ do you align these paragraphs to left; > they tend to do it by themselves, right? I've seen browsers way-back-when that didn't automatically assume left justification in table cells, so I explicityly say it if I can remember to.

Ah! The question of how to write logic expressions in intriguing, one of my favorites in syntax unification indeed. I wrote my comments on that issue into a separate blog entry, see <http://www.heikniemi.net/hc/archives/000103.html>. I'm not going to refuse to r+ this because of the ?:s and ifs, but the generic code browsability issues mentioned in the blog entry are the reason I was paying so much attention to them: The details are (in some parts of the code) verbose and confusing enough to steal attention from the code flow in general. I'll start reviewing now and hope to finish before Monday morning ;-)

Comment on attachment 154157 [details] [diff] [review] New scheduled query whining Ok, I think I almost understand the admin interface now ;-) I have a couple of comments, but they're later on in the code. One thing that we really need here is a preview option - even with the current instructions, it's insanely hard to imagine how the whining would actually turn out, particularly for the first time. Any chance you could cough that up? It should be relatively straightforward since you have the mail templates available, although it may require some refactoring in the backend to get the proper query executed and data fed to the templates. If it's really hard, do it on another bug - but I think we should definitely have it. After that, I'd be more willing to let the current UI go... >Index: checksetup.pl >=================================================================== >+# place one group within another group >+sub group_in_group { >+ my $subset = shift; >+ my $superset = shift; Suggestions on terminology changes: group_in_group -> add_group_to_group subset -> child superset -> parent (the set algebra terminology is perhaps a bit misleading here?) Also, please add a $bless param and start using this everywhere in checksetup (where group_group_map is updated now), or revert to a single insert statement. It hurts to think there would be both a method and inline implementations for the same thing. Either approach is easy. :-) >+ $dbh->do("INSERT IGNORE INTO group_group_map >+ (member_id, grantor_id, isbless) >+ VALUES ($subset_id, $superset_id, 0)"); Don't use INSERT IGNORE but rather do an extra select - you're going to have to get rid of MySQL specifics anyway. >Index: editwhines.cgi >=================================================================== >+# $events will contain all of this user's whine events >+my $events = get_events($userid); Mention the data type (array/hash ref?). Why is it a ref anyway? What's its structure? >+if ($cgi->param('update')) { >+ # get the list of events >+ >+ if ($cgi->param("add_event")) { What's that comment talking about? >+ # we create a new event >+ $sth = $dbh->prepare("INSERT INTO whine_events " >+ . "(owner_userid) " >+ . "VALUES (?)"); $sth = $dbh->prepare("FOO" . "BAR"); If you don't agree to move the dots to the ends of the lines, at least align the "s. Repeat this comment for all your SQL statements ;-) >+ my $subject = ($cgi->param("event_" . $eventid . "_subject") or ''); >+ my $body = ($cgi->param("event_" . $eventid . "_body") or ''); Nit: Unnecessary catenation; "event_${eventid}_subject" will do. >+ $sth = $dbh->prepare("SELECT whine_schedules.id FROM whine_schedules " Nit: Exceeding 80 chars. >Index: whine.pl >=================================================================== >+# This script needs to check through the database for schedules that have >+# run_next set to NULL, which means that schedule is new or has been altered. >+# It then sets it to run immediately if it runs multiple times today, >+# otherwise to the appropriate day and time. Good! I'd still fix the wording a bit, since people can't necessarily know "if it runs multiple times today" means that it runs at intervals that are not fixed with regards to the clock. A reader easily thinks that if a query has been scheduled to run at 8:00 and 16:00 each day, then its run multiple times per day in the sense meant here. Yeah, it's a complex issue with lots of new terminology ;-( >+# exit quietly if the system is shut down >+if (Param('shutdownhtml')) { >+ exit; >+} I guess we could say _something_ just to avoid confusion (whines are not working, but no error message is printed!)? >+# There really is not a good global mail-from address yet, and whine messages >+# shouldn't be sent from the address of the person who created them because of >+# bounces and other types of replies that are more useful directed at the >+# maintainer. Err, hmm. "Send whines from the maintainer address. It's not a good idea to use the whine creator address because the admin can make more use of bounces and other replies." would probably be a better wording. >+# get the current date and time from the database >+$sth = $dbh->prepare( "SELECT DATE_FORMAT( NOW(), '\%y,\%m,\%e,\%w,\%k,\%i')"); What are those backslashes doing? >+# Alter February in case of a leap year. This simple way to do it only >+# applies if you won't be looking at February of next year, which whining >+# doesn't need to do. >+unless ($now_year % 4) { $daysinmonth[2] = 29 }; You also have to note 100 and 400 year rules. Ok, I agree this code quite probably will never see year 2100, but I don't want somebody pointing at Bugzilla code and saying they can't even do leap year detection properly. >+ "SELECT DISTINCT whine_events.id, whine_events.owner_userid, " . >+ " whine_events.subject, whine_events.body " . Nit: align "whine_"s on both lines. >+ return if (-d $datadir && -e "$datadir/nomail"); Err, no. The function nomail file is to contain the email addresses of the people who should receive no email. You should go through the contents of that file and filter the recipient list accordingly. >+# print "##################\n"; >+# print $msg; >+# print "##################\n\n"; >+# die; Kill debug code. >+ SendSQL($sqlquery); Is there a particular reason for using old-style SendSQL here, by the way? >+ index("SunMonTueWedThuFriSat", $run_day)/3 == $now_weekday) >+ { >+ return 1; Nit: { to the end of the last line. >+ || ($run_day eq $now_day) >+ ) >+ { >+ return 1; Nit: Here too. >+ # If the event is supposed to run today, and it runs many times per day, >+ # it shall be scheduled to run immediately. Slightly akin to what I commented on earlier, be careful with your wording. I don't think you mean what you say here; you're talking about how often this _schedule_ fires, not about the actual executions of the _event_ (which can be run several times a day even if any schedule were not set to run several times a day). Right? >Index: template/en/default/account/prefs/saved-searches.html.tmpl >=================================================================== >+ [% IF q.usedinwhine %] >+ Remove from <a href="editwhines.cgi">whining</a> first >+ [% ELSE %] >+ <a href="buglist.cgi?cmdtype=dorem&amp;remaction=forget&amp;namedcmd= >+ [% q.name FILTER html %]">Forget</a> >+ [% END %] Nit: Two space indents for the blocks. This UI is a bit confusing... But I can't come up with a better one, and nobody should encounter it without first manually setting up the whines, so I guess that's ok. >+ [% ELSIF error == "unknown_userid" %] >+ [% title = "Unknown User ID" %] >+ There is no user with ID <em>[% userid FILTER html %]</em>. This isn't really right, you know. The parameter you call userid is actually the mailto value input by the user. Do we have a habit of validating the results of match_field? If yes, use the method used elsewhere; if not, don't validate unless there's a specific reason. (it's a code complexity thing) >Index: template/en/default/whine/mail.html.tmpl >=================================================================== >+ # user: user object for the person who caused this whine to happen "for the person who scheduled this whine" -- the one who causes whines to happen is the developer who slouches enough to make his bugs exceed the whine trigger limit ;-) The same goes for mail.txt.tmpl. >Index: template/en/default/whine/mail.txt.tmpl >=================================================================== >+ Severity: [% bug.bug_severity -%] >+ Platform: [% bug.rep_platform %] >+ Assignee: [% bug.$loginname %] >+ Status: [% bug.bug_status %] [% IF bug.resolution -%] Nit: Wrong indentation for Assignee: >Index: template/en/default/whine/schedule.html.tmpl >=================================================================== >+ To set up a new whine event, click "Add a new event." Enter a subject line >+ for the message that will be sent, along with a block of text that will >+ accompany the [% terms.bug %] list in the body of the message. This is not strictly correct for onemailperbug whines, but I guess it's ok to ignore that. >+ under "Search" and add a title for the [% terms.bug %] table. The optional >+ number entered under "Sort" will determine the order (lowest to highest) for >+ multiple saved searches. If you check "One message per [% terms.bug %]," Explain: What "order" -- you're talking about the order of bug lists in the message IF multiple queries are added to a single whine report. >+ <td align="left" bgcolor="#FFEEEE"> >+ Not scheduled to run<br> >+ <input type="submit" value="Add Schedule" >+ name="add_schedule_[% event.key %]"> Make this button "Add a new schedule" (for reasons explained below) >+ <input type="submit" value="Add Schedule" >+ name="add_schedule_[% event.key %]"> ... and same here. This, because when you click Add Schedule to add your first schedule, you get the new schedule fields and this button. It's pretty natural to think that you'd have to click "Add schedule" to actually add this one, but in reality you have to hit commit / update to make it happen. By throwing in the "a new" this confusion will be avoided (although the UI will still be a bit awkward, but perhaps we can work on it incrementally once we have more experience on the system). >+ <input type="submit" value="Add Query" name="add_query_[% event.key %]"> Same here. Don't forget to change the introductory texts when you change the button labels. >+ <th> >+ Sort >+ </th> >+ <th> >+ Search >+ </th> >+ <th> >+ Title >+ </th> Nit: Compress to "<th>Sort</th>" and so on. >+ { 'All' => 'Each day', }, >+ { 'MF' => 'Monday through Friday', }, >+ { 'Sun' => 'Sunday', }, >+ { 'Mon' => 'Monday', }, Oh ok, although I'd imagine an array of anonymous arrays would be quite a lot more effective, but this is acceptable too... All in all, the patch is considerably better now (as you can tell by the lack of readability complaints wrt whine.pl :-)). Good work!

I still need to run through this again, but a few comments on Jouni's comments... I'd sugest implementing the preview in one of 2 ways or, for extra credit, both. 1) Run this report for me and send it to me now 2) Run this report for me and display the text/html version for me now by "both", I mean that we do (1) and then display (2) Also, we should all understand that checksetup is extremely database dependent and always will be. See bug 248175. In a nutshell, the legacy checksetup will be used to get mysql installations migrated to an epoch state where a new, db-independent checksetup takes over.

Comment on attachment 154157 [details] [diff] [review] New scheduled query whining A few additional comments. In checksetup, Change GroupDoesExist to return the id of the Group and then use it. Use the terms "grantor" and "member" for consitency with the schema. +# Run the queries for each event +for my $event (@events) { + $sth = $dbh->prepare( "SELECT whine_schedules.id, mailto_userid " . + "FROM whine_schedules " . + "LEFT JOIN whine_events " . + "ON eventid = ? " . + "WHERE run_next <= NOW() "); In whine.pl, you have some loops that will presumably go through a large number of iterations. Inside those loops, you keep reusing the same $sth variable and doing $dbh->prepare followed by $sth->execute. That defeats the purpose of having a distinct prepare and execute. Prepare once (either outside the loop or only if the handle is not already set and use a distinct name for each reusable prepare. This keeps the load down by compiling the sql once and reusing it for repeated executes. +# Get a list of all of the events that need to be run and who owns them +my @events = (); +$sth = $dbh->prepare( + "SELECT DISTINCT whine_events.id, whine_events.owner_userid, " . + " whine_events.subject, whine_events.body " . + "FROM whine_events " . + "LEFT JOIN profiles " . + " ON profiles.userid = whine_events.owner_userid " . + "LEFT JOIN whine_schedules " . + " ON whine_schedules.eventid = whine_events.id " . + "WHERE whine_schedules.run_next <= NOW() " . + "AND profiles.disabledtext = ''" +); +$sth->execute; + +# Add the user to the list only if they are allowed to use whines +for (@{$sth->fetchall_arrayref}) { + my $eventid = $_->[0]; + my $uid = $_->[1]; + my $subject = $_->[2]; + my $body = $_->[3]; + my $user = Bugzilla::User->new($uid); + $subject =~ s/\s/ /sg; # remove newlines etc. + if ($user->in_group('bz_canusewhines')) { + push @events, { + "eventid" => $eventid, + "user" => $user, + "subject" => $subject, + "body" => $body, + }; + } +} + [[[TRY PUTTING A LONG SLEEP STATEMENT HERE]] +# Run the queries for each event +for my $event (@events) { Imagine 50,000 events. Instead if pulling all of the events into memory and creating new hashes for each, I think it would be better to fetch one even at a time and process it. Also, make sure that 100 events that use the same user object will get 100 references to the same user object rather than each getting their own as I suspect will happen now. What happens if the cron is set to run these every 15 minutes and one of these is still running as the cron launches the second one? Will the second one try to redo everything the first one had already started on? [test this by putting a long sleep statement where I indicated above. I am not a big fan of locking by process id, etc.. The simplest solution may be to confirm immediately before processing each event that it is STILL scheduled before NOW() and call the reset_timer() function as soon as possible thereafter. Essentially, switch to main, LOCK, verify it is STILL scheduled int the past, reset its timer, UNLOCK, then send the report. I think we would rather skip sending a report if some odd error happens anyway than try a failing report every time we service the queue, so I view the fact that it wont try again right away as an advantage not a disadvantage. Is there anything that sanitycheck.cgi should be checking for? Queries owned by users who dont exist?? Scheduled queries that should have been run more than 4 hours ago but havent been?

(In reply to comment #38) > Ah! The question of how to write logic expressions in intriguing, one of my > favorites in syntax unification indeed. I wrote my comments on that issue into a > separate blog entry, see <http://www.heikniemi.net/hc/archives/000103.html>. > > I'm not going to refuse to r+ this because of the ?:s and ifs, but the generic > code browsability issues mentioned in the blog entry are the reason I was paying > so much attention to them: The details are (in some parts of the code) verbose > and confusing enough to steal attention from the code flow in general. I don't want to give the impression that I don't like that kind of syntax. I'm just really gun-shy about using terse logic in a bugzilla patch due to past experience. If there's enough of a general agreement to shorten syntax if possible, great, but I've been deliberately using long form believing the opposite sentiment to be popular. I'll try to clean some of that stuff up in my next patch.

(In reply to comment #39) > (From update of attachment 154157 [details] [diff] [review]) > Ok, I think I almost understand the admin interface now ;-) I have a couple of > comments, but they're later on in the code. One thing that we really need here > is a preview option - even with the current instructions, it's insanely hard to > imagine how the whining would actually turn out, particularly for the first > time. Any chance you could cough that up? It should be relatively > straightforward since you have the mail templates available, although it may > require some refactoring in the backend to get the proper query executed and > data fed to the templates. If it's really hard, do it on another bug - but I > think we should definitely have it. After that, I'd be more willing to let the > current UI go... I'm working on a preview. > Also, please add a $bless param and start using this everywhere in checksetup > (where group_group_map is updated now), or revert to a single insert statement. > It hurts to think there would be both a method and inline implementations for > the same thing. Either approach is easy. :-) I have to bat that around. > >+ $dbh->do("INSERT IGNORE INTO group_group_map > >+ (member_id, grantor_id, isbless) > >+ VALUES ($subset_id, $superset_id, 0)"); > > Don't use INSERT IGNORE but rather do an extra select - you're going to have to > get rid of MySQL specifics anyway. I think I'll go with Joel's comment about how checksetup will probably never rid itself of mysql-dependence. > >Index: editwhines.cgi > >=================================================================== > > >+# $events will contain all of this user's whine events > >+my $events = get_events($userid); > > Mention the data type (array/hash ref?). Why is it a ref anyway? What's its > structure? The main reason it's a ref is to keep the arrows consistent as in $ref->{'key'}->{'subkey'}->{'etc'} rather than $ref{'key'}->{'subkey'}->{'etc'} It seems to make more sense to just use a ref in cases like that. > >+ # we create a new event > >+ $sth = $dbh->prepare("INSERT INTO whine_events " > >+ . "(owner_userid) " > >+ . "VALUES (?)"); > > $sth = $dbh->prepare("FOO" . > "BAR"); > > If you don't agree to move the dots to the ends of the lines, at least align > the "s. Repeat this comment for all your SQL statements ;-) Somehow, the last patch got through with the dots only moved in one of the files. I've fixed that for the next version. I wasn't being stubborn, just absent-minded. > >Index: whine.pl > >=================================================================== > >+# exit quietly if the system is shut down > >+if (Param('shutdownhtml')) { > >+ exit; > >+} > > I guess we could say _something_ just to avoid confusion (whines are not > working, but no error message is printed!)? Hopefully the administrator is aware that Bugzilla has been shut down, and doesn't need to be harassed by a cron job that runs as frequently as every 15 minutes. > >+# get the current date and time from the database > >+$sth = $dbh->prepare( "SELECT DATE_FORMAT( NOW(), '\%y,\%m,\%e,\%w,\%k,\%i')"); > > What are those backslashes doing? I could have sworn I put those in there after something went haywire due to them not being in there, but I tried it recently and it seems fine. I've changed it to not use backslashes, but just to make myself happy I swapped the single and double quotes. > You also have to note 100 and 400 year rules. Ok, I agree this code quite > probably will never see year 2100, but I don't want somebody pointing at > Bugzilla code and saying they can't even do leap year detection properly. I was actually unaware of that. Fixed. > Kill debug code. ARGH! I had these files completely covered in debug code while I was working on it and I thought I got rid of all of it. > >+ SendSQL($sqlquery); > > Is there a particular reason for using old-style SendSQL here, by the way? I thought there was, but there doesn't seem to be. I've changed it to the new style, and hopefully it'll test OK (not 100% finished yet). > >+ || ($run_day eq $now_day) > >+ ) > >+ { > >+ return 1; > > Nit: Here too. Actually, I've been told to use { lined up with } if there's a multi-line if condition, like: if (foo and bar and baz) { something(); } ...but I can change it. > Slightly akin to what I commented on earlier, be careful with your wording. I > don't think you mean what you say here; you're talking about how often this > _schedule_ fires, not about the actual executions of the _event_ (which can be > run several times a day even if any schedule were not set to run several times > a day). Right? Yeah, it's a common wording mistake I've made. > >Index: template/en/default/account/prefs/saved-searches.html.tmpl > >=================================================================== > This UI is a bit confusing... But I can't come up with a better one, and nobody > should encounter it without first manually setting up the whines, so I guess > that's ok. That's sort of what went through my head when I made it. It's ugly but it's probably harmless. > >+ [% ELSIF error == "unknown_userid" %] > >+ [% title = "Unknown User ID" %] > >+ There is no user with ID <em>[% userid FILTER html %]</em>. > > This isn't really right, you know. The parameter you call userid is actually > the mailto value input by the user. Do we have a habit of validating the > results of match_field? If yes, use the method used elsewhere; if not, don't > validate unless there's a specific reason. (it's a code complexity thing) This was a weird one. I was certain when I added that stuff that there was a way a bad login could get passed through CGI, somehow bypassing match_field, but I went back and read it, scratched my head, and deleted it after not being able to re-think of a way that would happen. > >Index: template/en/default/whine/schedule.html.tmpl > >=================================================================== > > >+ To set up a new whine event, click "Add a new event." Enter a subject line > >+ for the message that will be sent, along with a block of text that will > >+ accompany the [% terms.bug %] list in the body of the message. > > This is not strictly correct for onemailperbug whines, but I guess it's ok to > ignore that. Well, it would be a list consisting of one bug... > >+ { 'All' => 'Each day', }, > >+ { 'MF' => 'Monday through Friday', }, > >+ { 'Sun' => 'Sunday', }, > >+ { 'Mon' => 'Monday', }, > > Oh ok, although I'd imagine an array of anonymous arrays would be quite a lot > more effective, but this is acceptable too... Originally, there was a good reason to use hashes, but that reason is gone and I've changed it to arrays. > All in all, the patch is considerably better now (as you can tell by the lack > of readability complaints wrt whine.pl :-)). Good work! Thanks. I'm glad to see it get closer to something we can land.

(In reply to comment #40) > I still need to run through this again, but a few comments on Jouni's comments... > > I'd sugest implementing the preview in one of 2 ways or, for extra credit, both. > 1) Run this report for me and send it to me now > 2) Run this report for me and display the text/html version for me now > by "both", I mean that we do (1) and then display (2) Actually, getting a preview is looking complicated. I think I'll file a new bug for that one.

(In reply to comment #43) > I think I'll go with Joel's comment about how checksetup will probably never > rid itself of mysql-dependence. Yes, we talked about it with Joel on IRC during the weekend, and I tend to agree. > Hopefully the administrator is aware that Bugzilla has been shut down, and > doesn't need to be harassed by a cron job that runs as frequently as every 15 > minutes. Hmm. Does Cron mail out both stdout and stderr contents? Is there nothing we can do here?-) > Actually, I've been told to use { lined up with } if there's a multi-line if > condition, like: Interesting... Anyway, the Bugzilla dev guide doesn't recognize this rule, so let's go with having { on the same line as ).

(In reply to comment #45) > > Hopefully the administrator is aware that Bugzilla has been shut down, and > > doesn't need to be harassed by a cron job that runs as frequently as every 15 > > minutes. > > Hmm. Does Cron mail out both stdout and stderr contents? Is there nothing we can > do here?-) I appear to be missing the point. What good does it do to log the fact that Bugzilla has been shut down? The shutdownhtml param is supposed to be a convenient way to just make Bugzilla stop doing anything, and whine.pl complies quietly. There is no accident that could cause this. So why does it need to complain when it observes it?

OK, here's a new patch. Notable notes: 1. I tested this and it seems to work, but I haven't been as thorough as I'd like. You may look at it but I'm not requesting a review until I've had more time to kick the tires. 2. The added function in checksetup was pulled out. I'm sticking with the way the rest of checksetup works. 3. When whine.pl looks for events to complete and send, it only pulls them one at a time, immediately scheduling them for the next time they must run, and using a table lock. That means if there are multiple copies of whine.pl running, they'll play nicely and share the queue. 4. sanitycheck definitely should look at the whine tables, but I'd like to reserve that for an update rather than an initial checkin.

Heh. Had some debug code still in the previous one. This looks OK to me. Thoughts?

Comment on attachment 154707 [details] [diff] [review] WORKSFORME + $dbh->do("INSERT IGNORE INTO group_group_map " . + "(member_id, grantor_id, isbless) " . + "VALUES (" . + $whine_group . ", " . + $whineatothers_group . ", 0)"); +} That's grant_type now Fix that and r=joel

Comment on attachment 154707 [details] [diff] [review] WORKSFORME For starters, let me admit I'm starting to suck at reviewing this. I'm too numb for some parts of the code; it's quite possible that some issues will slip. That's pretty normal for patches of this size, though... All the more reason to get this in pretty fast. Anyway: >Index: whine.pl >=================================================================== >+# get the event that's scheduled with the lowest run_next value >+my $sth_first_scheduled_event = $dbh->prepare( Should this be called "next_scheduled_event"? (I could go either way, slightly leaning towards "next") >+# exit quietly if the system is shut down Ok, you got me convinced it's ok to do this totally quietly... >+if (($now_year % 4 == 0) && (($now_year % 100) || ($now_year % 400 == 0))) { Please use != 0 after the % 100 comparison; it's more like the other expressions then. >+while (my ($schedule_id, $day, $time) = $sched_h->fetchrow_array) { >+ my $this_day = 0; $this_day is never used? >+ # fill in some defaults in case they're blank >+ $day = '0' unless $day; >+ $time = '0' unless $time; We often just write $day ||= '0' -- you could use it as well. (get_next_event) >+# This function will: >+# 1. Lock whine_schedules >+# 2. Grab the most overdue pending schedules on the same event that must run >+# 3. Update those schedules' run_next value >+# 4. Unlock the table >+# 5. Return an event hashref Please prefix the comment by the method name. It's a bit clearer if you understand what "this function" is right from the start. I suspect there's a bug in this code, but I can't put my finger on it. I got an endless loop with event 1 being processed over and over again. However, an immense burst of stupidity and forgetfulness from my part resulted in me sweeping the event tables clean, so I no longer have an idea of what configuration caused it. Sorry :-( After several attempts at reproing it, it doesn't seem to be a particularly common phenomenon anyway. I only remember it was an event setup long time ago but never run. Take another good look at the code and think if reset_timer could misbehave with some settings... >+ my %uids; # Used for keeping track of who has been added I thought we were pushing user objects into this hash; in that sense, %uids is a confusing name. >+ # Check the nomail file for an entry with the recipient's address >+ if (open(NOMAIL, '<', "$datadir/nomail")) { >+ while (<NOMAIL>) { >+ return if (trim($_) eq $args->{'recipient'}->{'login'}); >+ } >+ } Ok, but ideally we'd want to cache this on a file-global level and then just look at a hash on every mail() call (or even in get_next_event?) >+ $args->{'alternatives'} = [] unless defined $args->{'alternatives'}; Here again, ||= would probably be more readable. >+ my $onequery = [ >+ { >+ 'name' => $thisquery->{'name'}, >+ 'title' => $thisquery->{'title'}, >+ 'bugs' => [ $bug ], >+ }, >+ ]; >+ $args->{'queries'} = $onequery; You could just as well assign directly to $args->{'queries'} and get rid of the $onequery. (get_named_query) >+ my $result = $fetched ? $fetched->[0] : ''; >+ return $result; Avoid these unnecessary temp variables; it's getting confusing. Just return the expression you're assigning to $result. >+ # This would be a really simple one except we're using DATE_SUB() to drop >+ # the seconds from current time. By the way, why? >+ [% IF q.usedinwhine %] >+ Remove from <a href="editwhines.cgi">whining</a> first >+ [% ELSE %] After some thinking: remove that "first"; it's not really understandable unless you know which link was replaced by this, and even then it's rather confusing. >Index: template/en/default/global/useful-links.html.tmpl >=================================================================== Note that you should also add a link to global/site-navigation.html.tmpl. >Index: template/en/default/whine/mail.txt.tmpl >=================================================================== Take a look at a plaintext mail generated by this patch. It's very ugly and obviously untested ;-) >+[% SET loginname="map_assigned_to.login_name" %] This variable would better be "assignee_address" or "assignee_login", whichever you prefer. Also apply for mail.html.tmpl. >+[%+ query.title %] >+[% "-" FILTER repeat(query.title.length) %] Pre_chomp works pretty badly here... >+ [% terms.Bug %] [% bug.bug_id %]: ... as it does here ... >+ Status: [% bug.bug_status %] [% IF bug.resolution -%] >+ Resolution: [% bug.resolution -%] >+ [%- END %] ... and here (although a bit differently). >Index: template/en/default/whine/schedule.html.tmpl >=================================================================== >+ [% IF q == thisquery %] >+ <option selected value="[% q FILTER html %]">[% q FILTER html %]</option> >+ [% ELSE %] >+ <option value="[% q FILTER html %]">[% q FILTER html %]</option> >+ [% END %] I still think something like <option value="[% q FILTER html %]" [% "SELECTED" IF q == thisquery %]> [% q FILTER html %] </option> would be better to avoid unnecessary repetition...

Comment on attachment 154707 [details] [diff] [review] WORKSFORME OK, based on Joini's point, I'm going to flip-flop on this. 2 additional things... + $dbh->do("LOCK TABLE whine_schedules WRITE"); + $sth = $dbh->prepare( "UPDATE whine_schedules " . + "SET run_next=NOW() " . + "WHERE id=?"); + $sth->execute($schedule_id); + $dbh->do("UNLOCK TABLES"); You don't need to lock around a single update 2) The only theory I have on Jouni's report is that the date scheduling function returned something in the past. I can't see what would cause that, but let's put a check that would report it. Go ahead and die() if that happens. Cron will send email. Just make sure you report the original date in the die message as well as what it got set to. You could do the check by having a final database compare between now() and the scheduled event or by keeping a list of the events already processed this time and watching for duplication.

(In reply to comment #50) > I suspect there's a bug in this code, but I can't put my finger on it. I got an > endless loop with event 1 being processed over and over again. However, an > immense burst of stupidity and forgetfulness from my part resulted in me > sweeping the event tables clean, so I no longer have an idea of what > configuration caused it. Sorry :-( After several attempts at reproing it, it > doesn't seem to be a particularly common phenomenon anyway. I only remember it > was an event setup long time ago but never run. Take another good look at the > code and think if reset_timer could misbehave with some settings... I found one major one where scheduled whines that belong to users having no whine access caused an infinite loop. My next patch has that fixed. In addition, I put a failsafe into the code that updates run_next so that any schedule not updated or somehow given a zero or less time adjustment is changed to NULL, which forces that particular schedule to not be processed again until the next time whine.pl runs. > >+ # This would be a really simple one except we're using DATE_SUB() to drop > >+ # the seconds from current time. > > By the way, why? Because all of the scheduling only deals in whole minutes, there's no reason we should have to keep track of seconds, so they're ignored. But when you add an offset in minutes to NOW(), then, you need to drop the seconds there too. I updated the comment to reflect this. > >+ [% IF q.usedinwhine %] > >+ Remove from <a href="editwhines.cgi">whining</a> first > >+ [% ELSE %] > > After some thinking: remove that "first"; it's not really understandable unless > you know which link was replaced by this, and even then it's rather confusing. Actually, it made more sense to me with the 'first' there, but I'll remove it. > >Index: template/en/default/whine/mail.txt.tmpl > >=================================================================== > > Take a look at a plaintext mail generated by this patch. It's very ugly and > obviously untested ;-) I'm not certain it's very ugly. There were a couple of places where whitespace clobbering was giving me trouble, but the new patch should have that fixed. If it still looks ugly to you, either we differ in what we think is ugly or you're getting something in your output that I am not. > >+[% SET loginname="map_assigned_to.login_name" %] > > This variable would better be "assignee_address" or "assignee_login", whichever > you prefer. Actually, it's just a string containing, literally, "map_assigned_to.login_name"... I changed it to "assignee_login_string" and added a comment explaining it. Everything else should be fixed, including Joel's comments.

And here is the patch.

Comment on attachment 155050 [details] [diff] [review] (more) more whining >Index: whine.pl >=================================================================== >+ # This is a kind of safeguard against infinite loops. >+ $sth = $dbh->prepare("UPDATE whine_schedules " . Explain how this works (when can it be run and why does nulling the run_next help). Do it in the comment, I figured it out already. With that, r=jouni. We'll probably be meeting some new bugs afterwards, but it doesn't look likely this would break any existing functionality. Get it in before I change my mind ;-)

This patch takes the above into account, and also removes the 'urlbase' var from the templates in favor of directly calling [% Param('urlbase') %]

Comment on attachment 155180 [details] [diff] [review] whining system r=joel (jouni's r+ still applies -- last change is trivial)

checked in for erik

This is a nice feature, but it seems I still need to use whineatnews for the whinedays parameter? Any plans to merge the whineatnews.pl feature into whine.pl?

(In reply to comment #58) > This is a nice feature, but it seems I still need to use whineatnews for the > whinedays parameter? Any plans to merge the whineatnews.pl feature into > whine.pl? Yes, but there's a number of things that have to be done first. Bug 253721 is one of those, seems like there's others, too. There's also so optimizations that would need to be done for query aggregation to prevent it from becoming a performance drain. The current whineatnews.pl grabs everyone at once in the same query and sorts on the assignee for generating the mail. With the existing revamped whine infrastructure (and the role pronouns) it would have to run the query again for each potential recipient.

small bug in the editwhines.cgi: if you do not change time or day of schedule, you can not update mailto: specifically, following lines: 239 if ( ($o_day ne $day) || 240 ($o_time ne $time) ){ there should one more condition: "|| ($o_mailto ne $mailto)"

(In reply to comment #60) > small bug in the editwhines.cgi: > > if you do not change time or day of schedule, you can not update mailto: Please file this as a new bug

clearing the docs? flag, documentation for this now lives on bug 274404

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

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