Bugzilla – Attachment 155927 Details for
Bug 34153
Add ability to allow items with additional materials notes to be checked out via SIP
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 34153: (QA follow-up) Tidy code
Bug-34153-QA-follow-up-Tidy-code.patch (text/plain), 11.71 KB, created by
Kyle M Hall (khall)
on 2023-09-20 10:25:46 UTC
(
hide
)
Description:
Bug 34153: (QA follow-up) Tidy code
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2023-09-20 10:25:46 UTC
Size:
11.71 KB
patch
obsolete
>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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 34153
:
152864
|
155740
|
155920
|
155921
|
155927
|
156250
|
156251
|
156252