https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 110603 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110603&action=edit Bug 26515: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110603|0 |1 is obsolete| | --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 110641 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110641&action=edit Bug 26515: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #5 from 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110642|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 110752 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110752&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |26555 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26555 [Bug 26555] Add a way for Koha::Object(s) to carry errors information -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110642|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110752|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110752|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110642|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110752|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 110939 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110939&action=edit Bug 26515: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #110940|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 110971 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=110971&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I focused on the bad cases and forgot one little detail :-D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |26577 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26577 [Bug 26577] Make basket.pl and cancelorder.pl use $order->cancel -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |21204 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21204 [Bug 21204] Move C4::Acquisition code to the Koha namespace [OMNIBUS] -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 111172 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111172&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #26 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 111372 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111372&action=edit Bug 26515: Remove branchcode from mock_userenv It's the default behaviour of mock_userenv -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #28 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 --- Comment #30 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 111724 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=111724&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26515 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #32 from Lucas Gass <lucas@bywatersolutions.com> --- enhancement will not be backported to 20.05.x -- You are receiving this mail because: You are watching all bug changes.