We need such accessors
Created attachment 97467 [details] [review] Bug 24440: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
Koha::Acquisition::Order->holds does not make sense to me. There is not hold placed on an order.
(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.
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.
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!
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>
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>
Created attachment 98196 [details] [review] Bug 24440: ->biblio tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
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>
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>
- 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).
(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' );
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>
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...?
(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.
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>
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>
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>
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>
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>
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>
Not blocker, but part of this patch: Bug 24440: (follow-up) Make related objects prefetchable should have been moved to its own bug report.
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x