This is confusing - CatalogModule Relink is described as: Do/Don't automatically relink headings that have previously been linked when saving records in the cataloging module. The cronjob interface is not the cataloging module? LinkerRelink describes itself in the DB: ('LinkerRelink','1',NULL,'If ON the authority linker will relink headings that have previously been linked every time it runs.','YesNo') In the cronjob: my $allowrelink = C4::Context->preference("CatalogModuleRelink") || '';
I guess today's the day I bump a bunch of the authority linker bugs. This user expects "CatalogingModuleRelink = Do" to update links when I hit "Link Authorities automatically" in the cataloging module. And I expect "LinkerRelink = Do" to update links when the cron job runs.
Created attachment 141038 [details] [review] Bug 30024: Make link_bibs_to_authorities.pl rely on LinkerRelink This patch makes the trivial change of making the linker script rely on LinkerRelink instead of CatalogModuleRelink, which is confusing and incorrect (according to both sysprefs descriptions). To test: 1. Verify that relinking is now tied to the right syspref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This definitely seems right, but I want to highlight what I see in the routine LinkBibHeadingsToAuthorities. When an existing link is found we check the value of 'allowrelink' as passed in AND the value of LinkerRelink: if ( defined $current_link && (!$allowrelink || !C4::Context->preference('LinkerRelink')) ) So really, it is currently controlled by both, and only enabling both causes relink beforehand Before | After LinkerRelink 0 + CatalogModuleRelink 0 = No relink | No relink LinkerRelink 1 + CatalogModuleRelink 0 = No relink | Relink LinkerRelink 0 + CatalogModuleRelink 1 = No relink | No relink LinkerRelink 1 + CatalogModuleRelink 1 = Relink | Relink It is worth noting - if we have a current link, but don't find an authority record (i.e. linked to missing record) we also check 'allowrelink' in addition to 'LinkerKeepStale' Relink + LinkerKeepStale 0 = Remove link (unless authrecord found in DB but not zebra) Relink + LinkerKeepStale 1 = Keep stale NoRelink + LinkerKeepStale 0 = Keep stale NoRelink + LinkerKeepStale 1 = Keep stale
(In reply to Nick Clemens from comment #3) > So really, it is currently controlled by both, and only enabling both causes > relink beforehand Does that mean this patch leads to a change in behaviour that might come as a surprise to some, or is it the intended effect of the patch?
(In reply to Magnus Enger from comment #4) > (In reply to Nick Clemens from comment #3) > > So really, it is currently controlled by both, and only enabling both causes > > relink beforehand > > Does that mean this patch leads to a change in behaviour that might come as > a surprise to some, or is it the intended effect of the patch? The change in behaviour is intended. Currently things don't work the way they say they would - so I think it more likely for a user to be experiencing unexpected results now. After it will follow what the descriptions say will happen. It should be noted in the release notes, but is correcting a bug.
Testing notes (with ktd): You can't just run link_bibs_to_authorities.pl, since bib 369 is unparseable and crashes it. Instead you have to find a single bib to run against with, e.g., --bib-limit=biblionumber=16 You can't relink anything but 6xx headings without bug 33277 Test plan: 1. Set the preferences AutoLinkBiblios and CatalogModuleRelink to "Don't" so you can cause something to be mislinked, set LinkerRelink to "Do" (those are all default values) and set EnableAdvancedCatalogingEditor to "Enable." 2. Load http://127.0.0.1:8081/cgi-bin/koha/cataloguing/editor.pl#catalog/16 and change subfield 9 for one of the subjects to a recognizably wrong value like 999999 and save, ensuring that it didn't get automatically relinked 3. misc/link_bibs_to_authorities.pl --bib-limit=biblionumber=16 should list "Number of bibs modified: 0" because CatalogModuleRelink is still "Don't" 4. Switch CatalogModuleRelink to "Do", misc/link_bibs_to_authorities.pl --bib-limit=biblionumber=16 should list "Number of bibs modified: 1" and reloading http://127.0.0.1:8081/cgi-bin/koha/cataloguing/editor.pl#catalog/16 should show the 999999 replaced with the correct authid 5. Apply patch, restart 6. Redo Step 1 and 2. 7. misc/link_bibs_to_authorities.pl --bib-limit=biblionumber=16 should list "Number of bibs modified: 1" and reloading http://127.0.0.1:8081/cgi-bin/koha/cataloguing/editor.pl#catalog/16 should show the 999999 replaced with the correct authid when you have CatalogModuleRelink at "Don't" and LinkerRelink at "Do".
Created attachment 150780 [details] [review] Bug 30024: Make link_bibs_to_authorities.pl rely on LinkerRelink This patch makes the trivial change of making the linker script rely on LinkerRelink instead of CatalogModuleRelink, which is confusing and incorrect (according to both sysprefs descriptions). To test: 1. Verify that relinking is now tied to the right syspref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
What about: C4/Biblio.pm: LinkBibHeadingsToAuthorities( $linker, $record, $frameworkcode, C4::Context->preference("CatalogModuleRelink") || '', undef, $verbose );
Please provide feedback
(In reply to Marcel de Rooy from comment #9) > Please provide feedback Not that anyone wanted my often-confused feedback, but at least I'll give it :) That call is in BiblioAutoLink, which is only called by svc/cataloguing/automatic_linker.pl, which is called by the "Link authorities automatically" button in the basic bib editor. That seems pretty solidly CatalogModuleRelink. Or did you mean that BiblioAutoLink might look attractive to someone who isn't CatalogModule, and it should instead make callers give it a relink value so it wouldn't be wrong if a non-CatalogModule use appeared?
(In reply to Phil Ringnalda from comment #10) > (In reply to Marcel de Rooy from comment #9) > > Please provide feedback > > Not that anyone wanted my often-confused feedback, but at least I'll give it > :) > > That call is in BiblioAutoLink, which is only called by > svc/cataloguing/automatic_linker.pl, which is called by the "Link > authorities automatically" button in the basic bib editor. That seems pretty > solidly CatalogModuleRelink. > > Or did you mean that BiblioAutoLink might look attractive to someone who > isn't CatalogModule, and it should instead make callers give it a relink > value so it wouldn't be wrong if a non-CatalogModule use appeared? This is all correct - that call is out of the scope of this bug and should be a new issue
Created attachment 158354 [details] [review] Bug 30024: Make link_bibs_to_authorities.pl rely on LinkerRelink This patch makes the trivial change of making the linker script rely on LinkerRelink instead of CatalogModuleRelink, which is confusing and incorrect (according to both sysprefs descriptions). To test: 1. Verify that relinking is now tied to the right syspref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.06
Nice work everyone! Pushed to 22.11.x for next release