Bug 33159 - Thesaurus is not defined by second indicator for controlled fields outside of 6XX
Summary: Thesaurus is not defined by second indicator for controlled fields outside of...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize
URL:
Keywords:
: 31924 (view as bug list)
Depends on: 30280 33100
Blocks: 33277
  Show dependency treegraph
 
Reported: 2023-03-07 16:27 UTC by Nick Clemens (kidclamp)
Modified: 2023-11-03 23:34 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.05


Attachments
Bug 33159: Unit tests (5.29 KB, patch)
2023-03-07 19:38 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Correctly form thesaurus term for non-subject headings (3.46 KB, patch)
2023-03-07 19:38 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.42 KB, patch)
2023-03-08 18:09 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.47 KB, patch)
2023-03-08 19:32 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33159: Unit tests (5.34 KB, patch)
2023-03-08 23:41 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 33159: Correctly form thesaurus term for non-subject headings (3.52 KB, patch)
2023-03-08 23:41 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.47 KB, patch)
2023-03-08 23:41 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 33159: Unit tests (7.02 KB, patch)
2023-03-09 20:52 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Correctly form thesaurus term for non-subject headings (3.52 KB, patch)
2023-03-09 20:52 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.47 KB, patch)
2023-03-09 20:52 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Unit tests (7.13 KB, patch)
2023-03-10 09:59 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 33159: Correctly form thesaurus term for non-subject headings (3.62 KB, patch)
2023-03-10 09:59 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.58 KB, patch)
2023-03-10 09:59 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 33159: Unit tests (7.14 KB, patch)
2023-03-15 16:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33159: Correctly form thesaurus term for non-subject headings (3.63 KB, patch)
2023-03-15 16:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.59 KB, patch)
2023-03-15 16:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33159: Unit tests (7.25 KB, patch)
2023-03-20 14:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Correctly form thesaurus term for non-subject headings (3.63 KB, patch)
2023-03-20 14:31 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33159: Simplify ES handling and fix zebra handling (6.59 KB, patch)
2023-03-20 14:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-03-07 16:27:34 UTC
Bug 30280 added code to use the thesaurus defined by second indicator in controlled fields. This works for 6XX fields, but not other controlled fields like 1XX, 7XX
Comment 1 Nick Clemens (kidclamp) 2023-03-07 19:38:18 UTC
Created attachment 147881 [details] [review]
Bug 33159: Unit tests
Comment 2 Nick Clemens (kidclamp) 2023-03-07 19:38:20 UTC
Created attachment 147882 [details] [review]
Bug 33159: Correctly form thesaurus term for non-subject headings

Bug 30280 fixed things for subject heading, but didn't take into account
that all non-subject headings are considered LCSH - this means we can't
really handle different thesauri for controlled headings outside of
subjects - maybe a topic for a new bug, but we should fix matching for
them now

To test:
 1 - Set sysprefs:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - generate
    CatalogModuleRelink - Do
    AutoLinkBiblio - Do
    LinkerModule - first match
 2 - Find a record with an author attached to an authority, I used
  biblionumber 3 "Introduction to Attic Greek"
 3 - Edit in advanced editor
 4 - Delete the $9 link
 5 - Save the record
 6 - It generates a new number
 7 - Repeat 4-5, it does it again
 8 - Apply patch and restart all
 9 - Repeat 4-5
10 - It links to the original authority
Comment 3 Phil Ringnalda 2023-03-08 03:21:51 UTC
I'd like to say this works, because any progress is some progress, but...

Without the patch, I set the prefs, remove the $9 from both the author and the subject in bib 3 and save, the subject is relinked to authority 7

With the patch, I set the prefs, remove the $9 from both the author and the subject and save, the subject is linked to a newly created exact duplicate authority, as is the author.

Bonus fun (which is probably from bug 33100 being even more broken than we thought, though the patch there doesn't fix it) if I save again the author gets a new authority and a relink to it every time, and if I add the period to the subject which should be there according to punctuation rules, then it gets a relink to a new authority every time too.

Even ignoring punctuation issues, it looks like this patch would fix linking to non-subjects, but at the cost of autocreating a new duplicate record for every record. Which means for the non-autocreate case, it would break auto-linking for everything, instead of just for non-subjects.
Comment 4 Frank Hansen 2023-03-08 17:53:42 UTC
Nick,

I have almost the same thought. Koha hardcodes non-6XX fields to use LCSH (in C4/Header/MARC21.pm). But actually, Thesaurus searches are only relevant for 6XX fields. So I would like to limit thesaurus searches to be done on 6XX fields only. In addition to this, we get problems if thesaurus searches are performed, because our non-6XX authority records that arrives from Libris (Royal Library of Sweden) are coded as "|" (No attempt to code) in 008/11, which means that e.g. personal names cannot be linked unless they are lcsh. So therefore have made the following change:

-    if ( $thesaurus ) {
+    if ( $thesaurus && $self->{field}->{_tag} =~ m/6../ ) {
 
It would work for us. But I don't know if it has other consequences.

Regards,
Frank Hansen
Comment 5 Nick Clemens (kidclamp) 2023-03-08 18:09:10 UTC
Created attachment 147970 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES
Comment 6 ByWater Sandboxes 2023-03-08 19:32:06 UTC
Created attachment 147980 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 7 Phil Ringnalda 2023-03-08 21:27:38 UTC
Probably just an artifact of how sandboxes are set up, but for Zebra in a sandbox I had to do a full Zebra reindex before name authorities worked correctly.
Comment 8 Phil Ringnalda 2023-03-08 23:41:46 UTC
Created attachment 147986 [details] [review]
Bug 33159: Unit tests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 9 Phil Ringnalda 2023-03-08 23:41:48 UTC
Created attachment 147987 [details] [review]
Bug 33159: Correctly form thesaurus term for non-subject headings

Bug 30280 fixed things for subject heading, but didn't take into account
that all non-subject headings are considered LCSH - this means we can't
really handle different thesauri for controlled headings outside of
subjects - maybe a topic for a new bug, but we should fix matching for
them now

To test:
 1 - Set sysprefs:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - generate
    CatalogModuleRelink - Do
    AutoLinkBiblio - Do
    LinkerModule - first match
 2 - Find a record with an author attached to an authority, I used
  biblionumber 3 "Introduction to Attic Greek"
 3 - Edit in advanced editor
 4 - Delete the $9 link
 5 - Save the record
 6 - It generates a new number
 7 - Repeat 4-5, it does it again
 8 - Apply patch and restart all
 9 - Repeat 4-5
10 - It links to the original authority

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 10 Phil Ringnalda 2023-03-08 23:41:50 UTC
Created attachment 147988 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 11 Frank Hansen 2023-03-09 15:38:10 UTC
(In reply to Nick Clemens from comment #5)
> Created attachment 147970 [details] [review] [review]
> Bug 33159: Simplify ES handling and fix zebra handling
> 
> Before this patch we used two indexes for the thesaurus values, we can
> simply index both needed fields into a single index and just form the
> search correctly.
> 
> This patch also ensures we pass the 'thesaurus' vlaue for the heading
> directly to the query builder - for zebra it goes through, and for ES
> we convert it to the expected code.
> 
> This patch also moves the necessary mappings out of the user definable
> mappings and hardcodes them. There is precedent for this with
> 'match-heading', it ensures matching works as expected
> 
> To test:
> 1 - Follow previous test plan in Zebra and ES

It seems to work with Zebra now. But for ES, after resetting the mappings and re-index (recreating) the indexes, linking doesn't work anymore. I notices that both Subject-heading-thesaurus and Subject-heading-thesaurus-conventions index was removed from mappings.yaml. Was that a mistake? I thought you want want a single index for both 008/_11 and 040f like Janusz did?

I added these lines to the mappings file:

@@ -805,6 +805,20 @@ authorities:
         sort: ~
         suggestible: ''
     type: ''
+  Subject-heading-thesaurus:
+    label: Subject-heading-thesaurus
+    mappings:
+      - facet: ''
+        marc_field: 008_/11
+        marc_type: marc21
+        sort: ~
+        suggestible: ''
+      - facet: ''
+        marc_field: 040f
+        marc_type: marc21
+        sort: ~
+        suggestible: ''
+    type: thesaurus
   authtype:
     label: authtype
     mappings:

After resetting the mappings and re-index, linking works again in ES.

Regards,
Frank
Comment 12 Nick Clemens (kidclamp) 2023-03-09 15:41:06 UTC
(In reply to Frank Hansen from comment #11)
> (In reply to Nick Clemens from comment #5)
> > Created attachment 147970 [details] [review] [review] [review]
> > Bug 33159: Simplify ES handling and fix zebra handling
> > 
> > Before this patch we used two indexes for the thesaurus values, we can
> > simply index both needed fields into a single index and just form the
> > search correctly.
> > 
> > This patch also ensures we pass the 'thesaurus' vlaue for the heading
> > directly to the query builder - for zebra it goes through, and for ES
> > we convert it to the expected code.
> > 
> > This patch also moves the necessary mappings out of the user definable
> > mappings and hardcodes them. There is precedent for this with
> > 'match-heading', it ensures matching works as expected
> > 
> > To test:
> > 1 - Follow previous test plan in Zebra and ES
> 
> It seems to work with Zebra now. But for ES, after resetting the mappings
> and re-index (recreating) the indexes, linking doesn't work anymore. I
> notices that both Subject-heading-thesaurus and
> Subject-heading-thesaurus-conventions index was removed from mappings.yaml.
> Was that a mistake? I thought you want want a single index for both 008/_11
> and 040f like Janusz did?
> 
> I added these lines to the mappings file:
> 
> @@ -805,6 +805,20 @@ authorities:
>          sort: ~
>          suggestible: ''
>      type: ''
> +  Subject-heading-thesaurus:
> +    label: Subject-heading-thesaurus
> +    mappings:
> +      - facet: ''
> +        marc_field: 008_/11
> +        marc_type: marc21
> +        sort: ~
> +        suggestible: ''
> +      - facet: ''
> +        marc_field: 040f
> +        marc_type: marc21
> +        sort: ~
> +        suggestible: ''
> +    type: thesaurus
>    authtype:
>      label: authtype
>      mappings:
> 
> After resetting the mappings and re-index, linking works again in ES.
> 
> Regards,
> Frank

They are removed from the mappings, and moved into Koha::SearchEngine::Elasticsearch - they are hardcoded to always be present, so it should work after recreating the indexes - I will retest on my end
Comment 13 Frank Hansen 2023-03-09 16:37:24 UTC
(In reply to Nick Clemens from comment #12)
> (In reply to Frank Hansen from comment #11)
> > (In reply to Nick Clemens from comment #5)
> > > Created attachment 147970 [details] [review] [review] [review] [review]
> > > Bug 33159: Simplify ES handling and fix zebra handling
> > 
> > It seems to work with Zebra now. But for ES, after resetting the mappings
> > and re-index (recreating) the indexes, linking doesn't work anymore. I
> > notices that both Subject-heading-thesaurus and
> > Subject-heading-thesaurus-conventions index was removed from mappings.yaml.
> > Was that a mistake? I thought you want want a single index for both 008/_11
> 
> They are removed from the mappings, and moved into
> Koha::SearchEngine::Elasticsearch - they are hardcoded to always be present,
> so it should work after recreating the indexes - I will retest on my end

Ah, I gave it another try and Subject-heading-thesaurus is actually created. I think its ok then.

Regards,
Frank
Comment 14 Nick Clemens (kidclamp) 2023-03-09 20:52:28 UTC
Created attachment 148032 [details] [review]
Bug 33159: Unit tests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 15 Nick Clemens (kidclamp) 2023-03-09 20:52:31 UTC
Created attachment 148033 [details] [review]
Bug 33159: Correctly form thesaurus term for non-subject headings

Bug 30280 fixed things for subject heading, but didn't take into account
that all non-subject headings are considered LCSH - this means we can't
really handle different thesauri for controlled headings outside of
subjects - maybe a topic for a new bug, but we should fix matching for
them now

To test:
 1 - Set sysprefs:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - generate
    CatalogModuleRelink - Do
    AutoLinkBiblio - Do
    LinkerModule - first match
 2 - Find a record with an author attached to an authority, I used
  biblionumber 3 "Introduction to Attic Greek"
 3 - Edit in advanced editor
 4 - Delete the $9 link
 5 - Save the record
 6 - It generates a new number
 7 - Repeat 4-5, it does it again
 8 - Apply patch and restart all
 9 - Repeat 4-5
10 - It links to the original authority

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 16 Nick Clemens (kidclamp) 2023-03-09 20:52:33 UTC
Created attachment 148034 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 17 Frank Hansen 2023-03-10 09:59:17 UTC
Created attachment 148050 [details] [review]
Bug 33159: Unit tests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 18 Frank Hansen 2023-03-10 09:59:20 UTC
Created attachment 148051 [details] [review]
Bug 33159: Correctly form thesaurus term for non-subject headings

Bug 30280 fixed things for subject heading, but didn't take into account
that all non-subject headings are considered LCSH - this means we can't
really handle different thesauri for controlled headings outside of
subjects - maybe a topic for a new bug, but we should fix matching for
them now

To test:
 1 - Set sysprefs:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - generate
    CatalogModuleRelink - Do
    AutoLinkBiblio - Do
    LinkerModule - first match
 2 - Find a record with an author attached to an authority, I used
  biblionumber 3 "Introduction to Attic Greek"
 3 - Edit in advanced editor
 4 - Delete the $9 link
 5 - Save the record
 6 - It generates a new number
 7 - Repeat 4-5, it does it again
 8 - Apply patch and restart all
 9 - Repeat 4-5
10 - It links to the original authority

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 19 Frank Hansen 2023-03-10 09:59:22 UTC
Created attachment 148052 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 20 Martin Renvoize 2023-03-15 16:14:28 UTC
Created attachment 148227 [details] [review]
Bug 33159: Unit tests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2023-03-15 16:14:31 UTC
Created attachment 148228 [details] [review]
Bug 33159: Correctly form thesaurus term for non-subject headings

Bug 30280 fixed things for subject heading, but didn't take into account
that all non-subject headings are considered LCSH - this means we can't
really handle different thesauri for controlled headings outside of
subjects - maybe a topic for a new bug, but we should fix matching for
them now

To test:
 1 - Set sysprefs:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - generate
    CatalogModuleRelink - Do
    AutoLinkBiblio - Do
    LinkerModule - first match
 2 - Find a record with an author attached to an authority, I used
  biblionumber 3 "Introduction to Attic Greek"
 3 - Edit in advanced editor
 4 - Delete the $9 link
 5 - Save the record
 6 - It generates a new number
 7 - Repeat 4-5, it does it again
 8 - Apply patch and restart all
 9 - Repeat 4-5
10 - It links to the original authority

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2023-03-15 16:14:34 UTC
Created attachment 148229 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2023-03-15 16:17:18 UTC
I'm happy to have seen this worked through.. all appears to be good in the code to my eye and I'm trusting Phil and Franks cataloguing knowledge.

Passing QA
Comment 24 Janusz Kaczmarek 2023-03-17 20:24:53 UTC
For me, it does not seem to work (tested with ./link_bibs_to_authorities.pl -v -l --test  and  AutoCreateAuthorities set to 'don't generate'--I don't want new auths to be generated since I have them already, and I want them to be linked).  

Consider following (a bit simplified) real world example.  These records (in sense of auth. 008/040 and the use in biblio) all conform to MARC 21 definitions:

BIBLIO:

00460nam a2200145 i 4500
008 230210s2023    xxk           000 0 eng d
100 1  $a Shakespeare, William
245    $a Hamlet ; $b Othello / $c William Shakespeare.
246 3  $a Othello
490 1  $a Great Books for Busy People
600 07 $a Hamlet $c (postać fikcyjna) $2 DBN
655  4 $a Literatura światowa
655  7 $a Literatura angielska $2 DBN
700 12 $a Shakespeare, William. $t Othello
830  0 $a Great Books for Busy People

AUTHS:

00116nz  a2200049n  4500
008 230210nn a|nnnabbn           a aaa     d
100 1  $a Shakespeare, William

00126nz  a2200049n  4500
008 230210nn a|nnnabbn           a aaa     d
100 1  $a Shakespeare, William. $t Othello

00142nz  a2200061n  4500
008 230210nn a|znnbabn           a aaa     d
040    $f DBN
100 0  $a Hamlet $c (postać fikcyjna)

00123nz  a2200049n  4500
008 230210nn aznabbban           a ana     d
130  0 $a Great Books for Busy People

00116nz  a2200049n  4500
008 230210 n anznnbabn           a ana     d
155    $a Literatura angielska

00116nz  a2200049n  4500
008 230210 n an|nnbabn           a ana     d
155    $a Literatura światowa


All the six headings (100, 600, 2 x 655, 700 and 830) should be properly linked with their appropriate auth records.  But only one of the is actually being linked (Hamlet (postać fikcyjna)), the rest is not (nor under Zebra, nor with ES).


BTW, this is correct MARC 21 data that should be linked in any case.  But since there are libraries with local data, that are not paying much attention on 008 etc., the should be IMHO a syspref to switch the strict behavior off.
Comment 25 Janusz Kaczmarek 2023-03-17 22:22:50 UTC
Here some more details, what and why goes wrong:

100 1  $a Shakespeare, William

-- It does not make sense to include in the query (and check) subject-heading-thesaurus, since we are looking for auth for 100, not for 6XX (It would have sense to check 008/14 of the auth. rec., but this is another story)

655  4 $a Literatura światowa

--For ind2 == 4 we should look for an auth with 008/11 == ‘|’.  In the proposal the search is being performed for subject-heading-thesaurus = 'n', which is wrong (008/11 = n is used in auth when "1XX heading does not conform to subject heading system/thesaurus conventions and is not appropriate for use as a subject added entry in bibliographic records" -- that is non intended for use in 6XX, would have 008/15 = 'b')

655  7 $a Literatura angielska $2 DBN

-- It is correct to look for auth subject-heading-thesaurus = 'DBN', but in case of failure the second search should be performed with subject-heading-thesaurus = 'z'.  This is because 040 $f is not obligatory for 008/11 = 'z' (according to MARC 21: "A MARC code for the conventions used to formulate the heading MAY be contained in subfield $f (Subject heading/thesaurus conventions) in field 040 (Cataloging Source)".  If there is only one thesaurus present in the system, it is legal not to have 040 $f in the auth recs., having $2 in 6XX in biblio.

700 12 $a Shakespeare, William. $t Othello

-- Same as for 100 -- no sense of querying for subject-heading-thesaurus.

830  0 $a Great Books for Busy People

-- Same as for 100 -- no sense of querying for subject-heading-thesaurus.


I have not enough courage to remove the status 'Passed QA' but IMHO the patch should be reviewed again.  Otherwise we would have to patch the patch right after its application.
Comment 26 Phil Ringnalda 2023-03-17 23:15:54 UTC
There is sense in looking for a thesaurus for 100, 700, and 830, because they share authority records with 600 and 630. If you don't look for a thesaurus to decide between two or more Shakespeare, William authority records, then the default linker says "oops, more than one result, no link for you" (and worse yet, first match or last match could decide to link to the authority record for a subject thesaurus that you could later decide you didn't want to use, and when you deleted the authority record you would not only delete the subject headings, you would also delete the author headings).

This bug is about making what bug 30280 tried to do actually work, a narrow enough scope that is has made it to Passed QA and is ready to land.

"Bug 30280 shouldn't have mapped ind2=4 to 008/11=n" and "Bug 30280 should have treated 008/11=z with no 040$f as a wildcard authority that matches any value of $2 when no other authority with a match in 040$f exists" are both legitimate other followup bugs (as is "Bug 30280 should have put its new behavior behind a preference that defaults to off"), but they aren't part of making what it tried to do actually work.
Comment 27 Phil Ringnalda 2023-03-17 23:16:58 UTC
*** Bug 31924 has been marked as a duplicate of this bug. ***
Comment 28 Janusz Kaczmarek 2023-03-18 00:02:34 UTC
(In reply to Phil Ringnalda from comment #26)
> There is sense in looking for a thesaurus for 100, 700, and 830, because
> they share authority records with 600 and 630. If you don't look for a
> thesaurus to decide between two or more Shakespeare, William authority
> records, then the default linker says "oops, more than one result, no link
> for you" (and worse yet, first match or last match could decide to link to
> the authority record for a subject thesaurus that you could later decide you
> didn't want to use, and when you deleted the authority record you would not
> only delete the subject headings, you would also delete the author headings).

Phil, thank you for your comment.  Maybe what you are saying is true in some contexts (US?), but in others -- not.  Here is a real authority record for William Shakespeare from NLPol: https://dbn.bn.org.pl/descriptor-details/a0000001180305.  You will not find it in Koha when unnecessarily enriching the query for the main heading with subject-heading-thesaurus = 'a', because it has nothing to do with LCSH.  If we really wanted to narrow the query in this case, we should enrich it with Heading-use-main-or-added-entry = 'a'.  And yes, this auth record it is shared for the use in 600 (008/15 = 'a'), but the thesaurus is not LCSH but DBN (note, that it is not explicitly declared in 040 $f, which is correct). 

So even if in some (not all) cases the auth record you are looking for for your main or added entry (1XX/7XX) was shared with 6XX, from the heading field of biblio you cannot state what thesaurus is would be.  It is not prudent to assume that it is definitely a LCSH what is made now. 

> This bug is about making what bug 30280 tried to do actually work, a narrow
> enough scope that is has made it to Passed QA and is ready to land.
> 
> "Bug 30280 shouldn't have mapped ind2=4 to 008/11=n" and "Bug 30280 should
> have treated 008/11=z with no 040$f as a wildcard authority that matches any
> value of $2 when no other authority with a match in 040$f exists" are both
> legitimate other followup bugs (as is "Bug 30280 should have put its new
> behavior behind a preference that defaults to off"), but they aren't part of
> making what it tried to do actually work.

IMHO, if we are trying to fix the problems introduced by Bug 30280, it should be done with possibly complete and universal set of test data.  Of course one can always prepare test data that pass the test, but in real life the application will fail for the people.  Now the linking is not working for some libraries (before 30280 it was not perfect, but was acceptable in most cases), and it will still not work after applying this patch.  So what would be the point to introduce a patch that is known to be problematic, to put it mildly...
Comment 29 Nick Clemens (kidclamp) 2023-03-20 14:26:00 UTC
I filed bug 33277 - I think we can require that one before pushing, but still work on a new bug, expanding the tests and code there, but agreeing on this base as it solves the problem for some cases.
Comment 30 Nick Clemens (kidclamp) 2023-03-20 14:31:56 UTC
Created attachment 148414 [details] [review]
Bug 33159: Unit tests

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Nick Clemens (kidclamp) 2023-03-20 14:31:58 UTC
Created attachment 148415 [details] [review]
Bug 33159: Correctly form thesaurus term for non-subject headings

Bug 30280 fixed things for subject heading, but didn't take into account
that all non-subject headings are considered LCSH - this means we can't
really handle different thesauri for controlled headings outside of
subjects - maybe a topic for a new bug, but we should fix matching for
them now

To test:
 1 - Set sysprefs:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - generate
    CatalogModuleRelink - Do
    AutoLinkBiblio - Do
    LinkerModule - first match
 2 - Find a record with an author attached to an authority, I used
  biblionumber 3 "Introduction to Attic Greek"
 3 - Edit in advanced editor
 4 - Delete the $9 link
 5 - Save the record
 6 - It generates a new number
 7 - Repeat 4-5, it does it again
 8 - Apply patch and restart all
 9 - Repeat 4-5
10 - It links to the original authority

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Nick Clemens (kidclamp) 2023-03-20 14:32:01 UTC
Created attachment 148416 [details] [review]
Bug 33159: Simplify ES handling and fix zebra handling

Before this patch we used two indexes for the thesaurus values, we can
simply index both needed fields into a single index and just form the
search correctly.

This patch also ensures we pass the 'thesaurus' vlaue for the heading
directly to the query builder - for zebra it goes through, and for ES
we convert it to the expected code.

This patch also moves the necessary mappings out of the user definable
mappings and hardcodes them. There is precedent for this with
'match-heading', it ensures matching works as expected

To test:
1 - Follow previous test plan in Zebra and ES

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Janusz Kaczmarek 2023-03-20 21:27:29 UTC
(In reply to Nick Clemens from comment #29)
> I filed bug 33277 - I think we can require that one before pushing, but
> still work on a new bug, expanding the tests and code there, but agreeing on
> this base as it solves the problem for some cases.

Nick, OK, I understand.  But we have still remember about two other classes of problems:

1. 6XX _7 $a NNN $2 <thesaurus_code>

   but no 040 $f in auth rec.  This is common and perfectly legal when is 
   planed to have only one thesaurus in the system - like in data from NLPol,
   e.g.:
   http://data.bn.org.pl/api/institutions/authorities.marcxml?id=a0000001003606
   -- the auth record is to be used in 6XX (note 008/15='a') with 2nd ind. = 7
   (note 008/11 = 'z')

   and the use example: http://data.bn.org.pl/api/networks/bibs.marcxml?id=b0000003061976:
   <datafield ind1=" " ind2="7" tag="655">
       <subfield code="a">Literatura angielska</subfield>
       <subfield code="2">DBN</subfield>
   </datafield>


2. 6XX _4 -- we should look for an auth rec. with 008/11 = '|', non 'n' ('n'
   means it is not, roughly speaking, 'it is not for 6XX', and more precisely:
   "is not appropriate for use as a subject added entry in bibliographic
   records").


You can have a look at the code proposed in Bug 32916 (where all the problems have been solved) and adapt it.
Comment 34 Tomás Cohen Arazi 2023-03-21 19:21:35 UTC
Nick: I haven't had the chance of checking what you're doing here, but I notice Janusz insists with a different approach that covers more cases.

Is that something you're dealing with on bug 33277? Should new bugs be filled for the remaining cases now this one is PQA?

Janusz: Do you think we could go with this one for now and work on follow-up bugs for the rest of it?

I'd like to hear from you before pushing this needed fix to master.

Thanks in advance!
Comment 35 Nick Clemens (kidclamp) 2023-03-21 19:45:08 UTC
(In reply to Janusz Kaczmarek from comment #33)
> (In reply to Nick Clemens from comment #29)
> > I filed bug 33277 - I think we can require that one before pushing, but
> > still work on a new bug, expanding the tests and code there, but agreeing on
> > this base as it solves the problem for some cases.
> 
> Nick, OK, I understand.  But we have still remember about two other classes
> of problems:
> 
> 1. 6XX _7 $a NNN $2 <thesaurus_code>
> 
>    but no 040 $f in auth rec.  This is common and perfectly legal when is 
>    planed to have only one thesaurus in the system - like in data from NLPol,
>    e.g.:
>   
> http://data.bn.org.pl/api/institutions/authorities.marcxml?id=a0000001003606
>    -- the auth record is to be used in 6XX (note 008/15='a') with 2nd ind. =
> 7
>    (note 008/11 = 'z')
> 
>    and the use example:
> http://data.bn.org.pl/api/networks/bibs.marcxml?id=b0000003061976:
>    <datafield ind1=" " ind2="7" tag="655">
>        <subfield code="a">Literatura angielska</subfield>
>        <subfield code="2">DBN</subfield>
>    </datafield>
> 
> 
> 2. 6XX _4 -- we should look for an auth rec. with 008/11 = '|', non 'n' ('n'
>    means it is not, roughly speaking, 'it is not for 6XX', and more
> precisely:
>    "is not appropriate for use as a subject added entry in bibliographic
>    records").
> 
> 
> You can have a look at the code proposed in Bug 32916 (where all the
> problems have been solved) and adapt it.

I addressed those as well as I could on 33277, please test. The second scenario requires larger work, but didn't work before 30280 either.

While I agree on fixing all the problems we see, I think we should push this, work on 33277, and file more bugs if needed.

It seems that 30280 has revealed many problems with our linking, and I love getting all the feedback here, but some of these issues predate 30280. Fixing each issue on its own will allow for easier testing, quicker fixes, and less likelihood of new breakages
Comment 36 Janusz Kaczmarek 2023-03-21 20:36:18 UTC
(In reply to Tomás Cohen Arazi from comment #34)
> Nick: I haven't had the chance of checking what you're doing here, but I
> notice Janusz insists with a different approach that covers more cases.
> 
> Is that something you're dealing with on bug 33277? Should new bugs be
> filled for the remaining cases now this one is PQA?
> 
> Janusz: Do you think we could go with this one for now and work on follow-up
> bugs for the rest of it?
> 
> I'd like to hear from you before pushing this needed fix to master.
> 
> Thanks in advance!

Tomás, thank you for asking.

I do not claim that my fix is the best possible, but it seems to me that it solves all the problems introduced by the unfortunate 30280 while retaining its advantages, and works not only in the context of Anglo-American libraries, but universally.  In addition, it provides the old-styled loose linking for libraries that didn't care about 008 correctness in their authority records (although I didn't add sysprefs - for initial testing purposes, you'd have to do it manually).  The only thing missing there (and which we haven't had at all so far) is checking the 008/14-16 value of auth records, depending on which biblio field you want to link to.  This should be definitely the next step (I’m doing this locally, in a clumsy way, in my installations anyway, so it would be nice to have it in the official version). 

I understand Nick's argument that small fixes are easier to test, but in this case I'm not convinced that correcting 30280 with bugfix 33159, which is known in advance to require another bug (33277), is the prudent and efficient (in terms of time and effort) way to go. 

At the end, however, the point is that libraries should get a tool in the next version that works no worse than before 30280 in linking, and one that does not create unnecessary automatic authority records with AutoCreateAuthorities enabled, when the right (often "official") record already exists in the database.  So, I don’t want to generate problems or tensions.  Nick has contributed much more to the project and knows the code much better--we can follow his path even if I am not fully convinced, provided we make an effort to give in one release a working solution for the libraries that would not require local patching with every new release to work.
Comment 37 Tomás Cohen Arazi 2023-03-31 10:04:23 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 38 Matt Blenkinsop 2023-04-11 11:20:09 UTC
Nice work everyone!

Pushed to stable for 22.11.x