The current code considers a value of 0 in a search query as an empty string, that is, it won't search on it. If a library wants to search for records that specifically have the value "0" in a given field, it will not be possible. This patch aims to correct this.
Created attachment 70122 [details] [review] Bug 19873 - Ability to search on 0 value Test plan: - create a st-numeric index in zebra conf related to a numeric field i.e: yourindex 1=yourindex 4=109 - fill a field with 0 in a biblio, - reindex your biblios, - search yourindex=0, - should not work, - apply this patch, - test again, - should work
+ original_operand => ($operands[$i] ne '') ? $operands[$i] : '', I think it could be more pretty : + original_operand => $operands[$i] // '',
Comment on attachment 70122 [details] [review] Bug 19873 - Ability to search on 0 value Review of attachment 70122 [details] [review]: ----------------------------------------------------------------- ::: C4/Search.pm @@ +1532,4 @@ > for ( my $i = 0 ; $i <= @operands ; $i++ ) { > > # COMBINE OPERANDS, INDEXES AND OPERATORS > + if ( $operands[$i] ne '' ) { perhaps a $operands[$i] // q{} ne q{} would be in order. We don't want floody logs if $operands[$i] is undef. @@ +1677,4 @@ > query_desc => $query_desc, > operator => ($operators[ $i - 1 ]) ? $operators[ $i - 1 ] : '', > parsed_operand => $operand, > + original_operand => ($operands[$i] ne '') ? $operands[$i] : '', perhaps a $operands[$i] // q{} is sufficient? We don't want floody logs if $operands[$i] is undef.
(In reply to M. Tompsett from comment #3) > Comment on attachment 70122 [details] [review] [review] > Bug 19873 - Ability to search on 0 value > > Review of attachment 70122 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: C4/Search.pm > @@ +1532,4 @@ > > for ( my $i = 0 ; $i <= @operands ; $i++ ) { > > > > # COMBINE OPERANDS, INDEXES AND OPERATORS > > + if ( $operands[$i] ne '' ) { > > perhaps a $operands[$i] // q{} ne q{} would be in order. > We don't want floody logs if $operands[$i] is undef. > > @@ +1677,4 @@ > > query_desc => $query_desc, > > operator => ($operators[ $i - 1 ]) ? $operators[ $i - 1 ] : '', > > parsed_operand => $operand, > > + original_operand => ($operands[$i] ne '') ? $operands[$i] : '', > > perhaps a $operands[$i] // q{} is sufficient? > We don't want floody logs if $operands[$i] is undef. We want to search on 0 value. And $operands[$i] // q{} is false if $operands[$i] equals 0: (0 ne '') => 1 (0 // '') => 0 (0 // q{}) => 0 (0 ne q{}) => 0
Provide tests please.
Created attachment 72307 [details] [review] Bug 19873 - Add unit tests
Created attachment 72839 [details] [review] Bug 19873 - Ability to search on 0 value Test plan: - create a st-numeric index in zebra conf related to a numeric field i.e: yourindex 1=yourindex 4=109 - fill a field with 0 in a biblio, - reindex your biblios, - search yourindex=0, - should not work, - apply this patch, - test again, - should work Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 72840 [details] [review] Bug 19873 - Add unit tests Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
(In reply to Alex Arnaud from comment #4) > (In reply to M. Tompsett from comment #3) > > Comment on attachment 70122 [details] [review] [review] [review] > > Bug 19873 - Ability to search on 0 value > > > > Review of attachment 70122 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: C4/Search.pm > > @@ +1532,4 @@ > > > for ( my $i = 0 ; $i <= @operands ; $i++ ) { > > > > > > # COMBINE OPERANDS, INDEXES AND OPERATORS > > > + if ( $operands[$i] ne '' ) { > > > > perhaps a $operands[$i] // q{} ne q{} would be in order. > > We don't want floody logs if $operands[$i] is undef. > > > > @@ +1677,4 @@ > > > query_desc => $query_desc, > > > operator => ($operators[ $i - 1 ]) ? $operators[ $i - 1 ] : '', > > > parsed_operand => $operand, > > > + original_operand => ($operands[$i] ne '') ? $operands[$i] : '', > > > > perhaps a $operands[$i] // q{} is sufficient? > > We don't want floody logs if $operands[$i] is undef. > > We want to search on 0 value. And $operands[$i] // q{} is false if > $operands[$i] equals 0: > > (0 ne '') => 1 > (0 // '') => 0 > (0 // q{}) => 0 > (0 ne q{}) => 0 True but I think that what Mark suggested is to replace "if" condition by ($operands[$i] // '') ne '' # I don't like "q{}" string for empty strings :) which is true when $operands[$i] == 0, and doesn't generate a warning when undef and the other change by $operands[$i] // '' which has the same behaviour, with the additional benefit of preventing warnings I will provide a follow-up after testing
I'm not sure how to do: > - search yourindex=0 If I type this in the search form, it searches for "kw,wrdl: yourindex=0", unless I add 'yourindex' in C4::Search::getIndexes, in which case it searches for 'yourindex: 0', and this search works without the patch. Can you explain what I'm doing wrong ?
(In reply to Julian Maurice from comment #10) > I'm not sure how to do: > > - search yourindex=0 > If I type this in the search form, it searches for "kw,wrdl: yourindex=0", > unless I add 'yourindex' in C4::Search::getIndexes, in which case it > searches for 'yourindex: 0', and this search works without the patch. > > Can you explain what I'm doing wrong ? Ok, I had to do a query like this: http://pro.master.koha-dev/cgi-bin/koha/catalogue/search.pl?idx=yourindex&q=0 to trigger the bug. The patch fixes this.
Created attachment 74622 [details] [review] Bug 19873: Ability to search on 0 value Test plan: - create a st-numeric index in zebra conf related to a numeric field i.e: yourindex 1=yourindex 4=109 - fill a field with 0 in a biblio, - reindex your biblios, - search yourindex=0, - should not work, - apply this patch, - test again, - should work Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 74623 [details] [review] Bug 19873: Add unit tests Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 74624 [details] [review] Bug 19873: (QA follow-up) Prevent warnings "Use of uninitialized value $operands[X] in string ne" Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Pushed to master for 18.05, thanks to everybody involved!
Pushed to 17.11.x for 17.11.08