Bug 8132 - Batch delete tool deletes items with holds on them
Summary: Batch delete tool deletes items with holds on them
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P4 major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL: https://gitlab.com/joubu/Koha/commits...
Keywords: rel_20_05_target
Depends on: 23463
Blocks: 8149
  Show dependency treegraph
 
Reported: 2012-05-21 15:33 UTC by Nicole C. Engard
Modified: 2019-12-12 12:48 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8132: No changes but disable checkboxes (3.84 KB, patch)
2019-12-03 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (3.91 KB, patch)
2019-12-03 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (11.39 KB, patch)
2019-12-03 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.54 KB, patch)
2019-12-03 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
delete message (180.96 KB, image/png)
2019-12-04 18:40 UTC, Kelly McElligott
Details
Bug 8132: No changes but disable checkboxes (3.84 KB, patch)
2019-12-10 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (4.45 KB, patch)
2019-12-10 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (12.02 KB, patch)
2019-12-10 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.54 KB, patch)
2019-12-10 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-05-21 15:33:01 UTC
I don't think it always did this - but I tested today on 3.8 and it deleted an item and a bib record (cause I checked that box) that had holds on it.
Comment 1 Liz Rea 2012-05-24 15:10:13 UTC
This is how you end up with phantom holds that can never be deleted.
Comment 2 Paul Poulain 2012-11-21 17:02:24 UTC
Nicole, would you agree if we placed a foreign key from reserves.itemnumber to items.itemnumber ON DELETE CASCADE.

It would mean deleting an item automatically delete the hold ?

I think you won't. What should be the expected behaviour ? don't delete the item and warn ?
Comment 3 Nicole C. Engard 2012-11-21 17:13:48 UTC
I think that would be okay (maybe with a warning that holds will be cancelled), but I would like others to provide feedback as well.
Comment 4 Christopher Brannon 2012-11-21 18:00:46 UTC
I think, what ever the various ideas people bring up here, there should always be options about handling these items.

First of all, when you process the batch deletion, if it is detecting items with hold, it should give the option to view the items in a new window.

Second, perhaps people should be given options to cancel the deletion, delete only items without holds, or maybe an option to replace the hold.  It could guide you through picking another item, changing to Next Available, or canceling the hold (with notification to the patron).

I think all of these options are possible actions.  Perhaps a default choice can be made in admin, but you can also choose before or during the deletion process as well.

That's my two cents.

Christopher Brannon
Comment 5 Nicole C. Engard 2012-11-21 19:50:15 UTC
Comment from a library:

"Right now, if an item is checked out, it can't be deleted. What if a similar trigger were added to the batch delete feature, where if the item to be deleted has a hold on it OR the bib has a hold and the last item is about to be deleted, the item can't be deleted, until the hold is removed?"
Comment 6 Nicole C. Engard 2012-11-21 19:50:59 UTC
Another library comment:

"Ideally, when deleting items in a batch, I’d like it if Koha would react similarly to the way it reacts when you try to delete a non-existent barcode in a batch of good barcodes.

In that case, when you scan a pile of stuff and one of the items has a non-cataloged barcode number, you get a yellow warning banner saying something like “Some barcodes not found” (I forget the exact text of the message) followed by a single column table listing all of the barcode numbers entered but not found in separate rows.

I think the best behavior for handling items with holds that are being deleted by a batch process would be to have a warning banner saying “Some Items Have Holds” followed by a table listing all of the barcodes that have holds on them.  And if those barcode numbers could link directly to the item records (possibly opening them in a new tab or new window), that would be even better."
Comment 7 Nicole C. Engard 2012-11-21 19:51:35 UTC
Another library comment:

"I think the deletion should be blocked with a message that there are still holds on some items. The librarian can then resolve the situation first, then delete."
Comment 8 Katrin Fischer 2012-11-24 18:39:08 UTC
I think a warning with a list of items linked to the records would be nice. 

Also I would like options to
- delete items and holds
- delete all items without holds or remove items with holds from the batch delete list

Would that make sense?
Comment 9 Nicole C. Engard 2012-11-25 01:09:59 UTC
Sounds like it covers all the bases.

Nicole
Comment 10 Nicole C. Engard 2012-11-25 13:35:34 UTC
One more library comment:

This has been a frustrating bug for us as well and we’d like to see it fixed.  Maybe a column that shows if there are holds on an item so we can uncheck any items that have the check mark so the Batch Item Delete tool doesn’t process those records but can complete the deletion of the remaining items in the batch.  At the very least the Batch Item Delete tool should skip over these items as it does other records that can’t be deleted and report the undeleted barcodes at the end of the process.  Thanks.
Comment 11 Nicole C. Engard 2012-11-27 12:51:58 UTC
Another email I received:

------

I would second the final comment on the ticket:

“Maybe a column that shows if there are holds on an item so we can uncheck any items that have the check mark so the Batch Item Delete tool doesn’t process those records but can complete the deletion of the remaining items in the batch.  At the very least the Batch Item Delete tool should skip over these items as it does other records that can’t be deleted and report the undeleted barcodes at the end of the process.”
Comment 12 Barton Chittenden 2014-05-20 22:05:06 UTC
This issue needs higher priority, this is not simply a tools issue; any time that all items are deleted, holds will be orphaned.

I have replicated this issue by placing a biblio level hold, then deleting all items from the 'normal' tab on detail.pl.

At that point, visiting reserve/request.pl?biblionumber=XXX gives the message

   "Cannot place hold: this record has no items attached."

regardless of the fact that the hold in question is a biblio level hold, and that there are items in the hold queue.
Comment 13 Katrin Fischer 2015-06-06 23:28:17 UTC
I think it works nicely for item-level holds. When you try to delete an item there, it reports correctly:

751 	barc 	Item has a waiting hold

The problem appears for record level holds.
Comment 14 Jonathan Druart 2015-07-10 11:33:59 UTC
What about merging the Batch item deletion tool with the Batch record deletion tool?
We could have

 Record type
    Bibliographic:
    Authorities:
    Items:

Not sure an item is a record...

Because at the moment, the batch item deletion and modification use the same script/template and it will be very hard to modify.
Moreover the batch item deletion display all the item info, which is not needed. We would prefer to see the number of holds and checkouts, as is it for the Batch record deletion tool.
Comment 15 Jonathan Druart 2016-05-11 13:23:07 UTC
(In reply to Jonathan Druart from comment #14)
> What about merging the Batch item deletion tool with the Batch record
> deletion tool?
> We could have
> 
>  Record type
>     Bibliographic:
>     Authorities:
>     Items:

Does it make sense?
Comment 16 Martin Renvoize 2019-02-11 15:39:39 UTC
Is this one still alive and kicking?
Comment 17 Jonathan Druart 2019-02-22 13:46:05 UTC
(In reply to Martin Renvoize from comment #16)
> Is this one still alive and kicking?

Answering a question with a question? :)
Comment 18 Katrin Fischer 2019-02-24 12:28:37 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > What about merging the Batch item deletion tool with the Batch record
> > deletion tool?
> > We could have
> > 
> >  Record type
> >     Bibliographic:
> >     Authorities:
> >     Items:
> 
> Does it make sense?

It could make sense, but I'd like to keep the separate permissions for items and records.
Comment 19 Katrin Fischer 2019-02-24 12:30:36 UTC
> Because at the moment, the batch item deletion and modification use the same
> script/template and it will be very hard to modify.
> Moreover the batch item deletion display all the item info, which is not
> needed. We would prefer to see the number of holds and checkouts, as is it
> for the Batch record deletion tool.

I think the display of all information for deleting is very helpful for confirming that you are actually deleting the right things. displaying holds and checkouts (would they even be displayed now?) would be good additions.
Comment 20 Liz Rea 2019-05-16 18:30:33 UTC
I don't like the idea of combining them. Records are records. Items are items. 

Yes, I'm a curmudgeon. ^.^

The way I'd like to see it done:

- use the existing tool for batch deleting items
- give feedback regarding the items that were actually deleted (existing)
- Check an item's biblio to see if that item is the last on the biblio - if it is, use the existing pathway for deleting records if all items are to be deleted (existing), and add on deleting the reserves on that biblio in that pathway.
- give actionable feedback regarding any items with item level holds that couldn't be deleted because of holds - either links to the affected items that open in a new tab so a librarian can deal to the holds or some other mechanism like a button "Delete this item's holds I know what I'm doing"

This could be a good project for an intern somewhere.

Liz
Comment 21 Jonathan Druart 2019-10-24 15:01:48 UTC
Trying to get back to this.

Are we supposed to silently delete an item with an item-level hold that is not found (W or T)?
Comment 22 Christopher Brannon 2019-10-24 15:11:05 UTC
(In reply to Jonathan Druart from comment #21)
> Trying to get back to this.
> 
> Are we supposed to silently delete an item with an item-level hold that is
> not found (W or T)?

In my opinion, staff or patron should be notified.  At least staff, because they could make a decision to move the hold to another record, order the item, or notify the patron that the item is no longer available.
Comment 23 Katrin Fischer 2019-11-03 23:14:01 UTC
(In reply to Christopher Brannon from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > Trying to get back to this.
> > 
> > Are we supposed to silently delete an item with an item-level hold that is
> > not found (W or T)?
> 
> In my opinion, staff or patron should be notified.  At least staff, because
> they could make a decision to move the hold to another record, order the
> item, or notify the patron that the item is no longer available.

I think a notification in the GUI would be good, Maybe at the top, sth like: Following items can't be deleted because they have holds on them?

What if it's a record level hold and it's the last item? Maybe we should just check for items in general.
Comment 24 Jonathan Druart 2019-12-03 10:11:39 UTC
Created attachment 95936 [details] [review]
Bug 8132: No changes but disable checkboxes

When the list of items is displayed we already know if there will be a
problem during the deletion. So let's disable the checkbox to tell the
user in advance that items cannot be deleted.
Comment 25 Jonathan Druart 2019-12-03 10:11:43 UTC
Created attachment 95937 [details] [review]
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion

If an item is the last one of a biblio that have biblio-level hold
placed on it, we should block the deletion.
It takes effect if the hold is found (W or T), to follow existing
behavior for item-level holds.
If we want to block deletion for any holds we should deal with it on a
separate bug report.

Test plan:
0/ Setup
Create Biblio B1 with 1 item
Create Biblio B2 with 2 items
Create Biblio B3 with 1+ item
Create Biblio B4 with 1+ item
Place a biblio-level hold on B1 and B2
Place an item-level hold on B3
Confirm the holds (to mark them waiting)

1/ Use the 5 items and delete them in a batch.
=> delete of item from B1 is blocked on first screen
=> delete of items from B2 is *not* blocked on first screen
=> delete of item from B3 is blocked on first screen
=> delete of item from B4 is *not* blocked

Note that you can only select items from B2 and B4

2/ Select them and confirm the deletion
=> Nothing happened and you get a message saying that one of the 2 items
from B2 is blocking the whole deletion process

3/ Remove the biblio-level hold
4/ Repeat 1
=> The deletion has been effective!
Comment 26 Jonathan Druart 2019-12-03 10:11:46 UTC
Created attachment 95938 [details] [review]
Bug 8132: Delete the items in a transaction and rollback if something wrong

The last_item_for_hold case cannot be guessed (easily), and so we are going to
delete the items in a transaction. If something wrong happened we
rollback and display a list of items that caused the rollback.
Comment 27 Jonathan Druart 2019-12-03 10:11:50 UTC
Created attachment 95939 [details] [review]
Bug 8132: Adding message when deleting from the UI
Comment 28 Jonathan Druart 2019-12-03 10:13:03 UTC
Here is a try to fix this long standing issue. It is based on top of bug 23463.
A remote branch is available for testing on my gitlab repop: https://gitlab.com/joubu/Koha/commits/bug_8132
Comment 29 Kelly McElligott 2019-12-04 18:40:17 UTC
I did not see the message that is indicated in step 2. " Nothing happened and you get a message saying that one of the 2 items
from B2 is blocking the whole deletion process" . 

This would be an extremely helpful message.  I received a message that 2 items were deleted.  Yes, they were the correct items to be deleted and left the item with the hold along.  I am not sure with this test plan, if this message that you note in step 2 should be see.  

See my screenshot.

Kelly
Comment 30 Kelly McElligott 2019-12-04 18:40:49 UTC
Created attachment 95997 [details]
delete message
Comment 31 Jonathan Druart 2019-12-10 11:41:15 UTC
(In reply to Kelly McElligott from comment #29)
> I did not see the message that is indicated in step 2. " Nothing happened
> and you get a message saying that one of the 2 items
> from B2 is blocking the whole deletion process" . 
> 
> This would be an extremely helpful message.  I received a message that 2
> items were deleted.  Yes, they were the correct items to be deleted and left
> the item with the hold along.  I am not sure with this test plan, if this
> message that you note in step 2 should be see.  
> 
> See my screenshot.
> 
> Kelly

Hi Kelly, thanks for testing.
Can you confirm the holds were "found"?
So far we prevent deletion if reserves.found is W or T.
Comment 32 Jonathan Druart 2019-12-10 12:10:09 UTC
Update test plan:

> 0/ Setup
> Create Biblio B1 with 1 item
> Create Biblio B2 with 2 items
> Create Biblio B3 with 1+ item
> Create Biblio B4 with 1+ item
> Place a biblio-level hold on B1 and B2
> Place an item-level hold on B3
> Confirm the holds (to mark them waiting)

Read: Confirm the item-level hold, the one placed on B3 only.

> 1/ Use the 5 items and delete them in a batch.
> => delete of item from B1 is blocked on first screen
> => delete of items from B2 is *not* blocked on first screen
> => delete of item from B3 is blocked on first screen
> => delete of item from B4 is *not* blocked
> 
> Note that you can only select items from B2 and B4
> 
> 2/ Select them and confirm the deletion
> => Nothing happened and you get a message saying that one of the 2 items
> from B2 is blocking the whole deletion process
> 
> 3/ Remove the biblio-level hold
> 4/ Repeat 1
> => The deletion has been effective!
Comment 33 Jonathan Druart 2019-12-10 12:18:14 UTC
(In reply to Kelly McElligott from comment #29)
> I did not see the message that is indicated in step 2. " Nothing happened
> and you get a message saying that one of the 2 items
> from B2 is blocking the whole deletion process" . 
> 
> This would be an extremely helpful message.  I received a message that 2
> items were deleted.  Yes, they were the correct items to be deleted and left
> the item with the hold along.  I am not sure with this test plan, if this
> message that you note in step 2 should be see.  
> 
> See my screenshot.
> 
> Kelly

There is something wrong indeed, working on it!
Comment 34 Jonathan Druart 2019-12-10 12:32:42 UTC
Created attachment 96147 [details] [review]
Bug 8132: No changes but disable checkboxes

When the list of items is displayed we already know if there will be a
problem during the deletion. So let's disable the checkbox to tell the
user in advance that items cannot be deleted.
Comment 35 Jonathan Druart 2019-12-10 12:32:46 UTC
Created attachment 96148 [details] [review]
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion

If an item is the last one of a biblio that have biblio-level hold
placed on it, we should block the deletion.
It takes effect if the hold is found (W or T), to follow existing
behavior for item-level holds.
If we want to block deletion for any holds we should deal with it on a
separate bug report.

Test plan:
0/ Setup
Create Biblio B1 with 1 item
Create Biblio B2 with 2 items
Create Biblio B3 with 1+ item
Create Biblio B4 with 1+ item
Create Biblio B5 with 1+ item
Place a biblio-level hold on B1 and B2
Place an item-level hold on B3 and B4
Confirm the item-level hold for the items of B3 to mark it waiting.

1/ Delete those 6 items in a batch
=> delete of item from B1 is blocked on first screen - only 1 item left
and there is a biblio-level hold on the record
=> delete of items from B2 is *not* blocked on first screen - One of
them will block the deletion, but so far we are not aware of that
situation
=> delete of item from B3 is blocked on first screen - there is a
waiting item-level hold placed on the item
=> delete of item from B4 is *not* blocked - there is a hold but it is
not found
=> delete of item from B5 is *not* - there is no reason to block its
deletion

Note that you can only select items from B2, B4 and B5

2/ Select them and confirm the deletion
=> Nothing happened and you get a message saying that one of the 2 items
from B2 is blocking the whole deletion process

3/ Remove the biblio-level hold
4/ Repeat 1
=> The deletion has been effective!

=> Note that there is something a bit weird as we are blocking items
from a biblio that has biblio-level holds on it (not found), but we
do not blocking the deletion of an item with a waiting item-level hold
Comment 36 Jonathan Druart 2019-12-10 12:32:50 UTC
Created attachment 96149 [details] [review]
Bug 8132: Delete the items in a transaction and rollback if something wrong

The last_item_for_hold case cannot be guessed (easily), and so we are going to
delete the items in a transaction. If something wrong happened we
rollback and display a list of items that caused the rollback.
Comment 37 Jonathan Druart 2019-12-10 12:32:53 UTC
Created attachment 96150 [details] [review]
Bug 8132: Adding message when deleting from the UI
Comment 38 Jonathan Druart 2019-12-10 12:33:53 UTC
I have adjusted the test plan and updated the patch (remote branch is still up-to-date).
Comment 39 Kelly McElligott 2019-12-10 16:06:35 UTC
Jonathan,
Two issues that I have with this:

In this step:

2/ Select them and confirm the deletion
=> Nothing happened and you get a message saying that one of the 2 items
from B2 is blocking the whole deletion process

This does block the whole process!  So all the item that could be deleted - don't get deleted, the button to return to Batch Item Deletion, results in having to re-enter all the items that could have been deleted but weren't because of 1 item.   Could the return to deletion button bring back the screen prior to this.  This is just a lot of extra work.

In this step:
3/ Remove the biblio-level hold
4/ Repeat 1
=> The deletion has been effective!

Koha is deleting both items on Bib4 - one of which had an item level hold (not triggered).  The hold does not get orphaned, because it too is deleted.  Is this what you would like to see happen, a library can delete an item with an item level hold if it isn't triggered?
Comment 40 Jonathan Druart 2019-12-10 16:26:39 UTC
(In reply to Kelly McElligott from comment #39)
> Jonathan,
> Two issues that I have with this:
> 
> In this step:
> 
> 2/ Select them and confirm the deletion
> => Nothing happened and you get a message saying that one of the 2 items
> from B2 is blocking the whole deletion process
> 
> This does block the whole process!  So all the item that could be deleted -
> don't get deleted, the button to return to Batch Item Deletion, results in
> having to re-enter all the items that could have been deleted but weren't
> because of 1 item.   Could the return to deletion button bring back the
> screen prior to this.  This is just a lot of extra work.

I will see what I can do. But yes, I do expect the whole process to be blocked.
If something is wrong, the whole transaction is rolled back.

> In this step:
> 3/ Remove the biblio-level hold
> 4/ Repeat 1
> => The deletion has been effective!
> 
> Koha is deleting both items on Bib4 - one of which had an item level hold
> (not triggered).  The hold does not get orphaned, because it too is deleted.
> Is this what you would like to see happen, a library can delete an item with
> an item level hold if it isn't triggered?

It is how it works so far. That's why I added a note in the commit message about this behavior.
The hold has to be found (waiting or in transfer) to see the deletion blocked. I guess it's how it works as well in the cataloguing module.
It's not a behavior's change I want to introduce with this patchset, if we decide to modify it we should do it on its own bug report.
Comment 41 Jonathan Druart 2019-12-11 12:29:39 UTC
(In reply to Kelly McElligott from comment #39)
> Jonathan,
> Two issues that I have with this:
> 
> In this step:
> 
> 2/ Select them and confirm the deletion
> => Nothing happened and you get a message saying that one of the 2 items
> from B2 is blocking the whole deletion process
> 
> This does block the whole process!  So all the item that could be deleted -
> don't get deleted, the button to return to Batch Item Deletion, results in
> having to re-enter all the items that could have been deleted but weren't
> because of 1 item.   Could the return to deletion button bring back the
> screen prior to this.  This is just a lot of extra work.

Browser back button will do the job correctly, it will even keep the selection.
Isn't it enough?
Comment 42 Kelly McElligott 2019-12-12 12:48:32 UTC
Let me test again, using the back button did not save my barcodes in the Batch Item Deletion tool.  I will attempt this again.

I understand what you mean with the non-triggered holds and agree that this would be different than other Koha procedures.