From 23dd6651f83e42200185130bd340520ea2e945f1 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 21 Nov 2019 18:39:15 +0100 Subject: [PATCH] Bug 23403: Remove cardnumber from SIP == Test plan == 1 - Have two patrons with userids and no cardnumber 2 - Note which of these has the higher borrower number 3 - Use the SIP cli emulator to connect and checkout a book to the patron with higher borrowernumber See example after 4 - Note the book may checkout to the wrong patron! 5 - Apply patch 6 - Checkout to both patrons via sip 7 - The patrons get the correct checkouts === SIP CLI emulator === ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 \ -l CPL --patron 23529001000463 -m checkout --item 39999000001259 translation: via the koha user term1, checkout item 39999000001259 to patron 23529001000463 Signed-off-by: Bouzid Fergani Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- C4/SIP/ILS/Item.pm | 8 +++--- C4/SIP/ILS/Transaction/Checkout.pm | 44 ++++++++++++++---------------- C4/SIP/ILS/Transaction/Hold.pm | 27 ++++-------------- C4/SIP/ILS/Transaction/Renew.pm | 3 +- 4 files changed, 32 insertions(+), 50 deletions(-) diff --git a/C4/SIP/ILS/Item.pm b/C4/SIP/ILS/Item.pm index afbb02aae0..246acfa766 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -96,7 +96,7 @@ sub new { if ($issue) { $self->{due_date} = dt_from_string( $issue->date_due, 'sql' )->truncate( to => 'minute' ); my $patron = Koha::Patrons->find( $issue->borrowernumber ); - $self->{patron} = $patron->cardnumber; + $self->{borrowernumber} = $patron->borrowernumber; } my $biblio = Koha::Biblios->find( $self->{biblionumber} ); my $holds = $biblio->current_holds->unblessed; @@ -254,7 +254,7 @@ sub title_id { sub sip_circulation_status { my $self = shift; - if ( $self->{patron} ) { + if ( $self->{borrowernumber} ) { return '04'; # charged } elsif ( grep { $_->{itemnumber} == $self->{itemnumber} } @{ $self->{hold_shelf} } ) { @@ -355,8 +355,8 @@ sub available { my $count = (defined $self->{pending_queue}) ? scalar @{$self->{pending_queue}} : 0; my $count2 = (defined $self->{hold_shelf} ) ? scalar @{$self->{hold_shelf} } : 0; $debug and print STDERR "availability check: pending_queue size $count, hold_shelf size $count2\n"; - if (defined($self->{patron_id})) { - ($self->{patron_id} eq $for_patron) or return 0; + if (defined($self->{borrowernumber})) { + ($self->{borrowernumber} eq $for_patron) or return 0; return ($count ? 0 : 1); } else { # not checked out ($count2) and return $self->barcode_is_borrowernumber($for_patron, $self->{hold_shelf}[0]->{borrowernumber}); diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm index daa43e9c4e..01aa8fbc60 100644 --- a/C4/SIP/ILS/Transaction/Checkout.pm +++ b/C4/SIP/ILS/Transaction/Checkout.pm @@ -46,16 +46,13 @@ sub new { } sub do_checkout { - my $self = shift; - siplog('LOG_DEBUG', "ILS::Transaction::Checkout performing checkout..."); - my $shelf = $self->{item}->hold_shelf; - my $barcode = $self->{item}->id; - my $patron_barcode = $self->{patron}->id; - my $overridden_duedate; # usually passed as undef to AddIssue - $debug and warn "do_checkout: patron (" . $patron_barcode . ")"; - my $patron = Koha::Patrons->find( { cardnumber => $patron_barcode } ); - my $borrower = $patron->unblessed; - $debug and warn "do_checkout borrower: . " . Dumper $borrower; + my $self = shift; + siplog('LOG_DEBUG', "ILS::Transaction::Checkout performing checkout..."); + my $shelf = $self->{item}->hold_shelf; + my $barcode = $self->{item}->id; + my $patron = Koha::Patrons->find($self->{patron}->{borrowernumber}); + my $overridden_duedate; # usually passed as undef to AddIssue + $debug and warn "do_checkout borrower: . " . $patron->borrowernumber; my ($issuingimpossible, $needsconfirmation) = _can_we_issue($patron, $barcode, C4::Context->preference("AllowItemsOnHoldCheckoutSIP") ); @@ -73,7 +70,7 @@ sub do_checkout { if ($confirmation eq 'RENEW_ISSUE'){ $self->screen_msg("Item already checked out to you: renewing item."); } elsif ($confirmation eq 'RESERVED' or $confirmation eq 'RESERVE_WAITING') { - my $x = $self->{item}->available($patron_barcode); + my $x = $self->{item}->available($patron->borrowernumber); if ($x) { $self->screen_msg("Item was reserved for you."); } else { @@ -110,15 +107,15 @@ sub do_checkout { } my $itemnumber = $self->{item}->{itemnumber}; foreach (@$shelf) { - $debug and warn "shelf has ($_->{itemnumber} for $_->{borrowernumber}). this is ($itemnumber, $self->{patron}->{borrowernumber})"; + $debug and warn sprintf( "shelf has (%s for %s). this is (%is, %s)", $_->{itemnumber}, $_->{borrowernumber}, $itemnumber, $patron->borrowernumber ); ($_->{itemnumber} eq $itemnumber) or next; # skip it if not this item - ($_->{borrowernumber} == $self->{patron}->{borrowernumber}) and last; + ($_->{borrowernumber} == $patron->borrowernumber) and last; # if item was waiting for this patron, we're done. AddIssue takes care of the "W" hold. $debug and warn "Item is on hold shelf for another patron."; $self->screen_msg("Item is on hold shelf for another patron."); $noerror = 0; } - my ($fee, undef) = GetIssuingCharges($itemnumber, $self->{patron}->{borrowernumber}); + my ($fee, undef) = GetIssuingCharges($itemnumber, $patron->borrowernumber); if ( $fee > 0 ) { $self->{sip_fee_type} = '06'; $self->{fee_amount} = sprintf '%.2f', $fee; @@ -126,16 +123,15 @@ sub do_checkout { $noerror = 0; } } - unless ($noerror) { - $debug and warn "cannot issue: " . Dumper($issuingimpossible) . "\n" . Dumper($needsconfirmation); - $self->ok(0); - return $self; - } - # can issue - $debug and warn "do_checkout: calling AddIssue(\$borrower,$barcode, $overridden_duedate, 0)\n" - # . "w/ \$borrower: " . Dumper($borrower) - . "w/ C4::Context->userenv: " . Dumper(C4::Context->userenv); - my $issue = AddIssue( $borrower, $barcode, $overridden_duedate, 0 ); + unless ($noerror) { + $debug and warn "cannot issue: " . Dumper($issuingimpossible) . "\n" . Dumper($needsconfirmation); + $self->ok(0); + return $self; + } + # can issue + $debug and warn sprintf("do_checkout: calling AddIssue(%s, %s, %s, 0)\n", $patron->borrowernumber, $barcode, $overridden_duedate) + . "w/ C4::Context->userenv: " . Dumper(C4::Context->userenv); + my $issue = AddIssue( $patron->unblessed, $barcode, $overridden_duedate, 0 ); $self->{due} = $self->duedatefromissue($issue, $itemnumber); $self->ok(1); diff --git a/C4/SIP/ILS/Transaction/Hold.pm b/C4/SIP/ILS/Transaction/Hold.pm index c86011e734..c14a1c8b4e 100644 --- a/C4/SIP/ILS/Transaction/Hold.pm +++ b/C4/SIP/ILS/Transaction/Hold.pm @@ -37,17 +37,12 @@ sub queue_position { sub do_hold { my $self = shift; - unless ( $self->{patron} ) { + my $patron = Koha::Patrons->find( $self->{patron}->borrowernumber ); + unless ( $patron ) { $self->screen_msg('do_hold called with undefined patron'); $self->ok(0); return $self; } - my $patron = Koha::Patrons->find( { cardnumber => $self->{patron}->id } ); - unless ($patron) { - $self->screen_msg( 'No borrower matches cardnumber "' . $self->{patron}->id . '".' ); - $self->ok(0); - return $self; - } my $item = Koha::Items->find({ barcode => $self->{item}->id }); unless ($item) { $self->screen_msg( 'No biblio record matches barcode "' . $self->{item}->id . '".' ); @@ -83,14 +78,9 @@ sub do_hold { sub drop_hold { my $self = shift; - unless ($self->{patron}) { - $self->screen_msg('drop_hold called with undefined patron'); - $self->ok(0); - return $self; - } - my $patron = Koha::Patrons->find( { cardnumber => $self->{patron}->id } ); + my $patron = Koha::Patrons->find( $self->{patron}->borrowernumber ); unless ($patron) { - $self->screen_msg('No borrower matches cardnumber "' . $self->{patron}->id . '".'); + $self->screen_msg('drop_hold called with undefined patron'); $self->ok(0); return $self; } @@ -108,14 +98,9 @@ sub drop_hold { sub change_hold { my $self = shift; - unless ($self->{patron}) { - $self->screen_msg('change_hold called with undefined patron'); - $self->ok(0); - return $self; - } - my $patron = Koha::Patrons->find( { cardnumber => $self->{patron}->id } ); + my $patron = Koha::Patrons->find( $self->{patron}->borrowernumber ); unless ($patron) { - $self->screen_msg('No borrower matches cardnumber "' . $self->{patron}->id . '".'); + $self->screen_msg('change_hold called with undefined patron'); $self->ok(0); return $self; } diff --git a/C4/SIP/ILS/Transaction/Renew.pm b/C4/SIP/ILS/Transaction/Renew.pm index af96f93901..7a21c89144 100644 --- a/C4/SIP/ILS/Transaction/Renew.pm +++ b/C4/SIP/ILS/Transaction/Renew.pm @@ -61,7 +61,8 @@ sub do_renew_for { sub do_renew { my $self = shift; - my $patron = Koha::Patrons->find( { cardnumber => $self->{patron}->id } ); + my $patron = Koha::Patrons->find( $self->{patron}->borrowernumber ); + $patron or return; # FIXME we should log that return $self->do_renew_for($patron->unblessed); } -- 2.39.5