[Koha-patches] [PATCH] SIP Holds processing on checkout
Galen Charlton
galen.charlton at liblime.com
Tue Oct 14 20:11:33 CEST 2008
From: Joe Atzberger (siptest <atz4sip at arwen.metavore.com>
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 at liblime.com>
---
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 15de403..1a4fc54 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 04b7e41..2107458 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 86d0bac..257f9e2 100644
--- 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
--- 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;
}
--
1.5.5.GIT
More information about the Koha-patches
mailing list