Bug 33277

Summary: Correctly handle linking subfields with no defined thesaurus
Product: Koha Reporter: Nick Clemens <nick>
Component: MARC Authority data supportAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: blocker    
Priority: P5 - low CC: caroline.cyr-la-rose, frank.hansen, januszop, jonathan.druart, lucas, m.de.rooy, phil, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33718
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06
Bug Depends on: 30280, 33159    
Bug Blocks: 33295, 33296, 33557    
Attachments: Bug 33277: Do not search using thesaurus when linking non 6XX fields
Authority records for testing
Bib record for testing
Bug 33277: Do not search using thesaurus when linking non 6XX fields
Bug 33277: Support custom thesaurus authority with no 040 specified
Bug 33277: Add comments and missing thesauri
Bug 33277: Don't search thesaurus when heading 'notspecified'
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
Bug 33277: Do not search using thesaurus when linking non 6XX fields
Bug 33277: Support custom thesaurus authority with no 040 specified
Bug 33277: Add comments and missing thesauri
Bug 33277: Search thesaurus for 'notspecified'
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
Updated authority records for testing
Bug 33277: Adjust source file for zebra mappings
Updated authority records for testing
Bug 33277: Do not search using thesaurus when linking non 6XX fields
Bug 33277: Support custom thesaurus authority with no 040 specified
Bug 33277: Add comments and missing thesauri
Bug 33277: Search thesaurus for 'notspecified'
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
Bug 33277: Adjust source file for zebra mappings
Updated authority records for testing
Bug 33277: (follow-up) Don't use regex, tidy
Bug 33277: Do not search using thesaurus when linking non 6XX fields
Bug 33277: Support custom thesaurus authority with no 040 specified
Bug 33277: Add comments and missing thesauri
Bug 33277: Search thesaurus for 'notspecified'
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
Bug 33277: Adjust source file for zebra mappings
Bug 33277: (follow-up) Don't use regex, tidy

Description Nick Clemens 2023-03-20 14:23:57 UTC
This is a follow-up to bug 33159, which is a followup to bug 30280

Currently we are handling all subfield other than 6XX as if they are lcsh - this doesn't work in all scenarios. When a record has 008 character 11 =n - "Not applicable" the match won't go through
Comment 1 Nick Clemens 2023-03-20 15:52:18 UTC
Created attachment 148417 [details] [review]
Bug 33277: Do not search using thesaurus when linking non 6XX fields

This patch sets thesaurus as undefined when linking any field except 6XX

This fixes the case where authrotiy records don't have the thesaurus defined

Consequently - this means that Koha does not support multiple thesaurus records
for authorities outside of subjects

i.e. Using the default linker, and having both an LCSH and Sears record for 'Shakespeare,William'
A 100 entry will find two results and the heading won't be linked. Previously we always linked to the LCSH

To test:
1 - Import the attached auths and biblio (from bug 33159 comment 24)
2 - Set system preferences:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - don't generate
    CatalogModuleRelink - Do
    LinkerKeepStale - Don't
    LinkerModule - default
    LinkerRelink - do
3 - Edit the imported bib
4 - Save it
5 - Headings are not linked except 600
6 - Apply patch
7 - Restart all
8 - Edit and save record
9 - Headings are successfully linked
Comment 2 Nick Clemens 2023-03-20 15:52:49 UTC
Created attachment 148418 [details]
Authority records for testing
Comment 3 Nick Clemens 2023-03-20 15:53:24 UTC
Created attachment 148419 [details]
Bib record for testing
Comment 4 Phil Ringnalda 2023-03-20 21:24:14 UTC
Not sure how many other cases it would enable, but the bug 33159 comment 28 thing of searching for Heading-use-main-or-added-entry = 'a' while linking 1xx/7xx would work for LCSH plus FAST, since the FAST name authority records correctly have 008/14 set to b.
Comment 5 Nick Clemens 2023-03-21 19:36:05 UTC
Created attachment 148492 [details] [review]
Bug 33277: Do not search using thesaurus when linking non 6XX fields

This patch sets thesaurus as undefined when linking any field except 6XX

This fixes the case where authrotiy records don't have the thesaurus defined

Consequently - this means that Koha does not support multiple thesaurus records
for authorities outside of subjects

i.e. Using the default linker, and having both an LCSH and Sears record for 'Shakespeare,William'
A 100 entry will find two results and the heading won't be linked. Previously we always linked to the LCSH

To test:
1 - Import the attached auths and biblio (from bug 33159 comment 24)
2 - Set system preferences:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - don't generate
    CatalogModuleRelink - Do
    LinkerKeepStale - Don't
    LinkerModule - default
    LinkerRelink - do
3 - Edit the imported bib
4 - Save it
5 - Headings are not linked except 600
6 - Apply patch
7 - Restart all
8 - Edit and save record
9 - Headings are successfully linked
Comment 6 Nick Clemens 2023-03-21 19:36:07 UTC
Created attachment 148493 [details] [review]
Bug 33277: Support custom thesaurus authority with no 040 specified
Comment 7 Nick Clemens 2023-03-21 19:36:09 UTC
Created attachment 148494 [details] [review]
Bug 33277: Add comments and missing thesauri
Comment 8 Nick Clemens 2023-03-21 19:36:11 UTC
Created attachment 148495 [details] [review]
Bug 33277: Don't search thesaurus when heading 'notspecified'
Comment 9 Nick Clemens 2023-03-21 19:41:03 UTC
(In reply to Phil Ringnalda from comment #4)
> Not sure how many other cases it would enable, but the bug 33159 comment 28
> thing of searching for Heading-use-main-or-added-entry = 'a' while linking
> 1xx/7xx would work for LCSH plus FAST, since the FAST name authority records
> correctly have 008/14 set to b.

Please file as a new bug :-) dependent on this one
Comment 10 Janusz Kaczmarek 2023-03-21 20:53:08 UTC
(In reply to Nick Clemens from comment #8)
> Created attachment 148495 [details] [review] [review]
> Bug 33277: Don't search thesaurus when heading 'notspecified'

Nick, in this case (i.e. 2nd ind == 4), according to my knowledge and experience, if we want to be strict (and we want, I suppose) we should look for an authority record with 008/11 = '|', and non simply 'not n'

For 6XX we read (e.g. https://www.loc.gov/marc/bibliographic/bd650.html):
2nd ind == "4 - Source not specified

Subject added entry conforms to a controlled list that cannot be identified by second indicator values 0-3, 5-6 or by a code in subfield $2."

So it does not mean "I, the librarian, do not care, take anything you can find", It means rather "there is no appropriate thesaurus identification", so, I would interpret it as "not coded" == '|'.

It is not straightforward to do a search for '|', but it is possible, both in Zebra and ES.
Comment 11 Phil Ringnalda 2023-03-21 22:17:30 UTC
With the patches applied, the 655 _ 7 ‡aLiteratura angielska‡2DBN heading doesn't get linked.
Comment 12 Janusz Kaczmarek 2023-03-21 23:38:38 UTC
I can confirm -- it does not work with Zebra for the case of 008/11 = 'z' and no 040 $f.  The second attempt is made, but without modification of zebra auth index definition there will be 'notdefined' in the index instead of 'z' (authority-zebra-indexdefs.xsl):

        <xslo:when test="$thesaurus_code1 = 'z'">
          <xslo:choose>
            <xslo:when test="//marc:datafield[@tag='040']/marc:subfield[@code='f']">
              <xslo:value-of select="//marc:datafield[@tag='040']/marc:subfield[@code='f']"/>
            </xslo:when>
            <xslo:otherwise>
              <xslo:text>notdefined</xslo:text>
            </xslo:otherwise>
          </xslo:choose>
        </xslo:when>


With ES it seems OK.

The only doubt that I still have is putting nothing in the query for 6XX 2nd ind == '4'.  As the consequence, if we had more than one auth records with the same 1XX, we would get more than one from the query, which is bad.
Comment 13 Nick Clemens 2023-03-22 00:27:24 UTC
(In reply to Janusz Kaczmarek from comment #10)
> (In reply to Nick Clemens from comment #8)
> > Created attachment 148495 [details] [review] [review] [review]
> > Bug 33277: Don't search thesaurus when heading 'notspecified'
> 
> Nick, in this case (i.e. 2nd ind == 4), according to my knowledge and
> experience, if we want to be strict (and we want, I suppose) we should look
> for an authority record with 008/11 = '|', and non simply 'not n'

I misunderstood what you were saying on bug 33159 comment 33 - I thought you meant either pipe or non n

The description here:
https://www.loc.gov/marc/bibliographic/bd600.html

Seems like it could be either z or | when looking here:
https://www.loc.gov/marc/authority/ad008.html

So I am happy to go either way - I will submit a patch to use | tomorrow
Comment 14 Nick Clemens 2023-03-22 00:33:53 UTC
Created attachment 148500 [details] [review]
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
Comment 15 Nick Clemens 2023-03-22 00:38:48 UTC
(In reply to Janusz Kaczmarek from comment #12)
> I can confirm -- it does not work with Zebra for the case of 008/11 = 'z'
> and no 040 $f.  The second attempt is made, but without modification of
> zebra auth index definition there will be 'notdefined' in the index instead
> of 'z' (authority-zebra-indexdefs.xsl):

I believe this is fixed, adjusted the mapping in ES to translate the value so that Zebra works directly - since we already translated most values in ES QueryBuilder and pass the word rather than code in other instances it seems to be more consistent

> 
> With ES it seems OK.
> 
> The only doubt that I still have is putting nothing in the query for 6XX 2nd
> ind == '4'.  As the consequence, if we had more than one auth records with
> the same 1XX, we would get more than one from the query, which is bad.

This was true before Koha knew about thesauri at all too - and we still have to deal with the separate bug 31925. I will defer to you and Phil on whether | or nothing is more appropriate, but will attach a patch using pipe so we can decide
Comment 16 Phil Ringnalda 2023-03-22 01:52:13 UTC
I've never seen an authority record for a heading intended to be coded with ind2=4, so I don't have a practical opinion. If someone ordered me to create one, I would have used 008/11=z and an empty 040$f to say "this is an Other thesaurus, but I cannot speak its name." But if Janusz has experience with ones using 008/11=| I'm quite happy to defer to experience, and wait to see if someone using z objects later.

(Because I think horrible thoughts, a WhatSubjectThesaurusCodeIsUnknown preference came to mind to solve disagreements, but anyone who feels we need that really ought to either be forced to write it, or be forced to pay someone handsomely for writing it.)
Comment 17 Nick Clemens 2023-03-22 12:39:00 UTC
Created attachment 148527 [details] [review]
Bug 33277: Do not search using thesaurus when linking non 6XX fields

This patch sets thesaurus as undefined when linking any field except 6XX

This fixes the case where authrotiy records don't have the thesaurus defined

Consequently - this means that Koha does not support multiple thesaurus records
for authorities outside of subjects

i.e. Using the default linker, and having both an LCSH and Sears record for 'Shakespeare,William'
A 100 entry will find two results and the heading won't be linked. Previously we always linked to the LCSH

To test:
1 - Import the attached auths and biblio (from bug 33159 comment 24)
2 - Set system preferences:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - don't generate
    CatalogModuleRelink - Do
    LinkerKeepStale - Don't
    LinkerModule - default
    LinkerRelink - do
3 - Edit the imported bib
4 - Save it
5 - Headings are not linked except 600
6 - Apply patch
7 - Restart all
8 - Edit and save record
9 - Headings are successfully linked
Comment 18 Nick Clemens 2023-03-22 12:39:03 UTC
Created attachment 148528 [details] [review]
Bug 33277: Support custom thesaurus authority with no 040 specified
Comment 19 Nick Clemens 2023-03-22 12:39:05 UTC
Created attachment 148529 [details] [review]
Bug 33277: Add comments and missing thesauri
Comment 20 Nick Clemens 2023-03-22 12:39:07 UTC
Created attachment 148530 [details] [review]
Bug 33277: Search thesaurus  for 'notspecified'
Comment 21 Nick Clemens 2023-03-22 12:39:09 UTC
Created attachment 148531 [details] [review]
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
Comment 22 Frank Hansen 2023-03-22 13:40:56 UTC
(In reply to Phil Ringnalda from comment #16)
> I've never seen an authority record for a heading intended to be coded with
> ind2=4, so I don't have a practical opinion. If someone ordered me to create
> one, I would have used 008/11=z and an empty 040$f to say "this is an Other
> thesaurus, but I cannot speak its name." But if Janusz has experience with
> ones using 008/11=| I'm quite happy to defer to experience, and wait to see
> if someone using z objects later.
> 
> (Because I think horrible thoughts, a WhatSubjectThesaurusCodeIsUnknown
> preference came to mind to solve disagreements, but anyone who feels we need
> that really ought to either be forced to write it, or be forced to pay
> someone handsomely for writing it.)

I understand that different countries may have different interpretations of Library of Congress MARC specifications. And the Library of Congress allows free interpretation. I'm pretty sure that in Sweden we have our interpretation, which is also available here: https://katalogverk.kb.se/katalogisering/Formathandboken/index.html (only in Swedish). And Sweden are not to decide the format of course. And different countries may also have different needs.
 
I would still like to comment on ind2=4. As for us, we treat them as local subject headings only and they are not included in any thesaurus. Therefore, I would prefer not to see any authority records for either 008/11=| (or 008/11=n) in our catalog, even if the format allows it. Here in Sweden, a kind of transition is made from field 653 for uncontrolled subject headings to using 650 with ind2=4 instead. That's another reason why I don't want to see any authority records with 008/11=| in our catalog. We are don't autocreate any authority records in our catalog. Almost all is imported from Netional Library (Libris).

I also want to comment on 040$f. According to the Swedish cataloguing guidelines here https://katalogverk.kb.se/katalogisering/Formathandboken/kallorsystem/index.html (only in Swedish) 040$f must be filled in if 008/11=z. We in Sweden use many different thesauruses. In Library of Congress list here, https://www.loc.gov/standards/sourcelist/subject.html both the Swedish and Polish are listed. As I understand, 040 is a required field in Koha. However, 040$f is optional. Even though Library of Congress allows for the omission of 040$f, I can't see why it should be omitted, except for convenience. So I question about allowing to omit 040$f when 008/11=z.

What causes the most trouble for me is, as mentioned earlier, that non-6XX fields are hardcoded to LCSH. This creates problems for libraries outside the US, as we typically do not use LCSH names. I wish we could skip thesaurus search for those fields.
Comment 23 Nick Clemens 2023-03-22 14:02:13 UTC
(In reply to Frank Hansen from comment #22)

> I would still like to comment on ind2=4. As for us, we treat them as local
> subject headings only and they are not included in any thesaurus. Therefore,
> I would prefer not to see any authority records for either 008/11=| (or
> 008/11=n) in our catalog, even if the format allows it.

Are you saying you won't import those records - or you want those records to be ignored when linking? The former sounds easy enough, the second seems it would require a syspref to not link those

> I also want to comment on 040$f. According to the Swedish cataloguing
> guidelines here
...
> So I question about allowing to omit 040$f when 008/11=z.

This sounds like a comment on cataloging standards, and maybe something that can be set in frameworks. But I don't think we need code for this?

> What causes the most trouble for me is, as mentioned earlier, that non-6XX
> fields are hardcoded to LCSH. This creates problems for libraries outside
> the US, as we typically do not use LCSH names. I wish we could skip
> thesaurus search for those fields.

These patches do now skip searching thesaurus when linking non-subject fields
Comment 24 Frank Hansen 2023-03-22 15:57:39 UTC
(In reply to Nick Clemens from comment #23)
> (In reply to Frank Hansen from comment #22)
> 
> > I would still like to comment on ind2=4. As for us, we treat them as local
> > subject headings only and they are not included in any thesaurus. Therefore,
> > I would prefer not to see any authority records for either 008/11=| (or
> > 008/11=n) in our catalog, even if the format allows it.
> 
> Are you saying you won't import those records - or you want those records to
> be ignored when linking? The former sounds easy enough, the second seems it
> would require a syspref to not link those
> 

Sorry if I was unclear. Records with ind2=4 is not exported from National Library catalogue at first. So they will not be imported to our local catalogue. We are not planning to create any either manually or automatically.

> > I also want to comment on 040$f. According to the Swedish cataloguing
> > guidelines here
> ...
> > So I question about allowing to omit 040$f when 008/11=z.
> 
> This sounds like a comment on cataloging standards, and maybe something that
> can be set in frameworks. But I don't think we need code for this?
> 
Maybe not. We will use 040$f to specify system/thesaurus conventions in any case. I just want to make sure that it not have a negative impact on filtering on thesauruses if we omit 040$f.

> > What causes the most trouble for me is, as mentioned earlier, that non-6XX
> > fields are hardcoded to LCSH. This creates problems for libraries outside
> > the US, as we typically do not use LCSH names. I wish we could skip
> > thesaurus search for those fields.
> 
> These patches do now skip searching thesaurus when linking non-subject fields

I'm testing bug33277 and what I have seen so far looks good. (both ES and Zebra)
Comment 25 Janusz Kaczmarek 2023-03-23 00:16:58 UTC
Nick, I have looked into the corrected version. I see one problem: with Zebra, for:

655 7 $a Literatura angielska $2 DBN

you search first for: "Literatura angielska + DBN" and, since there is no 040 $f, there is the second query for: "Literatura angielska + undefined'

But having two auth records:

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 angielska

you will get both as response, while only one of them is appropriate (the first one) [note 008/11].

Why not to index, in Zebra, directly 008/11 and 040 $f into the same index, without the 'index_subject_thesaurus' help-template which inserts 'undefined' for lacking 040 $f?

Note that in ES in this case you are adding subject-heading-thesaurus == 'z' to the query, not '|'.



Also, it is not clear for me why do you hardcoded subject-heading-thesaurus in Koha/SearchEngine/Elasticsearch.pm, instead of putting it in mapping.yaml as:

  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: ''

which seems to me more general and therefor natural...
Comment 26 Nick Clemens 2023-03-23 11:13:09 UTC
(In reply to Janusz Kaczmarek from comment #25)
> Nick, I have looked into the corrected version. I see one problem: with
> Zebra, for:
> 
> 655 7 $a Literatura angielska $2 DBN
> 
> you search first for: "Literatura angielska + DBN" and, since there is no
> 040 $f, there is the second query for: "Literatura angielska + undefined'

The second query passes "notdefined" - not undef 

> But having two auth records:
> 
> 00116nz  a2200049n  4500
> 008 230210 n anznnbabn           a ana     d
> 155    $a Literatura angielska

This is stored as thesaurus "notdefined"
> 
> 00116nz  a2200049n  4500
> 008 230210 n an|nnbabn           a ana     d
> 155    $a Literatura angielska

This is stored as thesaurus "notspecified"

> 
> you will get both as response, while only one of them is appropriate (the
> first one) [note 008/11].

Here is what the searches look like in Zebra

11:09:06-23/03 zebrasrv(1) [request] Search authorities OK 0 1 1+0 RPN @attrset Bib-1 @or @attr 1=Local-Number @attr 4=109 @attr 7=1 0 @and @and @attr 5=100 @attr 1=authtype GENRE/FORM @attr 6=3 @attr 5=100 @attr 4=1 @attr 1=Match-heading "Literatura angielska" @attr 5=100 @attr 4=1 @attr 1=Subject-heading-thesaurus DBN
11:09:06-23/03 zebrasrv(1) [request] Search authorities OK 1 1 1+0 RPN @attrset Bib-1 @or @attr 1=Local-Number @attr 4=109 @attr 7=1 0 @and @and @attr 5=100 @attr 1=authtype GENRE/FORM @attr 6=3 @attr 5=100 @attr 4=1 @attr 1=Match-heading "Literatura angielska" @attr 5=100 @attr 4=1 @attr 1=Subject-heading-thesaurus notdefined

It should only return the second

> 
> Why not to index, in Zebra, directly 008/11 and 040 $f into the same index,
> without the 'index_subject_thesaurus' help-template which inserts
> 'undefined' for lacking 040 $f?


 
> Note that in ES in this case you are adding subject-heading-thesaurus == 'z'
> to the query, not '|'.
> 

In ES we are storing both the code and the 040f, so one with 040f will match on that - then when searching without the z will match

> 
> Also, it is not clear for me why do you hardcoded subject-heading-thesaurus
> in Koha/SearchEngine/Elasticsearch.pm, instead of putting it in mapping.yaml
> as:

The thesaurus search is used only internally for linking - this should not be customised or updated by the user. We do the same for 'Match-heading' to ensure consistent linking
Comment 27 Phil Ringnalda 2023-03-23 17:32:57 UTC
I just followed the test plan in a sandbox, first with Zebra and then with ElasticSearch, and in neither case was the Literatura angielska genre heading linked.
Comment 28 Nick Clemens 2023-03-23 18:00:52 UTC
(In reply to Phil Ringnalda from comment #27)
> I just followed the test plan in a sandbox, first with Zebra and then with
> ElasticSearch, and in neither case was the Literatura angielska genre
> heading linked.

Did you reindex after applying the patches? While I tried to avoid it, it is required
Comment 29 Phil Ringnalda 2023-03-23 18:25:21 UTC
I didn't for the Zebra case, since I applied the patches on sandbox creation so I didn't think about needing to. Just switched it back to Zebra, did a reindex, and it still didn't link. For ElasticSearch I did reindex before trying anything, since nothing works post-switch until you do.
Comment 30 Phil Ringnalda 2023-03-23 18:54:30 UTC
On https://sandboxes.biblibre.eu/ if you want to poke at it, see where I might have gone wrong.
Comment 31 Janusz Kaczmarek 2023-03-23 23:30:22 UTC
(In reply to Nick Clemens from comment #26)
> (In reply to Janusz Kaczmarek from comment #25)
> > Nick, I have looked into the corrected version. I see one problem: with
> > Zebra, for:
> > 
> > 655 7 $a Literatura angielska $2 DBN
> > 
> > you search first for: "Literatura angielska + DBN" and, since there is no
> > 040 $f, there is the second query for: "Literatura angielska + undefined'
> 
> The second query passes "notdefined" - not undef 
[...]

Nick, thank you very much for you comment.  My eyes misses the difference between 'notdefined' for 'z' without 040 $f and 'notspecified' for '|'.  I am sorry for unnecessarily bothering you.


I understand your explanation about hardcoding Subject-heading-thesaurus but, to be sincere, I do not share this argument.  Firstly, in Zebra also we have the definitions in the configuration file--if somebody changes there something he/she shouldn't, its his/her fault.  And secondly, which is IMHO more important, shall we than, in the next step, hardcode also Heading-use-main-or-added-entry for 008/14, Heading-use-series-added-entry for 008/16, Heading-use-subject-added-entry for 008/15 when we will be wanting to narrow the search for linking even more specifically (which should definitely be done at some point)?  So, I am still not convinced if it is the right idea...
Comment 32 Nick Clemens 2023-03-27 08:37:50 UTC
Created attachment 148734 [details]
Updated authority records for testing

Replaces an 'n' in the 008 for '|'
Comment 33 Nick Clemens 2023-03-27 08:39:25 UTC
(In reply to Phil Ringnalda from comment #27)
> I just followed the test plan in a sandbox, first with Zebra and then with
> ElasticSearch, and in neither case was the Literatura angielska genre
> heading linked.

I had the wrong marker in the authority record, I believe I fixed it. The 008 thesaurus was set as 'n' - not applicable - rather then '|' - no attempt to code. All headings should link with the new file, or you can make that edit yourself
Comment 34 Nick Clemens 2023-03-27 08:44:35 UTC
(In reply to Janusz Kaczmarek from comment #31)
> (In reply to Nick Clemens from comment #26)
> I understand your explanation about hardcoding Subject-heading-thesaurus
> but, to be sincere, I do not share this argument.  Firstly, in Zebra also we
> have the definitions in the configuration file--if somebody changes there
> something he/she shouldn't, its his/her fault.  And secondly, which is IMHO
> more important, shall we than, in the next step, hardcode also
> Heading-use-main-or-added-entry for 008/14, Heading-use-series-added-entry
> for 008/16, Heading-use-subject-added-entry for 008/15 when we will be
> wanting to narrow the search for linking even more specifically (which
> should definitely be done at some point)?  So, I am still not convinced if
> it is the right idea...

I would suggest we hardcode the new fields if needed for linking. Linking is an internal process that shouldn't be adjusted by the user. Rather than allow them to break the system, let us save them :-)

Zebra can technically be adjusted by the end user, however, ES allows direct updating via the interface. The intention of allowing the user access, IMHO, is to allow them to adjust searching for their needs. Adjusting internal processes isn't something we need to provide access to. The same MARC fields can be indexed into another field if the user needs them for searching. Authority searching doesn't use or provide these fields either, so allowing to be user adjusted only allows linking to break.

Happy to continue that discussion, but I think we can address it wholesale on another bug.
Comment 35 Nick Clemens 2023-03-27 08:55:07 UTC
Created attachment 148735 [details] [review]
Bug 33277: Adjust source file for zebra mappings

The previous patches adjusted the mappings directly - moving this
change to the correct build file

Not needed for sign off, but QA can test that nothing changes when rebuilding the files:
xsltproc etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl etc/zebradb/marc_defs/marc21/authorities/authority-koha-indexdefs.xml > etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl
Comment 36 Phil Ringnalda 2023-03-29 22:17:08 UTC
Finally got my hands on a working sandbox again, and with the updated test authorities and the current patches, while the Literatura angielska‡2DBN genre now links, the ind2=4 Literatura światowa one now does not link (with either '|' or going back to 'n' in 008/11).
Comment 37 Nick Clemens 2023-03-30 08:42:05 UTC
Created attachment 148957 [details]
Updated authority records for testing

Hamlet auth was marked as LOC, changed to 'z'
Comment 38 Nick Clemens 2023-03-30 08:43:17 UTC
(In reply to Phil Ringnalda from comment #36)
> Finally got my hands on a working sandbox again, and with the updated test
> authorities and the current patches, while the Literatura angielska‡2DBN
> genre now links, the ind2=4 Literatura światowa one now does not link (with
> either '|' or going back to 'n' in 008/11).

This one actually cannot be tested on a sandbox - it requires copying the updated zebra files for indexing
Comment 39 Katrin Fischer 2023-04-15 23:59:24 UTC
Can someone help to get this tested?
Comment 40 Frank Hansen 2023-04-18 08:45:41 UTC
(In reply to Katrin Fischer from comment #39)
> Can someone help to get this tested?

I have now tested the patch on my testing docker. I get different results depending on whether I choose the ES or the Zebra engine. I am following the test plan in comment #17

ES works flawlessly. All headings are linked after the patch is applied. 

But when doing the same using Zebra, without the patch installed only field 100 is linked. After applying the patch and restarted not all headings are linked. Only 
 field 100, 655 (Literature English), 700 and 830 fields are linked after saving the record again.

Have I missed something.
Comment 41 Nick Clemens 2023-04-18 11:21:00 UTC
(In reply to Frank Hansen from comment #40)
> (In reply to Katrin Fischer from comment #39)
> > Can someone help to get this tested?
> 
> I have now tested the patch on my testing docker. I get different results
> depending on whether I choose the ES or the Zebra engine. I am following the
> test plan in comment #17
> 
> ES works flawlessly. All headings are linked after the patch is applied. 
> 
> But when doing the same using Zebra, without the patch installed only field
> 100 is linked. After applying the patch and restarted not all headings are
> linked. Only 
>  field 100, 655 (Literature English), 700 and 830 fields are linked after
> saving the record again.
> 
> Have I missed something.

The test plan is missing the steps for Zebra as the index files need to be copied and then a full reindex:
sudo cp /kohadevbox/koha/etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl /etc/koha/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl
restart_all
sudo koha-rebuild-zebra -v -f kohadev
Comment 42 Frank Hansen 2023-04-18 14:12:57 UTC
(In reply to Nick Clemens from comment #41)
> (In reply to Frank Hansen from comment #40)
> > (In reply to Katrin Fischer from comment #39)
> > > Can someone help to get this tested?
> > 
> > I have now tested the patch on my testing docker. I get different results
> > depending on whether I choose the ES or the Zebra engine. I am following the
> > test plan in comment #17
> > 
> > ES works flawlessly. All headings are linked after the patch is applied. 
> > 
> > But when doing the same using Zebra, without the patch installed only field
> > 100 is linked. After applying the patch and restarted not all headings are
> > linked. Only 
> >  field 100, 655 (Literature English), 700 and 830 fields are linked after
> > saving the record again.
> > 
> > Have I missed something.
> 
> The test plan is missing the steps for Zebra as the index files need to be
> copied and then a full reindex:
> sudo cp
> /kohadevbox/koha/etc/zebradb/marc_defs/marc21/authorities/authority-zebra-
> indexdefs.xsl
> /etc/koha/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl
> restart_all
> sudo koha-rebuild-zebra -v -f kohadev

Yes! It works for me. I'm signing off.
Comment 43 Katrin Fischer 2023-04-18 19:01:35 UTC
Thank you Frank. Note for QA: Please add sign-off lines!
Comment 44 Frank Hansen 2023-04-19 08:10:35 UTC
Created attachment 149837 [details] [review]
Bug 33277: Do not search using thesaurus when linking non 6XX fields

This patch sets thesaurus as undefined when linking any field except 6XX

This fixes the case where authrotiy records don't have the thesaurus defined

Consequently - this means that Koha does not support multiple thesaurus records
for authorities outside of subjects

i.e. Using the default linker, and having both an LCSH and Sears record for 'Shakespeare,William'
A 100 entry will find two results and the heading won't be linked. Previously we always linked to the LCSH

To test:
1 - Import the attached auths and biblio (from bug 33159 comment 24)
2 - Set system preferences:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - don't generate
    CatalogModuleRelink - Do
    LinkerKeepStale - Don't
    LinkerModule - default
    LinkerRelink - do
3 - Edit the imported bib
4 - Save it
5 - Headings are not linked except 600
6 - Apply patch
7 - Restart all
8 - Edit and save record
9 - Headings are successfully linked

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


Current status: Signed Off
Comment 45 Frank Hansen 2023-04-19 08:10:38 UTC
Created attachment 149838 [details] [review]
Bug 33277: Support custom thesaurus authority with no 040 specified

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 46 Frank Hansen 2023-04-19 08:10:41 UTC
Created attachment 149839 [details] [review]
Bug 33277: Add comments and missing thesauri

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 47 Frank Hansen 2023-04-19 08:10:43 UTC
Created attachment 149840 [details] [review]
Bug 33277: Search thesaurus for 'notspecified'

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 48 Frank Hansen 2023-04-19 08:10:46 UTC
Created attachment 149841 [details] [review]
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 49 Frank Hansen 2023-04-19 08:10:49 UTC
Created attachment 149842 [details] [review]
Bug 33277: Adjust source file for zebra mappings

The previous patches adjusted the mappings directly - moving this
change to the correct build file

Not needed for sign off, but QA can test that nothing changes when rebuilding the files:
xsltproc etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl etc/zebradb/marc_defs/marc21/authorities/authority-koha-indexdefs.xml > etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 50 Caroline Cyr La Rose 2023-05-02 16:22:17 UTC
I think this doesn't apply anymore.

Bug 33277 - Correctly handle linking subfields with no defined thesaurus

148530 - Bug 33277: Search thesaurus  for 'notspecified'
149837 - Bug 33277: Do not search using thesaurus when linking non 6XX fields
149838 - Bug 33277: Support custom thesaurus authority with no 040 specified
149839 - Bug 33277: Add comments and missing thesauri
149840 - Bug 33277: Search thesaurus for 'notspecified'
149841 - Bug 33277: (follow-up) Fix notdefined search for Zebra and ES
149842 - Bug 33277: Adjust source file for zebra mappings

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 33277: Search thesaurus for 'notspecified'
error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 33277: Search thesaurus for 'notspecified'
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-33277-Search-thesaurus-for-notspecified-1Gg31H.patch
Comment 51 Nick Clemens 2023-05-02 16:52:54 UTC
(In reply to Caroline Cyr La Rose from comment #50)
> I think this doesn't apply anymore.
There was an old patch the needed obseleted, should apply now
Comment 52 Jonathan Druart 2023-05-04 07:19:39 UTC
Janusz, do you think you could add your stamp to this (bug 33277) and bug 33557?

If you do we can consider they Passed QA I think.
Comment 53 Nick Clemens 2023-05-04 10:57:23 UTC
*** Bug 32916 has been marked as a duplicate of this bug. ***
Comment 54 Marcel de Rooy 2023-05-09 11:33:05 UTC
Looking here now
Comment 55 Marcel de Rooy 2023-05-09 12:57:11 UTC
Tested this with Zebra. Copied
cp etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl /etc/koha/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl 

Rebuilt index, restarted Zebra.
Conform test plan, opened record and saved it again. Checked sysprefs. The links are not relinked to the correct authids.
Comment 56 Marcel de Rooy 2023-05-09 12:58:20 UTC
> ! grep /$thesaurus/,('lcsh','lcac','mesh','nal','notspecified','cash','rvm','sears','aat')
This regex might not always do what you want.
Probably we could better do something like: none { $_ eq $thesaurus } etc

etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl
                            <xslo:when>
                                <xsl:attribute name="test"><xsl:value-of select="$alt_select"/></xsl:attribute>
                                <xslo:value-of>
                                    <xsl:attribute name="select"><xsl:value-of select="$alt_select"/></xsl:attribute>
                                </xslo:value-of>
                            </xslo:when>
                            <xslo:otherwise><xslo:text>notdefined</xslo:text></xslo:otherwise>
This could be done much simpler. Funny construction here. Out of scope.
Comment 57 Nick Clemens 2023-05-09 15:41:57 UTC
Created attachment 150890 [details]
Updated authority records for testing
Comment 58 Nick Clemens 2023-05-09 15:42:25 UTC
Created attachment 150891 [details] [review]
Bug 33277: (follow-up) Don't use regex, tidy
Comment 59 Nick Clemens 2023-05-09 15:44:14 UTC
(In reply to Marcel de Rooy from comment #55)
> Tested this with Zebra. Copied
> cp etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl
> /etc/koha/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl 
> 
> Rebuilt index, restarted Zebra.
> Conform test plan, opened record and saved it again. Checked sysprefs. The
> links are not relinked to the correct authids.

None of them? Or some of them?

Restart zebra (restart_all) then reindex

Make sure  AutoLinkBiblios is set to 'Do'(In reply to Marcel de Rooy from comment #56)
> > ! grep /$thesaurus/,('lcsh','lcac','mesh','nal','notspecified','cash','rvm','sears','aat')
> This regex might not always do what you want.
> Probably we could better do something like: none { $_ eq $thesaurus } etc

Updated
Comment 60 Marcel de Rooy 2023-05-10 11:22:16 UTC
(In reply to Nick Clemens from comment #59)
> (In reply to Marcel de Rooy from comment #55)
> None of them? Or some of them?
> 
None.

> Restart zebra (restart_all) then reindex
Will try that again.

> Make sure  AutoLinkBiblios is set to 'Do'
Yes, I verified all prefs with the test plan.
Comment 61 Marcel de Rooy 2023-05-10 12:05:08 UTC
Update: Most auth fields are linked now! The 600 Hamlet is not linked since it looks for:
  'value' => [
    "Hamlet posta\x{107} fikcyjna",
    'notdefined'
  ],

And the $c does not occur in the authority data. So thats explained too.
Comment 62 Marcel de Rooy 2023-05-10 12:08:12 UTC
Note: there was also a space in the bib side:
<subfield code="a">Hamlet </subfield>
Comment 63 Marcel de Rooy 2023-05-10 12:09:55 UTC
Created attachment 150977 [details] [review]
Bug 33277: Do not search using thesaurus when linking non 6XX fields

This patch sets thesaurus as undefined when linking any field except 6XX

This fixes the case where authrotiy records don't have the thesaurus defined

Consequently - this means that Koha does not support multiple thesaurus records
for authorities outside of subjects

i.e. Using the default linker, and having both an LCSH and Sears record for 'Shakespeare,William'
A 100 entry will find two results and the heading won't be linked. Previously we always linked to the LCSH

To test:
1 - Import the attached auths and biblio (from bug 33159 comment 24)
2 - Set system preferences:
    RequireChoosingExistingAuthority - don't require
    AutoCreateAuthorities - don't generate
    CatalogModuleRelink - Do
    LinkerKeepStale - Don't
    LinkerModule - default
    LinkerRelink - do
3 - Edit the imported bib
4 - Save it
5 - Headings are not linked except 600
6 - Apply patch
7 - Restart all
8 - Edit and save record
9 - Headings are successfully linked

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2023-05-10 12:09:58 UTC
Created attachment 150978 [details] [review]
Bug 33277: Support custom thesaurus authority with no 040 specified

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2023-05-10 12:10:01 UTC
Created attachment 150979 [details] [review]
Bug 33277: Add comments and missing thesauri

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2023-05-10 12:10:05 UTC
Created attachment 150980 [details] [review]
Bug 33277: Search thesaurus for 'notspecified'

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2023-05-10 12:10:08 UTC
Created attachment 150981 [details] [review]
Bug 33277: (follow-up) Fix notdefined search for Zebra and ES

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2023-05-10 12:10:11 UTC
Created attachment 150982 [details] [review]
Bug 33277: Adjust source file for zebra mappings

The previous patches adjusted the mappings directly - moving this
change to the correct build file

Not needed for sign off, but QA can test that nothing changes when rebuilding the files:
xsltproc etc/zebradb/xsl/koha-indexdefs-to-zebra.xsl etc/zebradb/marc_defs/marc21/authorities/authority-koha-indexdefs.xml > etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2023-05-10 12:10:14 UTC
Created attachment 150983 [details] [review]
Bug 33277: (follow-up) Don't use regex, tidy

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Tomás Cohen Arazi 2023-05-10 18:28:44 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 71 Marcel de Rooy 2023-05-11 08:45:52 UTC
Opened bug 33718 for issues with tests when you have the prefs set like in the test plan here.
Comment 72 Matt Blenkinsop 2023-05-15 10:42:58 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 73 Lucas Gass 2023-06-15 16:47:23 UTC
Missing 22.05.x dependencies, no backport.