Summary: | Make it possible to search on value 0 | ||
---|---|---|---|
Product: | Koha | Reporter: | Gaetan Boisson <gaetan.boisson> |
Component: | Searching | Assignee: | Alex Arnaud <alex.arnaud> |
Status: | CLOSED FIXED | QA Contact: | Julian Maurice <julian.maurice> |
Severity: | normal | ||
Priority: | P5 - low | CC: | alex.arnaud, fridolin.somers, jonathan.druart, julian.maurice, martin.renvoize |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Bug Depends on: | |||
Bug Blocks: | 28365 | ||
Attachments: |
Bug 19873 - Ability to search on 0 value
Bug 19873 - Add unit tests Bug 19873 - Ability to search on 0 value Bug 19873 - Add unit tests Bug 19873: Ability to search on 0 value Bug 19873: Add unit tests Bug 19873: (QA follow-up) Prevent warnings |
Description
Gaetan Boisson
2017-12-22 14:17:22 UTC
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 |