477601 - Adapt Bug.search to use Search.pm

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Rosie]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6) Gecko/2009020410 Fedora/3.0.6-1.fc9 Firefox/3.0.6
Build Identifier: 

Bug.search should be adapted to use Search.pm so that eventually it can become a complete search with the full functionality of query.cgi 

Reproducible: Always

Comment 1

[Rosie]

Attachment: v1

Patch reproduces functionality of Bug.search but uses Search.pm

Comment 2

[Max Kanat-Alexander]

Wow, I can't believe it took me so long to get to this. I hope that you can forgive me, Rosie!!!

Comment 3

[Max Kanat-Alexander]

Comment on attachment 361268 [details] [diff] [review]
v1

  Thank you so much for writing this patch--it's actually really nice, and it's one of the most important things we want in the WebServices.

>+    my $query = new Bugzilla::Search('fields' => [qw(bugs.bug_id)],

 In modern Bugzilla that would just be 'bug_id' now.

>+    my $dbh = Bugzilla->dbh;
>+    my $limit_str='';
>+    #eventually limit should be moved to Search.pm

  Wow. Yeah, it really should be moved to Search.pm. Would you possibly be interested in writing the patch to do that before we implement this? I'm concerned about the spread of this custom limit code all over Bugzilla, slowly, instead of just centralizing it now.

>+    my $results = $dbh->selectall_arrayref($query->getSQL().' 

  Just make that selectcol_arrayref, and then you'll just get back the bug_id.

>+    my $return = Bugzilla::Bug->new_from_list(\@list);
>+    return { bugs => $return };

  Oh, you can't just return the bug objects directly--look at what the current code does.

>+sub _search_to_cgi{
>+    #search should eventually use intelligent operarators for different values - here we do exact matches

  You'll want to put XXX in front of that comment. Also, usually we put a space between the # and the first word of the comment, and format them as standard sentences (so starting with a capital letter, ending with a period, etc.).

>+    my $chart_count=0;
>+    my %cgi_hash;

>+    while ( my ($key, $value) = each(%$params) ) {
>+        if ($key ne 'limit' and $key ne 'offset'){

  I think what would be better here is to do this as "eq" and then do "next" at the end of the block or something. That is, boolean conditions should generally be positive, not negative--it tends to make reading the code easier.

>+            my $key_str = FIELD_MAP->{$key} || $key;

  I think we have a function to do that now, in our current code.

>+            if(scalar grep($_ eq $key_str,('assigned_to', 'reporter', 'qa_contact', 'votes'))>0){

  You don't need the ">0" there. Also, you don't really need the "scalar". Usually we just put the grep in there directly. Also, a simpler formatting is probably:

  if (grep($_ eq $key_str, qw(assigned_to reporter qa_contact votes))) {

>+                $cgi_hash{"field$chart_count-0-0"}=$key_str;
>+                $cgi_hash{"type$chart_count-0-0"}='anyexact';
>+                $key_str="value$chart_count-0-0";

  Hmmm. Shouldn't everything actually be going into the same chart with an AND join? Or are there terms that require more than one row in the same chart?

>+            #summary, url, whiteboard, assigned_to, reporter and qa contact need a type set

  Actually, they should default to something sensible in Search.pm, I thought. Do they default to "equal" or something instead of "anyexact"?

>+            if(scalar grep($_ eq $key_str,('short_desc', 'bug_file_loc', 'status_whiteboard'))>0){

  Same note about the grep, here.

>+            #delta_ts needs to have both ch_fields set so set 'from' and change key to 'to'

  The current API documentation says "may not be an array".

Comment 4

[Rosie]

Attachment: v2

Thanks for the review Max!!

The attached patch uses the patch attached to bug 535571 but still works with the current Search.pm if you don't specify limit/offsets. 

I've addressed all your changes I think plus added some more conditions in the _search_to_cgi loop as not all searches are for exact matches any more

Comment 5

[Max Kanat-Alexander]

Comment on attachment 418194 [details] [diff] [review]
v2

>+    if(exists $params->{limit}){

  Nit: Space after "if" and before "{" at the end.

>+        $query = new Bugzilla::Search('fields' => [qw(bug_id)],

  Nit: Since there's only one field, might as well do 'bug_id'.

>+                                      'params' => $cgi_hash,
>+                                      'limit' => $params->{limit},
>+                                      'offset' => $offset

  I don't think you defined $offset.

>+    }
>+    else{
>+        $query = new Bugzilla::Search('fields' => [qw(bug_id)],
>+                                      'params' => $cgi_hash
>+                                     );

  Another option, instead of re-writing the parameters for Bugzilla::Search in two blocks, is to create an %options hash and just add limit and offset as necessary.

>+    # Search.pm won't return bugs that the user shouldn't see so no filtering is needed.
>+    my @bugs;
>+    foreach my $bug_id (@$results) {
>+        my $bug = Bugzilla::Bug->check($bug_id);

  You don't need to do check() (because of your comment above, there). Just use Bugzilla::Bug->new_from_list on the whole list.

>+sub _search_to_cgi {
>+    #search should eventually use intelligent operarators for different values -
>+    # here we mostly do exact matches.

  I think that's actually not true anymore--I think we do use intelligent operators for each value that can use them.

>+    while ( my ($key, $value) = each(%$params) ) {
>+        if ($key eq 'limit' or $key eq 'offset'){
>+            next;
>+        }

  Cool. That could be even shorter as a "next if".

>+        # Make sure Summary and Status Whiteboard default to 'contains any of the strings'
>+        if (grep($_ eq $key, qw(short_desc status_whiteboard))) {
>+            $cgi_hash{"field0-$and-0"}=$key;
>+            $cgi_hash{"type0-$and-0"}='anywordsubstr';
>+            $key="value0-$and-0";
>+            $and++;
>+        }

  You don't need to do those as boolean charts, you can just do "${field}_type".

>+        }
>+        if($key eq 'creation_ts'){

  Same for this. Also, this needs to be "greater than or equal to", to maintain API compatibility.

>+            $key="value0-$and-0";

  Nit: Spaces around "=". (This should be changed in a lot of places.)

>+        if(ref($value) eq 'ARRAY'){
>+                $cgi_hash{$key}=join(',',@$value);

  Ohhh, right, I forgot about that--the whole "you can't search for commas" problem. I don't know if that's an OK API change....

Comment 6

[Rosie]

Attachment: v3

(In reply to comment #5)
Cheers Max

I've hopefully formatted all my code properly now, I've also put the parameters to Search in a hash and used Bug->new_from_list on my list of bug ids. Thanks for the 'next if' pointer too.

>  Same for this. Also, this needs to be "greater than or equal to", to maintain
API compatibility.

I think I still need to use a boolean chart for creation_date don't I? If you use the chfield and chfrom values you can't search for last_changed_date and creation_date together. There doesn't seem to be a greater than or equal operator in the boolean charts so I've had to search for greaterthan and create an or equal in the same chart (if that makes sense :P) 

>   Ohhh, right, I forgot about that--the whole "you can't search for commas"
> problem. I don't know if that's an OK API change....

... sorry I don't understand - it's a while since I wrote this but I thought that was how arrays should be passed to Search.pm. Should I be passing it an array ref instead?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> I think I still need to use a boolean chart for creation_date don't I?

  I'm not entirely certain that you do, if there's a field in fielddefs that represents creation_ts. I recently modified Search.pm to take anything in fielddefs without a period as a valid field, even if that field isn't used by query.cgi. 

> >   Ohhh, right, I forgot about that--the whole "you can't search for commas"
> > problem. I don't know if that's an OK API change....
> 
> ... sorry I don't understand - it's a while since I wrote this but I thought
> that was how arrays should be passed to Search.pm. Should I be passing it an
> array ref instead?

  Well, it's actually an old bug in Search.pm that I'm talking about. (bug 67036). The problem is that with the current Search.pm implementation, users can indeed search for things that contain commas. With your new implementation, they can't.

Comment 8

[Rosie]

Attachment: v4

(In reply to comment #7)
>   I'm not entirely certain that you do, if there's a field in fielddefs that
> represents creation_ts. I recently modified Search.pm to take anything in
> fielddefs without a period as a valid field, even if that field isn't used by
> query.cgi. 

Well you can search for creation_ts > a value but there's no >= operator in Search.pm so to get the same behaviour as the old function you need to construct a boolean search.

>   Well, it's actually an old bug in Search.pm that I'm talking about. (bug
> 67036). The problem is that with the current Search.pm implementation, users
> can indeed search for things that contain commas. With your new implementation,
> they can't.

That should be fixed with this new patch which just passes arrays of values rather than joining them with commas

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Well you can search for creation_ts > a value but there's no >= operator in
> Search.pm so to get the same behaviour as the old function you need to
> construct a boolean search.

  Hmm. Do you think you could add a >= and <= operator, in another bug, to handle that? I think that would simplify things for us here.

> That should be fixed with this new patch which just passes arrays of values
> rather than joining them with commas

  Oh, if that works, then that's OK. :-)

Comment 10

[Rosie]

Attachment: v5

This is the new patch using the offset and limit params and greaterthaneq operator in Search.pm

Comment 11

[Max Kanat-Alexander]

Comment on attachment 420536 [details] [diff] [review]
v5

This looks great. Thank you so much, Rosie. :-)

Comment 12

[Max Kanat-Alexander]

Comment on attachment 420536 [details] [diff] [review]
v5

>+    if (exists $params->{limit}) {
>+        $options{limit} = $params->{limit};
>+        $options{offset} = $params->{offset} || 0;
>+     }

  Though, that shouldn't be necessary anymore, right?

Comment 13

[Rosie]

oops yes thought I'd taken that all out - obviously not!

Comment 14

[Chris Eveleigh]

Attachment: patch to enable use of Search.pm

this should obsolete the current attachment but it's not letting me do that.

apologies if this doesn't apply ... or if it doesn't work anymore.  i've had to edit the file by hand to change the license stuff.  and i've not been able to apply the patch to trunk or to get trunk running in order to test it.

sorry i won't have any time to make changes or test anything over the coming weeks.  hopefully just sorting out the license is enough to enable others to complete the work.  if not i'll see what i can do.

Comment 15

[Chris Eveleigh]

P.S. when i say i couldn't "get trunk running" i don't mean it's broken - i just didn't have the right version of perl (well, CGI.pm) available on fedora 13 and don't have time to figure out what to do about that.

Comment 16

[Max Kanat-Alexander]

Fixing the license is absolutely enough. Thank you so much, Chris! :-)

Comment 17

[Frédéric Buclin]

Why doesn't this patch have a review request now that the licensing issue is resolved? Or if it's a trivial fix, why isn't the bug approved yet?

Comment 18

[Chris Eveleigh]

thanks for reminder - done now, also for bug 535571.  can someone clean the whiteboard on these two bugs?

Comment 19

[Frédéric Buclin]

Comment on attachment 464088 [details] [diff] [review]
patch to enable use of Search.pm

>Index: Bugzilla/WebService/Bug.pm

>+use Bugzilla::CGI;

Unless I miss something, Search.pm no longer depends on CGI, see bug 398308 (checked in a year ago).

Comment 20

[Chris Eveleigh]

Attachment: patch against trunk

here's a new initial patch.  i've made a couple of HUGE assumptions:
1) our comment "The following helper function can be removed once bug 398308 is completed" can be taken literally.
2) the 'anywordsubstr' and 'greaterthaneq' parts of the helper function don't need to be moved into the main method.

so i've passed the received 'params' directly to the Search.pm function.

this patch passes the runtest.pl script but i've so far been unable to test the actual search functionality as i can't get the python bugzilla library to work against trunk - it gets an XML error parsing the first response.

i read somewhere that the runtest.pl script tests ALL of the web service API - do you know if that means it has done a sample search?  presumably it can't check the results as there are no bugs in my new instance.

so, if i can get the python bugzilla working then i'll try some searches but if someone else can beat me to it (or see problems without the need to try it out) please go for it.

hope this is helpful.

Comment 21

[Chris Eveleigh]

meh - next time i'll install the relevant perl packages before claiming the web service isn't working ;-)
i've told the python bugzilla library to treat v4.3 as v3.6 for now.  search crashes.  :-(

Comment 22

[Max Kanat-Alexander]

Comment on attachment 586441 [details] [diff] [review]
patch against trunk

Review of attachment 586441 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thank you!! :-D Some comments:

::: Bugzilla/WebService/Bug.pm
@@ +401,4 @@
>                         { param => 'limit', function => 'Bug.search()' });
>      }
>      
> +    #$params = Bugzilla::Bug::map_fields($params);

Removing map_fields is likely to break stuff, since that's what makes the API work. In fact, we're likely to need a more-complex version of map_fields for this. Or perhaps instead, we can implement support for the API names of fields inside of Bugzilla::Search, which seems like a better solution (since it has a centralized place that it can rename fields in).

@@ +401,5 @@
>                         { param => 'limit', function => 'Bug.search()' });
>      }
>      
> +    #$params = Bugzilla::Bug::map_fields($params);
> +    #delete $params->{WHERE};

This line can just be deleted. However, there are some other parameters that you have to delete. At the very least, include_fields and exclude_fields should be moved out, since they're not search criteria.

@@ +411,5 @@
> +    my %options = ('fields' => ['bug_id'], 'params' => $params);
> +    if (exists $params->{limit}) {
> +        $options{limit} = $params->{limit};
> +        $options{offset} = $params->{offset} || 0;
> +     }

You should only send {offset} if {offset} was sent by the client. Also, you should delete these from params as you set them, like:

$options{limit} = delete $params->{limit}

@@ +416,5 @@
> +
> +    my $query = new Bugzilla::Search(%options);
> +    my $dbh = Bugzilla->dbh;
> +
> +    my $results = $dbh->selectcol_arrayref($query->getSQL());

I'm pretty sure there's no getSQL function anymore. You probably just want ->sql.

@@ +423,5 @@
> +
> +    my @bugs;
> +    foreach my $bug (@$bug_objects) {
> +        push(@bugs, $self->_bug_to_hash($bug));
> +    }

This can be replaced with:

my @bugs = map { $self->_bug_to_hash($_, $params) } @bugs;

Comment 23

[Chris Eveleigh]

hi Max, thanks for the comments.  sorry for the delays - bit busy at the moment.
the getSQL was causing the crash.  changing it to ->sql fixed the crash but at the moment i'm not getting any search results.  will keep working on it but happy for someone else to pick this up.

Comment 24

[David Lawrence [:dkl]]

I can take this over as I need this to finish up the BMO REST extension I am working on.

One thing I would like to note, there may be quite a bit of current clients our there who are still relying on the old Bug.search behavior and this will break those clients. Should we implement this as a new method named Bug.full_search or are we ok in making people fix their clients for 4.4 to work with the new Bug.search?

dkl

Comment 25

[David Lawrence [:dkl]]

Attachment: Patch to make Bug.search use Search.pm (v1)

Patch that changes Bug.search to use Search.pm. Trying to achieve backwards compatibility with the old Bug.search as well. Couple additional features is the ability to pass in 'quicksearch' as well as use any saved searches of the user or another user who is sharing the search.

dkl

Comment 26

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #24)
> are we ok in making people fix their clients for 4.4 to
> work with the new Bug.search?

We are. This method is marked unstable, and the searching system is already complex enough to not support the old version too. About your patch, does it make sense to support regetlastlist? I totally understand the support for quicksearch and saved searches, but I have some problems understanding if regetlastlist makes sense. And is the BUGLIST cookie really passed to the method anyway?

Comment 27

[Max Kanat-Alexander]

Comment on attachment 603161 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v1)

Review of attachment 603161 [details] [diff] [review]:
-----------------------------------------------------------------

In general, it's a bad idea to make the internal API of Search.pm the external API of Bug.search. Instead of passing in things that look just like the CGI parameters, we want to pass in data structures. For an idea of what these might look like, See Bugzilla::Test::Search::Constants::CUSTOM_SEARCH_TESTS. One of the motivations behind my redesign of Search.pm was enabling the ability to pass in data structures much like that.

I also have a few more specific comments:

::: Bugzilla/WebService/Bug.pm
@@ +401,5 @@
> +    delete $params->{'exclude_fields'};
> +
> +    if ($params->{quicksearch} || $params->{savedsearch}) {
> +        # Force CGI.pm to return as query string and not POSTDATA
> +        $ENV{REQUEST_METHOD} = 'GET';

This seems dangerous, because you're in the WebService, which makes logical decisions based on this. You could convert this into a "local" with a trailing "if" instead.

@@ +408,5 @@
> +    # Determine whether this is a quicksearch query
> +    if ($params->{quicksearch}) {
> +        my $quicksearch = quicksearch(delete $params->{'quicksearch'});
> +        my $cgi = Bugzilla::CGI->new($quicksearch);
> +        $params = $cgi->Vars;

Maybe you should have a method for quicksearch to return a hash or a series of Clause objects or something. At least, seems like a worthwhile XXX comment.

@@ +412,5 @@
> +        $params = $cgi->Vars;
> +    }
> +
> +    # Load previously saved query and use it if specified
> +    if ($params->{savedsearch}) {

This is normally the sort of thing I would recommend goes into a separate patch.

@@ +438,5 @@
>      
> +        my $url = $saved_search_obj->url;
> +        my $cgi = Bugzilla::CGI->new($url);
> +        $params = $cgi->Vars;
> +        $order = $params->{'order'};

This is definitely a case where Bugzilla::Search::Saved should be able to return the object you need, instead, I would have to think. Or perhaps even return a Bugzilla::Search object.

Comment 28

[Max Kanat-Alexander]

Comment on attachment 603161 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v1)

Review of attachment 603161 [details] [diff] [review]:
-----------------------------------------------------------------

In general, it's a bad idea to make the internal API of Search.pm the external API of Bug.search. Instead of passing in things that look just like the CGI parameters, we want to pass in data structures. For an idea of what these might look like, See Bugzilla::Test::Search::Constants::CUSTOM_SEARCH_TESTS. One of the motivations behind my redesign of Search.pm was enabling the ability to pass in data structures much like that.

I also have a few more specific comments:

::: Bugzilla/WebService/Bug.pm
@@ +401,5 @@
> +    delete $params->{'exclude_fields'};
> +
> +    if ($params->{quicksearch} || $params->{savedsearch}) {
> +        # Force CGI.pm to return as query string and not POSTDATA
> +        $ENV{REQUEST_METHOD} = 'GET';

This seems dangerous, because you're in the WebService, which makes logical decisions based on this. You could convert this into a "local" with a trailing "if" instead.

@@ +408,5 @@
> +    # Determine whether this is a quicksearch query
> +    if ($params->{quicksearch}) {
> +        my $quicksearch = quicksearch(delete $params->{'quicksearch'});
> +        my $cgi = Bugzilla::CGI->new($quicksearch);
> +        $params = $cgi->Vars;

Maybe you should have a method for quicksearch to return a hash or a series of Clause objects or something. At least, seems like a worthwhile XXX comment.

@@ +412,5 @@
> +        $params = $cgi->Vars;
> +    }
> +
> +    # Load previously saved query and use it if specified
> +    if ($params->{savedsearch}) {

This is normally the sort of thing I would recommend goes into a separate patch.

@@ +438,5 @@
>      
> +        my $url = $saved_search_obj->url;
> +        my $cgi = Bugzilla::CGI->new($url);
> +        $params = $cgi->Vars;
> +        $order = $params->{'order'};

This is definitely a case where Bugzilla::Search::Saved should be able to return the object you need, instead, I would have to think. Or perhaps even return a Bugzilla::Search object.

@@ +463,5 @@
> +        }
> +        # set up the params for this new query
> +        my $cgi = $params = Bugzilla::CGI->new({ bug_id => $bug_ids, order => $order });
> +        $params = $cgi->Vars;
> +    }

This looks like there's some cut-and-paste from buglist.cgi; perhaps some of this should be abstracted out. (And also this might be a good thing to put into a separate patch, in general.)

@@ +484,5 @@
>          delete $params->{$_} foreach qw(estimated_time remaining_time deadline);
>      }
>  
> +    # Some fields require a search type such as short desc, etc.
> +    foreach my $param (qw(short_desc longdesc status_whiteboard bug_file_loc)) {

The custom fields that work similarly would need similar default types as well, no?

@@ +508,5 @@
> +        $params->{"field${last_chart_id}-0-0"} = $email;
> +        $params->{"type${last_chart_id}-0-0"}  = "anywordssubstr";
> +        $params->{"value${last_chart_id}-0-0"} = ref $value ? join(" ", @{$value}) : $value;
> +        $last_chart_id++;
> +    }

Also, just as a note, no new code should be relying on boolean chart syntax. It would be better to use the new Custom Search syntax.

Comment 29

[Max Kanat-Alexander]

Oh, and note Splinter bug: when I made new comments, it published my entire review again. Sorry about that.

Comment 30

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else on time for 5.0.

Comment 31

[Simon Green]

What's the status of this bug? brc want this implemented yesterday :)

I'm happy to pick it up if David (the current assignee) is not going to work on it.

Comment 32

[David Lawrence [:dkl]]

(In reply to Simon Green from comment #31)
> What's the status of this bug? brc want this implemented yesterday :)
> 
> I'm happy to pick it up if David (the current assignee) is not going to work
> on it.

We need it as well due to trying to move the BzAPI REST proxy over to BMO natively and we need to preserve the full search ability that BzAPI provides currently. And since Bug.search is still classified as UNSTABLE, now is the best time to switch over to a fully functional search method. 

How bout I revise the patch and you review it?

dkl

Comment 33

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #32)
> How bout I revise the patch

Sounds great.

> and you review it?

I don't have review privileges, so it will need an additional review from an approved person.

Comment 34

[David Lawrence [:dkl]]

Attachment: Patch to make Bug.search use Search.pm (v3)

Updated patch. Passes webservice_bug_search.t as well.

dkl

Comment 35

[David Lawrence [:dkl]]

Hi Simon. Do you think you will be able to look at this soon? We need it for some work being done internally so if you don't have the time, I can see if glob will take it. Let me know.

Thanks
dkl

Comment 36

[Simon Green]

Comment on attachment 765768 [details] [diff] [review]
Patch to make Bug.search use Search.pm (v3)

Review of attachment 765768 [details] [diff] [review]:
-----------------------------------------------------------------

I'd also like to see the POD in B/W/Bug.pm update to mention you can include custom search values.

::: Bugzilla/WebService/Bug.pm
@@ +426,5 @@
>          }
>      }
>      else {
> +        delete $params->{'limit'};
> +        delete $params->{'offset'};

Nit: Bugzilla doesn't have a specified preferences for quoted vs. non quoted strings in a hash. Don't change them for the sake of changing them.

@@ +438,5 @@
> +    my @field_ids = grep(/^f(\d+)$/, keys %$params);
> +    my $last_field_id = 1;
> +    if (@field_ids) {
> +        @field_ids = sort { $a <=> $b } uniq @field_ids;
> +        $last_field_id = (@field_ids)[-1];

Use List::Util's max function instead.

@@ +443,4 @@
>      }
>  
>      # Do special search types for certain fields.
> +    if (my $change_when = delete $params->{delta_ts}) {

http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html#search says the field is last_change_time, not delta_ts

@@ +448,5 @@
> +        $params->{"o${last_field_id}"} = 'greaterthaneq';
> +        $params->{"v${last_field_id}"} = $change_when;
> +        $last_field_id++;
> +    }
> +    if (my $creation_when = delete $params->{creation_ts}) {

http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html#search says the field is creation_time, not creation_ts

@@ +473,5 @@
> +        @email_ids = sort { $a <=> $b } uniq @email_ids;
> +        $last_email_id = (@email_ids)[-1];
> +    }
> +
> +    foreach my $role (qw(assigned_to reporter qa_contact longdesc cc)) {

http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.html#search says the fields are creator, not reporter (but reporter is also accepted for backwards compatibility

@@ +474,5 @@
> +        $last_email_id = (@email_ids)[-1];
> +    }
> +
> +    foreach my $role (qw(assigned_to reporter qa_contact longdesc cc)) {
> +        next if !exists $params->{$role};

Wouldn't it be better to just use the custom search for this? Means would wouldn't need to use the @email_ids in the section above, and can use last_field_id instead.

@@ +477,5 @@
> +    foreach my $role (qw(assigned_to reporter qa_contact longdesc cc)) {
> +        next if !exists $params->{$role};
> +        my $value = delete $params->{$role};
> +        $params->{"email$role$last_email_id"} = 1;
> +        $params->{"emailtype$last_email_id"}  = "anywordssubstr";

Shouldn't this be 'equals' instead of 'anywordssubstr'

Comment 37

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #35)
> Do you think you will be able to look at this soon?

Sorry about the delay. Finally got my VPS set up with some decent data (a sanitised BMO database) so am able do reviews now.

Comment 38

[David Lawrence [:dkl]]

Attachment: 477601_4.patch

(In reply to Simon Green from comment #36)
> @@ +443,4 @@
> > +    if (my $change_when = delete $params->{delta_ts}) {
> 
> http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.
> html#search says the field is last_change_time, not delta_ts

last_change_time is mapped to creation_ts earlier in the map_fields() call.

> @@ +448,5 @@
> > +    if (my $creation_when = delete $params->{creation_ts}) {
> 
> http://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/WebService/Bug.
> html#search says the field is creation_time, not creation_ts

same as above.

> @@ +477,5 @@
> > +        $params->{"emailtype$last_email_id"}  = "anywordssubstr";
> 
> Shouldn't this be 'equals' instead of 'anywordssubstr'

The old Search method allowed for certain values such as assignee, qa_contact, etc. to be a list of values so it was safer to use anywordssubstr just in case we are looking for multiple values.

Comment 39

[Simon Green]

Comment on attachment 787632 [details] [diff] [review]
477601_4.patch

+    if (!grep(!/(limit|offset)/i, keys %$params)
should be

+    if (!grep(!/^(limit|offset)$/i, keys %$params)

in case we ever have a field with limit or offset (for example a custom field). Other than that, I'm happy with the review, and the above can be fixed on checkin.

Comment 40

[David Lawrence [:dkl]]

\o/

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Bug.pm
Committed revision 8713.

Comment 41

[Gervase Markham [:gerv]]

\o/

Gerv

Comment 44

[Frédéric Buclin]

Added to relnotes for 5.0rc1.