Bug 18360

Summary: Allow deletion of cancelled order lines
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: AcquisitionsAssignee: Marcel de Rooy <m.de.rooy>
Status: Pushed to main --- QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: karen, m.de.rooy, martin.renvoize, michaela.sieber, nengard, s.hierl, severine.queune
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36066
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10869
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33664
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch set allows you to delete cancelled order lines from an open acquisition basket.
Version(s) released in:
24.05.00
Bug Depends on:    
Bug Blocks: 36731    
Attachments: Bug 18360: Allow to delete a cancelled order from open basket
Bug 18360: Allow to delete a cancelled order from open basket
Bug 18360: Allow to delete a cancelled order from open basket
Bug 18360: Allow to delete a cancelled order from open basket
Bug 18360: (follow-up) Fix for CSRF

Description Katrin Fischer 2017-03-31 08:49:33 UTC
Sometimes an order is just a mistake (double entry or similar) and it should be possible to delete those entries entirely from the database so they won't show up in any reports.
Comment 1 Sebastian Hierl 2017-07-24 11:39:30 UTC
Our Acquisitions Assistant confirms that this would be a welcome improvement / bugfix.  Let us know if we can help with sign-off.
-Sebastian
Comment 2 Karen 2017-09-06 22:34:43 UTC
One of our partners just told me that she would love to have the ability to delete an order entry.

She says,"What KOHA needs is a delete order line feature. That is quite different from cancelling an order. I guess because we are small in comparison to many libraries, keeping records of cancelled orders is not important to us and gets in the way actually."
Comment 3 Katrin Fischer 2020-01-11 22:59:28 UTC
*** Bug 14091 has been marked as a duplicate of this bug. ***
Comment 4 Marcel de Rooy 2024-02-12 10:09:34 UTC
See bug 36066 too. We only allow API to delete cancelled lines there.

This bug is for the interface.
Comment 5 Marcel de Rooy 2024-02-20 13:19:43 UTC
Created attachment 162294 [details] [review]
Bug 18360: Allow to delete a cancelled order from open basket

Doing this under flag of order_manage and not on the higher level
of baskets.

Starting here with a single order delete for cancelled lines. So
deleting an open order line requires cancelling it first. Can be
extended later if needed.

Test plan:
Pick an open basket with cancelled lines.
Try to delete a few lines.
Test that you cannot do this on an closed basket. (Bonus: manipulate
URL with basketno, ordernumber and op=cud-delete-order.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2024-02-20 13:21:25 UTC
(In reply to Katrin Fischer from comment #0)
> Sometimes an order is just a mistake (double entry or similar) and it should
> be possible to delete those entries entirely from the database so they won't
> show up in any reports.

The current proposal does build on this use case and just starts with only allowing deletion of cancelled order lines without a biblio record.
Can be extended later on a follow-up if needed.
First try to get this moving.
Comment 7 Michaela Sieber 2024-02-23 08:52:44 UTC
Hi Marcel,
thanks for the patch.
I tested it in Sandbox
http://staff-b18360.sandboxes.ptfs-europe.co.uk/

I'm not sure if I missed something but the Link to delete the cancelled lines only appears if the basket is open.

was that the intention?

With Bug 33664 the ability to cancel order lines in closed baskets was added.
I think it would make sense to be able to delete the line even if the basket is closed.
Comment 8 Marcel de Rooy 2024-02-23 09:33:40 UTC
(In reply to Michaela Sieber from comment #7)
> Hi Marcel,
> thanks for the patch.
> I tested it in Sandbox
> http://staff-b18360.sandboxes.ptfs-europe.co.uk/
> 
> I'm not sure if I missed something but the Link to delete the cancelled
> lines only appears if the basket is open.
> 
> was that the intention?
> 
> With Bug 33664 the ability to cancel order lines in closed baskets was added.
> I think it would make sense to be able to delete the line even if the basket
> is closed.

Hi Michaela,
Yes that was the intention. Normally we can only edit a basket and its lines when we reopen. Was not aware of 33664. Making things inconsistent imo..
Prefer to keep this as is now here.
Comment 9 PTFS Europe Sandboxes 2024-02-27 07:29:29 UTC
Created attachment 162484 [details] [review]
Bug 18360: Allow to delete a cancelled order from open basket

Doing this under flag of order_manage and not on the higher level
of baskets.

Starting here with a single order delete for cancelled lines. So
deleting an open order line requires cancelling it first. Can be
extended later if needed.

Test plan:
Pick an open basket with cancelled lines.
Try to delete a few lines.
Test that you cannot do this on an closed basket. (Bonus: manipulate
URL with basketno, ordernumber and op=cud-delete-order.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 10 Marcel de Rooy 2024-02-27 12:20:38 UTC
(In reply to PTFS Europe Sandboxes from comment #9)
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Thanks Michaela
Comment 11 Marcel de Rooy 2024-04-22 11:47:51 UTC
Created attachment 165273 [details] [review]
Bug 18360: Allow to delete a cancelled order from open basket

Doing this under flag of order_manage and not on the higher level
of baskets.

Starting here with a single order delete for cancelled lines. So
deleting an open order line requires cancelling it first. Can be
extended later if needed.

Test plan:
Pick an open basket with cancelled lines.
Try to delete a few lines.
Test that you cannot do this on an closed basket. (Bonus: manipulate
URL with basketno, ordernumber and op=cud-delete-order.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 12 Marcel de Rooy 2024-04-22 11:48:24 UTC
Tiny rebase (CSRF)
Comment 13 Martin Renvoize 2024-04-25 11:37:19 UTC
Created attachment 165525 [details] [review]
Bug 18360: Allow to delete a cancelled order from open basket

Doing this under flag of order_manage and not on the higher level
of baskets.

Starting here with a single order delete for cancelled lines. So
deleting an open order line requires cancelling it first. Can be
extended later if needed.

Test plan:
Pick an open basket with cancelled lines.
Try to delete a few lines.
Test that you cannot do this on an closed basket. (Bonus: manipulate
URL with basketno, ordernumber and op=cud-delete-order.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2024-04-25 11:38:11 UTC
Clear simple code, QA scripts happy and all working.

Whilst it regresses bug 33664 I think that's OK here and brings more consistency.

Passing QA
Comment 15 Marcel de Rooy 2024-04-25 13:04:42 UTC
(In reply to Martin Renvoize from comment #14)

> Whilst it regresses bug 33664 I think that's OK here and brings more
> consistency.

Could you explain? The other bug is about cancelling. This one about delete?

> Passing QA

Thanks
Comment 16 Marcel de Rooy 2024-04-25 13:07:42 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Martin Renvoize from comment #14)
> 
> > Whilst it regresses bug 33664 I think that's OK here and brings more
> > consistency.
> 
> Could you explain? The other bug is about cancelling. This one about delete?
> 
> > Passing QA
> 
> Thanks

From Mattermost:
ashimema:
About having to re-open the basket
you already noted it with Micheala
and she signed off so I felt it was OK

marcelr
Ah ok not really a regression. But perhaps inconsistent? Though I like the idea of reopening before deleting personally.
Comment 17 Katrin Fischer 2024-04-29 16:04:41 UTC
I think we might want to discuss the open/closed basket again, but this is progress and I love it :)
Comment 18 Katrin Fischer 2024-04-29 16:07:41 UTC
Taking a closer look - maybe also to be handled on a separate bug:

Why is it necessary to delete the record first? I could imagine libraries still having the record with older items or other linked orders.

+                        alert( _("Please delete the linked bibliographic record first.") );
Comment 19 Katrin Fischer 2024-04-29 16:10:05 UTC
Sorry, this needs a follow-up for CSRF:

Programming error - No CSRF token passed for POST http://localhost:8081/intranet/acqui/basket.pl?basketno=1 (referer: http://localhost:8081/cgi-bin/koha/acqui/basket.pl?basketno=1)
Comment 20 Marcel de Rooy 2024-04-30 12:09:38 UTC
(In reply to Katrin Fischer from comment #18)
> Taking a closer look - maybe also to be handled on a separate bug:
> 
> Why is it necessary to delete the record first? I could imagine libraries
> still having the record with older items or other linked orders.
> 
> +                        alert( _("Please delete the linked bibliographic
> record first.") );

Probably had in mind to prevent 'ghost records' in the catalog. Created by Acquisition and now having no links anymore.
Could imagine that there will be different opinions here.
Maybe still move that to a new report?
Comment 21 Katrin Fischer 2024-04-30 12:11:03 UTC
(In reply to Marcel de Rooy from comment #20)
> (In reply to Katrin Fischer from comment #18)
> > Taking a closer look - maybe also to be handled on a separate bug:
> > 
> > Why is it necessary to delete the record first? I could imagine libraries
> > still having the record with older items or other linked orders.
> > 
> > +                        alert( _("Please delete the linked bibliographic
> > record first.") );
> 
> Probably had in mind to prevent 'ghost records' in the catalog. Created by
> Acquisition and now having no links anymore.
> Could imagine that there will be different opinions here.
> Maybe still move that to a new report?

Yes, it was not meant as a blocker here. Just wondering about reasoning. I'll add additional_work_needed as a reminder for this loose end.
Comment 22 Marcel de Rooy 2024-04-30 12:14:06 UTC
Created attachment 165869 [details] [review]
Bug 18360: (follow-up) Fix for CSRF

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2024-04-30 12:17:52 UTC
(In reply to Katrin Fischer from comment #21)
> (In reply to Marcel de Rooy from comment #20)
> > (In reply to Katrin Fischer from comment #18)
> > > Taking a closer look - maybe also to be handled on a separate bug:
> > > 
> > > Why is it necessary to delete the record first? I could imagine libraries
> > > still having the record with older items or other linked orders.
> > > 
> > > +                        alert( _("Please delete the linked bibliographic
> > > record first.") );
> > 
> > Probably had in mind to prevent 'ghost records' in the catalog. Created by
> > Acquisition and now having no links anymore.
> > Could imagine that there will be different opinions here.
> > Maybe still move that to a new report?
> 
> Yes, it was not meant as a blocker here. Just wondering about reasoning.
> I'll add additional_work_needed as a reminder for this loose end.

Bug 36731
Comment 24 Katrin Fischer 2024-04-30 15:10:52 UTC
Pushed for 24.05!

Well done everyone, thank you!