Bug 8132 - Batch delete tool deletes items with holds on them
Summary: Batch delete tool deletes items with holds on them
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P4 major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23463
Blocks: 8149
  Show dependency treegraph
 
Reported: 2012-05-21 15:33 UTC by Nicole C. Engard
Modified: 2020-05-18 09:49 UTC (History)
18 users (show)

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


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
Bug 8132: No changes but disable checkboxes (3.84 KB, patch)
2020-02-11 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (3.69 KB, patch)
2020-02-11 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (15.87 KB, patch)
2020-02-11 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.54 KB, patch)
2020-02-11 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: No changes but disable checkboxes (3.83 KB, patch)
2020-03-23 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (3.69 KB, patch)
2020-03-23 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (15.85 KB, patch)
2020-03-23 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.54 KB, patch)
2020-03-23 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8132: No changes but disable checkboxes (3.91 KB, patch)
2020-04-17 12:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (3.75 KB, patch)
2020-04-17 12:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (15.93 KB, patch)
2020-04-17 12:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.60 KB, patch)
2020-04-17 12:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 8132: No changes but disable checkboxes (3.95 KB, patch)
2020-04-17 14:58 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (3.83 KB, patch)
2020-04-17 14:58 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (15.97 KB, patch)
2020-04-17 14:58 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.66 KB, patch)
2020-04-17 14:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 8132: No changes but disable checkboxes (4.06 KB, patch)
2020-05-08 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8132: Adding a new message 'last_item_for_hold' blocking item deletion (3.92 KB, patch)
2020-05-08 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8132: Delete the items in a transaction and rollback if something wrong (16.08 KB, patch)
2020-05-08 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8132: Adding message when deleting from the UI (2.76 KB, patch)
2020-05-08 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8132: (QA follow-up) Missing filter (1.42 KB, patch)
2020-05-08 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8132: Fix failing tests (2.02 KB, patch)
2020-05-11 10:45 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.
Comment 43 Martin Renvoize 2020-01-27 19:11:43 UTC
Still testing this Kelly?
Comment 44 Kelly McElligott 2020-01-29 15:50:16 UTC
Hi Martin,
Attempted to sign off on this bug Martin, 
Looks like something in the Bug that this is dependant (23463) on has changed, I received this error while testing:


Template process failed: undef error - The method Koha::Item->safe_to_delete is not covered by tests!
Comment 45 Jonathan Druart 2020-01-29 17:01:14 UTC
(In reply to Kelly McElligott from comment #44)
> Hi Martin,
> Attempted to sign off on this bug Martin, 
> Looks like something in the Bug that this is dependant (23463) on has
> changed, I received this error while testing:
> 
> 
> Template process failed: undef error - The method Koha::Item->safe_to_delete
> is not covered by tests!

Hi Kelly, are you able to use the remote branch? You should be able to specify it if you are using the sandboxes.
The problem is that the patches from bug 23463 are not attached to the bug report (but also on a remote branch).

The remote branch https://gitlab.com/joubu/Koha/commits/bug_8132 contains all the necessary commits.
Comment 46 Martin Renvoize 2020-02-10 15:47:18 UTC
Looks promising, but needs a rebase I'm afraid.
Comment 47 Jonathan Druart 2020-02-11 09:33:26 UTC
Created attachment 98694 [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 48 Jonathan Druart 2020-02-11 09:33:31 UTC
Created attachment 98695 [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 49 Jonathan Druart 2020-02-11 09:33:36 UTC
Created attachment 98696 [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 50 Jonathan Druart 2020-02-11 09:33:40 UTC
Created attachment 98697 [details] [review]
Bug 8132: Adding message when deleting from the UI
Comment 51 David Nind 2020-03-15 18:12:54 UTC
Patch no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8132: No changes but disable checkboxes
error: sha1 information is lacking or useless (tools/batchMod.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 8132: No changes but disable checkboxes
Comment 52 Jonathan Druart 2020-03-16 11:23:05 UTC
(In reply to David Nind from comment #51)
> Patch no longer applies:
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 8132: No changes but disable checkboxes
> error: sha1 information is lacking or useless (tools/batchMod.pl).
> error: could not build fake ancestor
> Patch failed at 0001 Bug 8132: No changes but disable checkboxes

It depends on bug 23463. It's FQA but you can still test this patchset if you like.
Comment 53 Jonathan Druart 2020-03-23 09:46:40 UTC
Created attachment 101404 [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 54 Jonathan Druart 2020-03-23 09:46:45 UTC
Created attachment 101405 [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 55 Jonathan Druart 2020-03-23 09:46:54 UTC
Created attachment 101406 [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 56 Jonathan Druart 2020-03-23 09:47:00 UTC
Created attachment 101407 [details] [review]
Bug 8132: Adding message when deleting from the UI
Comment 57 Koha Team University Lyon 3 2020-03-24 19:26:20 UTC
Hi,
I've tested following the test plan and I added a case when a items is in transit (T status in reserves table) for an hold.
When you are in the first table in the deletion items tool, there's the column called "holds" where you can see :
1/1 : if there's an item-level-hold on this item OR if the item is waiting to serve a hold OR the item is in transit to serve a hold
0/1 : if there's a biblio-level-hold but that this item isn't waiting or in transit for this hold
0 : if there's no hold

It's not a really clear notation, but I have nothing better to offer..

In this table, you can't tick nor the items with the waiting or transit status nor the one where is a biblio-level-hold but only one item on this biblio.

When you confirm the deletion, you have an alert for the case of the biblio-level-hold with 2 items that were selected for deletion : "Last item for bibliographic record wich biblio-level hold on it" and another message saying "At least one item blocked the deletion. The operation rolled back and nothing happened! "
So the deletion is cancelled for all the items selected.

It works as described in the test plan but, for me, it isn't consistent to block the deletion of an item because there's a biblio-level-hold on it but not block the deletion of an item with a item-level-hold on it.

Could we not prevent to tick all the 1/1 case in the first table in addition of what exists already?

I must admit that I haven't read all the comments above but I can't see a good reason to allow deletion of item with an item-level-hold if you don't allow the deletion of a biblio-level-hold.

I hope that my explanations are clear..

Sonia BOUIS
Comment 58 Jonathan Druart 2020-03-25 14:31:49 UTC
Thanks for testing Sonia!

What this change does basically is:
* Improve feedback to the end user: why this item cannot be deleted
* Block deletion of the last item for a biblio on which a hold is placed on, to avoid phantom holds (original issue reported)

(In reply to Koha Team University Lyon 3 from comment #57)
> When you confirm the deletion, you have an alert for the case of the
> biblio-level-hold with 2 items that were selected for deletion : "Last item
> for bibliographic record wich biblio-level hold on it" and another message
> saying "At least one item blocked the deletion. The operation rolled back
> and nothing happened! "
> So the deletion is cancelled for all the items selected.

Yes, the problem is that we cannot (easily) know that we are going to remove the last item on the biblio if we are going to remove several items from this biblio. We only know it when we remove the last one. That is why everything is rollbacked and no deletion was effective.

> Could we not prevent to tick all the 1/1 case in the first table in addition of > what exists already?

If we want to change the behavior on item's level holds, I think it should be decided on a separate bug report, unless everybody agrees.
Comment 59 Kelly McElligott 2020-03-27 13:55:10 UTC
I agree with Sonia.  Although yes the scope of the bug is to 

 Block deletion of the last item for a Biblio on which a hold is placed on, to avoid phantom holds (original issue reported)


I would say we would want the same behavior on item level holds. Is this another bug or the grand scope of batch item deletion? 

If I have 1 item level hold on a bib with 2 items, Koha will let me delete both items with no problem. Koha should stop this behavior, in my opinion as well. If Koha knows when I am deleting all the items off a record, shouldn't that be able to stop it?

That being said, the test plan does work as expected.
Comment 60 Jonathan Druart 2020-03-27 15:18:27 UTC
This patch is an enhancement in the sense that it displays useful info about "deletion impossible", but can be considered as a bugfix (and so backported) as it will prevent phantom holds.

I would deal with a behavior change on a separate bug report. IMO it's not obvious that everybody wants to block items (that have holds) deletion (there is an alert already to tell that holds will be deletion along with the item).
Comment 61 Jonathan Druart 2020-03-27 15:19:05 UTC
Switching back to NSO.

Thanks for the feedback, Sonia and Kelly!
Comment 62 Victor Grousset/tuxayo 2020-04-16 07:33:10 UTC
Trying to signoff.

> Create Biblio B3 with 1+ item

Does this mean that B3 *must* have more than on item?

IIUC, the patch must be applied from the start, right?
Comment 63 Jonathan Druart 2020-04-16 08:19:19 UTC
(In reply to Victor Grousset/tuxayo from comment #62)
> Trying to signoff.
> 
> > Create Biblio B3 with 1+ item
> 
> Does this mean that B3 *must* have more than on item?

At least one item.

> IIUC, the patch must be applied from the start, right?

Apply all the patches to test, yes.
Comment 64 Victor Grousset/tuxayo 2020-04-16 19:13:48 UTC
> => delete of item from B4 is *not* blocked - there is a hold but it is not found

I don't get it. No sign of something not being found.

I have this: https://pic.infini.fr/RLxIM2cL/eYdtLmXE.png (temporary hosting for 90 days)

> 3/ Remove the biblio-level hold

The biblio-level hold on B2 right?

I removed also the biblio-level hold on B1.

> 4/ Repeat 1
> => The deletion has been effective!

Is that the expected result?

https://pic.infini.fr/WaQVV1Gg/rTGE1oG3.png (temporary hosting for 90 days)

I guess that's not an issue. But I still have a item I can't delete.


I'm not comfortable with the steps so I might have misinterpreted few things.
Comment 65 Jonathan Druart 2020-04-17 09:11:16 UTC
(In reply to Victor Grousset/tuxayo from comment #64)
> > => delete of item from B4 is *not* blocked - there is a hold but it is not found
> 
> I don't get it. No sign of something not being found.
> 
> I have this: https://pic.infini.fr/RLxIM2cL/eYdtLmXE.png (temporary hosting
> for 90 days)

So it's not blocked, which is expected.

> > 4/ Repeat 1
> > => The deletion has been effective!
> 
> Is that the expected result?
> 
> https://pic.infini.fr/WaQVV1Gg/rTGE1oG3.png (temporary hosting for 90 days)

"Has been effective", so you should be on the next screen, not the confirmation screen.

> I guess that's not an issue. But I still have a item I can't delete.

If the reason makes sense, yes it's expected. That's the point of the patchset.
Comment 66 Victor Grousset/tuxayo 2020-04-17 11:04:06 UTC
> "Has been effective", so you should be on the next screen, not the confirmation screen.

Okay, indeed after confirmation the deletion works.

>> I guess that's not an issue. But I still have a item I can't delete.

> If the reason makes sense, yes it's expected. That's the point of the patchset.

Indeed, the item-level hold is the cause.


From what I understand, no issue found, so I will sign it off. Later in the day I think.
Comment 67 ByWater Sandboxes 2020-04-17 12:34:05 UTC
Created attachment 103147 [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.

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 68 ByWater Sandboxes 2020-04-17 12:34:09 UTC
Created attachment 103148 [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

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 69 ByWater Sandboxes 2020-04-17 12:34:12 UTC
Created attachment 103149 [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.

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 70 ByWater Sandboxes 2020-04-17 12:34:16 UTC
Created attachment 103150 [details] [review]
Bug 8132: Adding message when deleting from the UI

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Comment 71 Victor Grousset/tuxayo 2020-04-17 14:58:47 UTC
Created attachment 103166 [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.

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 72 Victor Grousset/tuxayo 2020-04-17 14:58:51 UTC
Created attachment 103167 [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
Apply the patches
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 from B2
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

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 73 Victor Grousset/tuxayo 2020-04-17 14:58:56 UTC
Created attachment 103168 [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.

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 74 Victor Grousset/tuxayo 2020-04-17 14:59:01 UTC
Created attachment 103169 [details] [review]
Bug 8132: Adding message when deleting from the UI

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 75 Victor Grousset/tuxayo 2020-04-17 15:00:58 UTC
Thanks Kelly for the double check, I wasn't confident about how holds should work.

So, 2nd sign off and minor details added to the test plan:
- about apply all the patches at the beginning
- about that the biblio-level hold to remove is on b2
Comment 76 Marcel de Rooy 2020-05-01 08:43:29 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-del.tt
   FAIL   filters
                missing_filter at line 127 (                        <td><input type="checkbox" name="itemnumber" value="[% item_loo.itemnumber | html %]" id="row[% item_loo.itemnumber | html %]" disabled="disabled" title="[% cannot_delete_reason %]"/></td>)
                missing_filter at line 261 (                    [% cannot_delete_reason %])
Comment 77 Marcel de Rooy 2020-05-01 08:44:43 UTC
QAing
Comment 78 Marcel de Rooy 2020-05-01 09:05:31 UTC
Bit weird that sub safe_to_delete returns 1 or a string.
Comment 79 Marcel de Rooy 2020-05-01 09:21:58 UTC
"If an item is the last one of a biblio that have biblio-level hold
placed on it, we should block the deletion."

Well, this is quite arguable. Imo probably cleanup_database could remove holds that make no sense like biblio level holds when there are no items.
The same commit says:
"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."

Lets not push inconsistency but resolve it?
Comment 80 Marcel de Rooy 2020-05-01 09:37:28 UTC
(In reply to Marcel de Rooy from comment #79)
> "If an item is the last one of a biblio that have biblio-level hold
> placed on it, we should block the deletion."
> 
> Well, this is quite arguable. Imo probably cleanup_database could remove
> holds that make no sense like biblio level holds when there are no items.
> The same commit says:
> "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."
> 
> Lets not push inconsistency but resolve it?

Additional thought:

Note that the user also has the choice to delete a biblio. If he wants to delete the biblio, the biblio level hold will be deleted too. So you could say that a choice to not delete the biblio, is a choice to keep the hold. Perhaps new items are to be expected here and the hold can be filled later. So delete the item.

If you choose delete biblio too, we should not block the deletion of the hold.
Comment 81 Jonathan Druart 2020-05-01 10:16:06 UTC
Hi Marcel, 
Thanks for taking a look at this.

(In reply to Marcel de Rooy from comment #79)
> "If an item is the last one of a biblio that have biblio-level hold
> placed on it, we should block the deletion."
> 
> Well, this is quite arguable. Imo probably cleanup_database could remove
> holds that make no sense like biblio level holds when there are no items.
> The same commit says:
> "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."
> 
> Lets not push inconsistency but resolve it?

What do you suggest?

I'd like to repeat that this bug is about preventing phantom holds, nothing else. I don't think we should fix other inconsistencies on this bug report (it has been considered critical during years...)
Comment 82 Marcel de Rooy 2020-05-01 11:32:12 UTC
(In reply to Jonathan Druart from comment #81)
> What do you suggest?

Maybe we should stick to the scope that the title suggests. Do not delete items with holds. Biblio level holds are out of scope.
Comment 83 Jonathan Druart 2020-05-01 12:12:41 UTC
(In reply to Marcel de Rooy from comment #82)
> (In reply to Jonathan Druart from comment #81)
> > What do you suggest?
> 
> Maybe we should stick to the scope that the title suggests. Do not delete
> items with holds. Biblio level holds are out of scope.

No, it's the main point of this bug report.

We want to prevent deletion of the last item of a bibliographic record with a biblio-level hold on it.
Comment 84 Marcel de Rooy 2020-05-01 12:16:32 UTC
(In reply to Jonathan Druart from comment #83)
> (In reply to Marcel de Rooy from comment #82)
> > (In reply to Jonathan Druart from comment #81)
> > > What do you suggest?
> > 
> > Maybe we should stick to the scope that the title suggests. Do not delete
> > items with holds. Biblio level holds are out of scope.
> 
> No, it's the main point of this bug report.
> 
> We want to prevent deletion of the last item of a bibliographic record with
> a biblio-level hold on it.

Sorry, I can't agree on that reading comments and patches.
Comment 85 Jonathan Druart 2020-05-01 12:26:04 UTC
(In reply to Marcel de Rooy from comment #84)
> (In reply to Jonathan Druart from comment #83)
> > (In reply to Marcel de Rooy from comment #82)
> > > (In reply to Jonathan Druart from comment #81)
> > > > What do you suggest?
> > > 
> > > Maybe we should stick to the scope that the title suggests. Do not delete
> > > items with holds. Biblio level holds are out of scope.
> > 
> > No, it's the main point of this bug report.
> > 
> > We want to prevent deletion of the last item of a bibliographic record with
> > a biblio-level hold on it.
> 
> Sorry, I can't agree on that reading comments and patches.

See comment 0:
"""
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.
"""
Then comment 1:
"""
This is how you end up with phantom holds that can never be deleted.
"""
And comment 20.

I can confirm that I wrote those patches to prevent such deletion, and so phantom holds.

That's why I rebuilt a bit the interface to display the why an item cannot be deleted, and so introduced the new "last_item_for_hold" error code.
Comment 86 Marcel de Rooy 2020-05-04 06:12:47 UTC
Resetting status, but not continuing here
Comment 87 Marcel de Rooy 2020-05-08 08:38:55 UTC
The second patch here actually says that it is not 'safe to delete' the only remaining item where the biblio has a biblio level hold.
I do not agree with that statement. The item can perfectly be deleted. The biblio level hold may be filled by a future item and can be removed too. This might be a preferential thing.

The critical or major aspect of this bug imo is that the batch delete tool still deletes an item with an ITEM level hold. Note btw that the remark about phantom holds in comment1 is not valid in this case (or at least any more). The item delete cascades to reserves. And a biblio delete cascades to reserves too. So we have no phantom holds. Might depend on the definition of 'phantom'..

And even to discuss: What if we said that an item delete should convert an item level hold to biblio level instead of deleting the hold ?
Comment 88 Jonathan Druart 2020-05-08 09:19:53 UTC
(In reply to Marcel de Rooy from comment #87)

Again, the only things this change does is:
* Add feedback on the interface about items that cannot be deleted (existing behaviour with the 4 error codes: book_on_loan, not_same_branch, book_reserved, linked_analytics). Note that "book_reserved" means that an item with item-level hold on it that is W or T cannot be deleted (*existing behaviour*)

* Prevent phantom holds that are created when the last item of a bibliographic record that with a [biblio-level] hold on it is deleted. In that case we forbid the deletion.

Nothing else.

It is the implementation of the need described several times in this bug report and this has been waited for a long time now.

I should not that, in master, you are not allowed to "delete all items" for the bibliographic record: "1 hold(s) on this record. You must delete all holds before deleting all items." Individual deletion is allowed however.

If I misunderstood the need or if the implementation does not answer it, then feel free to obsolete the patches and provide another approach. But I think it is what we want here.

If this is considered an improvement to the existing behaviour, but not perfect, then it should be pushed and improved later (your "And even to discuss" part is far behind what we are doing here).

It would be good to get opinions from other contributors as it seems we are stuck :)
Comment 89 Marcel de Rooy 2020-05-08 09:42:09 UTC
Created attachment 104567 [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.

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 90 Marcel de Rooy 2020-05-08 09:42:15 UTC
Created attachment 104568 [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
Apply the patches
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 from B2
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

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 91 Marcel de Rooy 2020-05-08 09:42:22 UTC
Created attachment 104569 [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.

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 92 Marcel de Rooy 2020-05-08 09:42:28 UTC
Created attachment 104570 [details] [review]
Bug 8132: Adding message when deleting from the UI

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 93 Marcel de Rooy 2020-05-08 09:42:33 UTC
Created attachment 104571 [details] [review]
Bug 8132: (QA follow-up) Missing filter

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 94 Marcel de Rooy 2020-05-08 09:43:59 UTC
Okay, last chance for string freeze.
@RM: As my latest comments indicate, I am not completely enthusiastic about aspects of this patch set. But no definitive blocker on the other hand..
Comment 95 Martin Renvoize 2020-05-11 09:00:53 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 96 Jonathan Druart 2020-05-11 10:45:45 UTC
Created attachment 104682 [details] [review]
Bug 8132: Fix failing tests

* Add missing html filter (xt/find-missing-filters.t)
* Fix AddReserve call
Comment 97 Martin Renvoize 2020-05-11 10:51:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 98 Joy Nelson 2020-05-11 22:34:17 UTC
not backported to 19.11 - missing dependencies