Bug 29788 inadvertantly replaced a call to safe_delete() with safe_to_delete() such that any time the script should delete an item it only checks to see if the item is delectable, after which deletion of the record fails because the items were not deleted.
Created attachment 145364 [details] [review] Bug 32656: Script delete_records_via_leader.pl no longer deletes items Bug 29788 inadvertantly replaced a call to safe_delete() with safe_to_delete() such that any time the script should delete an item it only checks to see if the item is delectable, after which deletion of the record fails because the items were not deleted. Test Plan: 1) Mark a record with items to be deleted via the record leader 2) Run delete_records_via_leader.pl -i -b -v 3) Note the script says it is deleting the items but then the record deletion fails. Note the items remain in the items table of the database. 4) Apply this patch 5) Repeat step 2 6) This time the items and record should be deleted!
Hi Kyle. I had a go at testing (using KTD and the sample data), but didn't have any luck: 1. I marked a record (146 with four items) as deleted (I edited the record, for 000 I used the plugin to change 5-Record status to d- Deleted). 2. Before the patch was applied, I ran misc/cronjobs/delete_records_via_leader.pl -i -t -v (step 2 in the test plan had -b, but I think that should -t to test?). 3. Output is: misc/cronjobs/delete_records_via_leader.pl -t -i -v RECORD: 1 TEST MODE: Item 1 would have been deleted ERROR DELETING BIBLIO 1: Test mode enabled DELETED 0 OF 1 RECORDS DELETED 0 OF 1 ITEMS 4. If I confirm the deletion (-c instead of -t) I get this (the record and items are not deleted, in the database the records still exist - SQL query: select * from items where biblionumber = 146;): misc/cronjobs/delete_records_via_leader.pl -c -i -v RECORD: 1 DELETED ITEM 1 ERROR DELETING BIBLIO 1: This Biblio has items attached, please delete them first before deleting this biblio DELETED 0 OF 1 RECORDS DELETED 1 OF 1 ITEMS 5. Applied the patch, then ran flush_memcached and restart_all. 6. Repeated step 3 above, the output is the same (as expected): misc/cronjobs/delete_records_via_leader.pl -t -i -v RECORD: 1 TEST MODE: Item 1 would have been deleted ERROR DELETING BIBLIO 1: Test mode enabled DELETED 0 OF 1 RECORDS DELETED 0 OF 1 ITEMS 7. Repeated step 4 above, the output is: misc/cronjobs/delete_records_via_leader.pl -c -i -v RECORD: 1 DELETED ITEM 1 DELETED BIBLIO 1 DELETED 1 OF 1 RECORDS DELETED 1 OF 1 ITEM 8. The record and items still exist. 9. I also tried on a record that has no items (Perl best practices - biblio number = 5), and get the same behavour after the patch is applied: misc/cronjobs/delete_records_via_leader.pl -t -i -v RECORD: 2 ERROR DELETING BIBLIO 2: Test mode enabled DELETED 0 OF 1 RECORDS DELETED 0 OF 0 ITEMS ...... misc/cronjobs/delete_records_via_leader.pl -c -i -v RECORD: 2 DELETED BIBLIO 2 DELETED 1 OF 1 RECORDS DELETED 0 OF 0 ITEMS So I think I may have misunderstood what should happen, or have just managed to confuse myself... Also, just to confirm that if a record has items then the records and items should not be deleted unless the -i option is used? I found the help text confusing (if you use -i then it will try to delete the items, but then says "Records with items cannot be deleted."; or does this mean that if items can't be deleted, for example if checked out, then the record won't be deleted?): -i --delete-items: Try deleting items before deleting record. Records with items cannot be deleted. David
Created attachment 145407 [details] [review] Bug 32656: Script delete_records_via_leader.pl no longer deletes items Bug 29788 inadvertantly replaced a call to safe_delete() with safe_to_delete() such that any time the script should delete an item it only checks to see if the item is delectable, after which deletion of the record fails because the items were not deleted. Test Plan: 1) Mark a record with items to be deleted via the record leader 2) Run delete_records_via_leader.pl -i -b -v 3) Note the script says it is deleting the items but then the record deletion fails. Note the items remain in the items table of the database. 4) Apply this patch 5) Repeat step 2 6) This time the items and record should be deleted!
(In reply to David Nind from comment #2) Thanks for testing David! It appears my previous patch was not sufficient. Can you give this one a try? Thanks!
(In reply to Kyle M Hall from comment #4) > Can you give this one a try? Thanks! Thanks Kyle! Everything now works as expected. Sign off on the way! Output after the patch is applied with --test: misc/cronjobs/delete_records_via_leader.pl -i -t -v RECORD: 146 TEST MODE: Item 312 would have been deleted TEST MODE: Item 313 would have been deleted TEST MODE: Item 314 would have been deleted TEST MODE: Item 315 would have been deleted ERROR DELETING BIBLIO 146: Test mode enabled DELETED 0 OF 1 RECORDS DELETED 0 OF 4 ITEMS Output after the patch is applied with --confirm: misc/cronjobs/delete_records_via_leader.pl -c -i -v RECORD: 146 DELETED ITEM 312 DELETED ITEM 313 DELETED ITEM 314 DELETED ITEM 315 DELETED BIBLIO 146 DELETED 1 OF 1 RECORDS DELETED 4 OF 4 ITEMS I also tested records with and without items using -i and not using -i: - A record with items: if -i is NOT used, then the record and items are not deleted (the message is "This Biblio has items attached, please delete them first before deleting this biblio") - A record without items: command with or without -i works as expected (record deleted, there are no items to delete) David
I'm having difficulty signing this off: /usr/bin/git-bz:1908: UnicodeWarning: Unicode equal comparison failed to convert both arguments to Unicode - interpreting them as being unequal obsoleted = (initial_description == patch.description) Traceback (most recent call last): File "/usr/bin/git-bz", line 2722, in <module> do_attach(*args) File "/usr/bin/git-bz", line 2074, in do_attach attach_commits(bug, commits, edit_comments=global_options.edit) File "/usr/bin/git-bz", line 1988, in attach_commits description, body, obsoletes, statuses, patch_complexities, depends = edit_attachment_comment(bug, commit.subject, body) File "/usr/bin/git-bz", line 1940, in edit_attachment_comment lines = edit_template(template.getvalue()) File "/usr/lib/python2.7/StringIO.py", line 271, in getvalue self.buf += ''.join(self.buflist) UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 3: ordinal not in range(128) I had this the other day with another bug when testing. It was to do with Unicode characters in the bug title: [PATCH] =?UTF-8?q?Bug=C2=A032656:=20Script=20delete=5Frecords=5Fv?= =?UTF-8?q?ia=5Fleader.pl=20no=20longer=20deletes=20items?=
I'll sign it for you. I've been seeing this with git-bz ever since I upgraded my Mac to Ventura. I think it has to do with copying and pasting web browser text into vim. It seems like it now preserves non-breaking spaces and other unicode characters, but git-bz, being written in python2, cannot handle them. I have no idea how to solve this. Git itself couldn't care less, neither does perltidy. (In reply to David Nind from comment #6) > I'm having difficulty signing this off: > > /usr/bin/git-bz:1908: UnicodeWarning: Unicode equal comparison failed to > convert both arguments to Unicode - interpreting them as being unequal > obsoleted = (initial_description == patch.description) > Traceback (most recent call last): > File "/usr/bin/git-bz", line 2722, in <module> > do_attach(*args) > File "/usr/bin/git-bz", line 2074, in do_attach > attach_commits(bug, commits, edit_comments=global_options.edit) > File "/usr/bin/git-bz", line 1988, in attach_commits > description, body, obsoletes, statuses, patch_complexities, depends = > edit_attachment_comment(bug, commit.subject, body) > File "/usr/bin/git-bz", line 1940, in edit_attachment_comment > lines = edit_template(template.getvalue()) > File "/usr/lib/python2.7/StringIO.py", line 271, in getvalue > self.buf += ''.join(self.buflist) > UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 3: > ordinal not in range(128) > > I had this the other day with another bug when testing. It was to do with > Unicode characters in the bug title: > > [PATCH] =?UTF-8?q?Bug=C2=A032656:=20Script=20delete=5Frecords=5Fv?= > =?UTF-8?q?ia=5Fleader.pl=20no=20longer=20deletes=20items?=
Created attachment 145460 [details] [review] Bug 32656: Script delete_records_via_leader.pl no longer deletes items Bug 29788 inadvertantly replaced a call to safe_delete() with safe_to_delete() such that any time the script should delete an item it only checks to see if the item is delectable, after which deletion of the record fails because the items were not deleted. Test Plan: 1) Mark a record with items to be deleted via the record leader 2) Run delete_records_via_leader.pl -i -b -v 3) Note the script says it is deleting the items but then the record deletion fails. Note the items remain in the items table of the database. 4) Apply this patch 5) Repeat step 2 6) This time the items and record should be deleted! Signed-off-by: David Nind <david@davidnind.com>
Created attachment 145506 [details] [review] Bug 32656: Script delete_records_via_leader.pl no longer deletes items Bug 29788 inadvertantly replaced a call to safe_delete() with safe_to_delete() such that any time the script should delete an item it only checks to see if the item is delectable, after which deletion of the record fails because the items were not deleted. Test Plan: 1) Mark a record with items to be deleted via the record leader 2) Run delete_records_via_leader.pl -i -b -v 3) Note the script says it is deleting the items but then the record deletion fails. Note the items remain in the items table of the database. 4) Apply this patch 5) Repeat step 2 6) This time the items and record should be deleted! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Keep up the good work! Pushed to 23.05.x for the next release
Nice work everyone! Pushed to stable for 22.11.x
Backported to 22.05.x for upcoming 22.05.10
applied to 21.11.x for 21.11.16
Not backported to 21.05.x