Closed Bug 185090 Opened 22 years ago Closed 20 years ago

Revamp whining system

Categories

(Bugzilla :: Email Notifications, enhancement, P3)

Product:
Bugzilla
Component:
Email Notifications
Version:
2.17.1
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.20

People

(Reporter: bugreport, Assigned: erik)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 13 obsolete files)

whining system
71.08 KB, patch
bugreport
: review+
Details | Diff | Splinter Review 
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
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
Attached patch fine_whine.pl and template (obsolete) — — Splinter Review 
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...
Blocks: 16743, 65388
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
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Depends on: 240325
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.
Attached patch Scheduled query whining (obsolete) — — Splinter Review 
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.
Attachment #148780 - Attachment is obsolete: true
Attached patch Scheduled query whining (fixed) (obsolete) — — Splinter Review 
There was a cvsdo snafu on the previous version of this patch.	This one is
fixed and has been tested.
Attachment #148897 - Attachment is obsolete: true
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.
Depends on: 244927
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.
Attached patch Scheduled query whining (obsolete) — — Splinter Review 
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.
Attachment #149232 - Attachment is obsolete: true
Blocks: 245375
Attached patch Scheduled query whining (obsolete) — — Splinter Review 
This one adds a configurable mail subject and body-blurb per scheduled event.
Attachment #149853 - Attachment is obsolete: true
Attachment #150029 - Flags: review?
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.
Attachment #150029 - Flags: review? → review-
Also, whine scheduling page is missing a title
Attached patch Scheduled query whining (obsolete) — — Splinter Review 
I am very tired right now, but this patch should address everything but the
javascript collapse.
Attachment #150029 - Attachment is obsolete: true
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.
Attached patch Scheduled query whining (obsolete) — — Splinter Review 
This one has bz_* instead of _*
Attachment #151573 - Attachment is obsolete: true
Attachment #151644 - Flags: review?(bugreport)
Attachment #151644 - Flags: review?(bugreport) → review+
Attachment #151644 - Flags: review?
Attached patch Scheduled query whining (obsolete) — — Splinter Review 
Oops.  The other patch still didn't have a title on the schedule page.
Assignee: bugreport → erik
Attachment #151644 - Attachment is obsolete: true
Attachment #151721 - Flags: review?(bugreport)
Attachment #151721 - Flags: review?
Attachment #151721 - Flags: review?(bugreport) → review+
Flags: approval?
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.
Attachment #151721 - Flags: review? → review-
Comment on attachment 151644 [details] [diff] [review]
Scheduled query whining

Removing review request on an old patch.
Attachment #151644 - Flags: review?
back to you erik
Flags: approval?
Blocks: 250969
Attached patch New scheduled query whining (obsolete) — — Splinter Review 
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.
Attachment #151721 - Attachment is obsolete: true
Attachment #154157 - Flags: review?(jouni)
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!
Attachment #154157 - Flags: review?(jouni) → review-
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?
Blocks: 253445
Attached patch more whining (obsolete) — — Splinter Review 
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.
Attachment #109161 - Attachment is obsolete: true
Attachment #154157 - Attachment is obsolete: true
Attached patch WORKSFORME (obsolete) — — Splinter Review 
Heh.  Had some debug code still in the previous one.

This looks OK to me.  Thoughts?
Attachment #154602 - Attachment is obsolete: true
Attachment #154707 - Flags: review?(jouni)
Attachment #154707 - Flags: review?(bugreport)
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
Attachment #154707 - Flags: review?(bugreport) → review+
Blocks: 253721
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...
Attachment #154707 - Flags: review?(jouni) → review-
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.
Attachment #154707 - Flags: review+
(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.
Attached patch (more) more whining (obsolete) — — Splinter Review 
And here is the patch.
Attachment #154707 - Attachment is obsolete: true
Attachment #155050 - Flags: review?(jouni)
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 ;-)
Attachment #155050 - Flags: review?(jouni) → review+
Attached patch whining system — — Splinter Review 
This patch takes the above into account, and also removes the 'urlbase' var
from the templates in favor of directly calling [% Param('urlbase') %]
Attachment #155050 - Attachment is obsolete: true
Comment on attachment 155180 [details] [diff] [review]
whining system

r=joel
(jouni's r+ still applies -- last change is trivial)
Attachment #155180 - Flags: review+
Flags: documentation?
Flags: approval?
Flags: approval? → approval+
checked in for erik
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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
Blocks: 255562
clearing the docs? flag, documentation for this now lives on bug 274404
Flags: documentation?
*** Bug 73004 has been marked as a duplicate of this bug. ***
*** Bug 62093 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: