Bug 19873 - Make it possible to search on value 0
Summary: Make it possible to search on value 0
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Alex Arnaud
QA Contact: Julian Maurice
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-22 14:17 UTC by Gaetan Boisson
Modified: 2018-04-20 16:45 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19873 - Ability to search on 0 value (1.52 KB, patch)
2017-12-22 14:28 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19873 - Add unit tests (1.48 KB, patch)
2018-03-01 13:24 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19873 - Ability to search on 0 value (1.58 KB, patch)
2018-03-14 11:16 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 19873 - Add unit tests (1.54 KB, patch)
2018-03-14 11:16 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 19873: Ability to search on 0 value (1.65 KB, patch)
2018-04-20 12:35 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19873: Add unit tests (1.60 KB, patch)
2018-04-20 12:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19873: (QA follow-up) Prevent warnings (1.36 KB, patch)
2018-04-20 12:36 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Gaetan Boisson 2017-12-22 14:17:22 UTC
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.
Comment 1 Alex Arnaud 2017-12-22 14:28:25 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
Comment 2 Fridolin SOMERS 2017-12-22 14:35:35 UTC
+ original_operand => ($operands[$i] ne '') ? $operands[$i] : '',

I think it could be more pretty :

+ original_operand => $operands[$i] // '',
Comment 3 M. Tompsett 2018-01-18 17:45:46 UTC
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.
Comment 4 Alex Arnaud 2018-01-22 14:35:15 UTC
(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
Comment 5 Jonathan Druart 2018-02-27 15:02:47 UTC
Provide tests please.
Comment 6 Alex Arnaud 2018-03-01 13:24:04 UTC
Created attachment 72307 [details] [review]
Bug 19873 - Add unit tests
Comment 7 Brendan Gallagher 2018-03-14 11:16:09 UTC
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>
Comment 8 Brendan Gallagher 2018-03-14 11:16:22 UTC
Created attachment 72840 [details] [review]
Bug 19873 - Add unit tests

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 9 Julian Maurice 2018-04-20 11:31:40 UTC
(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
Comment 10 Julian Maurice 2018-04-20 12:23:26 UTC
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 ?
Comment 11 Julian Maurice 2018-04-20 12:29:37 UTC
(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.
Comment 12 Julian Maurice 2018-04-20 12:35:57 UTC
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>
Comment 13 Julian Maurice 2018-04-20 12:36:06 UTC
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>
Comment 14 Julian Maurice 2018-04-20 12:36:10 UTC
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>
Comment 15 Jonathan Druart 2018-04-20 16:45:11 UTC
Pushed to master for 18.05, thanks to everybody involved!