From 96871ef43bd509530d70dca227a88be2a128a3e3 Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
Date: Wed, 20 Sep 2023 06:23:36 -0400
Subject: [PATCH] Bug 34153: (QA follow-up) Tidy code

---
 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 f8ca614b425..79a12f17845 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) = _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 ) = _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');
@@ -116,10 +118,11 @@ sub do_checkout {
                     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 1148b8d1540..4f342ca78a4 100755
--- a/t/db_dependent/SIP/Message.t
+++ b/t/db_dependent/SIP/Message.t
@@ -583,64 +583,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.30.2