Bugzilla – Attachment 166230 Details for
Bug 32256
Self checkout batch mode
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 32256: (QA follow-up) Tidy sco-main.pl
Bug-32256-QA-follow-up-Tidy-sco-mainpl.patch (text/plain), 23.59 KB, created by
Andrew Fuerste-Henry
on 2024-05-06 13:46:49 UTC
(
hide
)
Description:
Bug 32256: (QA follow-up) Tidy sco-main.pl
Filename:
MIME Type:
Creator:
Andrew Fuerste-Henry
Created:
2024-05-06 13:46:49 UTC
Size:
23.59 KB
patch
obsolete
>From a57da2b1d10a18dc350dc035ca1913c546a4e9cb Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Date: Wed, 1 May 2024 15:19:09 +0100 >Subject: [PATCH] Bug 32256: (QA follow-up) Tidy sco-main.pl > >This tidies the whole file as the patchset touched so much that we >basically needed to tidy all to make the QA scripts clean. > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> >--- > opac/sco/sco-main.pl | 441 ++++++++++++++++++++++--------------------- > 1 file changed, 226 insertions(+), 215 deletions(-) > >diff --git a/opac/sco/sco-main.pl b/opac/sco/sco-main.pl >index 26c76c2f76..28e0f74b25 100755 >--- a/opac/sco/sco-main.pl >+++ b/opac/sco/sco-main.pl >@@ -32,7 +32,7 @@ use Modern::Perl; > > use CGI qw ( -utf8 ); > >-use C4::Auth qw( in_iprange get_template_and_user checkpw ); >+use C4::Auth qw( in_iprange get_template_and_user checkpw ); > use C4::Circulation qw( barcodedecode AddReturn CanBookBeIssued AddIssue CanBookBeRenewed AddRenewal ); > use C4::Reserves; > use C4::Output qw( output_html_with_http_headers ); >@@ -49,39 +49,41 @@ use Koha::CookieManager; > > my $query = CGI->new; > >-unless (C4::Context->preference('WebBasedSelfCheck')) { >+unless ( C4::Context->preference('WebBasedSelfCheck') ) { >+ > # redirect to OPAC home if self-check is not enabled > print $query->redirect("/cgi-bin/koha/opac-main.pl"); > exit; > } > >-unless ( in_iprange(C4::Context->preference('SelfCheckAllowByIPRanges')) ) { >+unless ( in_iprange( C4::Context->preference('SelfCheckAllowByIPRanges') ) ) { >+ > # redirect to OPAC home if self-checkout not permitted from current IP > print $query->redirect("/cgi-bin/koha/opac-main.pl"); > exit; > } > >-$query->param(-name=>'sco_user_login',-values=>[1]); >+$query->param( -name => 'sco_user_login', -values => [1] ); > > my ( $template, $loggedinuser, $cookie ) = get_template_and_user( > { >- template_name => "sco/sco-main.tt", >- flagsrequired => { self_check => "self_checkout_module" }, >- query => $query, >- type => "opac", >+ template_name => "sco/sco-main.tt", >+ flagsrequired => { self_check => "self_checkout_module" }, >+ query => $query, >+ type => "opac", > } > ); > > # Get the self checkout timeout preference, or use 120 seconds as a default > my $selfchecktimeout = 120000; >-if (C4::Context->preference('SelfCheckTimeout')) { >+if ( C4::Context->preference('SelfCheckTimeout') ) { > $selfchecktimeout = C4::Context->preference('SelfCheckTimeout') * 1000; > } > $template->param( SelfCheckTimeout => $selfchecktimeout ); > > # Checks policy laid out by SCOAllowCheckin, defaults to 'on' if preference is undefined > my $allowselfcheckreturns = 1; >-if (defined C4::Context->preference('SCOAllowCheckin')) { >+if ( defined C4::Context->preference('SCOAllowCheckin') ) { > $allowselfcheckreturns = C4::Context->preference('SCOAllowCheckin'); > } > >@@ -97,51 +99,51 @@ my ( $op, $patronlogin, $patronpw, $barcodestr, $confirmed, $newissues, $load_ch > ); > > my $jwt = $query->cookie('JWT'); >+ > #FIXME: This needs to be changed to a POSTed logout... >-if ($op eq "logout") { >+if ( $op eq "logout" ) { > $template->param( loggedout => 1 ); > $query->param( patronlogin => undef, patronpw => undef ); > undef $jwt; > } > > my $barcodes = []; >-if ( $barcodestr ) { >+if ($barcodestr) { > push @$barcodes, split( /\s\n/, $barcodestr ); >- $barcodes = [ map { $_ =~ /^\s*$/ ? () : barcodedecode( $_ ) } @$barcodes ]; >+ $barcodes = [ map { $_ =~ /^\s*$/ ? () : barcodedecode($_) } @$barcodes ]; > } > >-my @newissueslist = split /,/, $newissues; >-my $issuenoconfirm = 1; #don't need to confirm on issue. >-my $issuer = Koha::Patrons->find( $issuerid )->unblessed; >+my @newissueslist = split /,/, $newissues; >+my $issuenoconfirm = 1; #don't need to confirm on issue. >+my $issuer = Koha::Patrons->find($issuerid)->unblessed; > >-my $patronid = $jwt ? Koha::Token->new->decode_jwt({ token => $jwt }) : undef; >-unless ( $patronid ) { >+my $patronid = $jwt ? Koha::Token->new->decode_jwt( { token => $jwt } ) : undef; >+unless ($patronid) { > if ( C4::Context->preference('SelfCheckoutByLogin') ) { > ( undef, $patronid ) = checkpw( $patronlogin, $patronpw ); >- } >- else { # People should not do that unless they know what they are doing! >- # SelfCheckAllowByIPRanges MUST be configured >+ } else { # People should not do that unless they know what they are doing! >+ # SelfCheckAllowByIPRanges MUST be configured > $patronid = $query->param('patronid'); > } >- $jwt = Koha::Token->new->generate_jwt({ id => $patronid }) if $patronid; >+ $jwt = Koha::Token->new->generate_jwt( { id => $patronid } ) if $patronid; > } > > my $patron; >-if ( $patronid ) { >+if ($patronid) { > Koha::Plugins->call( 'patron_barcode_transform', \$patronid ); > $patron = Koha::Patrons->find( { cardnumber => $patronid } ); > } > > undef $jwt unless $patron; > >-my $branch = $issuer->{branchcode}; >+my $branch = $issuer->{branchcode}; > my $confirm_required = 0; >-my $return_only = 0; >+my $return_only = 0; > > my $batch_checkouts_allowed; > if ( C4::Context->preference('BatchCheckouts') and $patron ) { > my @batch_category_codes = split ',', C4::Context->preference('BatchCheckoutsValidCategories'); >- my $categorycode = $patron->categorycode; >+ my $categorycode = $patron->categorycode; > if ( $categorycode && grep { $_ eq $categorycode } @batch_category_codes ) { > $batch_checkouts_allowed = 1; > } >@@ -150,187 +152,193 @@ if ( C4::Context->preference('BatchCheckouts') and $patron ) { > if ( $patron && $op eq "cud-returnbook" && $allowselfcheckreturns ) { > my $success = 1; > >- foreach my $barcode ( @$barcodes ) { >- my $item = Koha::Items->find( { barcode => $barcode } ); >- if ( $success && C4::Context->preference("CircConfirmItemParts") ) { >- if ( defined($item) >- && $item->materials ) >- { >- $success = 0; >- } >- } >- >- if ($success) { >- # Patron cannot checkin an item they don't own >- $success = 0 >- unless $patron->checkouts->find( { itemnumber => $item->itemnumber } ); >- } >- >- if ( $success ) { >- ($success) = AddReturn( $barcode, $branch ) >- } >- >- $template->param( >- returned => $success, >- barcode => $barcode >- ); >- } # foreach barcode in barcodes >-} >- >-elsif ( $patron && ( $op eq 'cud-checkout' ) ) { >- my @failed_checkouts; >- my @confirm_checkouts; >- foreach my $barcode ( @$barcodes ) { >- my $item = Koha::Items->find( { barcode => $barcode } ); >- my $impossible = {}; >- my $needconfirm = {}; >- ( $impossible, $needconfirm ) = CanBookBeIssued( >- $patron, >- $barcode, >- undef, >- 0, >- C4::Context->preference("AllowItemsOnHoldCheckoutSCO") >- ); >- my $issue_error; >- if ( $confirm_required = scalar keys %$needconfirm ) { >- for my $error ( qw( UNKNOWN_BARCODE max_loans_allowed ISSUED_TO_ANOTHER NO_MORE_RENEWALS NOT_FOR_LOAN DEBT WTHDRAWN RESTRICTED RESERVED ITEMNOTSAMEBRANCH EXPIRED DEBARRED CARD_LOST GNA INVALID_DATE UNKNOWN_BARCODE TOO_MANY DEBT_GUARANTEES DEBT_GUARANTORS USERBLOCKEDOVERDUE PATRON_CANT PREVISSUE NOT_FOR_LOAN_FORCING ITEM_LOST ADDITIONAL_MATERIALS ) ) { >- if ( $needconfirm->{$error} ) { >- $issue_error = $error; >- $confirmed = 0; >- last; >+ foreach my $barcode (@$barcodes) { >+ my $item = Koha::Items->find( { barcode => $barcode } ); >+ if ( $success && C4::Context->preference("CircConfirmItemParts") ) { >+ if ( defined($item) >+ && $item->materials ) >+ { >+ $success = 0; > } > } >- } > >- if (scalar keys %$impossible) { >- my $issue_error = (keys %$impossible)[0]; # FIXME This is wrong, we assume only one error and keys are not ordered >- my $title = ( $item ) ? $item->biblio->title : ''; >+ if ($success) { > >- my $failed_checkout = { >- "circ_error_$issue_error" => 1, >- title => $title, >- }; >- >- if ($issue_error eq 'DEBT') { >- $failed_checkout->{DEBT} = $impossible->{DEBT}; >- } >- if ( $issue_error eq "NO_MORE_RENEWALS" ) { >- $return_only = 1; >- $failed_checkout->{barcode} = $barcode; >- $failed_checkout->{returnitem} = 1; >+ # Patron cannot checkin an item they don't own >+ $success = 0 >+ unless $patron->checkouts->find( { itemnumber => $item->itemnumber } ); > } > >- push @failed_checkouts, $failed_checkout; >+ if ($success) { >+ ($success) = AddReturn( $barcode, $branch ); >+ } > > $template->param( >- hide_main => 1, >+ returned => $success, >+ barcode => $barcode > ); >- } elsif ( $needconfirm->{RENEW_ISSUE} ) { >- my $confirm_checkout = { >- renew => 1, >- barcode => $barcode, >- confirm => $item->biblio->title, >- confirm_renew_issue => 1, >- }; >- push @confirm_checkouts, $confirm_checkout; >- $template->param( >- hide_main => 1, >+ } # foreach barcode in barcodes >+ >+} elsif ( $patron && ( $op eq 'cud-checkout' ) ) { >+ my @failed_checkouts; >+ my @confirm_checkouts; >+ foreach my $barcode (@$barcodes) { >+ my $item = Koha::Items->find( { barcode => $barcode } ); >+ my $impossible = {}; >+ my $needconfirm = {}; >+ ( $impossible, $needconfirm ) = CanBookBeIssued( >+ $patron, >+ $barcode, >+ undef, >+ 0, >+ C4::Context->preference("AllowItemsOnHoldCheckoutSCO") > ); >- } elsif ( $confirm_required && !$confirmed ) { >- my $failed_checkout = { >- "circ_error_$issue_error" => 1, >- }; >- if ($issue_error eq 'DEBT') { >- $failed_checkout->{DEBT} = $needconfirm->{DEBT}; >- } >- push @failed_checkouts, $failed_checkout; >- } else { >- if ( $confirmed || $issuenoconfirm ) { # we'll want to call getpatroninfo again to get updated issues. >- my ( $hold_existed, $item ); >- if ( C4::Context->preference('HoldFeeMode') eq 'any_time_is_collected' ) { >- # There is no easy way to know if the patron has been charged for this item. >- # So we check if a hold existed for this item before the check in >- $item = Koha::Items->find({ barcode => $barcode }); >- $hold_existed = Koha::Holds->search( >- { >- -and => { >- borrowernumber => $patron->borrowernumber, >- -or => { >- biblionumber => $item->biblionumber, >- itemnumber => $item->itemnumber >- } >- } >- } >- )->count; >+ my $issue_error; >+ if ( $confirm_required = scalar keys %$needconfirm ) { >+ for my $error ( >+ qw( UNKNOWN_BARCODE max_loans_allowed ISSUED_TO_ANOTHER NO_MORE_RENEWALS NOT_FOR_LOAN DEBT WTHDRAWN RESTRICTED RESERVED ITEMNOTSAMEBRANCH EXPIRED DEBARRED CARD_LOST GNA INVALID_DATE UNKNOWN_BARCODE TOO_MANY DEBT_GUARANTEES DEBT_GUARANTORS USERBLOCKEDOVERDUE PATRON_CANT PREVISSUE NOT_FOR_LOAN_FORCING ITEM_LOST ADDITIONAL_MATERIALS ) >+ ) >+ { >+ if ( $needconfirm->{$error} ) { >+ $issue_error = $error; >+ $confirmed = 0; >+ last; >+ } > } >+ } > >- my $new_issue = AddIssue( $patron, $barcode ); >- $template->param( issued => 1, new_issue => $new_issue ); >- push @newissueslist, $barcode unless ( grep /^$barcode$/, @newissueslist ); >+ if ( scalar keys %$impossible ) { >+ my $issue_error = >+ ( keys %$impossible )[0]; # FIXME This is wrong, we assume only one error and keys are not ordered >+ my $title = ($item) ? $item->biblio->title : ''; > >- if ( $hold_existed ) { >- my $dtf = Koha::Database->new->schema->storage->datetime_parser; >- $template->param( >- # If the hold existed before the check in, let's confirm that the charge line exists >- # Note that this should not be needed but since we do not have proper exception handling here we do it this way >- patron_has_hold_fee => Koha::Account::Lines->search( >- { >- borrowernumber => $patron->borrowernumber, >- debit_type_code => 'RESERVE', >- description => $item->biblio->title, >- date => $dtf->format_date(dt_from_string) >- } >- )->count, >- ); >+ my $failed_checkout = { >+ "circ_error_$issue_error" => 1, >+ title => $title, >+ }; >+ >+ if ( $issue_error eq 'DEBT' ) { >+ $failed_checkout->{DEBT} = $impossible->{DEBT}; > } >- } else { >- $confirm_required = 1; >+ if ( $issue_error eq "NO_MORE_RENEWALS" ) { >+ $return_only = 1; >+ $failed_checkout->{barcode} = $barcode; >+ $failed_checkout->{returnitem} = 1; >+ } >+ >+ push @failed_checkouts, $failed_checkout; >+ >+ $template->param( >+ hide_main => 1, >+ ); >+ } elsif ( $needconfirm->{RENEW_ISSUE} ) { > my $confirm_checkout = { >- confirm => "Issuing title: " . $item->biblio->title, >- barcode => $barcode, >+ renew => 1, >+ barcode => $barcode, >+ confirm => $item->biblio->title, >+ confirm_renew_issue => 1, > }; > push @confirm_checkouts, $confirm_checkout; > $template->param( >- hide_main => 1, >+ hide_main => 1, > ); >+ } elsif ( $confirm_required && !$confirmed ) { >+ my $failed_checkout = { >+ "circ_error_$issue_error" => 1, >+ }; >+ if ( $issue_error eq 'DEBT' ) { >+ $failed_checkout->{DEBT} = $needconfirm->{DEBT}; >+ } >+ push @failed_checkouts, $failed_checkout; >+ } else { >+ if ( $confirmed || $issuenoconfirm ) { # we'll want to call getpatroninfo again to get updated issues. >+ my ( $hold_existed, $item ); >+ if ( C4::Context->preference('HoldFeeMode') eq 'any_time_is_collected' ) { >+ >+ # There is no easy way to know if the patron has been charged for this item. >+ # So we check if a hold existed for this item before the check in >+ $item = Koha::Items->find( { barcode => $barcode } ); >+ $hold_existed = Koha::Holds->search( >+ { >+ -and => { >+ borrowernumber => $patron->borrowernumber, >+ -or => { >+ biblionumber => $item->biblionumber, >+ itemnumber => $item->itemnumber >+ } >+ } >+ } >+ )->count; >+ } >+ >+ my $new_issue = AddIssue( $patron, $barcode ); >+ $template->param( issued => 1, new_issue => $new_issue ); >+ push @newissueslist, $barcode unless ( grep /^$barcode$/, @newissueslist ); >+ >+ if ($hold_existed) { >+ my $dtf = Koha::Database->new->schema->storage->datetime_parser; >+ $template->param( >+ >+ # If the hold existed before the check in, let's confirm that the charge line exists >+ # Note that this should not be needed but since we do not have proper exception handling here we do it this way >+ patron_has_hold_fee => Koha::Account::Lines->search( >+ { >+ borrowernumber => $patron->borrowernumber, >+ debit_type_code => 'RESERVE', >+ description => $item->biblio->title, >+ date => $dtf->format_date(dt_from_string) >+ } >+ )->count, >+ ); >+ } >+ } else { >+ $confirm_required = 1; >+ my $confirm_checkout = { >+ confirm => "Issuing title: " . $item->biblio->title, >+ barcode => $barcode, >+ }; >+ push @confirm_checkouts, $confirm_checkout; >+ $template->param( >+ hide_main => 1, >+ ); >+ } > } >- } >- } # foreach barcode in barcodes >+ } # foreach barcode in barcodes > >- $template->param( >- impossible => \@failed_checkouts, >- confirm => \@confirm_checkouts, >- ); >-} # $op >+ $template->param( >+ impossible => \@failed_checkouts, >+ confirm => \@confirm_checkouts, >+ ); >+} # $op > > if ( $patron && ( $op eq 'cud-renew' ) ) { >- foreach my $barcode ( @$barcodes ) { >- my $item = Koha::Items->find({ barcode => $barcode }); >- >- if ( $patron->checkouts->find( { itemnumber => $item->itemnumber } ) ) { >- my ($status,$renewerror) = CanBookBeRenewed( $patron, $item->checkout ); >- if ($status) { >- AddRenewal( >- { >- borrowernumber => $patron->borrowernumber, >- itemnumber => $item->itemnumber, >- seen => 1 >- } >- ); >- push @newissueslist, $barcode; >- $template->param( >- renewed => 1, >- barcode => $barcode >- ); >+ foreach my $barcode (@$barcodes) { >+ my $item = Koha::Items->find( { barcode => $barcode } ); >+ >+ if ( $patron->checkouts->find( { itemnumber => $item->itemnumber } ) ) { >+ my ( $status, $renewerror ) = CanBookBeRenewed( $patron, $item->checkout ); >+ if ($status) { >+ AddRenewal( >+ { >+ borrowernumber => $patron->borrowernumber, >+ itemnumber => $item->itemnumber, >+ seen => 1 >+ } >+ ); >+ push @newissueslist, $barcode; >+ $template->param( >+ renewed => 1, >+ barcode => $barcode >+ ); >+ } >+ } else { >+ $template->param( renewed => 0 ); > } >- } else { >- $template->param( renewed => 0 ); >- } >- } # foreach barcode in barcodes >+ } # foreach barcode in barcodes > } > >-if ( $patron) { >- my $borrowername = sprintf "%s %s", ($patron->firstname || ''), ($patron->surname || ''); >+if ($patron) { >+ my $borrowername = sprintf "%s %s", ( $patron->firstname || '' ), ( $patron->surname || '' ); > my @checkouts; > my $pending_checkouts = $patron->pending_checkouts; > if ( C4::Context->preference('SCOLoadCheckoutsByDefault') || $load_checkouts ) { >@@ -349,54 +357,54 @@ if ( $patron) { > m/priority/ and $show_priority = 1; > } > >- my $account = $patron->account; >- my $total = $account->balance; >+ my $account = $patron->account; >+ my $total = $account->balance; > my $accountlines = $account->lines; > >- my $holds = $patron->holds; >+ my $holds = $patron->holds; > my $waiting_holds_count = 0; > >- while(my $hold = $holds->next) { >+ while ( my $hold = $holds->next ) { > $waiting_holds_count++ if $hold->is_waiting; > } > > $template->param( >- validuser => 1, >- borrowername => $borrowername, >- issues_count => scalar(@checkouts) || $pending_checkouts->count(), >- ISSUES => \@checkouts, >- HOLDS => $holds, >- newissues => join(',',@newissueslist), >- patronlogin => $patronlogin, >- patronpw => $patronpw, >- waiting_holds_count => $waiting_holds_count, >- noitemlinks => 1 , >- load_checkouts => $load_checkouts, >- borrowernumber => $patron->borrowernumber, >- SuspendHoldsOpac => C4::Context->preference('SuspendHoldsOpac'), >+ validuser => 1, >+ borrowername => $borrowername, >+ issues_count => scalar(@checkouts) || $pending_checkouts->count(), >+ ISSUES => \@checkouts, >+ HOLDS => $holds, >+ newissues => join( ',', @newissueslist ), >+ patronlogin => $patronlogin, >+ patronpw => $patronpw, >+ waiting_holds_count => $waiting_holds_count, >+ noitemlinks => 1, >+ load_checkouts => $load_checkouts, >+ borrowernumber => $patron->borrowernumber, >+ SuspendHoldsOpac => C4::Context->preference('SuspendHoldsOpac'), > AutoResumeSuspendedHolds => C4::Context->preference('AutoResumeSuspendedHolds'), >- howpriority => $show_priority, >- ACCOUNT_LINES => $accountlines, >- total => $total, >- batch_checkouts_allowed => $batch_checkouts_allowed, >+ howpriority => $show_priority, >+ ACCOUNT_LINES => $accountlines, >+ total => $total, >+ batch_checkouts_allowed => $batch_checkouts_allowed, > ); > > my $patron_messages = Koha::Patron::Messages->search( > { > borrowernumber => $patron->borrowernumber, >- message_type => 'B', >+ message_type => 'B', > } > ); > $template->param( > patron_messages => $patron_messages, >- opacnote => $patron->opacnote, >+ opacnote => $patron->opacnote, > ); > > $template->param( > nofines => 1, > > ); >- if (C4::Context->preference('ShowPatronImageInWebBasedSelfCheck')) { >+ if ( C4::Context->preference('ShowPatronImageInWebBasedSelfCheck') ) { > my $patron_image = $patron->image; > $template->param( > display_patron_image => 1, >@@ -404,18 +412,21 @@ if ( $patron) { > } > } else { > $template->param( >- nouser => $patronid, >+ nouser => $patronid, > ); > } > my $cookie_mgr = Koha::CookieManager->new; >-$cookie = $cookie_mgr->replace_in_list( $cookie, $query->cookie( >- -name => 'JWT', >- -value => $jwt // '', >- -expires => $jwt ? '+1d' : '', >- -HttpOnly => 1, >- -secure => ( C4::Context->https_enabled() ? 1 : 0 ), >- -sameSite => 'Lax' >-)); >-$template->param(patronid => $patronid); >+$cookie = $cookie_mgr->replace_in_list( >+ $cookie, >+ $query->cookie( >+ -name => 'JWT', >+ -value => $jwt // '', >+ -expires => $jwt ? '+1d' : '', >+ -HttpOnly => 1, >+ -secure => ( C4::Context->https_enabled() ? 1 : 0 ), >+ -sameSite => 'Lax' >+ ) >+); >+$template->param( patronid => $patronid ); > > output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 }; >-- >2.39.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 32256
:
154866
|
154893
|
158585
|
158586
|
163878
|
163879
|
164571
|
164572
|
164573
|
164637
|
164638
|
164639
|
164640
|
164869
|
165719
|
165751
|
165752
|
165753
|
165754
|
165755
|
165978
|
165979
|
165980
|
165981
|
165982
|
165983
|
166179
|
166225
|
166226
|
166227
|
166228
|
166229
|
166230
|
166231
|
166320
|
166321
|
166322
|
166323
|
166324
|
166325
|
166326