https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 --- Comment #1 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 188416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188416&action=edit Bug 41097: Don't use original record twice This patch prevents us from passing the same record twice into the choose subs to avoid a situation where the original record is not chosen as the one to keep and we try to merge/delete the same record twice To test: 1 - Run the script on a test system that you can reset perl misc/maintenance/dedup_authorities.pl -v -v --confirm -a="PERSO_NAME" 2 - Note duplication of entries in the output like: Merging 1660,1662 into 1660. Updated 0 biblios Deleting 1662 Merge done. 3 - Note how many records are merged 4 - Run again to confirm nothing is merged, i.e. the script removed the dupes 5 - Reset your DB 6 - Apply patch 7 - Run the script again 8 - Note entries no longer have duplicated ids: Merging 1662 into 1660. 9 - Note the same number of records are merged 10 - Run again and confirm nothing is merged 11 - Sign off! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188416|0 |1 is obsolete| | --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 188437 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188437&action=edit Bug 41097: Don't use original record twice This patch prevents us from passing the same record twice into the choose subs to avoid a situation where the original record is not chosen as the one to keep and we try to merge/delete the same record twice To test: 1 - Run the script on a test system that you can reset perl misc/maintenance/dedup_authorities.pl -v -v --confirm -a="PERSO_NAME" 2 - Note duplication of entries in the output like: Merging 1660,1662 into 1660. Updated 0 biblios Deleting 1662 Merge done. 3 - Note how many records are merged 4 - Run again to confirm nothing is merged, i.e. the script removed the dupes 5 - Reset your DB 6 - Apply patch 7 - Run the script again 8 - Note entries no longer have duplicated ids: Merging 1662 into 1660. 9 - Note the same number of records are merged 10 - Run again and confirm nothing is merged 11 - Sign off! Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |m.de.rooy@rijksmuseum.nl --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking at the code: This seems to be the only call: my ( $recordid_to_keep, @recordids_to_merge ) = _choose_records( $authority->authid, @recordids ); The POD says: returns first the record to merge to and list of records to merge from The sub makes no distinction between $authority->authid and @recordids. Note btw that @recordids is a BAD name; it contains MARC records ! sub _choose_records { my @recordids = @_; It returns candidate authids (picked from 001) but they are sorted. Why should the original first argument still be first? Shouldnt we add the first parameter (which is NOT a MARC record), not include that one while sorting and push it in front when returning values? This needs a bit more attention to clarify current obscureness while touching those lines here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #5 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #4)

Looking at the code:

This seems to be the only call: my ( $recordid_to_keep, @recordids_to_merge ) = _choose_records( $authority->authid, @recordids );

The POD says: returns first the record to merge to and list of records to merge from

The sub makes no distinction between $authority->authid and @recordids. Note btw that @recordids is a BAD name; it contains MARC records ! sub _choose_records { my @recordids = @_;

It returns candidate authids (picked from 001) but they are sorted. Why should the original first argument still be first?

It shouldn't - I think that was just a mistake

Shouldn't we add the first parameter (which is NOT a MARC record), not include that one while sorting and push it in front when returning values?

The whole point of sorting is to determine the best candidate to merge into, and we don't know which record that is before we sort. Julian wrote the original, I just got it through QA, so I can't say why we originally wanted the first record, but I think the testing can demonstrate we don't want the same record id and to end up trying to merge into the same record

This needs a bit more attention to clarify current obscureness while touching those lines here.

I welcome any additions to clarify the code, but I do think this fix is pretty striaghtforward -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188437|0 |1 is obsolete| | --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193502 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193502&action=edit Bug 41097: Don't use original record twice This patch prevents us from passing the same record twice into the choose subs to avoid a situation where the original record is not chosen as the one to keep and we try to merge/delete the same record twice To test: 1 - Run the script on a test system that you can reset perl misc/maintenance/dedup_authorities.pl -v -v --confirm -a="PERSO_NAME" 2 - Note duplication of entries in the output like: Merging 1660,1662 into 1660. Updated 0 biblios Deleting 1662 Merge done. 3 - Note how many records are merged 4 - Run again to confirm nothing is merged, i.e. the script removed the dupes 5 - Reset your DB 6 - Apply patch 7 - Run the script again 8 - Note entries no longer have duplicated ids: Merging 1662 into 1660. 9 - Note the same number of records are merged 10 - Run again and confirm nothing is merged 11 - Sign off! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Nick Clemens (kidclamp) from comment #5)

I welcome any additions to clarify the code, but I do think this fix is pretty striaghtforward

The fix looks simple, the existing code was a bit obscure. The POD was unclear too. I have tried to fix the latter here with a small follow-up. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 --- Comment #9 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 26.05 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41097 --- Comment #10 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.