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 <galen.charlton@liblime.com>

diff --git a/C4/SIP/ILS.pm b/C4/SIP/ILS.pm
index 15de403..1a4fc54 100644 (file)
--- 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 04b7e41..2107458 100644 (file)
--- 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 86d0bac..257f9e2 100644 (file)
--- a/C4/SIP/ILS/Patron.pm
+++ b/C4/SIP/ILS/Patron.pm
@@ -319,7 +319,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 3222b65..f4f7ff8 100644 (file)
--- a/C4/SIP/ILS/Transaction/Checkout.pm
+++ b/C4/SIP/ILS/Transaction/Checkout.pm
@@ -23,7 +23,7 @@ use C4::Debug;
 use vars qw($VERSION @ISA $debug);
 
 BEGIN {
-       $VERSION = 1.02;
+       $VERSION = 1.03;
        @ISA = qw(ILS::Transaction);
 }
 
@@ -62,23 +62,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;
        }