|
Description
Nick Clemens (kidclamp)
2023-03-07 16:27:34 UTC
Created attachment 147881 [details] [review] Bug 33159: Unit tests 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 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. 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
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 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> 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. Created attachment 147986 [details] [review] Bug 33159: Unit tests Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> 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> 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> (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 (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 (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 Created attachment 148032 [details] [review] Bug 33159: Unit tests Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> 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> 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> 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> 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> 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> 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> 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> 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> 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 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. 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. 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. *** Bug 31924 has been marked as a duplicate of this bug. *** (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... 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. 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> 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> 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> (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. 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! (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 (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. Pushed to master for 23.05. Nice work everyone, thanks! Nice work everyone! Pushed to stable for 22.11.x |