From 643e38e835d868ba46c3614e6aeefd4eeb85a522 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Wed, 20 Sep 2023 06:23:36 -0400 Subject: [PATCH] Bug 34153: (QA follow-up) Tidy code Signed-off-by: Victor Grousset/tuxayo Rebased-by: Victor Grousset/tuxayo Signed-off-by: Tomas Cohen Arazi --- C4/SIP/ILS/Transaction/Checkout.pm | 65 +++++++++--------- t/db_dependent/SIP/Message.t | 107 ++++++++++++++++++----------- 2 files changed, 100 insertions(+), 72 deletions(-) diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm index 46674f7f9f..c13e568260 100644 --- a/C4/SIP/ILS/Transaction/Checkout.pm +++ b/C4/SIP/ILS/Transaction/Checkout.pm @@ -40,17 +40,17 @@ sub new { } sub do_checkout { - my $self = shift; - my $account = shift; - my $no_block_due_date = shift; - siplog('LOG_DEBUG', "ILS::Transaction::Checkout performing checkout..."); - my $shelf = $self->{item}->hold_attached; - my $barcode = $self->{item}->id; - my $patron = Koha::Patrons->find($self->{patron}->{borrowernumber}); - my $overridden_duedate; # usually passed as undef to AddIssue - my $prevcheckout_block_checkout = $account->{prevcheckout_block_checkout}; - my $allow_additional_materials_checkout = $account->{allow_additional_materials_checkout}; - my ($issuingimpossible, $needsconfirmation, $messages) = _can_we_issue($patron, $barcode, 0); + my $self = shift; + my $account = shift; + my $no_block_due_date = shift; + siplog( 'LOG_DEBUG', "ILS::Transaction::Checkout performing checkout..." ); + my $shelf = $self->{item}->hold_attached; + my $barcode = $self->{item}->id; + my $patron = Koha::Patrons->find( $self->{patron}->{borrowernumber} ); + my $overridden_duedate; # usually passed as undef to AddIssue + my $prevcheckout_block_checkout = $account->{prevcheckout_block_checkout}; + my $allow_additional_materials_checkout = $account->{allow_additional_materials_checkout}; + my ( $issuingimpossible, $needsconfirmation, $messages ) = _can_we_issue( $patron, $barcode, 0 ); if ( $no_block_due_date ) { my $year = substr($no_block_due_date,0,4); @@ -65,50 +65,52 @@ sub do_checkout { } my $noerror=1; # If set to zero we block the issue - if (keys %{$issuingimpossible}) { - foreach (keys %{$issuingimpossible}) { + if ( keys %{$issuingimpossible} ) { + foreach ( keys %{$issuingimpossible} ) { + # do something here so we pass these errors $self->screen_msg("Issue failed : $_"); $noerror = 0; last; } } else { - foreach my $confirmation (keys %{$needsconfirmation}) { - if ($confirmation eq 'RENEW_ISSUE'){ + foreach my $confirmation ( keys %{$needsconfirmation} ) { + if ( $confirmation eq 'RENEW_ISSUE' ) { $self->screen_msg("Item already checked out to you: renewing item."); - } elsif ($confirmation eq 'RESERVED' and !C4::Context->preference("AllowItemsOnHoldCheckoutSIP")) { + } elsif ( $confirmation eq 'RESERVED' and !C4::Context->preference("AllowItemsOnHoldCheckoutSIP") ) { $self->screen_msg("Item is reserved for another patron upon return."); $noerror = 0; - } elsif ($confirmation eq 'RESERVED' and C4::Context->preference("AllowItemsOnHoldCheckoutSIP")) { + } elsif ( $confirmation eq 'RESERVED' and C4::Context->preference("AllowItemsOnHoldCheckoutSIP") ) { next; - } elsif ($confirmation eq 'RESERVE_WAITING' - or $confirmation eq 'TRANSFERRED' - or $confirmation eq 'PROCESSING') { - $self->screen_msg("Item is on hold for another patron."); - $noerror = 0; - } elsif ($confirmation eq 'ISSUED_TO_ANOTHER') { + } elsif ( $confirmation eq 'RESERVE_WAITING' + or $confirmation eq 'TRANSFERRED' + or $confirmation eq 'PROCESSING' ) + { + $self->screen_msg("Item is on hold for another patron."); + $noerror = 0; + } elsif ( $confirmation eq 'ISSUED_TO_ANOTHER' ) { $self->screen_msg("Item already checked out to another patron. Please return item for check-in."); $noerror = 0; last; - } elsif ($confirmation eq 'DEBT') { + } elsif ( $confirmation eq 'DEBT' ) { $self->screen_msg('Outstanding Fines block issue'); $noerror = 0; last; - } elsif ($confirmation eq 'HIGHHOLDS') { + } elsif ( $confirmation eq 'HIGHHOLDS' ) { $overridden_duedate = $needsconfirmation->{$confirmation}->{returndate}; $self->screen_msg('Loan period reduced for high-demand item'); - } elsif ($confirmation eq 'RENTALCHARGE') { - if ($self->{fee_ack} ne 'Y') { + } elsif ( $confirmation eq 'RENTALCHARGE' ) { + if ( $self->{fee_ack} ne 'Y' ) { $noerror = 0; last; } - } elsif ($confirmation eq 'PREVISSUE') { + } elsif ( $confirmation eq 'PREVISSUE' ) { $self->screen_msg("This item was previously checked out by you"); $noerror = 0 if ($prevcheckout_block_checkout); last; } elsif ( $confirmation eq 'ADDITIONAL_MATERIALS' ) { - if ( $allow_additional_materials_checkout ) { - my $item = Koha::Items->find({ barcode => $barcode }); + if ($allow_additional_materials_checkout) { + my $item = Koha::Items->find( { barcode => $barcode } ); $self->screen_msg( 'Item has additional materials: ' . $item->materials ); } else { $self->screen_msg('Item must be checked out at a circulation desk'); @@ -120,10 +122,11 @@ sub do_checkout { $noerror = 0; last; } else { + # We've been returned a case other than those above $self->screen_msg("Item cannot be issued: $confirmation"); $noerror = 0; - siplog('LOG_DEBUG', "Blocking checkout Reason:$confirmation"); + siplog( 'LOG_DEBUG', "Blocking checkout Reason:$confirmation" ); last; } } diff --git a/t/db_dependent/SIP/Message.t b/t/db_dependent/SIP/Message.t index f0747b08f4..0d58cdd968 100755 --- a/t/db_dependent/SIP/Message.t +++ b/t/db_dependent/SIP/Message.t @@ -599,64 +599,89 @@ subtest 'test_allow_additional_materials_checkout' => sub { plan tests => 4; - my $builder = t::lib::TestBuilder->new(); - my $branchcode = $builder->build({ source => 'Branch' })->{branchcode}; - my $branchcode2 = $builder->build({ source => 'Branch' })->{branchcode}; + my $builder = t::lib::TestBuilder->new(); + my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; + my $branchcode2 = $builder->build( { source => 'Branch' } )->{branchcode}; my ( $response, $findpatron ); my $mocks = create_mocks( \$response, \$findpatron, \$branchcode ); # create some data - my $patron1 = $builder->build({ - source => 'Borrower', - value => { - password => hash_password( PATRON_PW ), - }, - }); - my $card1 = $patron1->{cardnumber}; - my $sip_patron1 = C4::SIP::ILS::Patron->new( $card1 ); + my $patron1 = $builder->build( + { + source => 'Borrower', + value => { + password => hash_password(PATRON_PW), + }, + } + ); + my $card1 = $patron1->{cardnumber}; + my $sip_patron1 = C4::SIP::ILS::Patron->new($card1); $findpatron = $sip_patron1; - my $item_object = $builder->build_sample_item({ - damaged => 0, - withdrawn => 0, - itemlost => 0, - restricted => 0, - homebranch => $branchcode, - holdingbranch => $branchcode, - materials => "This is a materials note", - }); + my $item_object = $builder->build_sample_item( + { + damaged => 0, + withdrawn => 0, + itemlost => 0, + restricted => 0, + homebranch => $branchcode, + holdingbranch => $branchcode, + materials => "This is a materials note", + } + ); my $mockILS = $mocks->{ils}; - my $server = { ils => $mockILS, account => {} }; + my $server = { ils => $mockILS, account => {} }; $mockILS->mock( 'institution', sub { $branchcode; } ); - $mockILS->mock( 'supports', sub { return; } ); - $mockILS->mock( 'checkout', sub { - shift; - return C4::SIP::ILS->checkout(@_); - }); + $mockILS->mock( 'supports', sub { return; } ); + $mockILS->mock( + 'checkout', + sub { + shift; + return C4::SIP::ILS->checkout(@_); + } + ); my $today = dt_from_string; - t::lib::Mocks::mock_userenv({ branchcode => $branchcode, flags => 1 }); - t::lib::Mocks::mock_preference( 'CircConfirmItemParts', '1' ); - - my $siprequest = CHECKOUT . 'YN' . siprequestdate($today) . - siprequestdate( $today->clone->add( days => 1) ) . - FID_INST_ID . $branchcode . '|'. - FID_PATRON_ID . $sip_patron1->id . '|' . - FID_ITEM_ID . $item_object->barcode . '|' . - FID_TERMINAL_PWD . 'ignored' . '|'; + t::lib::Mocks::mock_userenv( { branchcode => $branchcode, flags => 1 } ); + t::lib::Mocks::mock_preference( 'CircConfirmItemParts', '1' ); + + my $siprequest = + CHECKOUT . 'YN' + . siprequestdate($today) + . siprequestdate( $today->clone->add( days => 1 ) ) + . FID_INST_ID + . $branchcode . '|' + . FID_PATRON_ID + . $sip_patron1->id . '|' + . FID_ITEM_ID + . $item_object->barcode . '|' + . FID_TERMINAL_PWD + . 'ignored' . '|'; undef $response; my $msg = C4::SIP::Sip::MsgType->new( $siprequest, 0 ); $server->{account}->{allow_additional_materials_checkout} = 0; - $msg->handle_checkout( $server ); + $msg->handle_checkout($server); my $respcode = substr( $response, 0, 2 ); - check_field( $respcode, $response, FID_SCREEN_MSG, 'Item must be checked out at a circulation desk', 'Check screen msg', 'equals' ); - is( Koha::Checkouts->search({ itemnumber => $item_object->id })->count, 0, "Item was not checked out (allow_additional_materials_checkout disabled)"); + check_field( + $respcode, $response, FID_SCREEN_MSG, 'Item must be checked out at a circulation desk', + 'Check screen msg', 'equals' + ); + is( + Koha::Checkouts->search( { itemnumber => $item_object->id } )->count, 0, + "Item was not checked out (allow_additional_materials_checkout disabled)" + ); $server->{account}->{allow_additional_materials_checkout} = 1; - $msg->handle_checkout( $server ); + $msg->handle_checkout($server); $respcode = substr( $response, 0, 2 ); - check_field( $respcode, $response, FID_SCREEN_MSG, 'Item has additional materials: This is a materials note', 'Check screen msg', 'equals' ); - is( Koha::Checkouts->search({ itemnumber => $item_object->id })->count, 1, "Item was checked out (allow_additional_materials_checkout enabled"); + check_field( + $respcode, $response, FID_SCREEN_MSG, 'Item has additional materials: This is a materials note', + 'Check screen msg', 'equals' + ); + is( + Koha::Checkouts->search( { itemnumber => $item_object->id } )->count, 1, + "Item was checked out (allow_additional_materials_checkout enabled" + ); }; # Here is room for some more subtests -- 2.20.1