Bug 32656

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