Bug 24269 - Authority matching in Elasticsearch is broken when authority has subdivisions
Summary: Authority matching in Elasticsearch is broken when authority has subdivisions
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 5 votes (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
Depends on: 24421
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-18 19:39 UTC by Nick Clemens
Modified: 2020-03-13 00:46 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04


Attachments
Bug 24269: Adjust C4::Heading to generate headings from auth tags (16.39 KB, patch)
2020-01-14 15:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24269: Adjust C4::Heading to generate headings from auth tags (21.50 KB, patch)
2020-01-27 14:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24269: Adjust C4::Heading to generate headings from auth tags (21.48 KB, patch)
2020-01-27 14:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24269: Adjust C4::Heading to generate headings from auth tags (21.52 KB, patch)
2020-01-27 19:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24269: Adjust C4::Heading to generate headings from auth tags (21.54 KB, patch)
2020-02-14 21:00 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 24269: Adjust C4::Heading to generate headings from auth tags (22.00 KB, patch)
2020-02-19 12:48 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-12-18 19:39:59 UTC
In C4/Heading/MARC21.pm when we call _get_search_heading we look at the heading and turn some subfields into terms so:
150 $aChess$xOpenings

becomes

'Chess generalsubdiv openings'

126 my %subdivisions = (
127     'v' => 'formsubdiv',
128     'x' => 'generalsubdiv',
129     'y' => 'chronologicalsubdiv',
130     'z' => 'geographicsubdiv',
131 );

267                 $heading .= " $subdivisions{$code} $value";


We don't do this when indexing into ES so we will never find matches for these records
Comment 1 Nick Clemens 2020-01-14 15:58:19 UTC
Created attachment 97357 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags
Comment 2 Nick Clemens 2020-01-27 14:19:50 UTC
Created attachment 97981 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags

To test:
1 - Be using Elasticsearch
2 - Check that field 150 is mapped to 'Match-heading' or add it (the subfields don't matter)
3 - Add a topic term authority record like "150 $aCats$vFiction"
4 - Add a 650 with $aCats and $vFiction and  $e depicted to a bibliographic record
5 - Run the linker for the bib
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
6 - Confirm the record is not correctly linked to the record
7 - Apply patch
8 - Reindex authorities for ES
    perl misc/search_tools/rebuild_elasticsearch.pl -v -d -a
9 - Run the linker and confirm record is correctly linked
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
Comment 3 Nick Clemens 2020-01-27 14:22:23 UTC
Created attachment 97982 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags

To test:
1 - Be using Elasticsearch
2 - Check that field 150 is mapped to 'Match-heading' or add it (the subfields don't matter)
3 - Add a topic term authority record like "150 $aCats$vFiction"
4 - Add a 650 with $aCats and $vFiction and  $e depicted to a bibliographic record
5 - Run the linker for the bib
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
6 - Confirm the record is not correctly linked to the record
7 - Apply patch
8 - Reindex authorities for ES
    perl misc/search_tools/rebuild_elasticsearch.pl -v -d -a
9 - Run the linker and confirm record is correctly linked
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
Comment 4 Martin Renvoize 2020-01-27 19:25:51 UTC
Comment on attachment 97982 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags

Review of attachment 97982 [details] [review]:
-----------------------------------------------------------------

::: C4/Heading.pm
@@ +52,5 @@
>  
>  Given a C<MARC::Field> object containing a heading from a 
>  bib record, create a C<C4::Heading> object.
>  
> +The optional third parameter is 'auth' - it is handled as boolean. If supplied we treat the field as an auth record field. Otherwise if it is a bib field. The fields checked are the same in a UNIMARC system and this parameter is ignored

I'm not sure I understand this rewording.. you've removed the '$marc_flavour' param right, and instead, always use the application context?  I think this could be more clearly worded.
Comment 5 Martin Renvoize 2020-01-27 19:26:45 UTC
Created attachment 97996 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags

To test:
1 - Be using Elasticsearch
2 - Check that field 150 is mapped to 'Match-heading' or add it (the subfields don't matter)
3 - Add a topic term authority record like "150 $aCats$vFiction"
4 - Add a 650 with $aCats and $vFiction and  $e depicted to a bibliographic record
5 - Run the linker for the bib
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
6 - Confirm the record is not correctly linked to the record
7 - Apply patch
8 - Reindex authorities for ES
    perl misc/search_tools/rebuild_elasticsearch.pl -v -d -a
9 - Run the linker and confirm record is correctly linked
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-01-27 19:27:44 UTC
Signing off as this logically all makes sense, the tests pass and it passes it's test plan.

Minor comment about rewording the updated POD slightly as it's not entirely clear without reading the code.
Comment 7 Nick Clemens 2020-01-27 20:00:29 UTC
(In reply to Martin Renvoize from comment #4)
> Comment on attachment 97982 [details] [review] [review]
> Bug 24269: Adjust C4::Heading to generate headings from auth tags
> 
> Review of attachment 97982 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Heading.pm
> @@ +52,5 @@
> >  
> >  Given a C<MARC::Field> object containing a heading from a 
> >  bib record, create a C<C4::Heading> object.
> >  
> > +The optional third parameter is 'auth' - it is handled as boolean. If supplied we treat the field as an auth record field. Otherwise if it is a bib field. The fields checked are the same in a UNIMARC system and this parameter is ignored
> 
> I'm not sure I understand this rewording.. you've removed the
> '$marc_flavour' param right, and instead, always use the application
> context?  I think this could be more clearly worded.

I did, I didn't mention it in the POD, meant to mention in the commit message - we had that option, but it was never used and I didn't see a reason to preserve it
Comment 8 Bouzid Fergani 2020-02-14 21:00:10 UTC
Created attachment 99039 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags

To test:
1 - Be using Elasticsearch
2 - Check that field 150 is mapped to 'Match-heading' or add it (the subfields don't matter)
3 - Add a topic term authority record like "150 $aCats$vFiction"
4 - Add a 650 with $aCats and $vFiction and  $e depicted to a bibliographic record
5 - Run the linker for the bib
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
6 - Confirm the record is not correctly linked to the record
7 - Apply patch
8 - Reindex authorities for ES
    perl misc/search_tools/rebuild_elasticsearch.pl -v -d -a
9 - Run the linker and confirm record is correctly linked
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 9 Martin Renvoize 2020-02-19 08:47:39 UTC
We now have a double signoff on this including my own.. I'll do a quick final check before pushing but I think we can safely say this is PQA
Comment 10 Nick Clemens 2020-02-19 12:48:05 UTC
Created attachment 99246 [details] [review]
Bug 24269: Adjust C4::Heading to generate headings from auth tags

To test:
1 - Be using Elasticsearch
2 - Check that field 150 is mapped to 'Match-heading' or add it (the subfields don't matter)
3 - Add a topic term authority record like "150 $aCats$vFiction"
4 - Add a 650 with $aCats and $vFiction and  $e depicted to a bibliographic record
5 - Run the linker for the bib
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v
6 - Confirm the record is not correctly linked to the record
7 - Apply patch
8 - Reindex authorities for ES
    perl misc/search_tools/rebuild_elasticsearch.pl -v -d -a
9 - Run the linker and confirm record is correctly linked
    perl misc/link_bibs_to_authorities.pl --bib-limit "biblionumber=89" -v

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 11 Martin Renvoize 2020-02-19 13:35:19 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-03-13 00:46:51 UTC
Pushed to 19.11.x for 19.11.04