Bug 27893 - Deleting a bibliographic record should warn about attached acquisition orders and cancel them
Summary: Deleting a bibliographic record should warn about attached acquisition orders...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
: 16872 (view as bug list)
Depends on: 36068
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-08 10:54 UTC by Emma Ternrud
Modified: 2024-05-03 14:53 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 27893: Display the number of orders on the batch record deletion (4.08 KB, patch)
2021-03-10 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27893: Cancel acq orders in addbiblio.pl (2.51 KB, patch)
2024-02-19 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: Optionally skip biblio with open orders in batch delete (5.09 KB, patch)
2024-02-19 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: Cancel acq orders in addbiblio.pl (2.57 KB, patch)
2024-03-22 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: Optionally skip biblio with open orders in batch delete (5.09 KB, patch)
2024-03-22 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: Cancel acq orders in addbiblio.pl (2.57 KB, patch)
2024-03-22 10:28 UTC, David Nind
Details | Diff | Splinter Review
Bug 27893: Optionally skip biblio with open orders in batch delete (5.09 KB, patch)
2024-03-22 10:28 UTC, David Nind
Details | Diff | Splinter Review
Bug 27893: Cancel acq orders in addbiblio.pl (2.65 KB, patch)
2024-04-17 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27893: Optionally skip biblio with open orders in batch delete (5.16 KB, patch)
2024-04-17 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27893: (QA follow-up) Terminology (1.31 KB, patch)
2024-04-19 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: (QA follow-up) Rephrase warning (1.51 KB, patch)
2024-04-19 07:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: (follow-up) Check orderstatus in orders->cancel (2.25 KB, patch)
2024-04-26 09:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27893: (follow-up) Check orderstatus in orders->cancel (3.26 KB, patch)
2024-04-26 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emma Ternrud 2021-03-08 10:54:54 UTC
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.
Comment 1 Jonathan Druart 2021-03-10 14:46:45 UTC
Created attachment 118025 [details] [review]
Bug 27893: Display the number of orders on the batch record deletion
Comment 2 Jonathan Druart 2021-03-10 14:48:05 UTC
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.
Comment 3 Emma Ternrud 2021-03-18 09:52:44 UTC
It would be better to forbid the deletion than not to be warned at all.
Comment 4 Andrew Fuerste-Henry 2021-09-21 20:19:26 UTC
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.
Comment 5 Andrew Fuerste-Henry 2021-11-15 19:51:11 UTC
*** Bug 16872 has been marked as a duplicate of this bug. ***
Comment 6 Katrin Fischer 2023-07-04 21:11:42 UTC
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?
Comment 7 Marcel de Rooy 2024-02-12 08:43:14 UTC
(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.
Comment 8 Marcel de Rooy 2024-02-19 11:03:27 UTC
(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.
Comment 9 Marcel de Rooy 2024-02-19 11:14:33 UTC
Looking if I can get some new code here.
Comment 10 Marcel de Rooy 2024-02-19 14:26:13 UTC
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>
Comment 11 Marcel de Rooy 2024-02-19 14:26:16 UTC
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>
Comment 12 Marcel de Rooy 2024-02-19 14:27:45 UTC
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.
Comment 13 David Nind 2024-03-22 09:29:28 UTC
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
Comment 14 Marcel de Rooy 2024-03-22 10:02:27 UTC
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>
Comment 15 Marcel de Rooy 2024-03-22 10:02:29 UTC
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>
Comment 16 Marcel de Rooy 2024-03-22 10:03:06 UTC
(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.
Comment 17 David Nind 2024-03-22 10:28:07 UTC
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>
Comment 18 David Nind 2024-03-22 10:28:10 UTC
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>
Comment 19 Marcel de Rooy 2024-03-22 10:31:02 UTC
(In reply to David Nind from comment #18)
> Signed-off-by: David Nind <david@davidnind.com>

Thanks. Please note the dependency (ies).
Comment 20 Martin Renvoize 2024-04-17 12:33:37 UTC
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>
Comment 21 Martin Renvoize 2024-04-17 12:33:39 UTC
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>
Comment 22 Martin Renvoize 2024-04-17 12:35:22 UTC
All working as described, QA scripts happy, No regressions found.

Passing QA
Comment 23 Katrin Fischer 2024-04-18 14:02:20 UTC
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.
Comment 24 Katrin Fischer 2024-04-18 15:27:05 UTC
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?
Comment 25 Marcel de Rooy 2024-04-19 06:56:43 UTC
(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.
Comment 26 Marcel de Rooy 2024-04-19 07:00:22 UTC
(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.
Comment 27 Marcel de Rooy 2024-04-19 07:03:49 UTC
Created attachment 165151 [details] [review]
Bug 27893: (QA follow-up) Terminology

See comment24.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2024-04-19 07:06:43 UTC
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>
Comment 29 Marcel de Rooy 2024-04-19 07:07:09 UTC
(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.
Comment 30 Katrin Fischer 2024-04-19 15:40:42 UTC
(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.
Comment 31 Katrin Fischer 2024-04-19 16:07:20 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 32 Emily Lamancusa 2024-04-25 18:48:07 UTC
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"?
Comment 33 Marcel de Rooy 2024-04-26 07:40:28 UTC
(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.
Comment 34 Marcel de Rooy 2024-04-26 09:21:29 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2024-04-26 09:22:01 UTC
(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.
Comment 36 Marcel de Rooy 2024-04-26 09:30:48 UTC
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>
Comment 37 Katrin Fischer 2024-04-26 12:16:49 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 38 Katrin Fischer 2024-04-26 20:40:53 UTC
Follow-up has been pushed to main.
Comment 39 Emily Lamancusa 2024-05-03 14:53:16 UTC
Thanks for the follow-up, Marcel!