From 83ffb4ae67e54196c4dca56dad2f4bd658642d1a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 14 Dec 2020 07:37:53 -0300 Subject: [PATCH] Bug 27209: (follow-up) Add ->is_pickup_location_valid This patch simply refactors the pickup location check into a method that can be called on its own. Tests are added, and the tests for ->set_pickup_location should pass unmodified. To test: 1. Apply the first two patches 2. Run: $ kshell k$ prove t/db_dependent/Koha/Hold.t => SUCCESS: tests pass 3. Apply this refactoring patch 4. Notice the tests are similar, but check for boolean output 5. Repeat 2 => SUCCESS: New tests pass, set_pickup_location() behavior unchanged. 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Josef Moravec Signed-off-by: Jonathan Druart (cherry picked from commit 865a1dab5c346920c61237c4b7593202328e04f8) Signed-off-by: Fridolin Somers (cherry picked from commit 02df63c430df50d0d6985be23c4e5a98142b8a30) --- Koha/Hold.pm | 33 +++++++++++++++++---- t/db_dependent/Koha/Hold.t | 60 +++++++++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 25aee05fdb..8107280537 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -198,16 +198,19 @@ sub set_waiting { return $self; } -=head3 set_pickup_location +=head3 is_pickup_location_valid - $hold->set_pickup_location({ library_id => $library->id }); + if ($hold->is_pickup_location_valid({ library_id => $library->id }) ) { + ... + } -Updates the hold pickup location. It throws a I if -the passed pickup location is not valid. +Returns a I representing if the passed pickup location is valid for the hold. +It throws a I if the library_id parameter is not +passed. =cut -sub set_pickup_location { +sub is_pickup_location_valid { my ( $self, $params ) = @_; Koha::Exceptions::MissingParameter->throw('The library_id parameter is mandatory') @@ -222,7 +225,25 @@ sub set_pickup_location { @pickup_locations = $self->biblio->pickup_locations({ patron => $self->patron }); } - if ( any { $_->branchcode eq $params->{library_id} } @pickup_locations ) { + return any { $_->branchcode eq $params->{library_id} } @pickup_locations; +} + +=head3 set_pickup_location + + $hold->set_pickup_location({ library_id => $library->id }); + +Updates the hold pickup location. It throws a I if +the passed pickup location is not valid. + +=cut + +sub set_pickup_location { + my ( $self, $params ) = @_; + + Koha::Exceptions::MissingParameter->throw('The library_id parameter is mandatory') + unless $params->{library_id}; + + if ( $self->is_pickup_location_valid({ library_id => $params->{library_id} }) ) { # all good, set the new pickup location $self->branchcode( $params->{library_id} )->store; } diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index dfe0c379c5..a6a5c4a7d6 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 2; +use Test::More tests => 3; use Test::Exception; use Test::MockModule; @@ -140,3 +140,61 @@ subtest 'set_pickup_location() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'is_pickup_location_valid() tests' => sub { + + plan tests => 4; + + $schema->storage->txn_begin; + + my $mock_biblio = Test::MockModule->new('Koha::Biblio'); + my $mock_item = Test::MockModule->new('Koha::Item'); + + my $library_1 = $builder->build_object({ class => 'Koha::Libraries' }); + my $library_2 = $builder->build_object({ class => 'Koha::Libraries' }); + my $library_3 = $builder->build_object({ class => 'Koha::Libraries' }); + + # let's control what Koha::Biblio->pickup_locations returns, for testing + $mock_biblio->mock( 'pickup_locations', sub { + return Koha::Libraries->search( { branchcode => [ $library_2->branchcode, $library_3->branchcode ] } ); + }); + # let's mock what Koha::Item->pickup_locations returns, for testing + $mock_item->mock( 'pickup_locations', sub { + return Koha::Libraries->search( { branchcode => [ $library_2->branchcode, $library_3->branchcode ] } ); + }); + + my $biblio = $builder->build_sample_biblio; + my $item = $builder->build_sample_item({ biblionumber => $biblio->biblionumber }); + + # Test biblio-level holds + my $biblio_hold = $builder->build_object( + { + class => "Koha::Holds", + value => { + biblionumber => $biblio->biblionumber, + branchcode => $library_3->branchcode, + itemnumber => undef, + } + } + ); + + ok( !$biblio_hold->is_pickup_location_valid({ library_id => $library_1->branchcode }), 'Pickup location invalid'); + ok( $biblio_hold->is_pickup_location_valid({ library_id => $library_2->id }), 'Pickup location valid'); + + # Test item-level holds + my $item_hold = $builder->build_object( + { + class => "Koha::Holds", + value => { + biblionumber => $biblio->biblionumber, + branchcode => $library_3->branchcode, + itemnumber => $item->itemnumber, + } + } + ); + + ok( !$item_hold->is_pickup_location_valid({ library_id => $library_1->branchcode }), 'Pickup location invalid'); + ok( $item_hold->is_pickup_location_valid({ library_id => $library_2->id }), 'Pickup location valid' ); + + $schema->storage->txn_rollback; +}; -- 2.39.5