Bugzilla – Attachment 54827 Details for
Bug 15438
Checking out an on-hold item sends holder's borrowernumber in AF (screen message) field.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Proposed patch
0001-Bug-15438-Stop-leaking-data-into-SIP-screen-message.patch (text/plain), 5.56 KB, created by
Colin Campbell
on 2016-08-24 13:30:48 UTC
(
hide
)
Description:
Proposed patch
Filename:
MIME Type:
Creator:
Colin Campbell
Created:
2016-08-24 13:30:48 UTC
Size:
5.56 KB
patch
obsolete
>From 3df9dbb56e20b4148c84bc495196bf63d03939c6 Mon Sep 17 00:00:00 2001 >From: Colin Campbell <colin.campbell@ptfs-europe.com> >Date: Wed, 24 Aug 2016 13:52:52 +0100 >Subject: [PATCH] Bug 15438: Stop leaking data into SIP screen message > >The returns from C4::Circulation::CanBookBeIssued used >to be structured as a hashref of entries like > REASON => { > data => 'foo', > moredata => 'bar', > }; >Some entries still are. But many are now > REASON => 1, > data => 'foo', > moredata => 'bar', > >The sip Checkout routiner still assumed the former, as it >reports any causes it was not aware of (to maintain support for >a changing api) The data fields could leak into the screen message >field of the response. e.g. the borrowernumber or surname of the >borrower who has a hold on an issued title. Some real messages were >getting obscured by this > >This patch sanatizes the return from from CanBookBeIssued >by removing keys which are not all uppercase >It also fixes a case where the key's data element was used >for the screen messafe when we should use the key itself > >Updated the documentation of CanBookBeIssued to flag up >the assumption re case and the fact that 3 elements rather >than two may be returned > >The loopd through the returned keys were a bit bogus >so we now explicitly jump out if noerror is unset >--- > C4/Circulation.pm | 8 +++++++- > C4/SIP/ILS/Transaction/Checkout.pm | 41 ++++++++++++++++++++++++++++---------- > 2 files changed, 37 insertions(+), 12 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 1304d11..010c511 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -553,7 +553,7 @@ sub TooMany { > > =head2 CanBookBeIssued > >- ( $issuingimpossible, $needsconfirmation ) = CanBookBeIssued( $borrower, >+ ( $issuingimpossible, $needsconfirmation, [ $alerts ] ) = CanBookBeIssued( $borrower, > $barcode, $duedate, $inprocess, $ignore_reserves, $params ); > > Check if a book can be issued. >@@ -650,6 +650,12 @@ sticky due date is invalid or due date in the past > > if the borrower borrows to much things > >+=head3 NB >+ >+The assumption by users of the routine is that causes blocking >+the issue are keyed by uppercase labels and other returned >+data is keyed in lower case >+ > =cut > > sub CanBookBeIssued { >diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm >index 14a2b66..0d3901f 100644 >--- a/C4/SIP/ILS/Transaction/Checkout.pm >+++ b/C4/SIP/ILS/Transaction/Checkout.pm >@@ -56,19 +56,17 @@ sub do_checkout { > $debug and warn "do_checkout: patron (" . $patron_barcode . ")"; > my $borrower = $self->{patron}->getmemberdetails_object(); > $debug and warn "do_checkout borrower: . " . Dumper $borrower; >- my ($issuingimpossible,$needsconfirmation) = CanBookBeIssued( >- $borrower, >- $barcode, >- undef, >- 0, >+ my ($issuingimpossible, $needsconfirmation) = _can_we_issue($borrower, $barcode, > C4::Context->preference("AllowItemsOnHoldCheckout") > ); >- my $noerror=1; >- if (scalar keys %$issuingimpossible) { >- foreach (keys %$issuingimpossible) { >+ >+ my $noerror=1; # If set to zero we block the issue >+ if (keys %{$issuingimpossible}) { >+ foreach (keys %{$issuingimpossible}) { > # do something here so we pass these errors >- $self->screen_msg($_ . ': ' . $issuingimpossible->{$_}); >+ $self->screen_msg("Issue failed : $_"); > $noerror = 0; >+ last; > } > } else { > foreach my $confirmation (keys %{$needsconfirmation}) { >@@ -80,25 +78,29 @@ sub do_checkout { > $self->screen_msg("Item was reserved for you."); > } else { > $self->screen_msg("Item is reserved for another patron upon return."); >- # $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') { > $self->screen_msg('Outstanding Fines block issue'); > $noerror = 0; >+ last; > } 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') { > $noerror = 0; >+ last; > } > } else { >- $self->screen_msg($needsconfirmation->{$confirmation}); >+ # We've been returned a case other than those above >+ $self->screen_msg("Item cannot be issued: $confirmation"); > $noerror = 0; > syslog('LOG_DEBUG', "Blocking checkout Reason:$confirmation"); >+ last; > } > } > } >@@ -145,5 +147,22 @@ sub do_checkout { > return $self; > } > >+sub _can_we_issue { >+ my ( $borrower, $barcode, $pref ) = @_; >+ >+ my ( $issuingimpossible, $needsconfirmation, $alerts ) = >+ CanBookBeIssued( $borrower, $barcode, undef, 0, $pref ); >+ for my $href ( $issuingimpossible, $needsconfirmation ) { >+ >+ # some data is returned using lc keys we only >+ foreach my $key ( keys %{$href} ) { >+ if ( $key =~ m/[^A-Z_]/ ) { >+ delete $href->{$key}; >+ } >+ } >+ } >+ return ( $issuingimpossible, $needsconfirmation ); >+} >+ > 1; > __END__ >-- >2.7.4 >
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 15438
:
54783
|
54827
|
61738
|
64264
|
64265
|
64266
|
64757
|
66982
|
66983