244927 - Add search by time owner has been idle

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P3)

Description

[Joel Peshkin]

To determine if a bug is suffering from inattention, a boolean search field is
needed that compares its argument to the amount of time (hours or days) that the
bug has been continuously assigned to the same person but that person has done
nothing.

Comment 1

[Joel Peshkin]

Attachment: Patch - Adds search for "Time Since Owner Touched"

Adds search for "Time Since Owner Touched" which can be specified as a number
of days (default) or the interval can be suffixed with "h" to indicate a number
of hours.

Comment 2

[Dave Miller [:justdave]]

should it allow the same syntax as SqlifyDate for consistency?  This would mean
using a 'd' for days, and allowing 'w' for weeks, etc.  Would it confuse it too
badly to just use SqlifyDate on it? :)  (just an idea)

Comment 3

[Joel Peshkin]

I could add the changes to make it support a superset of what sqlifydate does. 
As for using that routine....  no.  sqlifydate need to be replaced.  It shuld
leave the sql server to do the date math.

Comment 4

[Jouni Heikniemi]

Comment on attachment 149474 [details] [diff] [review]
Patch - Adds search for "Time Since Owner Touched"

>I could add the changes to make it support a superset of what sqlifydate does. 
>As for using that routine....  no.  sqlifydate need to be replaced.  It shuld
>leave the sql server to do the date math.

At least relative weeks and months are useful, and once we're there, I think
it's easiest for the user if same relative date expressions are supported as
everywhere else. Can the relative date handling part of SqlifyDate be
abstracted out and used here? While I agree date math should be done on the DB,
I'd say it's better to just use one strategy (either app-tier date math or
db-tier one; not both). 

>Index: Bugzilla/Search.pm
>===================================================================

>+                my $notifless; 
>+                my $orifless;
>+                if ($t =~ /great/) {
>+                    $notifless = "";
>+                    $orifless = "AND";
>+                } else {
>+                    $notifless = "NOT";
>+                    $orifless = "OR";
>+                }; 

You know, this is pretty horrible. ;-) "notifless" makes sense - in a way - but
this is really hard to understand. I'd suggest not getting these operators into
variables, but rather using ?: operator to put them into place later on. If you
really feel this is better, I'm not going to jump on the walls, but this is
sick anyway. ;)

I'd say use the regexp to populate a boolean like "searchforgreater" (which is
true if $t =~ /great/). You could use that boolean in your ?: expressions later
on in the code. I think I'd be able to read that one :-)

>+                $v =~ /(\d+)(h|H)?/;

Please allow \s* between the \d+ and time unit (which would be so much nicer as
([hH]) or perhaps even /i :-)) Although if you expand the syntax, the regexp
will probably get rewritten anyway...


Tested and works, but let's resolve the stuff above first.

Comment 5

[Joel Peshkin]

Attachment: Patch v2

This cleans up the items from jouni's review.  It keeps the data math in the
databasse.  sqlifydate needs to be fixed, but that is a differrent bug.

Comment 6

[Jouni Heikniemi]

Comment on attachment 149671 [details] [diff] [review]
Patch v2

Indentation nitpicking rules!

>Index: Bugzilla/Search.pm

>+                my $cutoff = "DATE_SUB(NOW(), 
>+                                  INTERVAL $quantity $unitinterval)";

Please align I of INTERVAL under D of DATE_SUB.

>+                    push(@wherepart, "(comment_$table.who IS NULL " .
>+                        "AND activity_$table.who IS NULL)");
>+                    push(@wherepart, "(comment_$table.who IS NOT NULL " .
>+                        "OR activity_$table.who IS NOT NULL)");


I'd prefer a solution where the string catenation parts are aligned. The
following is my favorite:

		    push(@wherepart, "(comment_$table.who IS NULL " .
				     "AND activity_$table.who IS NULL)");

Other than those, good work. I still disagree on two separate approaches to
date math, but the "that's another bug" approach is fine with me. Just this
once. ;-)

r=jouni

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Hmm, this looks like the kind of relatively small, low-risk feature we're
willing to take for 2.18 although we aren't taking new features in general. a=myk

Comment 8

[Joel Peshkin]

checked in

Comment 9

[Frédéric Buclin]

There is actually no list where fields accessible from boolean charts are described. Maybe there should be one, but this should include all fields, not only the one implemented in this bug, and so should be done in a separate bug.

Removing these flags per discussion with justdave on IRC.