Bug 22688 - TT plugin for pickup locations code wrong
Summary: TT plugin for pickup locations code wrong
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 7614
Blocks: 22284
  Show dependency treegraph
 
Reported: 2019-04-11 11:11 UTC by Tomás Cohen Arazi
Modified: 2019-10-14 20:03 UTC (History)
7 users (show)

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


Attachments
Bug 22688: Unit tests (2.87 KB, patch)
2019-04-11 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: TT plugin for pickup locations code wrong (941 bytes, patch)
2019-04-11 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: Unit tests (4.63 KB, patch)
2019-04-11 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: Unit tests (4.44 KB, patch)
2019-04-11 14:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: TT plugin for pickup locations code wrong (1.05 KB, patch)
2019-04-11 14:34 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22688: Unit tests (4.58 KB, patch)
2019-04-11 14:34 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 22688: Unit tests (4.62 KB, patch)
2019-04-11 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22688: (follow-up) Unit tests (12.55 KB, patch)
2019-04-11 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22688: TT plugin for pickup locations code wrong (1.10 KB, patch)
2019-04-11 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22688: (QA follow-up) Use search_params as all() does (2.71 KB, patch)
2019-04-11 20:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22688: Unit tests (4.70 KB, patch)
2019-04-12 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: Unit tests (4.70 KB, patch)
2019-04-12 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: (follow-up) Unit tests (12.62 KB, patch)
2019-04-12 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: TT plugin for pickup locations code wrong (1.17 KB, patch)
2019-04-12 12:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22688: (QA follow-up) Use search_params as all() does (2.78 KB, patch)
2019-04-12 12:15 UTC, Kyle M Hall
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 2019-04-11 11:11:33 UTC
I haven't spent the time to determine the consequences, but bug 7614 got wrong at some point on rebasing and this code looks plain wrong, looking at the patches history in bugzilla it seems that the lines prefixed with minus sign need to be removed:

sub pickup_locations {
    my ( $self, $params ) = @_;
-    $params->{search_params} ||= {};
-    $params->{search_params}->{pickup_location} = 1;
-    return $self->all($params);

    my $selected  = $params->{selected};
    my $libraries = Koha::Libraries->pickup_locations($params);
    for my $l (@$libraries) {
        if ( defined $selected and $l->{branchcode} eq $selected
            or not defined $selected
            and C4::Context->userenv
            and $l->{branchcode} eq C4::Context->userenv->{branch} )
        {
            $l->{selected} = 1;
        }
    }

    return $libraries;
}
Comment 1 Kyle M Hall 2019-04-11 13:54:32 UTC
Created attachment 87764 [details] [review]
Bug 22688: Unit tests
Comment 2 Kyle M Hall 2019-04-11 13:54:44 UTC
Created attachment 87765 [details] [review]
Bug 22688: TT plugin for pickup locations code wrong

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Libraries.t
Comment 3 Kyle M Hall 2019-04-11 14:14:15 UTC
Created attachment 87766 [details] [review]
Bug 22688: Unit tests
Comment 4 Kyle M Hall 2019-04-11 14:18:58 UTC
Created attachment 87768 [details] [review]
Bug 22688: Unit tests
Comment 5 Agustín Moyano 2019-04-11 14:34:49 UTC
Created attachment 87772 [details] [review]
Bug 22688: TT plugin for pickup locations code wrong

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Libraries.t

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Comment 6 Agustín Moyano 2019-04-11 14:34:52 UTC
Created attachment 87773 [details] [review]
Bug 22688: Unit tests

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Comment 7 Tomás Cohen Arazi 2019-04-11 20:18:10 UTC
Kyle, it seems to me that the test case you picked is the one that doesn't test pickup limitations (compared to all()).

I think we should test:
- Passing a selected => branchcode, then if selected => 1 is added to the hashref
- Using mock_userenv({ branchcode => $library->branchcode }) to test the case in which C4::Context->userenv is used to pick the 'selected' library.

I confess I spend a good amount of time today trying to implement a fully mocked test, only to learn that there's a problem with the 'selected' param because it would be passed all the way down to Koha::Libraries->search...

This got into a rabbit hole (writing this tests).
Comment 8 Tomás Cohen Arazi 2019-04-11 20:19:41 UTC
Created attachment 87817 [details] [review]
Bug 22688: Unit tests

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2019-04-11 20:19:45 UTC
Created attachment 87818 [details] [review]
Bug 22688: (follow-up) Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2019-04-11 20:19:48 UTC
Created attachment 87819 [details] [review]
Bug 22688: TT plugin for pickup locations code wrong

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Libraries.t

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2019-04-11 20:19:52 UTC
Created attachment 87820 [details] [review]
Bug 22688: (QA follow-up) Use search_params as all() does

It looks like selected would be passed all the way down to
Koha::Libraries->search and this would fail all the time.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2019-04-11 20:20:37 UTC
I decided to submit what I had done. But it needs your review, Kyle, and maybe the mocked tests are not enough.
Comment 13 Tomás Cohen Arazi 2019-04-11 20:21:05 UTC
It is such a tiny thing, used in a single place...
Comment 14 Kyle M Hall 2019-04-12 12:12:22 UTC
Created attachment 87884 [details] [review]
Bug 22688: Unit tests

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2019-04-12 12:15:01 UTC
Created attachment 87885 [details] [review]
Bug 22688: Unit tests

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2019-04-12 12:15:21 UTC
Created attachment 87886 [details] [review]
Bug 22688: (follow-up) Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2019-04-12 12:15:24 UTC
Created attachment 87887 [details] [review]
Bug 22688: TT plugin for pickup locations code wrong

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Libraries.t

Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2019-04-12 12:15:27 UTC
Created attachment 87888 [details] [review]
Bug 22688: (QA follow-up) Use search_params as all() does

It looks like selected would be passed all the way down to
Koha::Libraries->search and this would fail all the time.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Nick Clemens 2019-04-16 13:27:21 UTC
Awesome work all!

Pushed to master for 19.05
Comment 20 Martin Renvoize 2019-04-25 13:31:12 UTC
Pushed to 18.11.x for 18.11.05
Comment 21 Lucas Gass 2019-04-26 16:00:51 UTC
missing dependencies for 18.05.x, not backporting