|
Description
Tomás Cohen Arazi (tcohen)
2020-09-23 14:33:47 UTC
Created attachment 110603 [details] [review] Bug 26515: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> Yeay for more moves towards Koha:: and OO. Code reads well, works as expected and tests are thorough and passing. Signing off 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. (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. 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 (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) But why checking for the existance of items then? This IS handled by DelBiblio - to avoid an extra db query? 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. Created attachment 110751 [details] [review] Bug 26515: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 110752 [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> Created attachment 110939 [details] [review] Bug 26515: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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 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. 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> 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> Works well and I feel is a great improvement. Signing off. I focused on the bad cases and forgot one little detail :-D 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> 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> 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> 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> 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> Created attachment 111372 [details] [review] Bug 26515: Remove branchcode from mock_userenv It's the default behaviour of mock_userenv + 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.
(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 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> 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> Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x enhancement will not be backported to 20.05.x |