Bug 11084 - Delete biblios on Leader 05 =d
Summary: Delete biblios on Leader 05 =d
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 10421 (view as bug list)
Depends on:
Blocks: 19712
  Show dependency treegraph
 
Reported: 2013-10-20 18:06 UTC by David Noe
Modified: 2017-12-20 22:06 UTC (History)
17 users (show)

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


Attachments
Bug 11084 - Delete biblios on Leader 05 =d (2.49 KB, patch)
2013-10-20 18:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 =d (3.10 KB, patch)
2013-10-20 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 =d (4.59 KB, patch)
2013-10-21 18:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 == d (4.67 KB, patch)
2013-10-31 12:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 == d (4.66 KB, patch)
2015-03-12 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 == d (4.66 KB, patch)
2015-03-12 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 == d (5.08 KB, patch)
2015-03-12 16:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 == d (5.45 KB, patch)
2015-04-13 08:48 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 11084 [QA Followup] (669 bytes, patch)
2015-04-16 11:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 [QA Followup] - Suppress userenv warning (796 bytes, patch)
2015-04-22 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11084 [QA Followup] - Document limitations of test mode (1.17 KB, patch)
2015-04-22 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11084 - Delete biblios on Leader 05 == d (5.52 KB, patch)
2015-04-22 19:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11084 [QA Followup] (730 bytes, patch)
2015-04-22 19:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11084 [QA Followup] - Suppress userenv warning (859 bytes, patch)
2015-04-22 19:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11084 [QA Followup] - Document limitations of test mode (1.23 KB, patch)
2015-04-22 19:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11084 - Delete biblios on Leader 05 == d (5.57 KB, patch)
2015-12-29 19:37 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11084 [QA Followup] (786 bytes, patch)
2015-12-29 19:38 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11084 [QA Followup] - Suppress userenv warning (914 bytes, patch)
2015-12-29 19:38 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 11084 [QA Followup] - Document limitations of test mode (1.29 KB, patch)
2015-12-29 19:38 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Noe 2013-10-20 18:06:17 UTC
Position 05 of the leader in MARC 21 indicates the record's status. A lower case “d” indicates that the record is to be deleted (http://www.loc.gov/marc/authority/adleader.html).

Koha should allow for deletion of bibliographic records that contain a “d” in the LEADER position 05 when by overlaying existing records.
Comment 1 Kyle M Hall 2013-10-20 18:10:05 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-10-20 18:15:40 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-10-20 18:29:56 UTC
See also - bug 10421
Comment 4 Mathieu Saby 2013-10-20 21:01:26 UTC
Note : the label for UNIMARC is buit the same way
pos5 = "d" means deleted
http://multimedia.bnf.fr/unimarcb_trad/Bsection5-Label_notice-6-2010.pdf

So it could be made too for unimarc.

Mathieu
Comment 5 David Noe 2013-10-21 16:19:27 UTC
Tool needs to be able to delete biblios with items (and the items). Also, it should be able to run in a test mode.
Comment 6 Kyle M Hall 2013-10-21 18:44:37 UTC Comment hidden (obsolete)
Comment 7 I'm just a bot 2013-10-30 07:42:35 UTC
Patch applied cleanly, go forth and signoff
Comment 8 Kyle M Hall 2013-10-30 12:39:40 UTC
*** Bug 10421 has been marked as a duplicate of this bug. ***
Comment 9 Marcel de Rooy 2013-10-31 12:34:43 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2013-10-31 12:35:32 UTC
Signing off but with some smaller comments:

typo delete_fixec_field_5.pl
test mode should be explained (not in usage)
name of the script could be confusing: you are obviously not deleting a fixed field, but a record :)
encoding: when you are printing the title to stdout, you should probably open stdout as utf8
verbose mode: would it be helpful to say which biblios or items were actually deleted (and count them?)
license statement: version 2 or 3? address no longer?
Comment 11 Katrin Fischer 2013-11-19 22:42:31 UTC
Hm, a few notes from me too: 

DelBiblio has a few shortcomings that could cause problems:

1) It just deletes attached subscriptions. Subscription data can be created over a long time and is not easily reproducable. There is no backup when we delete it.

 428     # We delete attached subscriptions
429     require C4::Serials;
430     my $subscriptions = C4::Serials::GetFullSubscriptionsFromBiblionumber($biblionumber);
431     foreach my $subscription (@$subscriptions) {
432         C4::Serials::DelSubscription( $subscription->{subscriptionid} );
433     }

2) It will also delete records that are used with orders, as this isn't checked at all.

Kyle, could you take a look at Marcel's and my comments?
Comment 12 Katrin Fischer 2013-11-19 22:49:10 UTC
Hm, bit confused about the help too - is there a way to get it to list all available options? -h didn't seem to do the trick.
Comment 13 Pablo AB 2014-06-02 16:45:26 UTC
(In reply to Kyle M Hall from comment #6)
> Position 05 of the leader in MARC 21 indicates the record's status.
> A lower case “d” indicates that the record is to be deleted
> (http://www.loc.gov/marc/authority/adleader.html).
JFTR, that link talks about authority records, but for biblio records is the same:
http://www.loc.gov/marc/bibliographic/bdleader.html
Comment 14 Kyle M Hall 2015-03-12 15:30:26 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-03-12 15:30:59 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-03-12 16:02:18 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-03-12 16:02:40 UTC
Script now displays all options.

(In reply to Katrin Fischer from comment #12)
> Hm, bit confused about the help too - is there a way to get it to list all
> available options? -h didn't seem to do the trick.
Comment 18 Kyle M Hall 2015-03-12 16:03:12 UTC
Updated patch should address all these concerns.

(In reply to M. de Rooy from comment #10)
> Signing off but with some smaller comments:
> 
> typo delete_fixec_field_5.pl
> test mode should be explained (not in usage)
> name of the script could be confusing: you are obviously not deleting a
> fixed field, but a record :)
> encoding: when you are printing the title to stdout, you should probably
> open stdout as utf8
> verbose mode: would it be helpful to say which biblios or items were
> actually deleted (and count them?)
> license statement: version 2 or 3? address no longer?
Comment 19 Kyle M Hall 2015-03-12 16:06:11 UTC
This is a definite issue, but appears to be outside the scope of this bug.

It would appear that misc/batchdeletebiblios.pl has the exact same issues. I would suggest we file a separate bug to handle that issue.

(In reply to Katrin Fischer from comment #11)
> Hm, a few notes from me too: 
> 
> DelBiblio has a few shortcomings that could cause problems:
> 
> 1) It just deletes attached subscriptions. Subscription data can be created
> over a long time and is not easily reproducable. There is no backup when we
> delete it.
> 
>  428     # We delete attached subscriptions
> 429     require C4::Serials;
> 430     my $subscriptions =
> C4::Serials::GetFullSubscriptionsFromBiblionumber($biblionumber);
> 431     foreach my $subscription (@$subscriptions) {
> 432         C4::Serials::DelSubscription( $subscription->{subscriptionid} );
> 433     }
> 
> 2) It will also delete records that are used with orders, as this isn't
> checked at all.
> 
> Kyle, could you take a look at Marcel's and my comments?
Comment 20 Magnus Enger 2015-04-13 08:48:27 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2015-04-15 21:02:15 UTC
Hi Kyle,

I am ok with this missing checks for subscriptions and orders - I think we can assume here checks have been made when setting the flag (although i would like a 'save delete' option or something similar :) )

But can you please fix:
 FAIL	misc/cronjobs/delete_records_via_leader.pl
   FAIL	  critic
		I/O layer ":utf8" used at line 24, column 1. Use ":encoding(UTF-8)" to get strict validation.
Comment 22 Kyle M Hall 2015-04-16 11:50:48 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2015-04-22 06:13:21 UTC
Hi Kyle,

I did some tests with the new patches:

Record has 1 item, is not checked out.

-t -c 
ERROR DELETING BIBLIO 24: Test mode enabled

I was not sure if this would have been an error outside the test mode (not being deleted) or if that just meant that it was not deleted because of the test mode.

-t -i -c
ERROR DELETING ITEM 102: Test mode enabled
ERROR DELETING BIBLIO 24: Test mode enabled

Ok, so both throw an error... so not sure: would those be deleted without -t?

-c
ERROR DELETING BIBLIO 24: This Biblio has items attached, please delete them first before deleting this biblio

This one is good - why not display this in the test mode too?

-c -i
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Items.pm line 2278.

No messages, but I didn't set verbose. Item and record appear to have been deleted - can you take a look at the error/warn?
Comment 24 Kyle M Hall 2015-04-22 12:59:27 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2015-04-22 13:06:50 UTC
(In reply to Katrin Fischer from comment #23)
> Hi Kyle,
> 
> I did some tests with the new patches:
> 
> Record has 1 item, is not checked out.
> 
> -t -c 
> ERROR DELETING BIBLIO 24: Test mode enabled
> 
> I was not sure if this would have been an error outside the test mode (not
> being deleted) or if that just meant that it was not deleted because of the
> test mode.

Right now it's not possible to tell if a record is deletable in Koha without trying to delete it! There are simply no library functions to do so. The test mode will tell you what records and items it would attempt to delete, but is not meant to tell you if they would be deleted successfully. 

> 
> -t -i -c
> ERROR DELETING ITEM 102: Test mode enabled
> ERROR DELETING BIBLIO 24: Test mode enabled
> 
> Ok, so both throw an error... so not sure: would those be deleted without -t?

See above.
 
> -c
> ERROR DELETING BIBLIO 24: This Biblio has items attached, please delete them
> first before deleting this biblio
> 
> This one is good - why not display this in the test mode too?

See above as well ; )
The errors come from DelBiblio, so the only way to know if the record can't be deleted is to try to delete it!

> -c -i
> C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Items.pm line
> 2278.
> 
> No messages, but I didn't set verbose. Item and record appear to have been
> deleted - can you take a look at the error/warn?

I've attached a followup to take care of this. userenv isn't defined for cli scripts, so we just need to check for it's existence before using it.
Comment 26 Katrin Fischer 2015-04-22 13:08:52 UTC
Can we make that clear in the documentation for the test mode? that it will only list items that it would 'try' to delete?
Comment 27 Kyle M Hall 2015-04-22 13:11:58 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2015-04-22 19:45:35 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2015-04-22 19:45:39 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2015-04-22 19:45:43 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2015-04-22 19:45:47 UTC Comment hidden (obsolete)
Comment 32 Brendan Gallagher 2015-12-29 19:37:57 UTC
Created attachment 46021 [details] [review]
Bug 11084 - Delete biblios on Leader 05 == d

Position 05 of the leader in MARC 21 indicates the record's status.
A lower case 'd' indicates that the record is to be deleted
(http://www.loc.gov/marc/authority/adleader.html).

This patch adds a cronjob script that may be run nightly to delete
records where position 05 of the leader is set to 'd'.

Test Plan:
1) Edit one or more records, and set the leader position 05 to 'd'.
2) Run misc/cronjobs/delete_records_via_leader.pl -c -v
3) Those records should either be deleted, or an error message should
   state why they were not.

Signed-off-by: Magnus Enger <magnus@enger.priv.no>

Works as advertised. Edited a record with an item to have leader05 = d,
then ran delete_records_via_leader.pl with -c and got a message that
the record could not be deleted because of the item. Removed the item
and the script deleted the record. Tried with another record with an
item, this time with the -i option and the record was deleted.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Comment 33 Brendan Gallagher 2015-12-29 19:38:33 UTC
Created attachment 46022 [details] [review]
Bug 11084 [QA Followup]

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Comment 34 Brendan Gallagher 2015-12-29 19:38:42 UTC
Created attachment 46023 [details] [review]
Bug 11084 [QA Followup] - Suppress userenv warning

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Comment 35 Brendan Gallagher 2015-12-29 19:38:54 UTC
Created attachment 46024 [details] [review]
Bug 11084 [QA Followup] - Document limitations of test mode

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Comment 36 Brendan Gallagher 2015-12-29 19:39:31 UTC
Pushed to master - Thanks
Comment 37 David Cook 2016-01-11 01:02:08 UTC
Comment on attachment 46021 [details] [review]
Bug 11084 - Delete biblios on Leader 05 == d

Review of attachment 46021 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/delete_records_via_leader.pl
@@ +67,5 @@
> +}
> +
> +my $schema = Koha::Database->new()->schema();
> +my @biblioitems =
> +  $schema->resultset('Biblioitem')->search( { marc => { LIKE => '_____d%' } } );

We're reading the 'marc' field as a string?

While that's pretty fast on smaller databases, it takes about 60 seconds on a database with about 1.1 million bib records. I suppose performance doesn't matter too much for a cronjob though - depending on the frequency that it's run.

Also, aren't we trying to get rid of the biblioitems.marc field? See: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10455

Wouldn't it be better if we started indexing LDR05 as 'rstatus' or "record_status' or some such?
Comment 38 David Cook 2016-01-11 01:07:37 UTC
More over, if we indexed LDR05, we would be able to hide "deleted" records from the OPAC like we do with "suppressed" records. 

While we had to worry about that OpacSuppression bug, which erroneously returned 0 results when the OpacSuppression index was completely empty, we don't need to worry about that too much since we started using @attr 14=1 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15198. We could prepend @attr 14=1 at the start of all queries (so that it applied to all query parts) and we wouldn't encounter that problem anymore. 

--

I desire the above behaviour also because of what David Noe originally described here. It would be nice if records with a LDR05 "d" were deleted during the overlay of existing records during an import.

In my case, I'm downloading records via OAI-PMH, but when I get a record with a "deleted" status, I'm not given a metadata record. I just have an identifier and a deleted status.

So I can create an empty MARC record, change LDR05 to "d", inject the identifier, and then match to existing records in Koha. 

However, I don't want to overlay an empty record over a non-empty record in Koha, and then have it show up in search results in the OPAC. Actually, I wouldn't want it to show up in search results in the staff client either, but that might be unavoidable... I suppose I could also add a fake title to the record like "This record has been deleted" or some such, although that's not translation friendly...
Comment 39 David Cook 2016-01-12 02:51:50 UTC
Comment on attachment 46021 [details] [review]
Bug 11084 - Delete biblios on Leader 05 == d

Review of attachment 46021 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/delete_records_via_leader.pl
@@ +67,5 @@
> +}
> +
> +my $schema = Koha::Database->new()->schema();
> +my @biblioitems =
> +  $schema->resultset('Biblioitem')->search( { marc => { LIKE => '_____d%' } } );

Once Bug 15537 (http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15537) is pushed, this could be replaced with the following query with C4::Search::SimpleSearch():

"Record-status=d"
Comment 40 Jonathan Druart 2016-08-25 09:32:08 UTC
I have modified this script on bug 10455 with:

-my @biblioitems =
-  $schema->resultset('Biblioitem')->search( { marc => { LIKE => '_____d%' } } );
+my @biblioitems = # Should be replaced by a call to C4::Search on zebra index
+                  # Record-status when bug 15537 will be pushed
+  $schema->resultset('Biblioitem')->search( { marcxml => { LIKE => '<leader>_____d%' } } );

Could you confirm you agree with that?