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.
This is how you end up with phantom holds that can never be deleted.
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 ?
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.
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 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?"
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."
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."
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?
Sounds like it covers all the bases. Nicole
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.
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.”
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.
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.
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.
(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?
Is this one still alive and kicking?
(In reply to Martin Renvoize from comment #16) > Is this one still alive and kicking? Answering a question with a question? :)
(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.
> 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.
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
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 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.
(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.
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.
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!
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.
Created attachment 95939 [details] [review] Bug 8132: Adding message when deleting from the UI
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
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
Created attachment 95997 [details] delete message
(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.
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!
(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!
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.
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
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.
Created attachment 96150 [details] [review] Bug 8132: Adding message when deleting from the UI
I have adjusted the test plan and updated the patch (remote branch is still up-to-date).
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?
(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.
(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?
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.
Still testing this Kelly?
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!
(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.
Looks promising, but needs a rebase I'm afraid.
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.
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
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.
Created attachment 98697 [details] [review] Bug 8132: Adding message when deleting from the UI
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
(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.
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.
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
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.
Created attachment 101407 [details] [review] Bug 8132: Adding message when deleting from the UI
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
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.
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.
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).
Switching back to NSO. Thanks for the feedback, Sonia and Kelly!
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?
(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.
> => 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.
(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.
> "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.
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>
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>
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>
Created attachment 103150 [details] [review] Bug 8132: Adding message when deleting from the UI Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
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>
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>
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>
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>
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
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 %])
QAing
Bit weird that sub safe_to_delete returns 1 or a string.
"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?
(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.
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...)
(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.
(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.
(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.
(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.
Resetting status, but not continuing here
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 ?
(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 :)
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>
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>
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>
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>
Created attachment 104571 [details] [review] Bug 8132: (QA follow-up) Missing filter Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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..
Nice work everyone! Pushed to master for 20.05
Created attachment 104682 [details] [review] Bug 8132: Fix failing tests * Add missing html filter (xt/find-missing-filters.t) * Fix AddReserve call
not backported to 19.11 - missing dependencies