Koha::Libraries->pickup_locations is wrong: we should not expect 2 different types for a given parameter, biblio should always be a Koha::Biblio (idem for item btw). See bug 24350 for further details
In my opinion, we might not need the TT plugin in a short term, as we could just implement API calls that invoke the underlying ->pickup_locations methods in Koha::Biblio and Koha::Item (bug 24302 for reference).
I have opened bug 25421, we really need this to be fixed asap.
(In reply to Martin Renvoize from comment #0) > Koha::Libraries->pickup_locations is wrong: we should not expect 2 > different types for a given parameter, biblio should always be a > Koha::Biblio (idem for item btw). > > See bug 24350 for further details I think ->pickup_locations shouldn't cover (AT ALL) the biblio/item use case and I'm will remove it. If there's any logic about transfers, etc; it should belong to Koha::Biblio->pickup_locations.
Created attachment 104615 [details] [review] Bug 24368: Remove Koha::Libraries->pickup_locations This patch removes the unused method, and cleans the tests. To test: 1. Verify Koha::Libraries->pickup_locations is not used in the code: $ git grep 'Koha::Libraries\->pickup_locations' => SUCCESS: no uses 2. Apply this patch 3. Run: $ kshell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 104616 [details] [review] Bug 24368: Comprehensive tests for Koha::Template::Plugin::Branches::pickup_locations Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This needs to be removed as it's been replaced by a combination of: - Koha::Biblio->pickup_locations - Koha::Item->pickup_locations The only remaining use was not passing parameters and thus all the logic in there was not used at all [1] This patches removes the method and tests, and also refactor the tests for the (once using this) Koha::Template::Plugin::Branches::pickup_locations method to cover the cases in more detail. [1] See bug 25421
Regarding the failing tests from bug 25421, I am not sure what you provided is equivalent. The POD of Koha::Libraries->pickup_locations said "- C. none of the above, simply all libraries with pickup_location => 1", but it's wrong, the code is: my @pickup_locations; foreach my $library ($libraries->as_list) { if ($item && $item->can_be_transferred({ to => $library })) { push @pickup_locations, $library->unblessed; } elsif ($biblio && $biblio->can_be_transferred({ to => $library })) { push @pickup_locations, $library->unblessed; } } With this change we are loosing the "can_be_transferred" conditions.
(In reply to Jonathan Druart from comment #7) > Regarding the failing tests from bug 25421, I am not sure what you provided > is equivalent. > The POD of Koha::Libraries->pickup_locations said "- C. none of the > above, simply all libraries with pickup_location => 1", but it's wrong, the > code is: > > my @pickup_locations; > foreach my $library ($libraries->as_list) { > if ($item && $item->can_be_transferred({ to => $library })) { > push @pickup_locations, $library->unblessed; > } elsif ($biblio && $biblio->can_be_transferred({ to => $library })) > { > push @pickup_locations, $library->unblessed; > } > } > > With this change we are loosing the "can_be_transferred" conditions. No we are not. That part is covered in $item->pickup_locations and $biblio->pickup_locations respectively, which is what is used in the template plugin. The only remaining use of Koha::Libraries->pickup_locations in the plugin was the 'just search pickup_locations => 1.
Yes we do :) 130 @libraries = Koha::Libraries->search( { pickup_location => 1 }, 131 { order_by => ['branchname'] } )->as_list 132 unless @libraries; There is no check on 'can_be_transferred'
(In reply to Jonathan Druart from comment #9) > Yes we do :) > > 130 @libraries = Koha::Libraries->search( { pickup_location => 1 }, > 131 { order_by => ['branchname'] } )->as_list > 132 unless @libraries; > > There is no check on 'can_be_transferred' if $item or $biblio, otherwise not.
Ho right, I get it now!
Created attachment 104789 [details] [review] Bug 24368: Remove Koha::Libraries->pickup_locations This patch removes the unused method, and cleans the tests. To test: 1. Verify Koha::Libraries->pickup_locations is not used in the code: $ git grep 'Koha::Libraries\->pickup_locations' => SUCCESS: no uses 2. Apply this patch 3. Run: $ kshell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 104790 [details] [review] Bug 24368: Comprehensive tests for Koha::Template::Plugin::Branches::pickup_locations Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 104830 [details] [review] Bug 24368: Remove Koha::Libraries->pickup_locations This patch removes the unused method, and cleans the tests. To test: 1. Verify Koha::Libraries->pickup_locations is not used in the code: $ git grep 'Koha::Libraries\->pickup_locations' => SUCCESS: no uses 2. Apply this patch 3. Run: $ kshell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104831 [details] [review] Bug 24368: Comprehensive tests for Koha::Template::Plugin::Branches::pickup_locations Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks guys, This set of bugs makes a lot of sense :) Tests are passing and make sense to me, and code removal is varified. Passing QA
Nice work everyone! Pushed to master for 20.05
missing dependencies - not backported to 19.11.x