| Summary: | Deduping authorities script (dedup_authorities.pl) can die on duplicated ids | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
| Component: | Command-line Utilities | Assignee: | Nick Clemens (kidclamp) <nick> |
| Status: | Signed Off --- | QA Contact: | Marcel de Rooy <m.de.rooy> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | david, m.de.rooy, robin |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| GIT URL: | Initiative type: | --- | |
| Sponsorship status: | --- | Comma delimited list of Sponsors: | |
| Crowdfunding goal: | 0 | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
This fixes the deduping authorities maintenance script (misc/maintenance/dedup_authorities.pl) so that it now works and displays the output from the merging of authority records as expected.
Previously, it seemed to generate duplicate IDs, for example:
Before
------
Processing authority 1660 (531/650 81.69%)
Merging 1660,1662 into 1660.
Updated 0 biblios
Deleting 1662
Merge done.
After
-----
Processing authority 1660 (532/650 81.85%)
Merging 1662 into 1660.
Updated 0 biblios
Deleting 1662
Merge done.
|
Version(s) released in: | |
| Circulation function: | |||
| Bug Depends on: | 13706 | ||
| Bug Blocks: | |||
| Attachments: |
Bug 41097: Don't use original record twice
Bug 41097: Don't use original record twice |
||
Created attachment 188416 [details] [review] 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! Created attachment 188437 [details] [review] 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> Testing notes (using KTD) - just noting my experience, have signed off:
Before the patch
==================
First run
---------
...
Processing authority 748 (360/650 55.38%)
Merging 748,748 into 403.
Updated 0 biblios
Deleting 748
Updated 0 biblios
Deleting 748
{UNKNOWN}: Can't call method "move_to_deleted" on an undefined value at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 754. at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 757
Second run, still get merges
----------------------------
...
Processing authority 1568 (514/597 86.10%)
Merging 1568,1568 into 403.
Updated 0 biblios
Deleting 1568
Updated 0 biblios
Deleting 1568
{UNKNOWN}: Can't call method "move_to_deleted" on an undefined value at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 754. at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 757
Third run
---------
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 6 authorities
No biblios to update
Fourth run
----------
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 0 authorities
No biblios to update
After the patch - with a reset_all
==================================
...
Processing authority 1662 (631/650 97.08%)
Merging 1660 into 1662.
Updated 0 biblios
Deleting 1660
Merge done.
....
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 50 authorities
No biblios to update
After the patch - with a shutdown and startup of KTD
====================================================
First run
---------
...
Processing authority 1660 (532/650 81.85%)
Merging 1662 into 1660.
Updated 0 biblios
Deleting 1662
Merge done.
...
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 104 authorities
No biblios to update
Second run
----------
...
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 0 authorities
No biblios to update
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.
(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 |
When running the script it seems to generate duplicate ids: Processing authority 1660 (531/650 81.69%) Merging 1660,1662 into 1660. Updated 0 biblios Deleting 1662 Merge done. This seems to be a product of _choose_records which gets all the candidates, but then adds the original record twice 318 my @records = map { C4::AuthoritiesMarc::GetAuthority($_) } @recordids; 319 my @candidate_auths = @records; and later: $choose_subs[2] ? $choose_subs[2]->($_) : 0 ] => ( $records[0], @candidate_auths );