682847 - Create request flag whining extension

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[Gervase Markham [:gerv]]

For the Review Queue Proposal:
https://wiki.mozilla.org/BMO/Review_Queue_Proposal
we need an extension which will whine at people weekly about their un-done reviews.

* Write a Bugzilla extension which sets up a whine for every Bugzilla account.
* This whine whines weekly about bugs with outstanding review requests.
* This is retrofitted to every existing account, and added automatically to new 
  ones. 

Ideally, it would check just for those with editbugs, to reduce the load, but it could also check for everyone if it had to. The issue is that detecting when people get editbugs and adding a whine might be tricky.

This could either be implemented by adding a load of whines to the normal whining system, or we could do it outside that. If we do it outside, that might make it easier - we just search for the set of user accounts meeting the criteria.

Anyway, we need to whine about un-done reviews somehow :-)

Gerv

Comment 1

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #0)
> This could either be implemented by adding a load of whines to the normal
> whining system, or we could do it outside that. If we do it outside, that
> might make it easier [..]

doing it outside of the normal whining mechanism would be much easier, and would mean we'd run only a handful of queries instead of thousands.

Comment 2

[David Lawrence [:dkl]]

We could add a hook to whine.pl that the extension could use and then we would not need to add another cron job. Whine AFAIK already runs in 15 min intervals.

dkl

Comment 3

[Gervase Markham [:gerv]]

From design brainstorming on IRC:

- We have a .pl file in cron (or add a hook to whine.pl)
- It runs every X days (suggested value: 7)
- It gets all unfulfilled requests (of any type) which match the following:
  * have a requestee
  * are older than Y days, where Y is configurable (suggested value: 7)
- It makes an email for each requestee,and sends it to them. In the email:
  * requests are grouped by type, and then ordered oldest first
  * There's a link to the policy
  * There's a link to your request queue (request.cgi)

Gerv

Comment 4

[Gervase Markham [:gerv]]

Attachment: Dummy attachment for flags

Hi dkl,

This is a dummy attachment so I can set the review flag. Please can you give the once-over to the code stored here?
http://bzr.mozilla.org/bugzilla/extensions/requestwhiner/trunk/files

It's basically just a .pl to be run from cron, plus some email templates. The frequency is hard-coded; I thought about adding a whole new param panel but thought it would be overkill.

Thanks,

Gerv

Comment 5

[David Lawrence [:dkl]]

Some sanity failures that should be fixed up:

#   Failed test '(en/default) extensions/RequestWhiner/template/en/default/requestwhiner/mail.html.tmpl has unfiltered directives:
#   32: threshold 
#   35: request 
#   41: urlbase 
#   41: bug.id 
#   42: bug.id 
#   42: bug.age 
#   49: urlbase 
#   49: recipient.email FILTER url

whineatreviews.pl:

+ my $whine_after_days = 7;

Create a RequestWhiner/lib/Constants.pm for this. Also maybe rename this to whineatrequests.pl to be consistent with the extension name.

+    $dbh->prepare("SELECT profiles.login_name,
+                          flagtypes.name, 
+                          bugs.bug_id, 
+                          bugs.short_desc, " .
+                          $dbh->sql_to_days('NOW()') .
+                            " - " .
+                          $dbh->sql_to_days('flags.modification_date') . "
+                       AS age_in_days
+                     FROM flags 
+                     JOIN bugs ON bugs.bug_id = flags.bug_id, 
+                          flagtypes, 
+                          profiles 
+                    WHERE flags.status = '?' 
+                      AND flags.requestee_id = profiles.userid 
+                      AND flags.type_id = flagtypes.id 
+                      AND " . $dbh->sql_to_days('NOW()') .
+                              " - " .
+                              $dbh->sql_to_days('flags.modification_date') .
+                              " > " .
+                              $whine_after_days . "
+                 ORDER BY flags.modification_date");

Need to join the flags.requestee to profiles.userid since you are selecting profiles.login_name. Simple join is fine and not left join since we don't care about flags without a specified requestee.

                       AND profiles.userid = flags.requestee

+    mail({
+        from      => Bugzilla->params->{'mailfrom'},
+        recipient => $user,
+        subject   => "Outstanding Requests",
+        requests  => $requests->{$recipient},
+        threshold => Bugzilla->params->{'request_whine_days'}
+    });

Change threshold value to be $whine_after_days or even better the Constants.pm nit pick I mentioned earlier. Also you are not creating a parameters panel for this anyway.

requestwhiner/mail.html.tmpl:

+<h3>[%+ request +%]</h3>

Nit: +'s are not needed

+[% bugs = requests.$request %]
+  [% FOREACH bug = bugs %]

Condense to
   
   [% FOREACH bug = requests.$request %]

+    <a href="[%+ urlbase %]show_bug.cgi?id=[% bug.id +%]">[%+ terms.Bug +%]
+    [%+ bug.id %]: [% bug.summary FILTER html %]</a> ([% bug.age %] days old)

Nit: +'s not needed here either. Also need proper FILTER's (part of the error in the beginning of my comment).

    <a href="[% urlbase FILTER none %]show_bug.cgi?id=[% bug.id FILTER url_quote %]">[% terms.Bug %]
    [% bug.id FILTER html %]: [% bug.summary FILTER html %]</a> ([% bug.age FILTER html %] days old)

+<p><a href="[%+ urlbase %]request.cgi?action=queue&requestee=[% recipient.email +FILTER url %]&group=type">See all your outstanding requests</a>.</p>

Nit: same with +'s. Also it is FILTER url_quote for 4.0 (FILTER uri for 4.2) and not FILTER url.

requestwhiner/mail.txt.tmpl:

+[%+ "-" FILTER repeat(request.length) +%]

Why not just have a fixed width dotted line? With this, if they had one bug, it would just be a single dash. With a hundred it would possibly break up to multiple lines. Are you going for emphasis to the user by making it standout more in the email if the flag has alot of requests?

+  [% bugs = requests.$request %]
+  [% FOREACH bug = bugs %]

Also condense to [% FOREACH bug = requests.$request %]

+[%+ urlbase %]request.cgi?action=queue&requestee=[% recipient.email FILTER url +%]&group=type

Same as above. Needs FILTER url_quote.

dkl

Comment 6

[Gervase Markham [:gerv]]

(In reply to David Lawrence [:dkl] from comment #5)
> +                     JOIN bugs ON bugs.bug_id = flags.bug_id, 
> +                          flagtypes, 
> +                          profiles 
> +                    WHERE flags.status = '?' 
> +                      AND flags.requestee_id = profiles.userid 
> +                      AND flags.type_id = flagtypes.id 
...
> Need to join the flags.requestee to profiles.userid since you are selecting
> profiles.login_name. Simple join is fine and not left join since we don't
> care about flags without a specified requestee.
> 
>                        AND profiles.userid = flags.requestee

I already have
AND flags.requestee_id = profiles.userid
Isn't that enough? If not, please tell me in more detail what I need to alter or add.

> +[%+ "-" FILTER repeat(request.length) +%]
> 
> Why not just have a fixed width dotted line? With this, if they had one bug,
> it would just be a single dash.

You misread it :-) It produces a dashed line the same length as the text, like this:

review
------

Other comments addressed; see repo :-)

Gerv

Comment 7

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #6)
> > Need to join the flags.requestee to profiles.userid since you are selecting
> > profiles.login_name. Simple join is fine and not left join since we don't
> > care about flags without a specified requestee.
> > 
> >                        AND profiles.userid = flags.requestee
> 
> I already have
> AND flags.requestee_id = profiles.userid
> Isn't that enough? If not, please tell me in more detail what I need to
> alter or add.

Is what I get for reviewing while should be sleeping ;) Yes you do have it there
so all is well. Missed it before.

> > +[%+ "-" FILTER repeat(request.length) +%]
> > 
> > Why not just have a fixed width dotted line? With this, if they had one bug,
> > it would just be a single dash.
> 
> You misread it :-) It produces a dashed line the same length as the text,
> like this:

Yes. I got it request.length mixed up with request.size and was thinking 'request' was a reference to the list of bugs that are in the hash. In this case it is just a text string. Also good to go. 

I will finish the review when I see the new code in the bugzilla/extensions repo.

dkl

Comment 8

[Gervase Markham [:gerv]]

dkl: sorry, forgot to push. The updated version is now in the repo.

Gerv

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 559617 [details]
Dummy attachment for flags

Ok. Looks good and works as expected. Should I commit to the bmo/4.0 tree for roll out in the near future or do you want to? Once it is there we can work with IT to get it enabled and add the proper cron entry to get it to run at the proper interval.

dkl

Comment 10

[Gervase Markham [:gerv]]

Give me a little time to make sure we have the necessary documentation in place, and that the emails point to it, and that we're all geared up to go with this.

Gerv

Comment 11

[Robert Longson [:longsonr]]

If I made the request I probably don't need to whine at myself. Is that covered?

Comment 12

[Gervase Markham [:gerv]]

dkl: we now have docs:
https://wiki.mozilla.org/BMO/Handling_Requests

Next, we need a script which does the bankruptcy.

As soon as we have that and the metrics, we are ready to go. In the mean time, can you commit the latest copy of this to the bmo-4.0 tree, and make sure we are all set up?

Gerv

Comment 13

[David Lawrence [:dkl]]

Committed. Should I go ahead and get IT to schedule the cron script on the next code push?

Committing to: bzr+ssh://[e-mail]/bmo/4.0
added extensions/RequestWhiner
added extensions/RequestWhiner/Config.pm
added extensions/RequestWhiner/Extension.pm
added extensions/RequestWhiner/bin
added extensions/RequestWhiner/lib
added extensions/RequestWhiner/template
added extensions/RequestWhiner/bin/whineatrequests.pl
added extensions/RequestWhiner/lib/Constants.pm
added extensions/RequestWhiner/template/en
added extensions/RequestWhiner/template/en/default
added extensions/RequestWhiner/template/en/default/requestwhiner
added extensions/RequestWhiner/template/en/default/requestwhiner/mail.html.tmpl
added extensions/RequestWhiner/template/en/default/requestwhiner/mail.txt.tmpl
Committed revision 7917

dkl

Comment 14

[Gervase Markham [:gerv]]

dkl: no; we don't want to start whining until:

a) we've done the bankruptcy (bug 688464)
b) we have metrics in place (I'm chasing the metrics team)

Gerv

Comment 15

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #14)
> dkl: no; we don't want to start whining until:
> 
> a) we've done the bankruptcy (bug 688464)
> b) we have metrics in place (I'm chasing the metrics team)
> 
> Gerv

Ack. Just let me know. The extension will sit there harmlessly til we add the cron job so it will still go out with the next push.

dkl

Comment 16

[Gervase Markham [:gerv]]

Enabling this on BMO is bug 711483.

Gerv