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; }
Created attachment 87764 [details] [review] Bug 22688: Unit tests
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
Created attachment 87766 [details] [review] Bug 22688: Unit tests
Created attachment 87768 [details] [review] Bug 22688: Unit tests
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>
Created attachment 87773 [details] [review] Bug 22688: Unit tests Signed-off-by: Agustín Moyano <agustinmoyano@theke.io>
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).
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>
Created attachment 87818 [details] [review] Bug 22688: (follow-up) Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
I decided to submit what I had done. But it needs your review, Kyle, and maybe the mocked tests are not enough.
It is such a tiny thing, used in a single place...
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>
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>
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>
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>
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>
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.05
missing dependencies for 18.05.x, not backporting