Attachment 27267 Details for Bug 11258 – [PASSED QA] Bug 11258 - Holds queue is still making transfer requests that contradict the branch holds policy

[patch] [PASSED QA] Bug 11258 - Holds queue is still making transfer requests that contradict the branch holds policy

PASSED-QA-Bug-11258---Holds-queue-is-still-making-.patch (text/plain), 4.49 KB, created by Katrin Fischer on 2014-04-17 18:19:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Katrin Fischer

Created: 2014-04-17 18:19:40 UTC

Size: 4.49 KB

patch

obsolete

>From 9a8ca9ece83f8918118feda838680ab1aa21f95d Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Fri, 15 Nov 2013 08:29:23 -0800
>Subject: [PATCH] [PASSED QA] Bug 11258 - Holds queue is still making transfer
> requests that contradict the branch holds policy
>
>The holds queue generator is still making requests that are not allowed.
>Previously it was making transfer requests that were not allowed.
>Currently, it is still making requests where the pickup branch is the
>same as the item's branch but not the patron's branch,
>even if there is a "Default holds policy by item type" rule that states
>this item can only fill holds for patron's of the same branch as the item.
>
>Test Plan:
>1) Create a test record with 2 items with different itemtypes
>2) Set the Default holds policy by item type for the first
>   item to "From any library"
>3) Set the Default holds policy by item type for the second
>   item to "From home library"
>4) Place a record level hold for a patron from another library,
>   but for pickup at the same library as the item is from
>5) Rebuild the holds queue
>6) View the holds queue, note the item is listed, though this
>   patron cannot place a hold on this item
>7) Apply this patch
>8) Repeat step 5, note the hold is no longer in the queue
>
>Signed-off-by: Liz Rea <liz@catalyst.net.nz>
>automated tests pass, functional tests pass. Bug replicated, eradicated by patch.
>
>Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
>I finally managed to reproduce this, patch works as described.
>Passes tests and QA script, provided tests fail without patch, but
>succeed with the patch.
>---
> C4/HoldsQueue.pm            | 4 ++--
> t/db_dependent/HoldsQueue.t | 7 ++++---
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/C4/HoldsQueue.pm b/C4/HoldsQueue.pm
>index 63f159a..0d645bd 100755
>--- a/C4/HoldsQueue.pm
>+++ b/C4/HoldsQueue.pm
>@@ -432,7 +432,6 @@ sub MapItemsToHoldRequests {
>                 }
>             }
>             $holdingbranch = $pickup_branch;
>-            $itemnumber ||= $holding_branch_items->[0]->{itemnumber};
>         }
>         elsif ($transport_cost_matrix) {
>             $pull_branches = [keys %items_by_branch];
>@@ -467,6 +466,7 @@ sub MapItemsToHoldRequests {
>                 $holdingbranch ||= $branch;
>                 foreach my $item (@$holding_branch_items) {
>                     next if $pickup_branch ne $item->{homebranch};
>+                    next if ( $item->{holdallowed} == 1 && $item->{homebranch} ne $request->{borrowerbranch} );
> 
>                     $itemnumber = $item->{itemnumber};
>                     $holdingbranch = $branch;
>@@ -476,7 +476,7 @@ sub MapItemsToHoldRequests {
> 
>             unless ( $itemnumber ) {
>                 foreach my $current_item ( @{ $items_by_branch{$holdingbranch} } ) {
>-                    if ( $holdingbranch && ( $current_item->{holdallowed} == 2 || $pickup_branch eq $current_item->{homebranch} ) ) {
>+                    if ( $holdingbranch && ( $current_item->{holdallowed} == 2 || $request->{borrowerbranch} eq $current_item->{homebranch} ) ) {
>                         $itemnumber = $current_item->{itemnumber};
>                         last; # quit this loop as soon as we have a suitable item
>                     }
>diff --git a/t/db_dependent/HoldsQueue.t b/t/db_dependent/HoldsQueue.t
>index 92bd843..0a96143 100755
>--- a/t/db_dependent/HoldsQueue.t
>+++ b/t/db_dependent/HoldsQueue.t
>@@ -12,7 +12,7 @@ use C4::Context;
> 
> use Data::Dumper;
> 
>-use Test::More tests => 20;
>+use Test::More tests => 21;
> 
> 
> use C4::Branch;
>@@ -287,8 +287,9 @@ $dbh->do("DELETE FROM default_circ_rules");
> $dbh->do("INSERT INTO default_circ_rules ( holdallowed ) VALUES ( 1 )");
> C4::HoldsQueue::CreateQueue();
> $holds_queue = $dbh->selectall_arrayref("SELECT * FROM tmp_holdsqueue", { Slice => {} });
>-ok( @$holds_queue == 1, "Holds queue filling correct number for default holds policy 'from home library'" );
>-ok( $holds_queue->[0]->{cardnumber} eq 'CARDNUMBER1', "Holds queue filling correct hold for default holds policy 'from home library'");
>+ok( @$holds_queue == 2, "Holds queue filling correct number for default holds policy 'from home library'" );
>+ok( $holds_queue->[0]->{cardnumber} eq 'CARDNUMBER1', "Holds queue filling 1st correct hold for default holds policy 'from home library'");
>+ok( $holds_queue->[1]->{cardnumber} eq 'CARDNUMBER2', "Holds queue filling 2nd correct hold for default holds policy 'from home library'");
> 
> $dbh->do("DELETE FROM default_circ_rules");
> $dbh->do("INSERT INTO default_circ_rules ( holdallowed ) VALUES ( 2 )");
>-- 
>1.8.3.2

Actions: View | Diff | Splinter Review

Attachments on bug 11258: 22965 | 22970 | 22971 | 23011 | 25224 | 27267