Bug 25670 - Add useful methods to Koha::Acquisition::Orders
Summary: Add useful methods to Koha::Acquisition::Orders
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 30092 20212 28509
  Show dependency treegraph
 
Reported: 2020-06-03 19:54 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This architectural enhancement adds helpful filter methods to the Koha::Acquisitions:: modules to ease the development of Acquisitions API's and future acquisitions related enhancements.
Version(s) released in:
21.05.00


Attachments
Bug 25670: Unit tests (3.07 KB, patch)
2020-06-03 20:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25670: Add Koha::Acquisition::Orders->filter_by_id_including_transfers (2.10 KB, patch)
2020-06-03 20:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25670: Unit tests (4.86 KB, patch)
2020-09-10 18:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25670: Add filter_by_active and filter_by_id_including_transfers (2.73 KB, patch)
2020-09-10 18:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25670: Unit tests (4.92 KB, patch)
2020-09-24 09:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25670: Add filter_by_active and filter_by_id_including_transfers (2.79 KB, patch)
2020-09-24 09:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25670: Unit tests (4.99 KB, patch)
2020-09-24 20:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25670: Add filter_by_active and filter_by_id_including_transfers (2.86 KB, patch)
2020-09-24 20:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25670: Regression tests (3.53 KB, patch)
2021-01-07 19:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25670: New orders included only if standing (1.50 KB, patch)
2021-01-07 19:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25670: Fix random failure on Orders.t (1.87 KB, patch)
2021-02-01 09:37 UTC, Jonathan Druart
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-06-03 19:54:37 UTC
This is something that happens in C4::Acquisition::SearchOrders and is needed to be used on the API controller.
Comment 1 Tomás Cohen Arazi 2020-06-03 20:05:56 UTC
Created attachment 105534 [details] [review]
Bug 25670: Unit tests
Comment 2 Tomás Cohen Arazi 2020-06-03 20:06:01 UTC
Created attachment 105535 [details] [review]
Bug 25670: Add Koha::Acquisition::Orders->filter_by_id_including_transfers

This patch introduces a method to filter on the ordernumber, looking for
it on the aqorders_transfers table ('ordernumber_from' field).

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Orders.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 3 Tomás Cohen Arazi 2020-09-10 18:51:13 UTC
Created attachment 109859 [details] [review]
Bug 25670: Unit tests
Comment 4 Tomás Cohen Arazi 2020-09-10 18:51:34 UTC
Created attachment 109860 [details] [review]
Bug 25670: Add filter_by_active and filter_by_id_including_transfers

This patch introduces a method to filter on the ordernumber, looking for
it on the aqorders_transfers table ('ordernumber_from' field).

It also adds a method for filtering on active orders.

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

lib
Comment 5 Martin Renvoize 2020-09-24 09:54:34 UTC
Created attachment 110643 [details] [review]
Bug 25670: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-09-24 09:54:39 UTC
Created attachment 110644 [details] [review]
Bug 25670: Add filter_by_active and filter_by_id_including_transfers

This patch introduces a method to filter on the ordernumber, looking for
it on the aqorders_transfers table ('ordernumber_from' field).

It also adds a method for filtering on active orders.

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

lib

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-09-24 09:55:05 UTC
Code reads well, tests are good.. Signing off
Comment 8 Katrin Fischer 2020-09-24 20:12:12 UTC
-=head2 Class Methods
+=head2 Class methods

Thank you!

I wonder if $orders->filter_by_active is a good name. Maybe filter_by_pending ?
Comment 9 Katrin Fischer 2020-09-24 20:12:59 UTC
Created attachment 110700 [details] [review]
Bug 25670: Unit tests

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2020-09-24 20:13:06 UTC
Created attachment 110701 [details] [review]
Bug 25670: Add filter_by_active and filter_by_id_including_transfers

This patch introduces a method to filter on the ordernumber, looking for
it on the aqorders_transfers table ('ordernumber_from' field).

It also adds a method for filtering on active orders.

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

lib

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Tomás Cohen Arazi 2021-01-07 19:38:07 UTC
Created attachment 114940 [details] [review]
Bug 25670: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2021-01-07 19:38:11 UTC
Created attachment 114941 [details] [review]
Bug 25670: New orders included only if standing

The original implementation included new orders regardless they were
standing orders.

This patch makes the query respect that conditions found on
C4::Acquisitions::SearchOrders.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Orders.t
=> FAIL: More orders than expected are returned
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! New orders are only considered if the basket
they are attached to, is marked as standing!
5. Verify the tests make sense
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Jonathan Druart 2021-01-28 16:34:38 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Jonathan Druart 2021-02-01 09:37:24 UTC
Created attachment 116141 [details] [review]
Bug 25670: Fix random failure on Orders.t

It has failed at least 3 times:

 #   Failed test 'The right order is returned'
 #   at t/db_dependent/Koha/Acquisition/Orders.t line 153.
 #          got: '50'
 #     expected: '49'

 #   Failed test 'The right order is returned'
 #   at t/db_dependent/Koha/Acquisition/Orders.t line 154.
 #          got: '49'
 #     expected: '50'
 # Looks like you failed 2 tests of 6.

I cannot manage to make it fail locally
Comment 15 Jonathan Druart 2021-02-01 09:39:28 UTC
Follow-up pushed to master.
Comment 16 Fridolin Somers 2021-02-01 13:42:15 UTC
Enhancement not pushed to 20.11.x
Comment 17 Jonathan Druart 2021-02-17 08:25:27 UTC
(In reply to Jonathan Druart from comment #14)
> Created attachment 116141 [details] [review] [review]
> Bug 25670: Fix random failure on Orders.t
> 
> It has failed at least 3 times:
> 
>  #   Failed test 'The right order is returned'
>  #   at t/db_dependent/Koha/Acquisition/Orders.t line 153.
>  #          got: '50'
>  #     expected: '49'
> 
>  #   Failed test 'The right order is returned'
>  #   at t/db_dependent/Koha/Acquisition/Orders.t line 154.
>  #          got: '49'
>  #     expected: '50'
>  # Looks like you failed 2 tests of 6.
> 
> I cannot manage to make it fail locally

Again today Koha_Master_D9 #1564:
16:34:46 koha_1       |     #   Failed test 'The 2 orders are returned'
16:34:46 koha_1       |     #   at t/db_dependent/Koha/Acquisition/Orders.t line 152.
16:34:46 koha_1       |     #     Structures begin differing at:
16:34:46 koha_1       |     #          $got->[0] = '100'
16:34:46 koha_1       |     #     $expected->[0] = '99'
16:34:46 koha_1       |     # Looks like you failed 1 test of 4.
16:34:46 koha_1       | 
16:34:46 koha_1       | #   Failed test 'filter_by_id_including_transfers() tests'
16:34:46 koha_1       | #   at t/db_dependent/Koha/Acquisition/Orders.t line 160.
Comment 18 Jonathan Druart 2021-03-03 13:49:11 UTC
Master_D10 (#484)

14:03:26 koha_1       |     #   Failed test 'The 2 orders are returned'
14:03:26 koha_1       |     #   at t/db_dependent/Koha/Acquisition/Orders.t line 152.
14:03:26 koha_1       |     #     Structures begin differing at:
14:03:26 koha_1       |     #          $got->[0] = '10'
14:03:26 koha_1       |     #     $expected->[0] = '9'
14:03:26 koha_1       |     # Looks like you failed 1 test of 4.
14:03:26 koha_1       | 
14:03:26 koha_1       | #   Failed test 'filter_by_id_including_transfers() tests'
14:03:26 koha_1       | #   at t/db_dependent/Koha/Acquisition/Orders.t line 160.
Comment 19 Fridolin Somers 2022-02-11 19:21:13 UTC
Still additional_work_needed ?

Looks like there has been :
Bug 20212: Fix test after 25670 follow-up
Comment 20 Jonathan Druart 2022-02-14 08:03:26 UTC
(In reply to Fridolin Somers from comment #19)
> Still additional_work_needed ?
> 
> Looks like there has been :
> Bug 20212: Fix test after 25670 follow-up

CommitDate: Thu Jan 28 16:44:12 2021 +0100

Last comment is from March.
Comment 21 Jonathan Druart 2022-02-15 08:10:05 UTC
Tomas opened bug 30092 to track this random failure.