Bug 30024 - link_bibs_to_authorities.pl relies on CatalogModuleRelink
Summary: link_bibs_to_authorities.pl relies on CatalogModuleRelink
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-04 14:48 UTC by Nick Clemens (kidclamp)
Modified: 2023-11-14 14:49 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.06,22.11.12


Attachments
Bug 30024: Make link_bibs_to_authorities.pl rely on LinkerRelink (1.89 KB, patch)
2022-09-27 19:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30024: Make link_bibs_to_authorities.pl rely on LinkerRelink (1.95 KB, patch)
2023-05-05 17:19 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 30024: Make link_bibs_to_authorities.pl rely on LinkerRelink (2.00 KB, patch)
2023-11-03 16:52 UTC, Kyle M Hall
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) 2022-02-04 14:48:14 UTC
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") || '';
Comment 1 Sarah Cornell 2022-07-07 15:25:16 UTC
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.
Comment 2 Tomás Cohen Arazi 2022-09-27 19:18:47 UTC
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>
Comment 3 Nick Clemens (kidclamp) 2022-09-28 11:07:33 UTC
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
Comment 4 Magnus Enger 2023-03-27 10:43:58 UTC
(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?
Comment 5 Nick Clemens (kidclamp) 2023-03-27 11:14:44 UTC
(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.
Comment 6 Phil Ringnalda 2023-05-05 17:19:03 UTC
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".
Comment 7 Phil Ringnalda 2023-05-05 17:19:48 UTC
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>
Comment 8 Marcel de Rooy 2023-05-12 09:56:55 UTC
What about:

C4/Biblio.pm:      LinkBibHeadingsToAuthorities( $linker, $record, $frameworkcode, C4::Context->preference("CatalogModuleRelink") || '', undef, $verbose );
Comment 9 Marcel de Rooy 2023-05-17 11:21:00 UTC
Please provide feedback
Comment 10 Phil Ringnalda 2023-07-02 01:40:30 UTC
(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?
Comment 11 Nick Clemens (kidclamp) 2023-10-25 11:11:57 UTC
(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
Comment 12 Kyle M Hall 2023-11-03 16:52:13 UTC
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>
Comment 13 Tomás Cohen Arazi 2023-11-03 17:23:12 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Fridolin Somers 2023-11-10 02:29:45 UTC
Pushed to 23.05.x for 23.05.06
Comment 15 Pedro Amorim 2023-11-14 14:49:43 UTC
Nice work everyone!

Pushed to 22.11.x for next release