Bug 24440 - Add ->current_item_level_holds to Koha::Acquisition::Order
Summary: Add ->current_item_level_holds to Koha::Acquisition::Order
Status: RESOLVED FIXED
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: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 20212 25297
  Show dependency treegraph
 
Reported: 2020-01-16 18:52 UTC by Tomás Cohen Arazi
Modified: 2020-04-30 10:49 UTC (History)
9 users (show)

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


Attachments
Bug 24440: Unit tests (2.30 KB, patch)
2020-01-16 19:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24440: Add ->holds and ->holds_count to Koha::Acquisition::Order (2.00 KB, patch)
2020-01-16 19:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24440: Unit tests (2.35 KB, patch)
2020-01-17 10:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 24440: Add ->holds and ->holds_count to Koha::Acquisition::Order (2.05 KB, patch)
2020-01-17 10:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 24440: Unit tests (2.98 KB, patch)
2020-01-31 03:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24440: Add ->current_holds to Koha::Acquisition::Order (3.04 KB, patch)
2020-01-31 03:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24440: ->biblio tests (1.65 KB, patch)
2020-01-31 03:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24440: (follow-up) Make related objects prefetchable (4.73 KB, patch)
2020-01-31 03:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24440: Unit tests (3.03 KB, patch)
2020-01-31 07:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 24440: Add ->current_holds to Koha::Acquisition::Order (3.08 KB, patch)
2020-01-31 07:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 24440: ->biblio tests (1.70 KB, patch)
2020-01-31 07:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 24440: (follow-up) Make related objects prefetchable (4.77 KB, patch)
2020-01-31 07:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 24440: Adding 2 more tests for ->biblio (1.41 KB, patch)
2020-01-31 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24440: Unit tests (3.11 KB, patch)
2020-02-03 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24440: Add ->current_holds to Koha::Acquisition::Order (3.16 KB, patch)
2020-02-03 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24440: ->biblio tests (1.78 KB, patch)
2020-02-03 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24440: (follow-up) Make related objects prefetchable (4.84 KB, patch)
2020-02-03 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24440: Adding 2 more tests for ->biblio (1.48 KB, patch)
2020-02-03 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24440: Rename current_holds with current_item_level_holds (2.62 KB, patch)
2020-02-03 12: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-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