Bug 26515 - Add Koha::Acquisition::Order->cancel
Summary: Add Koha::Acquisition::Order->cancel
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 26555
Blocks: 21204 26577 26582
  Show dependency treegraph
 
Reported: 2020-09-23 14:33 UTC by Tomás Cohen Arazi
Modified: 2020-10-15 13:25 UTC (History)
5 users (show)

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


Attachments
Bug 26515: Unit tests (6.63 KB, patch)
2020-09-23 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: Add Koha::Acquisition::Order->cancel (3.07 KB, patch)
2020-09-23 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: Unit tests (6.70 KB, patch)
2020-09-24 09:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26515: Add Koha::Acquisition::Order->cancel (3.13 KB, patch)
2020-09-24 09:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26515: Unit tests (4.81 KB, patch)
2020-09-25 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: Add Koha::Acquisition::Order->cancel (2.59 KB, patch)
2020-09-25 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: Unit tests (8.93 KB, patch)
2020-09-29 20:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: Add Koha::Acquisition::Order->cancel (3.33 KB, patch)
2020-09-29 20:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: Unit tests (9.00 KB, patch)
2020-09-30 08:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26515: Add Koha::Acquisition::Order->cancel (3.39 KB, patch)
2020-09-30 08:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26515: (follow-up) Do not count self when checking orders (7.27 KB, patch)
2020-09-30 14:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: (follow-up) Do not count self when checking orders (7.33 KB, patch)
2020-10-01 11:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26515: Unit tests (9.07 KB, patch)
2020-10-02 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26515: Add Koha::Acquisition::Order->cancel (3.46 KB, patch)
2020-10-02 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26515: (follow-up) Do not count self when checking orders (7.40 KB, patch)
2020-10-02 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26515: Remove branchcode from mock_userenv (1.05 KB, patch)
2020-10-08 10:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26515: Better feedback on errors (5.85 KB, patch)
2020-10-09 20:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26515: (QA follow-up) Preserve original behaviour (10.80 KB, patch)
2020-10-15 12:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-09-23 14:33:47 UTC
This method should replace C4::Acquisition::DelOrder
Comment 1 Tomás Cohen Arazi 2020-09-23 14:43:05 UTC
Created attachment 110603 [details] [review]
Bug 26515: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-09-23 14:43:10 UTC
Created attachment 110604 [details] [review]
Bug 26515: Add Koha::Acquisition::Order->cancel

This patch introduces an OO replacement for DelOrder. It does the same
thing, but raises exceptions instead of returning error values. It is
designed so the caller catches the exceptions and (1) presents the
situation to the end user or (2) does the right thing depending on the
exception.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass!
3. Read the tests carefully to understand how they cover all use cases.
4- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Martin Renvoize 2020-09-24 09:25:04 UTC
Created attachment 110641 [details] [review]
Bug 26515: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2020-09-24 09:25:11 UTC
Created attachment 110642 [details] [review]
Bug 26515: Add Koha::Acquisition::Order->cancel

This patch introduces an OO replacement for DelOrder. It does the same
thing, but raises exceptions instead of returning error values. It is
designed so the caller catches the exceptions and (1) presents the
situation to the end user or (2) does the right thing depending on the
exception.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass!
3. Read the tests carefully to understand how they cover all use cases.
4- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-09-24 09:50:14 UTC
Yeay for more moves towards Koha:: and OO.

Code reads well, works as expected and tests are thorough and passing.

Signing off
Comment 6 Katrin Fischer 2020-09-24 19:11:45 UTC
I feel like we are missing checks here.

In the GUI, we can't delete the record for a hold if:
- there are other items (is in the patch)
- if there are other orders using the record
- if there are subscriptions for the record

I think we should also have these checks in the new method proposed here?

Items are checked in DelBiblio, subscriptions and orders are not. DelBiblio should also be moved to the Koha namespace eventually.
Comment 7 Katrin Fischer 2020-09-24 19:14:56 UTC
(In reply to Katrin Fischer from comment #6)
> I feel like we are missing checks here.
> 
> In the GUI, we can't delete the record for a hold if:
> - there are other items (is in the patch)
> - if there are other orders using the record
> - if there are subscriptions for the record
> 
> I think we should also have these checks in the new method proposed here?
> 
> Items are checked in DelBiblio, subscriptions and orders are not. DelBiblio
> should also be moved to the Koha namespace eventually.

record for an _ORDER_... *hmpf*

DelOrder doesn't do the checks either, must be in the .pl or template...but if we are going to use these in the API, we should have them at a higher level.
Comment 8 Tomás Cohen Arazi 2020-09-24 19:27:42 UTC
Those checks would belong to deleting the biblio. An exception should be thrown there. I'm the meantime this is checked in both .pl that use this method. I think this method is ok as-is, and will work correctly once we also move DelBiblio to the Koha:: namespace and throw the relevant exceptions
Comment 9 Katrin Fischer 2020-09-24 20:05:19 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> Those checks would belong to deleting the biblio. An exception should be
> thrown there. I'm the meantime this is checked in both .pl that use this
> method. I think this method is ok as-is, and will work correctly once we
> also move DelBiblio to the Koha:: namespace and throw the relevant exceptions

I can see the point about it having to be in 'DelBiblio' (or its replacement). But not sure about the used twice... the new one is not used yet? (hope you are planning to make use of it)
Comment 10 Katrin Fischer 2020-09-24 20:06:24 UTC
But why checking for the existance of items then? This IS handled by DelBiblio - to avoid an extra db query?
Comment 11 Tomás Cohen Arazi 2020-09-25 12:30:47 UTC
After looking at how this is actually used in basket.pl and cancelorder.pl, I've decided to remove the part that deals with deleting the biblio. I was inheriting the feature from DelOrder, but it really smells. This needs to be handled in the controllers.
Comment 12 Tomás Cohen Arazi 2020-09-25 12:34:07 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2020-09-25 12:34:12 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2020-09-29 20:23:29 UTC
Created attachment 110939 [details] [review]
Bug 26515: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-09-29 20:23:34 UTC
Created attachment 110940 [details] [review]
Bug 26515: Add Koha::Acquisition::Order->cancel

This patch introduces an OO replacement for DelOrder. It does the same
thing. It doesn't die when trying to delete items or biblio. It sets an
error message on the order object so the caller knows what happened.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass!
3. Read the tests carefully to understand how they cover all use cases.
4- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 16 Tomás Cohen Arazi 2020-09-29 20:41:05 UTC
Ok, this is my fourth round.

This time I decided to make it:
- Survive errors deleting items and the biblio
- Rely on bug 26555 to notify the caller about those problems
- Moved some of the checks we do in basket.pl and cancelorder.pl here to centralize and properly test those situations with regression tests.

This will provide a good starting point for improvements in this area without introducing regressions.

Note: as with any Koha bit, what you find is you need to rewrite all the things, including C4::Biblio to make things 'perfect'. This implementation tries to be clean, and sane. It also is fully tested and provides a path for improvement without regressions, moving checks from controllers to the right place.

Caveat: I still think we should explode on any error, but it collides with 'keep the current behaviour' and 'do not rewrite all the UI'. The pattern I introduce in bug 26555 seems to be a good starting point for many of this situations. But I'm sure it will be a moving target until we feel comfortable with THE solution we collectively develop.
Comment 17 Martin Renvoize 2020-09-30 08:54:19 UTC
Created attachment 110970 [details] [review]
Bug 26515: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2020-09-30 08:54:22 UTC
Created attachment 110971 [details] [review]
Bug 26515: Add Koha::Acquisition::Order->cancel

This patch introduces an OO replacement for DelOrder. It does the same
thing. It doesn't die when trying to delete items or biblio. It sets an
error message on the order object so the caller knows what happened.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass!
3. Read the tests carefully to understand how they cover all use cases.
4- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2020-09-30 08:54:54 UTC
Works well and I feel is a great improvement.  Signing off.
Comment 20 Tomás Cohen Arazi 2020-09-30 13:26:58 UTC
I focused on the bad cases and forgot one little detail :-D
Comment 21 Tomás Cohen Arazi 2020-09-30 14:10:06 UTC
Created attachment 110992 [details] [review]
Bug 26515: (follow-up) Do not count self when checking orders

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Martin Renvoize 2020-10-01 11:25:16 UTC
Created attachment 111031 [details] [review]
Bug 26515: (follow-up) Do not count self when checking orders

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Kyle M Hall 2020-10-02 17:46:41 UTC
Created attachment 111170 [details] [review]
Bug 26515: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2020-10-02 17:47:02 UTC
Created attachment 111172 [details] [review]
Bug 26515: Add Koha::Acquisition::Order->cancel

This patch introduces an OO replacement for DelOrder. It does the same
thing. It doesn't die when trying to delete items or biblio. It sets an
error message on the order object so the caller knows what happened.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass!
3. Read the tests carefully to understand how they cover all use cases.
4- Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2020-10-02 17:47:08 UTC
Created attachment 111173 [details] [review]
Bug 26515: (follow-up) Do not count self when checking orders

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Jonathan Druart 2020-10-08 10:18:51 UTC
Created attachment 111372 [details] [review]
Bug 26515: Remove branchcode from mock_userenv

It's the default behaviour of mock_userenv
Comment 27 Jonathan Druart 2020-10-08 10:20:22 UTC
+            if (    $biblio->active_orders->count - 1 == 0 # minus ourself

What about something like
  $biblio->active_orders->search({ ordernumber => { '!=' => $self->ordernumber })->count
instead?
That makes it more explicit I think.
Comment 28 Tomás Cohen Arazi 2020-10-08 10:41:30 UTC
(In reply to Jonathan Druart from comment #27)
> +            if (    $biblio->active_orders->count - 1 == 0 # minus ourself
> 
> What about something like
>   $biblio->active_orders->search({ ordernumber => { '!=' =>
> $self->ordernumber })->count
> instead?
> That makes it more explicit I think.

I agree
Comment 29 Tomás Cohen Arazi 2020-10-09 20:04:13 UTC
Created attachment 111430 [details] [review]
Bug 26515: Better feedback on errors

This patch makes the possible causes of biblio removal failure to be
specified on the passed error message. This way the UI could render
better reports on the situation.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2020-10-15 12:43:30 UTC
Created attachment 111724 [details] [review]
Bug 26515: (QA follow-up) Preserve original behaviour

This patch removes the use of $self->items->safe_delete, as we don't
want to change the current behaviour (i.e. delete what can be deleted)
As safe_delete would rollback entirely, there was a behaviour change.

Now items are deleted in a loop that catches any problem and reports it
using the new ->add_message mechanism. The $item object is added to the
message payload so it doesn't need to be queried by the caller for
providing UI feedback.

Tests are augmented accordingly,

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Jonathan Druart 2020-10-15 13:25:09 UTC
Pushed to master for 20.11, thanks to everybody involved!