https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25421 CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I have opened bug 25421, we really need this to be fixed asap. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 104615 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104615&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25421 | Depends on| |25421 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25421 [Bug 25421] Inconsistent return value in Koha::Libraries->pickup_locations -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 104790 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104790&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104789|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104830 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104830&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #16 from 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24368 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |25516 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25516 [Bug 25516] Item/pickup_locations wantarray removed, so dies on Perl >=5.24 where "autoderef" feature absent -- You are receiving this mail because: You are watching all bug changes.