Bug 21581

Summary: Matching rules for authorities do not respect 'Search index' setting
Product: Koha Reporter: Nick Clemens <nick>
Component: MARC Authority data supportAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, januszop, josef.moravec, katrin.fischer, m.de.rooy, martin.renvoize
Version: master   
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: 21962    
Attachments: Bug 21581: Pass through authorities indexes
Bug 21581: Pass through authorities indexes
Bug 21581: Unit tests and updated data
Bug 21581: Pass through authorities indexes
Bug 21581: Unit tests and updated data
Bug 21581: Pass through authorities indexes
Bug 21581: (QA follow-up) Update comment according to code in SearchAuthorities

Description Nick Clemens 2018-10-16 15:08:56 UTC
When 'Search index' is passed to C4/AuthoritiesMarc.pm it is matched against a preset list, then defaulted to all - we should allow to pass through arbitrary indexes

To recreate:
1 - Define a rule:
  Matching rule code: LCAUTH
  Match threshold: 100
  Record type: Authority record
  MATCH POINTS
  Search index: LC-card-number
  Score: 100
  Tag: 010
  Subfield: a
2 - Have a record with an LC number in 010
3 - Export it
4 - Edit the record in Koha, move the value from 010 to another field
5 - Import the original record
6 - Use the matching rule above, you expect it won't match (nothing in 010 anymore)
7 - It matches?!?!?!
8 - Enable zebra logging in koha-conf.xml
  <zebra_loglevels>none,fatal,warn,request,info</zebra_loglevels>
9 - tail -f /var/log/koha/kohadev/zebra-output.log
10 - On manage marc apply matchign with no rule, then reapply with rule
11 - View the output in the log - the index searched is 'Any'
Comment 1 Nick Clemens 2018-10-16 15:18:44 UTC
Created attachment 80658 [details] [review]
Bug 21581: Pass through authorities indexes

FIXME: We need unit tests

To test:
 1 - Define a rule:
  Matching rule code: LCAUTH
  Match threshold: 100
  Record type: Authority record
  MATCH POINTS
  Search index: LC-card-number
  Score: 100
  Tag: 010
  Subfield: a
 2 - Have a record with an LC number in 010
 3 - Export it
 4 - Edit the record in Koha, move the value from 010 to another field
 5 - Import the original record
 6 - Use the matching rule above, you expect it won't match (nothing in 010 anymore)
 7 - It matches?!?!?!
 8 - Enable zebra logging in koha-conf.xml
  <zebra_loglevels>none,fatal,warn,request,info</zebra_loglevels>
 9 - tail -f /var/log/koha/kohadev/zebra-output.log
10 - On manage marc apply matchign with no rule, then reapply with rule
11 - View the output in the log - the index searched is 'Any'
12 - Apply patch
13 - Reapply matching
14 - Note we search the expected index and no match is found
Comment 2 Mark Tompsett 2018-10-16 16:25:56 UTC
Created attachment 80665 [details] [review]
Bug 21581: Pass through authorities indexes

FIXME: We need unit tests

To test:
 1 - Define a rule:
  Matching rule code: LCAUTH
  Match threshold: 100
  Record type: Authority record
  MATCH POINTS
  Search index: LC-card-number
  Score: 100
  Tag: 010
  Subfield: a
 2 - Have a record with an LC number in 010
 3 - Export it
 4 - Edit the record in Koha, move the value from 010 to another field
 5 - Import the original record
 6 - Use the matching rule above, you expect it won't match (nothing in 010 anymore)
 7 - It matches?!?!?!
 8 - Enable zebra logging in koha-conf.xml
  <zebra_loglevels>none,fatal,warn,request,info</zebra_loglevels>
 9 - tail -f /var/log/koha/kohadev/zebra-output.log
10 - On manage marc apply matchign with no rule, then reapply with rule
11 - View the output in the log - the index searched is 'Any'
12 - Apply patch
13 - Reapply matching
14 - Note we search the expected index and no match is found

NOTE: By applying bug 21579, I was able to view the difference, and
      see that it didn't match identically, because I had moved the
      value from 010 to 155. No log checking needed. clean/delete batch.
      Apply patch, no match. clean/delete batch.
      Edit authority back, matches.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 3 Nick Clemens 2018-10-24 11:57:14 UTC
Created attachment 81064 [details] [review]
Bug 21581: Unit tests and updated data

To test:
1 - Apply this patch only
2 - prove -v t/db_dependent/Search.t
3 - It fails
4 - Apply second patch
5 - prove -v t/db_dependent/Search.t
6 - It passes!
Comment 4 Nick Clemens 2018-10-24 11:57:17 UTC
Created attachment 81065 [details] [review]
Bug 21581: Pass through authorities indexes

To test:
 1 - Define a rule:
  Matching rule code: LCAUTH
  Match threshold: 100
  Record type: Authority record
  MATCH POINTS
  Search index: LC-card-number
  Score: 100
  Tag: 010
  Subfield: a
 2 - Have a record with an LC number in 010
 3 - Export it
 4 - Edit the record in Koha, move the value from 010 to another field
 5 - Import the original record
 6 - Use the matching rule above, you expect it won't match (nothing in 010 anymore)
 7 - It matches?!?!?!
 8 - Enable zebra logging in koha-conf.xml
  <zebra_loglevels>none,fatal,warn,request,info</zebra_loglevels>
 9 - tail -f /var/log/koha/kohadev/zebra-output.log
10 - On manage marc apply matchign with no rule, then reapply with rule
11 - View the output in the log - the index searched is 'Any'
12 - Apply patch
13 - Reapply matching
14 - Note we search the expected index and no match is found

NOTE: By applying bug 21579, I was able to view the difference, and
      see that it didn't match identically, because I had moved the
      value from 010 to 155. No log checking needed. clean/delete batch.
      Apply patch, no match. clean/delete batch.
      Edit authority back, matches.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Katrin Fischer 2018-10-28 12:54:31 UTC
I am having a bit of trouble with the test here - also on master:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Search.t
t/db_dependent/Search.t .. 
    #   Failed test 'All records have at least one item available'
    #   at t/db_dependent/Search.t line 464.
    #          got: 'false'
    #     expected: 'true'
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan/node.pm line 102.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan/node.pm line 102.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan/node.pm line 102.
Negative repeat count does nothing at /home/vagrant/kohaclone/Koha/QueryParser/Driver/PQF/query_plan/node.pm line 102.
    # Looks like you failed 1 test of 109.
t/db_dependent/Search.t .. 1/2 
#   Failed test 'MARC21 + DOM'
#   at t/db_dependent/Search.t line 939.
t/db_dependent/Search.t .. 2/2 # Looks like you failed 1 test of 2.
t/db_dependent/Search.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/db_dependent/Search.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1


Maybe someone else can have a look? Patch seems clear enough.
Comment 6 Josef Moravec 2018-10-29 22:38:26 UTC
Created attachment 81599 [details] [review]
Bug 21581: Unit tests and updated data

To test:
1 - Apply this patch only
2 - prove -v t/db_dependent/Search.t
3 - It fails
4 - Apply second patch
5 - prove -v t/db_dependent/Search.t
6 - It passes!

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2018-10-29 22:38:31 UTC
Created attachment 81600 [details] [review]
Bug 21581: Pass through authorities indexes

To test:
 1 - Define a rule:
  Matching rule code: LCAUTH
  Match threshold: 100
  Record type: Authority record
  MATCH POINTS
  Search index: LC-card-number
  Score: 100
  Tag: 010
  Subfield: a
 2 - Have a record with an LC number in 010
 3 - Export it
 4 - Edit the record in Koha, move the value from 010 to another field
 5 - Import the original record
 6 - Use the matching rule above, you expect it won't match (nothing in 010 anymore)
 7 - It matches?!?!?!
 8 - Enable zebra logging in koha-conf.xml
  <zebra_loglevels>none,fatal,warn,request,info</zebra_loglevels>
 9 - tail -f /var/log/koha/kohadev/zebra-output.log
10 - On manage marc apply matchign with no rule, then reapply with rule
11 - View the output in the log - the index searched is 'Any'
12 - Apply patch
13 - Reapply matching
14 - Note we search the expected index and no match is found

NOTE: By applying bug 21579, I was able to view the difference, and
      see that it didn't match identically, because I had moved the
      value from 010 to 155. No log checking needed. clean/delete batch.
      Apply patch, no match. clean/delete batch.
      Edit authority back, matches.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Josef Moravec 2018-10-29 22:38:35 UTC
Created attachment 81601 [details] [review]
Bug 21581: (QA follow-up) Update comment according to code in SearchAuthorities

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Nick Clemens 2018-10-31 15:04:39 UTC
Awesome work all!

Pushed to master for 18.11
Comment 10 Martin Renvoize 2018-11-09 13:06:04 UTC
Pushed to 18.05.x for 18.05.06.

RMaint followup added to prevent, long deprecated, GRS-1 test from running against this new bugfix/enhancement.
Comment 11 Fridolin Somers 2018-11-28 12:59:11 UTC
Pushed to 17.11.x for 17.11.13
Comment 12 Janusz Kaczmarek 2018-12-03 17:09:57 UTC
I seems to me that this patch creates a problem with Zebra searches while queryparser is off. 

Could you please verify the following scenario: search engine zebra, query parser off, and let's do an authority search for e.g. authority number or whatsoever with "Search entire record" (the forth option). Zebra gets the query:

@or @attr 1=Heading @attr 7=1 0 @attr 4=6 @attr 5=1 @attr 1=all <search term>

and answers ERROR 114, that is unknown use attribute.  In fact, the attr-1 'all' is not defined.  Before that patch 'all' was changed into @attr 1=Any.  Now it seems to create problems.

Tests were done with 18.05.06.

Thank you for verifying this.
Comment 13 Janusz Kaczmarek 2018-12-03 17:21:07 UTC
(In reply to Janusz Kaczmarek from comment #12)
> It seems to me that this patch creates a problem with Zebra searches while
> queryparser is off. 
> 
> Could you please verify the following scenario: search engine zebra, query
> parser off, and let's do an authority search for e.g. authority number or
> whatsoever with "Search entire record" (the forth option). Zebra gets the
> query:
> 
> @or @attr 1=Heading @attr 7=1 0 @attr 4=6 @attr 5=1 @attr 1=all <search term>
> 
> and answers ERROR 114, that is unknown use attribute.  In fact, the attr-1
> 'all' is not defined.  Before that patch 'all' was changed into @attr 1=Any.
> Now it seems to create problems.
> 
> Tests were done with 18.05.06.
> 
> Thank you for verifying this.

My fast work around is:

--- a/C4/AuthoritiesMarc.pm
+++ b/C4/AuthoritiesMarc.pm
@@ -163,6 +163,9 @@ sub SearchAuthorities {
                 elsif ( @$tags[$i] eq "thesaurus" ) {
                     $attr = " \@attr 1=Subject-heading-thesaurus ";
                 }
+                elsif ( @$tags[$i] eq "all" ) {
+                    $attr = " \@attr 1=Any ";
+                }
                 else {    # Use the index passed in params
                     $attr = " \@attr 1=" . @$tags[$i] . " ";
                 }

but I'm not sure if it wouldn't destruct the idea of the author of this patch, so I'm not submitting it.
Comment 14 Fridolin Somers 2018-12-06 09:55:42 UTC
(In reply to Janusz Kaczmarek from comment #13)
> (In reply to Janusz Kaczmarek from comment #12)
> > It seems to me that this patch creates a problem with Zebra searches while
> > queryparser is off. 
> > 
> > Could you please verify the following scenario: search engine zebra, query
> > parser off, and let's do an authority search for e.g. authority number or
> > whatsoever with "Search entire record" (the forth option). Zebra gets the
> > query:
> > 
> > @or @attr 1=Heading @attr 7=1 0 @attr 4=6 @attr 5=1 @attr 1=all <search term>
> > 
> > and answers ERROR 114, that is unknown use attribute.  In fact, the attr-1
> > 'all' is not defined.  Before that patch 'all' was changed into @attr 1=Any.
> > Now it seems to create problems.
> > 
> > Tests were done with 18.05.06.
> > 
> > Thank you for verifying this.
> 
> My fast work around is:
> 
> --- a/C4/AuthoritiesMarc.pm
> +++ b/C4/AuthoritiesMarc.pm
> @@ -163,6 +163,9 @@ sub SearchAuthorities {
>                  elsif ( @$tags[$i] eq "thesaurus" ) {
>                      $attr = " \@attr 1=Subject-heading-thesaurus ";
>                  }
> +                elsif ( @$tags[$i] eq "all" ) {
> +                    $attr = " \@attr 1=Any ";
> +                }
>                  else {    # Use the index passed in params
>                      $attr = " \@attr 1=" . @$tags[$i] . " ";
>                  }
> 
> but I'm not sure if it wouldn't destruct the idea of the author of this
> patch, so I'm not submitting it.

Thanks for your work.
But please open a new bug report, this one is already pushed to git branches.
Comment 15 Janusz Kaczmarek 2018-12-06 15:09:38 UTC
Cf. Bug 21962