Bug 18070 - Support clean removal of authority records
Summary: Support clean removal of authority records
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 16018
Blocks: 17908 18071 9988
  Show dependency treegraph
 
Reported: 2017-02-07 08:52 UTC by Marcel de Rooy
Modified: 2017-03-12 22:58 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18070: Deleting an authority should update the linked biblio records (2.31 KB, patch)
2017-02-07 09:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: Extend sub merge to remove fields for deleted authorities (9.73 KB, patch)
2017-02-07 09:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: Add script for clean removal of authorities (3.30 KB, patch)
2017-02-07 13:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: Correct error from podchecker on line 1376 (889 bytes, patch)
2017-02-07 13:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: Deleting an authority should update the linked biblio records (2.19 KB, patch)
2017-02-15 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: Extend sub merge to remove fields for deleted authorities (9.74 KB, patch)
2017-02-15 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: [Follow-up] What should DelAuthority return? (2.71 KB, patch)
2017-02-15 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18070: Correct error from podchecker on line 1376 (889 bytes, patch)
2017-02-15 13:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18070: Deleting an authority should update the linked biblio records (2.22 KB, patch)
2017-02-17 10:31 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18070: Extend sub merge to remove fields for deleted authorities (9.77 KB, patch)
2017-02-17 10:31 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18070: [Follow-up] What should DelAuthority return? (2.74 KB, patch)
2017-02-17 10:32 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18070: Correct error from podchecker on line 1376 (919 bytes, patch)
2017-02-17 10:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18070: Deleting an authority should update the linked biblio records (2.27 KB, patch)
2017-02-22 14:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18070: Extend sub merge to remove fields for deleted authorities (9.83 KB, patch)
2017-02-22 14:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18070: [Follow-up] What should DelAuthority return? (2.80 KB, patch)
2017-02-22 14:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18070: Correct error from podchecker on line 1376 (972 bytes, patch)
2017-02-22 14:46 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-02-07 08:52:38 UTC

    
Comment 1 Marcel de Rooy 2017-02-07 09:18:18 UTC
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>
Comment 2 Marcel de Rooy 2017-02-07 09:18:21 UTC
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>
Comment 3 Marcel de Rooy 2017-02-07 13:49:29 UTC
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>
Comment 4 Marcel de Rooy 2017-02-07 13:49:33 UTC
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>
Comment 5 Josef Moravec 2017-02-09 10:50:49 UTC
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....
Comment 6 Marcel de Rooy 2017-02-09 10:56:52 UTC
(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?
Comment 7 Marcel de Rooy 2017-02-09 10:58:33 UTC
And are you sure that your PERL5LIB points to the changed AuthoritiesMarc module?
Comment 8 Josef Moravec 2017-02-09 11:01:11 UTC
(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
Comment 9 Josef Moravec 2017-02-09 11:02:49 UTC
(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...
Comment 10 Josef Moravec 2017-02-09 11:04:01 UTC
(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
Comment 11 Marcel de Rooy 2017-02-09 11:06:12 UTC
(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 ?
Comment 12 Marcel de Rooy 2017-02-09 11:09:31 UTC
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.
Comment 13 Marcel de Rooy 2017-02-09 11:11:33 UTC
an=auth
Comment 14 Marcel de Rooy 2017-02-09 11:24:27 UTC
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.
Comment 15 Marcel de Rooy 2017-02-09 11:25:27 UTC
What we still miss, is: chmod 755 misc/maintenance/update_authorities.pl
Comment 16 Josef Moravec 2017-02-09 11:44:24 UTC
Hm, it really looks like indexing problem... but I can't figure it out still...
Comment 17 Marcel de Rooy 2017-02-14 16:03:56 UTC
Still found something
Comment 18 Marcel de Rooy 2017-02-15 13:05:45 UTC
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>
Comment 19 Marcel de Rooy 2017-02-15 13:05:49 UTC
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>
Comment 20 Marcel de Rooy 2017-02-15 13:05:53 UTC
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>
Comment 21 Marcel de Rooy 2017-02-15 13:05:58 UTC
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>
Comment 22 Marcel de Rooy 2017-02-15 13:14:49 UTC
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 !
Comment 23 Josef Moravec 2017-02-17 10:31:27 UTC
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>
Comment 24 Josef Moravec 2017-02-17 10:31:53 UTC
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>
Comment 25 Josef Moravec 2017-02-17 10:32:03 UTC
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>
Comment 26 Josef Moravec 2017-02-17 10:32:08 UTC
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>
Comment 27 Julian Maurice 2017-02-22 14:46:13 UTC
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>
Comment 28 Julian Maurice 2017-02-22 14:46:17 UTC
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>
Comment 29 Julian Maurice 2017-02-22 14:46:20 UTC
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>
Comment 30 Julian Maurice 2017-02-22 14:46:24 UTC
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>
Comment 31 Julian Maurice 2017-02-22 14:46:43 UTC
Passed QA
Comment 32 Kyle M Hall 2017-03-03 18:13:13 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 33 Katrin Fischer 2017-03-12 22:58:32 UTC
This won't get ported back to 16.11.x as it is an enhancement.