Created attachment 59972 [details] [review] Bug 18070: Deleting an authority should update the linked biblio records Adding a test where we delete an authority and prove that the linked biblio still contains a reference to it. Note: Currently, the interface does not allow you to delete an authority that still has linked biblio records. Especially, if security bug 18019 has been pushed. This report will add a maintenance script that allows you to delete such records and cleanup the biblios. This is a first step. Test plan: [1] Run t/db_dependent/Authorities/Merge.t Last test should fail: not ok 1 - Field 609 should be gone too Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59973 [details] [review] Bug 18070: Extend sub merge to remove fields for deleted authorities In order to accomplish this, we need to add some additional checks in the merge routine. The actual change to remove the field, is quite small. Furthermore, we need to add a merge call in DelAuthority and adjust the merge cron job accordingly. The change is well supported by additional tests, including a simulation of postponed removal via cron, if dontmerge is enabled. Note: Deleting an authority with linked biblios is tested on the next patch. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] Delete an authority without linked biblios from the interface. If the indexer is not fast enough, wait a bit and refresh to verify that the authority is gone on authorities-home.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59977 [details] [review] Bug 18070: Add script for clean removal of authorities This script now supports clean removal of authority records that still have linked biblio records. On BZ report 18071, we will extend its functionality to include updating 001 fields, triggering a merge (refresh biblio records), etc. Test plan: [1] Look for one or two authority records with linked biblio records. [2] Pass their authority id's to the script: perl misc/maintenance/update_authorities.pl -authid X,Y -del [3] Verify that authority record is gone and biblio records are clean. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59978 [details] [review] Bug 18070: Correct error from podchecker on line 1376 Resolves: *** ERROR: Spurious =cut command at line 1376 in file C4/AuthoritiesMarc.pm Test plan: [1] Run podchecker on C4/AuthoritiesMarc.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Marcel, I have tried this patchset, but have problem with deleting authority... The authority link is not removed when I run update_authorities.pl with delete param... the authority itself is removed but still linked from biblio....
(In reply to Josef Moravec from comment #5) > Marcel, > > I have tried this patchset, but have problem with deleting authority... > > The authority link is not removed when I run update_authorities.pl with > delete param... > > the authority itself is removed but still linked from biblio.... Can you show me the biblio record?
And are you sure that your PERL5LIB points to the changed AuthoritiesMarc module?
(In reply to Marcel de Rooy from comment #6) > (In reply to Josef Moravec from comment #5) > > Marcel, > > > > I have tried this patchset, but have problem with deleting authority... > > > > The authority link is not removed when I run update_authorities.pl with > > delete param... > > > > the authority itself is removed but still linked from biblio.... > > Can you show me the biblio record? For example this one: http://paste.koha-community.org/309
(In reply to Marcel de Rooy from comment #7) > And are you sure that your PERL5LIB points to the changed AuthoritiesMarc > module? I am testing on kohadevbox in koha-shell env, tried to read the file and looks ok...
(In reply to Josef Moravec from comment #8) > (In reply to Marcel de Rooy from comment #6) > > (In reply to Josef Moravec from comment #5) > > > Marcel, > > > > > > I have tried this patchset, but have problem with deleting authority... > > > > > > The authority link is not removed when I run update_authorities.pl with > > > delete param... > > > > > > the authority itself is removed but still linked from biblio.... > > > > Can you show me the biblio record? > > > For example this one: > > http://paste.koha-community.org/309 I deleted the authority linked in field 100
(In reply to Josef Moravec from comment #9) > (In reply to Marcel de Rooy from comment #7) > > And are you sure that your PERL5LIB points to the changed AuthoritiesMarc > > module? > > I am testing on kohadevbox in koha-shell env, tried to read the file and > looks ok... before you run the script please echo $PERL5LIB does it mention home/vagrant/kohaclone first or /usr/share/koha/lib ?
Or could it be an index issue? IF you added an authority to the biblio record, and it was not yet indexed, the search an=index will not yet find it.
an=auth
Just tested again: add authority PERSO_NAME, used it in a 100 of a biblio. And removed it with update_authorities. Record is clean too.. I may have tested with 600s, but doing this in 100 should not be a problem.
What we still miss, is: chmod 755 misc/maintenance/update_authorities.pl
Hm, it really looks like indexing problem... but I can't figure it out still...
Still found something
Created attachment 60251 [details] [review] Bug 18070: Deleting an authority should update the linked biblio records Adding a test where we delete an authority and prove that the linked biblio still contains a reference to it. Note: Currently, you can only delete a used authority from Tools, batch record deletion. If you do, the biblio records will still contain references to the deleted authority. Test plan: [1] Run t/db_dependent/Authorities/Merge.t Last test should fail: not ok 1 - Field 609 should be gone too Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60252 [details] [review] Bug 18070: Extend sub merge to remove fields for deleted authorities In order to accomplish this, we need to add some additional checks in the merge routine. The actual change to remove the field, is quite small. Furthermore, we need to add a merge call in DelAuthority and adjust the merge cron job accordingly. The change is well supported by additional tests, including a simulation of postponed removal via cron, if dontmerge is enabled. Note: Deleting an authority with linked biblios is tested on the next patch. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] Delete an authority without linked biblios from the Authorities module. If the indexer is not fast enough, wait a bit and refresh to verify that the authority is gone on authorities-home.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60253 [details] [review] Bug 18070: [Follow-up] What should DelAuthority return? Script tools/batch_delete_records.pl only checks the return value of DelAuthority. The return value depends on DBI rows, which is not always reliable. It may return -1 when it 'does not know'. Testing $@ in tools/batch_delete_records.pl should actually be enough. The return value was discarded in C4/ImportBatch.pm. Removing an unused variable. Test plan: [1] Pick an authority record with a few linked biblios. Delete this authority record via tools/batch_delete_records.pl. Check if the linked biblio records are cleaned up. [2] Bonus: Make a typo in the SQL statement of DelAuthority. Check if batch_delete_records shows you the error message. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60254 [details] [review] Bug 18070: Correct error from podchecker on line 1376 Resolves: *** ERROR: Spurious =cut command at line 1376 in file C4/AuthoritiesMarc.pm Test plan: [1] Run podchecker on C4/AuthoritiesMarc.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
What happened: Added the followup patch about the return value of DelAuthority. This introduced the dependency on 16018 for authorities/merge.pl. Moved the maintenance script to 18071; testing with tools/batch record delete should be enough here. Josef: If you got your indexing working again, please retry this one. Anyone else is welcome too !
Created attachment 60392 [details] [review] [SIGNED-OFF] Bug 18070: Deleting an authority should update the linked biblio records Adding a test where we delete an authority and prove that the linked biblio still contains a reference to it. Note: Currently, you can only delete a used authority from Tools, batch record deletion. If you do, the biblio records will still contain references to the deleted authority. Test plan: [1] Run t/db_dependent/Authorities/Merge.t Last test should fail: not ok 1 - Field 609 should be gone too Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60393 [details] [review] [SIGNED-OFF] Bug 18070: Extend sub merge to remove fields for deleted authorities In order to accomplish this, we need to add some additional checks in the merge routine. The actual change to remove the field, is quite small. Furthermore, we need to add a merge call in DelAuthority and adjust the merge cron job accordingly. The change is well supported by additional tests, including a simulation of postponed removal via cron, if dontmerge is enabled. Note: Deleting an authority with linked biblios is tested on the next patch. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] Delete an authority without linked biblios from the Authorities module. If the indexer is not fast enough, wait a bit and refresh to verify that the authority is gone on authorities-home.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60394 [details] [review] [SIGNED-OFF] Bug 18070: [Follow-up] What should DelAuthority return? Script tools/batch_delete_records.pl only checks the return value of DelAuthority. The return value depends on DBI rows, which is not always reliable. It may return -1 when it 'does not know'. Testing $@ in tools/batch_delete_records.pl should actually be enough. The return value was discarded in C4/ImportBatch.pm. Removing an unused variable. Test plan: [1] Pick an authority record with a few linked biblios. Delete this authority record via tools/batch_delete_records.pl. Check if the linked biblio records are cleaned up. [2] Bonus: Make a typo in the SQL statement of DelAuthority. Check if batch_delete_records shows you the error message. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60395 [details] [review] [SIGNED-OFF] Bug 18070: Correct error from podchecker on line 1376 Resolves: *** ERROR: Spurious =cut command at line 1376 in file C4/AuthoritiesMarc.pm Test plan: [1] Run podchecker on C4/AuthoritiesMarc.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60557 [details] [review] Bug 18070: Deleting an authority should update the linked biblio records Adding a test where we delete an authority and prove that the linked biblio still contains a reference to it. Note: Currently, you can only delete a used authority from Tools, batch record deletion. If you do, the biblio records will still contain references to the deleted authority. Test plan: [1] Run t/db_dependent/Authorities/Merge.t Last test should fail: not ok 1 - Field 609 should be gone too Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 60558 [details] [review] Bug 18070: Extend sub merge to remove fields for deleted authorities In order to accomplish this, we need to add some additional checks in the merge routine. The actual change to remove the field, is quite small. Furthermore, we need to add a merge call in DelAuthority and adjust the merge cron job accordingly. The change is well supported by additional tests, including a simulation of postponed removal via cron, if dontmerge is enabled. Note: Deleting an authority with linked biblios is tested on the next patch. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] Delete an authority without linked biblios from the Authorities module. If the indexer is not fast enough, wait a bit and refresh to verify that the authority is gone on authorities-home.pl. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 60559 [details] [review] Bug 18070: [Follow-up] What should DelAuthority return? Script tools/batch_delete_records.pl only checks the return value of DelAuthority. The return value depends on DBI rows, which is not always reliable. It may return -1 when it 'does not know'. Testing $@ in tools/batch_delete_records.pl should actually be enough. The return value was discarded in C4/ImportBatch.pm. Removing an unused variable. Test plan: [1] Pick an authority record with a few linked biblios. Delete this authority record via tools/batch_delete_records.pl. Check if the linked biblio records are cleaned up. [2] Bonus: Make a typo in the SQL statement of DelAuthority. Check if batch_delete_records shows you the error message. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 60560 [details] [review] Bug 18070: Correct error from podchecker on line 1376 Resolves: *** ERROR: Spurious =cut command at line 1376 in file C4/AuthoritiesMarc.pm Test plan: [1] Run podchecker on C4/AuthoritiesMarc.pm Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Passed QA
Pushed to master for 17.05, thanks Marcel!
This won't get ported back to 16.11.x as it is an enhancement.