Bugzilla – Attachment 21924 Details for
Bug 11020
Transfer limits should be checked when placing a hold/reservation in Staff client
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 11020 [ENH] - Transfer limits should be checked when placing a hold/reservation in Staff client
Bug-11020-ENH---Transfer-limits-should-be-checked-.patch (text/plain), 20.02 KB, created by
Olli-Antti Kivilahti
on 2013-10-09 15:40:57 UTC
(
hide
)
Description:
Bug 11020 [ENH] - Transfer limits should be checked when placing a hold/reservation in Staff client
Filename:
MIME Type:
Creator:
Olli-Antti Kivilahti
Created:
2013-10-09 15:40:57 UTC
Size:
20.02 KB
patch
obsolete
>From 40544fa19873df553c7b04809113d5fbc3d6805e Mon Sep 17 00:00:00 2001 >From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> >Date: Wed, 9 Oct 2013 18:39:33 +0300 >Subject: [PATCH] Bug 11020 [ENH] - Transfer limits should be checked when > placing a hold/reservation in Staff client > >--- > C4/Circulation.pm | 6 +- > C4/Reserves.pm | 11 +-- > .../prog/en/modules/reserve/request.tt | 79 +++++++++++++++++++--- > .../opac-tmpl/prog/en/modules/opac-reserve.tt | 2 +- > opac/opac-reserve.pl | 2 +- > reserve/placerequest.pl | 79 ++++++++++++++-------- > reserve/request.pl | 40 ++++++++--- > 7 files changed, 162 insertions(+), 57 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index c6cdd08..3297115 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -3256,8 +3256,8 @@ sub IsBranchTransferAllowed { > > A convenience function to easily check item transfer limits. > >- $allowed = CheckBranchTransferAllowed( $toBranch, $fromBranch, $item, $biblioitem ); >- $allowed = CheckBranchTransferAllowed( $toBranch, undef, $item, undef] ); >+ ($allowed, $errorMessage) = CheckBranchTransferAllowed( $toBranch, $fromBranch, $item, $biblioitem ); >+ ($allowed, $errorMessage) = CheckBranchTransferAllowed( $toBranch, undef, $item, undef] ); > > Checks if UseBranchTransferLimits global preference is in use. > Checks if the given item can be transferred from $fromBranch to $toBranch. >@@ -3333,7 +3333,7 @@ sub CheckBranchTransferAllowed { > > ## If a row is found, then that combination is not allowed, if no matching row is found, then the combination *is allowed* > if ( $limit->{'limitId'} ) { >- return 0, $toBranch . "::" . $code; >+ return 0, "$fromBranch -> $toBranch " . " :: " . $code; > } else { > return 1; > } >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 519053c..5b7c218 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -418,22 +418,23 @@ C<$pickupLibrary> OPTIONAL, undef OR the branchcode of the item transfer destina > sub CanBookBeReserved{ > my ($borrowernumber, $biblionumber, $pickupLibrary) = @_; > >- my $items = GetItemnumbersForBiblio($biblionumber); >+ my $itemNumbers = GetItemnumbersForBiblio($biblionumber); > #get items linked via host records > my @hostitems = get_hostitemnumbers_of($biblionumber); > if (@hostitems){ >- push (@$items,@hostitems); >+ push (@$itemNumbers,@hostitems); > } > >- foreach my $item (@$items){ >+ foreach my $itemNumber (@$itemNumbers){ > > if (defined $pickupLibrary) { >+ my $item = GetItem($itemNumber); > return 1 if ( CanItemBeReserved($borrowernumber, $item) and >- CheckBranchTransferLimits($pickupLibrary, $item->{holdingbranch}, $item, undef) >+ C4::Circulation::CheckBranchTransferAllowed($pickupLibrary, $item->{holdingbranch}, $item, undef) > ); > } > else { >- return 1 if ( CanItemBeReserved($borrowernumber, $item) ); >+ return 1 if ( CanItemBeReserved($borrowernumber, $itemNumber) ); > } > } > return 0; >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt >index 20853b3..accf5fe 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt >@@ -130,6 +130,19 @@ function checkMultiHold() { > }); > > $('#hold-request-form').preventDoubleFormSubmit(); >+ $('#reloadPickupLocationForm').preventDoubleFormSubmit(); >+ >+ //Setting the trigger to reload branchTransferLimits >+ $('#pickup').change(function() { >+ var newSelection = $(this).val(); >+ >+ //Make sure the pickup location is changed to the first select as well. >+ // The branch selection uses only the first given pickuplocations branch value. >+ $('#reloadPickupLocationForm > input[name="pickup"]').val(newSelection); >+ >+ $('#reloadPickupLocationForm').submit(); >+ }); >+ > > [% UNLESS ( borrowernumber || borrower_list ) %] > [% IF ( CircAutocompl ) %] >@@ -352,10 +365,15 @@ function checkMultiHold() { > [% END %] > > </ol> >- [% UNLESS ( multi_hold ) %] >+ >+ >+ >+ [% UNLESS ( multi_hold ) %] > <fieldset class="action"> > [% IF ( borrowernumber ) %] >- [% IF ( override_required ) %] >+ [% IF ( bookBranchTransferDenied ) %] >+ <input type="submit" disabled="disabled" value="Place hold" /> >+ [% ELSIF ( override_required ) %] > <input type="submit" class="warning" value="Place hold" /> > [% ELSIF ( none_available ) %] > <input type="submit" disabled="disabled" value="Place hold" /> >@@ -367,11 +385,17 @@ function checkMultiHold() { > [% FOREACH bibitemloo IN bibitemloop %] > <ol> > [% UNLESS ( item_level_itypes ) %] >- <li><span class="label">Item type:</span> [% bibitemloo.description %]</li> >+ <li><span class="label">Item type:</span> [% bibitemloo.description %]</li> > [% END %] > >- [% IF ( bibitemloo.publicationyear ) %]<li><span class="label">Publication year:</span> [% bibitemloo.publicationyear %]</li>[% END %] >+ [% IF ( bibitemloo.publicationyear ) %] >+ <li><span class="label">Publication year:</span> [% bibitemloo.publicationyear %]</li> >+ [% END %] > </ol> >+ >+ [% IF ( bibitemloo.suggestAnotherPickupLocation ) %] >+ <div id="suggestAnotherPickupLocation_[% bibitemloo.biblionumber %]" class="dialog alert">None of the available items can be transferred to your pickup location. You could try another pickup location.</div> >+ [% END %] > > <table id="requestspecific"> > <caption>Place a hold on a specific copy</caption> >@@ -396,12 +420,15 @@ function checkMultiHold() { > <td> > [% IF ( itemloo.available ) %] > <input type="radio" name="checkitem" value="[% itemloo.itemnumber %]" /> >+ [% ELSIF ( itemloo.branchTransferDenied ) %] >+ <input disabled="disabled" type="radio" name="checkitem" value="[% itemloo.itemnumber %]" /> >+ <img src="/intranet-tmpl/[% theme %]/img/famfamfam/silk/cross.png" title="Cannot transfer this item to your pickup location" alt="Cannot transfer this item to your pickup location" /> > [% ELSIF ( itemloo.override ) %] > <input type="radio" name="checkitem" class="needsoverride" value="[% itemloo.itemnumber %]" /> >- <img src="/intranet-tmpl/[% theme %]/img/famfamfam/silk/error.png" alt="Requires override of hold policy" /> >+ <img src="/intranet-tmpl/[% theme %]/img/famfamfam/silk/error.png" title="Requires override of hold policy" alt="Requires override of hold policy" /> > [% ELSE %] > <input disabled="disabled" type="radio" name="checkitem" value="[% itemloo.itemnumber %]" /> >- <img src="/intranet-tmpl/[% theme %]/img/famfamfam/silk/cross.png" alt="Cannot be put on hold" /> >+ <img src="/intranet-tmpl/[% theme %]/img/famfamfam/silk/cross.png" title="Cannot be put on hold" alt="Cannot be put on hold" /> > [% END %] > </td> > [% IF ( item_level_itypes ) %] >@@ -475,6 +502,7 @@ function checkMultiHold() { > [% END %] <!-- bibitemloop --> > > [% ELSE %]<!-- UNLESS multi_hold --> >+ > > <table id="requesttitles"> > <tr> >@@ -493,7 +521,12 @@ function checkMultiHold() { > [% END %] > <td> > <ul> >- <li><a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblioloo.biblionumber %]">[% biblioloo.title |html %]</a></li> >+ <li> >+ [% IF ( biblioloo.suggestAnotherPickupLocation ) %] >+ <img src="/intranet-tmpl/[% theme %]/img/famfamfam/silk/cross.png" title="Cannot transfer this item to your pickup location" alt="Cannot transfer this item to your pickup location" /> >+ [% END %] >+ <a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblioloo.biblionumber %]">[% biblioloo.title |html %]</a> >+ </li> > [% IF ( biblioloo.publicationyear ) %] > <li><span class="label">Publication year:</span> [% biblioloo.publicationyear %]</li> > [% END %] >@@ -538,12 +571,20 @@ function checkMultiHold() { > </tr> > [% END %] > </table> >+ >+ [% IF ( bookBranchTransferDenied ) %] >+ <div class="dialog alert"> >+ Some books cannot be transferred to your pickup location. Please try another pickup location, or reserve fewer items at once. >+ </div> >+ [% END %] > > [% END %]<!-- /multi_hold --> > > <fieldset class="action"> > [% IF ( borrowernumber ) %] >- [% IF ( override_required ) %] >+ [% IF ( bookBranchTransferDenied ) %] >+ <input type="submit" disabled="disabled" value="Place hold" /> >+ [% ELSIF ( override_required ) %] > <input type="submit" class="warning" value="Place hold" /> > [% ELSIF ( none_available ) %] > <input type="submit" disabled="disabled" value="Place hold" /> >@@ -765,4 +806,26 @@ function checkMultiHold() { > > </div> > </div> >+[% IF ( multi_hold ) %] >+<form id="reloadPickupLocationForm" name="reloadPickupLocationForm" method="post" action="request.pl"> >+ <input type="hidden" name="borrowernumber" value="[% borrowernumber %]" /> >+ <input type="hidden" name="type" value="str8" /> >+ <input type="hidden" name="multi_hold" value="[% multi_hold %]"/> >+ <input type="hidden" name="biblionumbers" id="multi_hold_bibs" value="[% biblionumbers %]"/> >+ <input type="hidden" name="bad_bibs" id="bad_bibs" value=""/> >+ <input type="hidden" name="request" value="any"/> >+ <input type="hidden" name="pickup" value="fromJavascript" /> >+ [% FOREACH biblioloo IN biblioloop %] >+ <input type="hidden" name="title_[% biblioloo.biblionumber %]" value="[% biblioloo.title |html %]"/> >+ <input type="hidden" name="rank_[% biblioloo.biblionumber %]" value="[% biblioloo.rank %]"/> >+ [% END %] >+</form> >+[% ELSE %] >+<form id="reloadPickupLocationForm" name="reloadPickupLocationForm" method="post" action="request.pl"> >+ <input type="hidden" name="borrowernumber" value="[% borrowernumber %]" /> >+ <input type="hidden" name="biblionumber" value="[% biblionumber %]" /> >+ <input type="hidden" name="pickup" value="fromJavascript" /> >+</form> >+[% END %] >+ > [% INCLUDE 'intranet-bottom.inc' %] >diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt >index 040bc25..5e656af 100644 >--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt >+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt >@@ -399,7 +399,7 @@ > <li class="branch"> > <label for="branch_[% bibitemloo.biblionumber %]">Pick up location:</label> > [% UNLESS ( bibitemloo.holdable ) %] >- <select onClick="alert('digg')" name="branch" id="branch_[% bibitemloo.biblionumber %]" disabled="disabled"> >+ <select name="branch" id="branch_[% bibitemloo.biblionumber %]" disabled="disabled"> > [% FOREACH branchloo IN bibitemloo.branchloop %] > [% IF ( branchloo.selected ) %] > <option value="[% branchloo.branchcode %]" selected="selected">[% branchloo.branchname %]</option> >diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl >index d53b2db..1183347 100755 >--- a/opac/opac-reserve.pl >+++ b/opac/opac-reserve.pl >@@ -406,7 +406,7 @@ foreach my $biblioNum (@biblionumbers) { > } > > #Collect the amout of items that pass the CheckBranchTransferAllowed-check. This is needed to tell >- # the Borrower if some or all Items cannot be transferred to the pickup location. >+ # the user if some or all Items cannot be transferred to the pickup location. > my $branchTransferableItemsCount = 0; > > $biblioLoopIter{itemLoop} = []; >diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl >index 3fe459c..27b6a6b 100755 >--- a/reserve/placerequest.pl >+++ b/reserve/placerequest.pl >@@ -59,6 +59,9 @@ my $multi_hold = $input->param('multi_hold'); > my $biblionumbers = $multi_hold ? $input->param('biblionumbers') : ($biblionumber . '/'); > my $bad_bibs = $input->param('bad_bibs'); > >+my $transferOk; # Used to check if this reservation passes UseBranchTransferLimits check. >+my $errorMessages; # Used to concatenate generated error messages from multiple biblios. >+ > my %bibinfos = (); > my @biblionumbers = split '/', $biblionumbers; > foreach my $bibnum (@biblionumbers) { >@@ -67,12 +70,11 @@ foreach my $bibnum (@biblionumbers) { > $bibinfo{rank} = $input->param("rank_$bibnum"); > $bibinfos{$bibnum} = \%bibinfo; > } >- > my $found; > > # if we have an item selectionned, and the pickup branch is the same as the holdingbranch > # of the document, we force the value $rank and $found . >-if ($checkitem ne ''){ >+if (defined $checkitem && $checkitem ne ''){ > $rank[0] = '0' unless C4::Context->preference('ReservesNeedReturns'); > my $item = $checkitem; > $item = GetItem($item); >@@ -97,35 +99,52 @@ if ($type eq 'str8' && $borrower){ > } > } > my $const; >- >- if ($checkitem ne ''){ >- my $item = GetItem($checkitem); >- if ($item->{'biblionumber'} ne $biblionumber) { >- $biblionumber = $item->{'biblionumber'}; >- } >- } >- >- >- >- if ($multi_hold) { >- my $bibinfo = $bibinfos{$biblionumber}; >- AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',[$biblionumber], >- $bibinfo->{rank},$startdate,$expirationdate,$notes,$bibinfo->{title},$checkitem,$found); >- } else { >- if ($input->param('request') eq 'any'){ >- # place a request on 1st available >- AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem,$found); >- } elsif ($reqbib[0] ne ''){ >- # FIXME : elsif probably never reached, (see top of the script) >- # place a request on a given item >- AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'o',\@reqbib,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); >- } else { >- AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); >- } >- } >+ >+ if (defined $checkitem && $checkitem ne ''){ >+ my $item = GetItem($checkitem); >+ >+ #Making sure the biblionumber is present. Pretty robust! And possibly scary? >+ if ($item->{'biblionumber'} ne $biblionumber) { >+ $biblionumber = $item->{'biblionumber'}; >+ } >+ my $errorMsg; >+ ($transferOk, $errorMsg) = CheckBranchTransferAllowed($branch, $item->{holdingbranch}, $item, undef); >+ if (! $transferOk) { >+ $errorMessages .= $errorMsg . ', '; >+ } >+ } >+ else { >+ #Final check if UseBranchTransferLimits validate for this Biblio. >+ $transferOk = CanBookBeReserved( $borrowernumber, $biblionumber, $branch ); >+ } >+ >+ >+ if ($transferOk) { >+ >+ if ($multi_hold) { >+ my $bibinfo = $bibinfos{$biblionumber}; >+ AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',[$biblionumber], >+ $bibinfo->{rank},$startdate,$expirationdate,$notes,$bibinfo->{title},$checkitem,$found); >+ } >+ else { >+ if ($input->param('request') eq 'any'){ >+ # place a request on 1st available >+ AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem,$found); >+ } elsif ($reqbib[0] ne ''){ >+ # FIXME : elsif probably never reached, (see top of the script) >+ # place a request on a given item >+ AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'o',\@reqbib,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); >+ } else { >+ AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); >+ } >+ } >+ } > } >- >- if ($multi_hold) { >+ if ($errorMessages) { >+ print $input->header(); >+ print "The following branch transfer limits are blocked:\n".$errorMessages; >+ } >+ elsif ($multi_hold) { > if ($bad_bibs) { > $biblionumbers .= $bad_bibs; > } >diff --git a/reserve/request.pl b/reserve/request.pl >index c411ce9..d3c6651 100755 >--- a/reserve/request.pl >+++ b/reserve/request.pl >@@ -69,6 +69,7 @@ my $userbranch = ''; > if (C4::Context->userenv && C4::Context->userenv->{'branch'}) { > $userbranch = C4::Context->userenv->{'branch'}; > } >+my $pickupBranch = $input->param('pickup') ? $input->param('pickup') : $userbranch; > > > # Select borrowers infos >@@ -282,12 +283,16 @@ foreach my $biblionumber (@biblionumbers) { > my $num_available = 0; > my $num_override = 0; > my $hiddencount = 0; >+ >+ #Collect the amout of items that pass the CheckBranchTransferAllowed-check. This is needed to tell >+ # the user if some or all Items cannot be transferred to the pickup location. >+ my $branchTransferableItemsCount = 0; > > $biblioitem->{description} = > $itemtypes->{ $biblioitem->{itemtype} }{description}; >- if($biblioitem->{biblioitemnumber} ne $biblionumber){ >- $biblioitem->{hostitemsflag}=1; >- } >+ if($biblioitem->{biblioitemnumber} ne $biblionumber){ >+ $biblioitem->{hostitemsflag}=1; >+ } > $biblioloopiter{description} = $biblioitem->{description}; > $biblioloopiter{itypename} = $biblioitem->{description}; > $biblioloopiter{imageurl} = >@@ -311,10 +316,10 @@ foreach my $biblionumber (@biblionumbers) { > $branches->{ $item->{holdingbranch} }{branchname}; > } > >- if($item->{biblionumber} ne $biblionumber){ >- $item->{hostitemsflag}=1; >- $item->{hosttitle} = GetBiblioData($item->{biblionumber})->{title}; >- } >+ if($item->{biblionumber} ne $biblionumber){ >+ $item->{hostitemsflag}=1; >+ $item->{hosttitle} = GetBiblioData($item->{biblionumber})->{title}; >+ } > > # add information > $item->{itemcallnumber} = $item->{itemcallnumber}; >@@ -389,6 +394,17 @@ foreach my $biblionumber (@biblionumbers) { > } > } > } >+ >+ # UseBranchTransferLimits checking >+ my ($transferOk, $errorMessage) = CheckBranchTransferAllowed( $pickupBranch, $item->{holdingbranch}, $item, undef ); >+ if ( ! $transferOk ) { >+ $item->{cantreserve} = 1 ; >+ $item->{branchTransferDenied} = 1 ; >+ } >+ else { >+ $branchTransferableItemsCount++; >+ } >+ > > my $branch = C4::Circulation::_GetCircControlBranch($item, $borrowerinfo); > >@@ -417,7 +433,7 @@ foreach my $biblionumber (@biblionumbers) { > } > elsif ( C4::Context->preference('AllowHoldPolicyOverride') ) { > >-# If AllowHoldPolicyOverride is set, it should override EVERY restriction, not just branch item rules >+ # If AllowHoldPolicyOverride is set, it should override EVERY restriction, not just branch item rules > $item->{override} = 1; > $num_override++; > } >@@ -446,6 +462,12 @@ foreach my $biblionumber (@biblionumbers) { > $biblioloopiter{warn} = 1; > $biblioloopiter{none_avail} = 1; > } >+ if ( $branchTransferableItemsCount == 0) { >+ $biblioloopiter{suggestAnotherPickupLocation} = 1; >+ $biblioitem->{suggestAnotherPickupLocation} = 1; >+ $template->param( bookBranchTransferDenied => 1 ); >+ } >+ > $template->param( hiddencount => $hiddencount); > > push @bibitemloop, $biblioitem; >@@ -538,7 +560,7 @@ foreach my $biblionumber (@biblionumbers) { > my $time = time(); > > $template->param( >- branchloop => GetBranchesLoop($userbranch), >+ branchloop => GetBranchesLoop($pickupBranch), > time => $time, > fixedRank => $fixedRank, > ); >-- >1.8.1.2
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 11020
:
21924
|
21925
|
21997
|
21998
|
22321
|
23165