When a biblio with a linked order gets deleted you can not cancel the order. There are two methods to delete a biblio: - Delete record in the edit menu - A check box when deleting all items using the batch deletion (see test plan). The first method generates a warning message and the second method deletes with out warnings. There should be warnings in both cases. Test plan: 1. Find a biblio with a linked order and at least one item. 2. Mark all items and press "Delete selected items". 3. At the Batch deletion page mark "Delete records if no items remain". 4. The items and the biblio will be removed without warnings.
Created attachment 118025 [details] [review] Bug 27893: Display the number of orders on the batch record deletion
I've started with the batch record deletion tool. About the batch item deletion tool I am not sure how to process. Would it be correct to forbid the deletion in this case (in Koha::Item->safe_to_delete)? It is not trivial to add a confirmation screen on this tool.
It would be better to forbid the deletion than not to be warned at all.
I think blocking the deletion entirely is preferable to a confirmation box. Why warn the user that their intended action will make a mess that can only be corrected via a database update and then let them do it anyway? They should be instructed to go cancel the order(s) in acq.
*** Bug 16872 has been marked as a duplicate of this bug. ***
Blocking the deletion might no longer be the desirable or necessary action here. We still need to resolve bug 10869, but with bug 33262 now pushed, we have much better means to fix any remaining issues with deleted records in the acq module. Should we try and move this back to a warning and NSO?
(In reply to Emma Ternrud from comment #0) > When a biblio with a linked order gets deleted you can not cancel the order. > > There are two methods to delete a biblio: > - Delete record in the edit menu > - A check box when deleting all items using the batch deletion (see test > plan). > > The first method generates a warning message and the second method deletes > with out warnings. > > There should be warnings in both cases. I think that we should definitely not delete biblio records if we have attached order lines that are new, ordered or partial. If there are, warn and do not delete. The user should cancel these lines and only then proceed to delete the biblio record.
(In reply to Marcel de Rooy from comment #7) > I think that we should definitely not delete biblio records if we have > attached order lines that are new, ordered or partial. > If there are, warn and do not delete. The user should cancel these lines and > only then proceed to delete the biblio record. This discussion is going on over various related reports. Since the user can delete a whole basket incl biblios, i cannot stick to definitely not delete as above. But we should not just delete a biblio, and leave acquisition in a troubled state. So confirming a delete in the interface should cancel attached order lines. Also note that 18360 is about deleting cancelled order lines. Changing title to express this direction.
Looking if I can get some new code here.
Created attachment 162271 [details] [review] Bug 27893: Cancel acq orders in addbiblio.pl When the user confirms removal although we have linked orders, we should cancel them. Test plan: Add order line to a basket. Goto newly added biblio record, remove items, remove biblio. Notice the warning about attached order lines. Confirm. Check the basket again after deletion took place (cancelled line). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 162272 [details] [review] Bug 27893: Optionally skip biblio with open orders in batch delete If the user unchecks the skip checkbox (on by default), we must make sure that order cancellation is done in background job. Test plan: Pick two biblios. One has linked open orders, the other not. Go to batch delete records. Select 'Enter list of numbers'. Enter both biblio numbers and check that only one is used on the follow-up form. Run the deletion without the skip open orders and verify that linked order line is cancelled. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hope this can start moving again. A simple approach for batch delete which can be extended later on if really needed. When we decide to delete, we now make sure that orders are cancelled.
The patch no longer applies (also has whitespace errors): Bug 27893 Depends on bug 36068 (Needs Signoff) Follow? [(y)es, (n)o] y Bug 36068 - Add maintenance script acq_cancel_obsolete_orders.pl 162261 - Bug 36068: Add maint script acq_cancel_obsolete_orders.pl 162262 - Bug 36068: Do not overwrite cancellation date in ->cancel Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 36068: Add maint script acq_cancel_obsolete_orders.pl Applying: Bug 36068: Do not overwrite cancellation date in ->cancel Bug 27893 - Delete of biblio should warn about attached acq orders and cancel them 162271 - Bug 27893: Cancel acq orders in addbiblio.pl 162272 - Bug 27893: Optionally skip biblio with open orders in batch delete Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 27893: Cancel acq orders in addbiblio.pl .git/rebase-apply/patch:59: trailing whitespace. warning: 1 line adds whitespace errors. Using index info to reconstruct a base tree... M cataloguing/addbiblio.pl .git/rebase-apply/patch:59: trailing whitespace. warning: 1 line adds whitespace errors. Falling back to patching base and 3-way merge... Auto-merging cataloguing/addbiblio.pl CONFLICT (content): Merge conflict in cataloguing/addbiblio.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 27893: Cancel acq orders in addbiblio.pl
Created attachment 163669 [details] [review] Bug 27893: Cancel acq orders in addbiblio.pl When the user confirms removal although we have linked orders, we should cancel them. Test plan: Add order line to a basket. Goto newly added biblio record, remove items, remove biblio. Notice the warning about attached order lines. Confirm. Check the basket again after deletion took place (cancelled line). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 163670 [details] [review] Bug 27893: Optionally skip biblio with open orders in batch delete If the user unchecks the skip checkbox (on by default), we must make sure that order cancellation is done in background job. Test plan: Pick two biblios. One has linked open orders, the other not. Go to batch delete records. Select 'Enter list of numbers'. Enter both biblio numbers and check that only one is used on the follow-up form. Run the deletion without the skip open orders and verify that linked order line is cancelled. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to David Nind from comment #13) > The patch no longer applies (also has whitespace errors): > > Bug 27893 Depends on bug 36068 (Needs Signoff) > Follow? [(y)es, (n)o] y > > Bug 36068 - Add maintenance script acq_cancel_obsolete_orders.pl > > 162261 - Bug 36068: Add maint script acq_cancel_obsolete_orders.pl > 162262 - Bug 36068: Do not overwrite cancellation date in ->cancel > > Apply? [(y)es, (n)o, (i)nteractive] y > Applying: Bug 36068: Add maint script acq_cancel_obsolete_orders.pl > Applying: Bug 36068: Do not overwrite cancellation date in ->cancel > > Bug 27893 - Delete of biblio should warn about attached acq orders and > cancel them > > 162271 - Bug 27893: Cancel acq orders in addbiblio.pl > 162272 - Bug 27893: Optionally skip biblio with open orders in batch delete > > Apply? [(y)es, (n)o, (i)nteractive] y > Applying: Bug 27893: Cancel acq orders in addbiblio.pl > .git/rebase-apply/patch:59: trailing whitespace. > > warning: 1 line adds whitespace errors. > Using index info to reconstruct a base tree... > M cataloguing/addbiblio.pl > .git/rebase-apply/patch:59: trailing whitespace. > > warning: 1 line adds whitespace errors. > Falling back to patching base and 3-way merge... > Auto-merging cataloguing/addbiblio.pl > CONFLICT (content): Merge conflict in cataloguing/addbiblio.pl > error: Failed to merge in the changes. > Patch failed at 0001 Bug 27893: Cancel acq orders in addbiblio.pl Thx for testing. Rebased now for CSRF changes.
Created attachment 163676 [details] [review] Bug 27893: Cancel acq orders in addbiblio.pl When the user confirms removal although we have linked orders, we should cancel them. Test plan: Add order line to a basket. Goto newly added biblio record, remove items, remove biblio. Notice the warning about attached order lines. Confirm. Check the basket again after deletion took place (cancelled line). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 163677 [details] [review] Bug 27893: Optionally skip biblio with open orders in batch delete If the user unchecks the skip checkbox (on by default), we must make sure that order cancellation is done in background job. Test plan: Pick two biblios. One has linked open orders, the other not. Go to batch delete records. Select 'Enter list of numbers'. Enter both biblio numbers and check that only one is used on the follow-up form. Run the deletion without the skip open orders and verify that linked order line is cancelled. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
(In reply to David Nind from comment #18) > Signed-off-by: David Nind <david@davidnind.com> Thanks. Please note the dependency (ies).
Created attachment 165002 [details] [review] Bug 27893: Cancel acq orders in addbiblio.pl When the user confirms removal although we have linked orders, we should cancel them. Test plan: Add order line to a basket. Goto newly added biblio record, remove items, remove biblio. Notice the warning about attached order lines. Confirm. Check the basket again after deletion took place (cancelled line). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165003 [details] [review] Bug 27893: Optionally skip biblio with open orders in batch delete If the user unchecks the skip checkbox (on by default), we must make sure that order cancellation is done in background job. Test plan: Pick two biblios. One has linked open orders, the other not. Go to batch delete records. Select 'Enter list of numbers'. Enter both biblio numbers and check that only one is used on the follow-up form. Run the deletion without the skip open orders and verify that linked order line is cancelled. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All working as described, QA scripts happy, No regressions found. Passing QA
This depends on an enh in FQA with some questions - please check if this dependency is strictly needed and send this back into my queue when ready.
Sorry, more questions :) 1) It looks like cancel is not actually checking for the status of an order line. Should we be able to cancel an already cancelled order, add a "not cancelled" to our search or do I miss something? + my @result = Koha::Acquisition::Orders->search( { biblionumber => $biblionumber } )->cancel; 2) + #FIXME This should be handled in template alert 3) Terminology So we don't forget about this later: Please don't use biblio, use bibliographic. + <label for="skip_open_orders">Skip biblio records with open acquisition orders</label> 3) Warning text Warning: This record is used in 1 order(s). Deleting it could cause serious issues on acquisition module. Are you sure you want to delete this record? Should we keep it like that? I think it would be nice to remove the "serious issues" (we mostly fixed those now) with a note that open orders will be cancelled. What do you think?
(In reply to Katrin Fischer from comment #24) > 1) It looks like cancel is not actually checking for the status of an order > line. Should we be able to cancel an already cancelled order, add a "not > cancelled" to our search or do I miss something? > > + my @result = Koha::Acquisition::Orders->search( { biblionumber => > $biblionumber } )->cancel; Out of scope a bit? This plural cancel is just a wrapper around the singular cancel which did not check that before. The good news is that it actually does not really matter. It can be run again. Note the new check on not overwriting the cancellation date that I added recently.
(In reply to Katrin Fischer from comment #24) > 2) + #FIXME This should be handled in template alert The context is: if ($error) { + #FIXME This should be handled in template alert warn "ERROR when DELETING BIBLIO $biblionumber : $error"; print "Content-Type: text/html\n\n<html><body><h1>ERROR when DELETING BIBLIO $biblionumber : $error</h1></body></html>"; You could also read this FIXME as a TODO. It would be much nicer to do that. But this report is not about improving error reporting of that script. Sometimes you see things to improve within the context of development. And it could be handy to make a note like that. The warn and print here are just existing code.
Created attachment 165151 [details] [review] Bug 27893: (QA follow-up) Terminology See comment24. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 165152 [details] [review] Bug 27893: (QA follow-up) Rephrase warning See comment24 as well. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Katrin Fischer from comment #24) > 3) Terminology > > So we don't forget about this later: Please don't use biblio, use > bibliographic. > > + <label for="skip_open_orders">Skip biblio records with open > acquisition orders</label> Fixed in follow-up. > 3) Warning text Actually 4 :) > Warning: This record is used in 1 order(s). Deleting it could cause serious > issues on acquisition module. Are you sure you want to delete this record? > > Should we keep it like that? I think it would be nice to remove the "serious > issues" (we mostly fixed those now) with a note that open orders will be > cancelled. What do you think? I agree. Fixed too.
(In reply to Marcel de Rooy from comment #25) > (In reply to Katrin Fischer from comment #24) > > > 1) It looks like cancel is not actually checking for the status of an order > > line. Should we be able to cancel an already cancelled order, add a "not > > cancelled" to our search or do I miss something? > > > > + my @result = Koha::Acquisition::Orders->search( { biblionumber => > > $biblionumber } )->cancel; > > Out of scope a bit? This plural cancel is just a wrapper around the singular > cancel which did not check that before. The good news is that it actually > does not really matter. It can be run again. Note the new check on not > overwriting the cancellation date that I added recently. I didn't expect you to fix cancel, just wondered if there should be another parameter to limit by status. - but reassured by your comment that the cancellation date would not be changed.
Pushed for 24.05! Well done everyone, thank you!
It seems strange to me that even complete orders are cancelled when the linked biblio record is deleted. Is there a reason they need to be cancelled, rather than staying as "complete"?
(In reply to Emily Lamancusa from comment #32) > It seems strange to me that even complete orders are cancelled when the > linked biblio record is deleted. Is there a reason they need to be > cancelled, rather than staying as "complete"? Good question. Looking now.
Created attachment 165620 [details] [review] Bug 27893: (follow-up) Check orderstatus in orders->cancel Test plan: Run t/db_dependent/Koha/Acquisition/Orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Emily Lamancusa from comment #32) > It seems strange to me that even complete orders are cancelled when the > linked biblio record is deleted. Is there a reason they need to be > cancelled, rather than staying as "complete"? Have a look please at the follow-up.
Created attachment 165621 [details] [review] Bug 27893: (follow-up) Check orderstatus in orders->cancel Removed the results[0] test too in addbiblio, since there can be warnings without effectively cancelled order lines. Also reworded the warn (cancelled, not deleted). Test plan: Run t/db_dependent/Koha/Acquisition/Orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Follow-up has been pushed to main.
Thanks for the follow-up, Marcel!
Depends on Bug 36068 not in 23.11.x