From 75ff7c6ade271998becbe4aa6975e6e0b217d1e5 Mon Sep 17 00:00:00 2001 From: Henri-Damien LAURENT Date: Wed, 12 Nov 2008 14:47:05 +0100 Subject: [PATCH] SIP Holds processing on checkout Allow valid comparison of hold_queue to current user barcode and permit checkout if the current user is at the front of the queue. Effectively, this allows a user to checkout a book he has held. Here are the example checkout (11) and checkout response (12) statements in a SIP telnet session, showing failure (120) previously and success (121) after patch application. Before: 11YN20060329 203000 AOCPL|AA1|AB502326000820|ACterm1 120NUN20081009 140222AOCPL|AA1|AB502326000820|AJKidnapped :|AH|AF2008-10-09 : Koha Admin (1)|BLY| After: 11YN20060329 203000 AOCPL|AA1|AB502326000820|ACterm1 121NNY20081009 150204AOCPL|AA1|AB502326000820|AJKidnapped :|AH2008-10-10|AFItem was reserved for you.| This patch also resolves security/privacy issues related to the display of "needsconfirmation" values that identify, for example, the user currently issued an item, or with a hold on the item. These messages should not be passed to an end-user interface. Signed-off-by: Galen Charlton --- C4/SIP/ILS.pm | 4 +- C4/SIP/ILS/Item.pm | 100 ++++++++++++++++++----------- C4/SIP/ILS/Patron.pm | 2 +- C4/SIP/ILS/Transaction/Checkout.pm | 26 +++++--- 4 files changed, 84 insertions(+), 48 deletions(-) diff --git a/C4/SIP/ILS.pm b/C4/SIP/ILS.pm index 15de40373a..1a4fc542b9 100644 --- a/C4/SIP/ILS.pm +++ b/C4/SIP/ILS.pm @@ -138,7 +138,7 @@ sub checkout { $circ->screen_msg("Patron Blocked"); } elsif (!$item) { $circ->screen_msg("Invalid Item"); - } elsif ($item->hold_queue && @{$item->hold_queue} && ($patron_id ne $item->hold_queue->[0])) { + } elsif ($item->hold_queue && @{$item->hold_queue} && ! $item->barcode_is_borrowernumber($patron_id, $item->hold_queue->[0]->{borrowernumber})) { $circ->screen_msg("Item on Hold for Another User"); } elsif ($item->{patron} && ($item->{patron} ne $patron_id)) { # I can't deal with this right now @@ -302,7 +302,7 @@ sub cancel_hold { # record but not on the item record, we'll treat that as success. foreach my $i (0 .. scalar @{$item->hold_queue}) { $hold = $item->hold_queue->[$i]; - if ($hold->{patron_id} eq $patron->id) { + if ($item->barcode_is_borrowernumber($patron->id, $hold->{borrowernumber})) { # found it: delete it. splice @{$item->hold_queue}, $i, 1; last; # ?? should we keep going, in case there are multiples diff --git a/C4/SIP/ILS/Item.pm b/C4/SIP/ILS/Item.pm index 04b7e41d0d..2107458a8a 100644 --- a/C4/SIP/ILS/Item.pm +++ b/C4/SIP/ILS/Item.pm @@ -25,7 +25,7 @@ use C4::Reserves; use vars qw($VERSION @ISA @EXPORT @EXPORT_OK); BEGIN { - $VERSION = 2.00; + $VERSION = 2.10; require Exporter; @ISA = qw(Exporter); @EXPORT_OK = qw(); @@ -34,34 +34,47 @@ BEGIN { =head2 EXAMPLE our %item_db = ( - '1565921879' => { - title => "Perl 5 desktop reference", - id => '1565921879', - sip_media_type => '001', - magnetic_media => 0, - hold_queue => [], - }, - '0440242746' => { - title => "The deep blue alibi", - id => '0440242746', - sip_media_type => '001', - magnetic_media => 0, - hold_queue => [], - }, - '660' => { - title => "Harry Potter y el cáliz de fuego", - id => '660', - sip_media_type => '001', - magnetic_media => 0, - hold_queue => [], - }, - ); + '1565921879' => { + title => "Perl 5 desktop reference", + id => '1565921879', + sip_media_type => '001', + magnetic_media => 0, + hold_queue => [], + }, + '0440242746' => { + title => "The deep blue alibi", + id => '0440242746', + sip_media_type => '001', + magnetic_media => 0, + hold_queue => [ + { + itemnumber => '823', + priority => '1', + reservenotes => undef, + constrainttype => 'a', + reservedate => '2008-10-09', + found => undef, + rtimestamp => '2008-10-09 11:15:06', + biblionumber => '406', + borrowernumber => '756', + branchcode => 'CPL' + } + ], + }, + '660' => { + title => "Harry Potter y el cáliz de fuego", + id => '660', + sip_media_type => '001', + magnetic_media => 0, + hold_queue => [], + }, +); =cut sub priority_sort { - defined $a->{priority} or return -1; - defined $b->{priority} or return 1; - return $a->{priority} <=> $b->{priority}; + defined $a->{priority} or return -1; + defined $b->{priority} or return 1; + return $a->{priority} <=> $b->{priority}; } sub new { @@ -105,7 +118,7 @@ sub sip_item_properties { return $self->{sip_item_properties}; } -sub status_update { +sub status_update { # FIXME: this looks unimplemented my ($self, $props) = @_; my $status = new ILS::Transaction; $self->{sip_item_properties} = $props; @@ -133,19 +146,19 @@ sub current_location { sub sip_circulation_status { my $self = shift; if ($self->{patron}) { - return '04'; + return '04'; # charged } elsif (scalar @{$self->{hold_queue}}) { - return '08'; + return '08'; # waiting on hold shelf } else { - return '03'; - } + return '03'; # available + } # FIXME: 01-13 enumerated in spec. } sub sip_security_marker { - return '02'; + return '02'; # FIXME? 00-other; 01-None; 02-Tattle-Tape Security Strip (3M); 03-Whisper Tape (3M) } sub sip_fee_type { - return '01'; + return '01'; # FIXME? 01-09 enumerated in spec. We just use O1-other/unknown. } sub fee { @@ -173,8 +186,8 @@ sub hold_queue_position { foreach (@{$self->{hold_queue}}) { $i++; $_->{patron_id} or next; - if ($_->{patron_id} eq $patron_id) { - return $i; + if ($self->barcode_is_borrowernumber($patron_id, $_->{borrowernumber})) { + return $i; # maybe should return $_->{priority} } } return 0; @@ -201,6 +214,7 @@ sub print_line { return $self->{print_line} || ''; } +# This is a partial check of "availability". It is not supposed to check everything here. # An item is available for a patron if it is: # 1) checked out to the same patron and there's no hold queue # OR @@ -215,11 +229,25 @@ sub available { return ($count ? 0 : 1); } else { # not checked out ($count) or return 1; - ($self->{hold_queue}[0] eq $for_patron) and return 1; + ($self->barcode_is_borrowernumber($for_patron, $self->{hold_queue}[0]->{borrowernumber})) and return 1; } return 0; } +sub barcode_to_borrowernumber ($) { + my $known = shift; + (defined($known)) or return undef; + my $member = GetMember($known) or return undef; # borrowernumber is default type for GetMember lookup + return $member->{cardnumber}; +} +sub barcode_is_borrowernumber ($$$) { # because hold_queue only has borrowernumber... + my $self = shift; # not really used + my $barcode = shift; + my $number = shift or return undef; # can't be zero + (defined($barcode)) or return undef; # might be 0 or 000 or 000000 + my $converted = barcode_to_borrowernumber($barcode) or return undef; + return ($number eq $converted); # even though both *should* be numbers, eq is safer. +} 1; __END__ diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm index 498c70e3db..bf0e7a8166 100644 --- a/C4/SIP/ILS/Patron.pm +++ b/C4/SIP/ILS/Patron.pm @@ -314,7 +314,7 @@ sub enable { syslog("LOG_DEBUG", "Patron(%s)->enable: charge: %s, renew:%s, recall:%s, hold:%s", $self->{id}, $self->{charge_ok}, $self->{renew_ok}, $self->{recall_ok}, $self->{hold_ok}); - $self->{screen_msg} = "All privileges restored."; + $self->{screen_msg} = "All privileges restored."; # FIXME: not really affecting patron record return $self; } diff --git a/C4/SIP/ILS/Transaction/Checkout.pm b/C4/SIP/ILS/Transaction/Checkout.pm index 048a9c9ab4..d62d3c46b0 100644 --- a/C4/SIP/ILS/Transaction/Checkout.pm +++ b/C4/SIP/ILS/Transaction/Checkout.pm @@ -22,7 +22,7 @@ use C4::Members; use vars qw($VERSION @ISA $debug); BEGIN { - $VERSION = 1.01; + $VERSION = 1.02; @ISA = qw(ILS::Transaction); $debug = 0; } @@ -59,23 +59,31 @@ sub do_checkout { my $noerror=1; foreach ( keys %$issuingimpossible ) { # do something here so we pass these errors - $self->screen_msg($issuingimpossible->{$_}); + $self->screen_msg($_ . ': ' . $issuingimpossible->{$_}); $noerror = 0; } foreach my $confirmation ( keys %$needsconfirmation ) { if ($confirmation eq 'RENEW_ISSUE'){ - my ($renewokay,$renewerror)= CanBookBeRenewed($borrower->{borrowernumber},$self->{item}->{itemnumber}); - if (! $renewokay){ - $noerror = 0; - warn "cannot renew $borrower->{borrowernumber} $self->{item}->{itemnumber} $renewerror"; - } - } else { + $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); + if ($x) { + $self->screen_msg("Item was reserved for you."); + } else { + $self->screen_msg("Item is reserved 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; + } elsif ($confirmation eq 'DEBT') { # don't do anything, it's the minor debt, and alarms fire elsewhere + } else { $self->screen_msg($needsconfirmation->{$confirmation}); $noerror = 0; } } unless ($noerror) { - warn "cannot issue: " . Dumper $issuingimpossible . "\n" . $needsconfirmation; + warn "cannot issue: " . Dumper($issuingimpossible) . "\n" . Dumper($needsconfirmation); $self->ok(0); return $self; } -- 2.39.5