Bug 24440

Summary: Add ->current_item_level_holds to Koha::Acquisition::Order
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, agustinmoyano, jonathan.druart, kyle, liz, martin.renvoize, nick, nicolas.legrand, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 20212, 25297    
Attachments: Bug 24440: Unit tests
Bug 24440: Add ->holds and ->holds_count to Koha::Acquisition::Order
Bug 24440: Unit tests
Bug 24440: Add ->holds and ->holds_count to Koha::Acquisition::Order
Bug 24440: Unit tests
Bug 24440: Add ->current_holds to Koha::Acquisition::Order
Bug 24440: ->biblio tests
Bug 24440: (follow-up) Make related objects prefetchable
Bug 24440: Unit tests
Bug 24440: Add ->current_holds to Koha::Acquisition::Order
Bug 24440: ->biblio tests
Bug 24440: (follow-up) Make related objects prefetchable
Bug 24440: Adding 2 more tests for ->biblio
Bug 24440: Unit tests
Bug 24440: Add ->current_holds to Koha::Acquisition::Order
Bug 24440: ->biblio tests
Bug 24440: (follow-up) Make related objects prefetchable
Bug 24440: Adding 2 more tests for ->biblio
Bug 24440: Rename current_holds with current_item_level_holds

Description Tomás Cohen Arazi 2020-01-16 18:52:19 UTC
We need such accessors
Comment 1 Tomás Cohen Arazi 2020-01-16 19:01:39 UTC
Created attachment 97467 [details] [review]
Bug 24440: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-01-16 19:01:43 UTC
Created attachment 97468 [details] [review]
Bug 24440: Add ->holds and ->holds_count to Koha::Acquisition::Order

This patch introduces a method to fetch the holds associated with the
items associated to an order line. It also adds a method to get that
holds count, which will be handy on the API for embedding such
information on request.

To test:
1. Apply this patches
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 3 David Nind 2020-01-17 10:35:44 UTC
Created attachment 97491 [details] [review]
Bug 24440: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2020-01-17 10:35:47 UTC
Created attachment 97492 [details] [review]
Bug 24440: Add ->holds and ->holds_count to Koha::Acquisition::Order

This patch introduces a method to fetch the holds associated with the
items associated to an order line. It also adds a method to get that
holds count, which will be handy on the API for embedding such
information on request.

To test:
1. Apply this patches
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>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Jonathan Druart 2020-01-22 14:59:36 UTC
Koha::Acquisition::Order->holds does not make sense to me. There is not hold placed on an order.
Comment 6 Tomás Cohen Arazi 2020-01-23 11:25:46 UTC
(In reply to Jonathan Druart from comment #5)
> Koha::Acquisition::Order->holds does not make sense to me. There is not hold
> placed on an order.

Well, on reading parcel.pl and those checks you added a patch to (thanks) I noticed we could want to display a modal with the lists of holds for the librarian to know what's going on. So an endpoint to fetch those related holds didn't seem unrealistic ( GET /acq/orders/:order_id holds. And also, if we get rid of the _count method we really need the holds method to retrieve the count.
Comment 7 Tomás Cohen Arazi 2020-01-29 13:13:34 UTC
The holds_count method will be dropped now we introduced bug 24528. But there's still some discussion about the need for ->holds, so setting it to IN (short) DISCUSSION.
Comment 8 Tomás Cohen Arazi 2020-01-31 03:12:24 UTC
During the QA review, it was pointed out that this method was not well conceived as it needed to replicate the behaviour from parcel.pl and it wasn't doing that.

I have now reworked it.

Thanks Jonathan for your feedback!
Comment 9 Tomás Cohen Arazi 2020-01-31 03:33:20 UTC
Created attachment 98194 [details] [review]
Bug 24440: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Tomás Cohen Arazi 2020-01-31 03:33:24 UTC
Created attachment 98195 [details] [review]
Bug 24440: Add ->current_holds to Koha::Acquisition::Order

This patch introduces a method to fetch the current holds associated with the
items linked to an order line. It basically implements what's done in
parcel.pl, but fully tested and suitable for using on the API.

To test:
1. Apply this patches
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 11 Tomás Cohen Arazi 2020-01-31 03:33:29 UTC
Created attachment 98196 [details] [review]
Bug 24440: ->biblio tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2020-01-31 03:57:54 UTC
Created attachment 98197 [details] [review]
Bug 24440: (follow-up) Make related objects prefetchable

This patch creates aliases for the relations used to retrieve the linked
objects from other tables. It makes the accessor names match the used
relation name, for consistency.

This is important for code trying to be smart and guessing what needs to
be prefetched, like the API code.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat (1)
=> SUCCESS: Tests pass! No behaviour change
4. Verify the POD changes make sense
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 David Nind 2020-01-31 07:05:29 UTC
Created attachment 98205 [details] [review]
Bug 24440: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2020-01-31 07:05:33 UTC
Created attachment 98206 [details] [review]
Bug 24440: Add ->current_holds to Koha::Acquisition::Order

This patch introduces a method to fetch the current holds associated with the
items linked to an order line. It basically implements what's done in
parcel.pl, but fully tested and suitable for using on the API.

To test:
1. Apply this patches
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>

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2020-01-31 07:05:36 UTC
Created attachment 98207 [details] [review]
Bug 24440: ->biblio tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2020-01-31 07:05:40 UTC
Created attachment 98208 [details] [review]
Bug 24440: (follow-up) Make related objects prefetchable

This patch creates aliases for the relations used to retrieve the linked
objects from other tables. It makes the accessor names match the used
relation name, for consistency.

This is important for code trying to be smart and guessing what needs to
be prefetched, like the API code.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat (1)
=> SUCCESS: Tests pass! No behaviour change
4. Verify the POD changes make sense
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Jonathan Druart 2020-01-31 14:02:46 UTC
-    my $biblio_rs= $self->_result->biblionumber;
+    my $biblio_rs= $self->_result->biblio;
+    return unless $biblio_rs;

You are not testing that. It looks correct but I think it deserves its own bug report (bug fix, could be backported).
Comment 18 Jonathan Druart 2020-01-31 14:04:35 UTC
(In reply to Jonathan Druart from comment #17)
> -    my $biblio_rs= $self->_result->biblionumber;
> +    my $biblio_rs= $self->_result->biblio;
> +    return unless $biblio_rs;
> 
> You are not testing that. It looks correct but I think it deserves its own
> bug report (bug fix, could be backported).

There is a test!

 73     is( $order->biblio, undef, 'If no linked biblio, undef is returned' );
Comment 19 Jonathan Druart 2020-01-31 14:10:22 UTC
Created attachment 98237 [details] [review]
Bug 24440: Adding 2 more tests for ->biblio

Not directly related to the other changes but seems good to have them
here.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2020-01-31 14:11:50 UTC
Tomas, I am still not sure.
->current_holds will actually return the item-level hold only.
Maybe it should be item_holds or current_item_level_holds, or...?
Comment 21 Tomás Cohen Arazi 2020-01-31 14:50:06 UTC
(In reply to Jonathan Druart from comment #20)
> Tomas, I am still not sure.
> ->current_holds will actually return the item-level hold only.
> Maybe it should be item_holds or current_item_level_holds, or...?

I agree with renaming it. I like current_item_level_holds.
Comment 22 Jonathan Druart 2020-02-03 12:37:28 UTC
Created attachment 98283 [details] [review]
Bug 24440: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2020-02-03 12:37:32 UTC
Created attachment 98284 [details] [review]
Bug 24440: Add ->current_holds to Koha::Acquisition::Order

This patch introduces a method to fetch the current holds associated with the
items linked to an order line. It basically implements what's done in
parcel.pl, but fully tested and suitable for using on the API.

To test:
1. Apply this patches
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>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2020-02-03 12:37:36 UTC
Created attachment 98285 [details] [review]
Bug 24440: ->biblio tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2020-02-03 12:37:40 UTC
Created attachment 98286 [details] [review]
Bug 24440: (follow-up) Make related objects prefetchable

This patch creates aliases for the relations used to retrieve the linked
objects from other tables. It makes the accessor names match the used
relation name, for consistency.

This is important for code trying to be smart and guessing what needs to
be prefetched, like the API code.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Acquisition/Order.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat (1)
=> SUCCESS: Tests pass! No behaviour change
4. Verify the POD changes make sense
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2020-02-03 12:37:44 UTC
Created attachment 98287 [details] [review]
Bug 24440: Adding 2 more tests for ->biblio

Not directly related to the other changes but seems good to have them
here.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2020-02-03 12:37:48 UTC
Created attachment 98288 [details] [review]
Bug 24440: Rename current_holds with current_item_level_holds

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2020-02-03 12:39:42 UTC
Not blocker, but part of this patch:

  Bug 24440: (follow-up) Make related objects prefetchable

should have been moved to its own bug report.
Comment 29 Martin Renvoize 2020-02-04 10:22:16 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 30 Joy Nelson 2020-02-10 23:58:47 UTC
enhancement not backported to 19.11.x