226434 - Need new query capabilities

Status: RESOLVED FIXED

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

Description

[Joel Peshkin]

1) Need to be able to specify ASSIGNEE, REPORTER, or QA_CONTACT in the RHS of
boolean charts so that queries can say....
"Status Whiteboard" "changed by" "ASSIGNEE"

2) Need to provide negation for boolean charts.
This is a bit more complex than it sounds.  In the cases of simple row-based
criteria, that chart's contribution to wherepart can simply be inverted.
In the case of joined tables, the joins required for that chart's contribtion
must become left joins, the contribution that would have been made to wherepart
becomes the ON clause in the join, and wherepart has to ensure that the join
results in a NULL record.

Naturally, this would slow down any queries that use the negation feature, but
should not slow down queries that do not use it.

Comment 1

[Joel Peshkin]

Attachment: Patch - adds pronouns

This patch adds the ability to specify %user% (for the user running the query),
%reporter%, %assignee%, or %qacontact% in the RHS of boolean charts where the
LHS is reporter, assignee, qa_contact, or CC and the verb is equals, notequals,
anyexact, or substring.

A further enhancment will make this work for commenter or changedby.

This makes it possible to use conditions like reporter <> assignee

Comment 2

[Joel Peshkin]

Attachment: Add escaping of backslash

Comment 3

[Joel Peshkin]

Note:  This is not done yet....  the "$f,$t,$v" =~ /$key/ would match unanchored
funcdefs like...
         ",equals" => sub {
             $term = "$ff = $q";
         },
if the value contains ",equals"

As far as I can tell, there thing to do is to anchor those patterns and change
them to 
         "^,equals" => sub {
             $term = "$ff = $q";
         },
and fix the progressive matching code to chop off the part of the pattern that
is already used.  

Comment 4

[Joel Peshkin]

Attachment: Patch

Comment 5

[Joel Peshkin]

Attachment: v2 including new commenter field

Comment 6

[Jouni Heikniemi]

Comment on attachment 149392 [details] [diff] [review]
v2 including new commenter field

This patch freaks out if you're not logged in (Can't call method "id" on an
undefined value at Bugzilla/Search.pm line 344.) and try to use %pronoun%s.

>+         "^(?:assigned_to|reporter|qa_contact),(?:equals|anyexact|substring),(%\\w+%)" => sub {
>+             $term = "bugs.$f = " . pronoun($1, $user->id);

Umm... Is it intentional you're treating substring search just as equals or
anyexact search? If %assignee% is [e-mail], shouldn't a |REPORTER
SUBSTRING "%assignee%"| search also match bugs reported by [e-mail]?

Although it seems pretty trivial, the difference might be relevant in
installations that don't use domain suffixes in their user accounts.

On another note, why are you passing user->id to pronoun? Couldn't it just dig
it up by itself? Preparing for the whining system?

>+             # $term = "bugs.$f = bugs.assigned_to";

Kill this comment.

>+          },
>+         "^(?:assigned_to|reporter|qa_contact),(?:notequals),(%\\w+%)" => sub {
>+             $term = "bugs.$f <> " . pronoun($1, $user->id);
>+          },
>          "^(assigned_to|reporter)," => sub {
>              push(@supptables, "profiles AS map_$f");
>              push(@wherepart, "bugs.$f = map_$f.userid");
>@@ -333,6 +340,26 @@
>              $f = "map_$f.login_name";
>          },
> 
>+         "^cc,(?:equals|anyexact|substring),(%\\w+%)" => sub {
>+             my $match = pronoun($1, $user->id);
>+             my $chartseq;
>+             if ($chartid eq "") {
>+                 $chartseq = "CC$sequence";
>+                 $sequence++;
>+             }
>+             push(@supptables, "LEFT JOIN cc cc_$chartseq ON bugs.bug_id = cc_$chartseq.bug_id AND cc_$chartseq.who = $match");
>+             $term = "cc_$chartseq.who IS NOT NULL";

This doesn't work as you'd expect. At least you should assign an initial value
to $chartseq like you do for commenters. The result is that the push and term
lines cause undef warnings and produce sql without table identifiers.

Also, the push line is way above 80 chars.

>+             my $chartseq;
>+             $chartseq = $chartid;

Why not just combine these two lines?

Comment 7

[Joel Peshkin]

Jouni's point on chartseq is correct, but needs to be fixed in all the places it
occurs.  That will break a lot unless bug 245158 lands first.

Comment 8

[Joel Peshkin]

Attachment: v3 - doesnt break if logged out

Yes, it is intentional that I dont do this for searches other than those that
match exact things.  I dont want the pronoun syntax to get in the way of
someone doing a really funky regexp search.  Therefore, it is supported in
places where the usage model is so narrow that nobody could conceivably mean
something that looks like a pronoun.  (since a valid email cannot be %word%
ever)

On passing the user instead of figuring it out from global context... yes, this
is a preparation for whining as well as Series execution group controls. 
Search relies on the calling cgi identifying which user's view should be
generated.  buglist does that today and series currently specifies a logged out
user.  Search should search based on the permissions of the user it is asked to
use, not try to figure out who is logged in.

On second thought, this does not really depend on the fix for multiple left
joins as it would not be any more broken.  However, we may as well land them in
the right order.

I fixed the rest.  Enjoy.

Comment 9

[Jouni Heikniemi]

Comment on attachment 152859 [details] [diff] [review]
v3 - doesnt break if logged out

>+         "^(?:assigned_to|reporter|qa_contact),(?:equals|anyexact|substring),(%\\w+%)" => sub {
>+             $term = "bugs.$f = " . pronoun($1, $user);
>+          },

As discussed on IRC, remove substring from the pattern. Since we don't support
its natural semantic meaning (the patch would make it equivalent to anyexact
and equals), perhaps its best we don't try to support it at all. Repeat for all
funcdefs mentioning substring.

r=jouni, but have you noticed that the Boolean Chart UI doesn't show
"commenter" in the options, although we have code for it?

Comment 10

[Joel Peshkin]

checked in with the substring part removed.

Comment 11

[Joel Peshkin]

by the way, "commenter" is created higher in Search.pm when hard-coded charts
are parsed.

Comment 12

[Joel Peshkin]

Documentation is being tracked in bug 281185