Bug 24368 - Koha::Libraries->pickup_locations needs refactoring/ratifying
Summary: Koha::Libraries->pickup_locations needs refactoring/ratifying
Status: Pushed to master
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: Testopia
URL:
Keywords:
Depends on: 25421 24350
Blocks: 25516
  Show dependency treegraph
 
Reported: 2020-01-07 17:06 UTC by Martin Renvoize
Modified: 2020-05-19 21:53 UTC (History)
3 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 24368: Remove Koha::Libraries->pickup_locations (19.53 KB, patch)
2020-05-08 19:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24368: Comprehensive tests for Koha::Template::Plugin::Branches::pickup_locations (5.50 KB, patch)
2020-05-08 19:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24368: Remove Koha::Libraries->pickup_locations (19.60 KB, patch)
2020-05-12 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24368: Comprehensive tests for Koha::Template::Plugin::Branches::pickup_locations (5.57 KB, patch)
2020-05-12 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24368: Remove Koha::Libraries->pickup_locations (19.67 KB, patch)
2020-05-13 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24368: Comprehensive tests for Koha::Template::Plugin::Branches::pickup_locations (5.63 KB, patch)
2020-05-13 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-01-07 17:06:26 UTC
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
Comment 1 Tomás Cohen Arazi 2020-01-07 17:19:31 UTC
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).
Comment 2 Jonathan Druart 2020-05-08 10:57:01 UTC
I have opened bug 25421, we really need this to be fixed asap.
Comment 3 Tomás Cohen Arazi 2020-05-08 12:03:46 UTC
(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.
Comment 4 Tomás Cohen Arazi 2020-05-08 19:29:08 UTC
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>
Comment 5 Tomás Cohen Arazi 2020-05-08 19:29:12 UTC
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>
Comment 6 Tomás Cohen Arazi 2020-05-08 19:37:06 UTC
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
Comment 7 Jonathan Druart 2020-05-12 14:09:10 UTC
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.
Comment 8 Tomás Cohen Arazi 2020-05-12 14:11:39 UTC
(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.
Comment 9 Jonathan Druart 2020-05-12 14:20:22 UTC
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'
Comment 10 Tomás Cohen Arazi 2020-05-12 14:23:26 UTC
(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.
Comment 11 Jonathan Druart 2020-05-12 14:31:40 UTC
Ho right, I get it now!
Comment 12 Jonathan Druart 2020-05-12 14:33:06 UTC
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>
Comment 13 Jonathan Druart 2020-05-12 14:33:10 UTC
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>
Comment 14 Martin Renvoize 2020-05-13 09:28:23 UTC
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>
Comment 15 Martin Renvoize 2020-05-13 09:28:27 UTC
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>
Comment 16 Martin Renvoize 2020-05-13 09:29:44 UTC
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
Comment 17 Martin Renvoize 2020-05-13 09:31:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Joy Nelson 2020-05-19 21:53:23 UTC
missing dependencies - not backported to 19.11.x