Bugzilla – Attachment 122282 Details for
Bug 28503
When ReservesControlBranch = "patron's home library" and Hold policy = "From home library" all holds are allowed
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library'
Bug-28503-Compare-item-homebranch-to-patron-branch.patch (text/plain), 10.41 KB, created by
Nick Clemens (kidclamp)
on 2021-06-22 13:27:51 UTC
(
hide
)
Description:
Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library'
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2021-06-22 13:27:51 UTC
Size:
10.41 KB
patch
obsolete
>From 832b3af36b8086db5a4b3b1c64a5e75487bb7c6e Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Wed, 2 Jun 2021 15:20:08 +0000 >Subject: [PATCH] Bug 28503: Compare item homebranch to patron branch when hold > policy set to 'from_home_library' > >This fixes an issue in the way we calculate the check for hold policy 'from_home_library' > >Currently we change the comparison based on ReservesControlBranch, however, that should >only control the rule we fetch, not how we compare > >When ReservesControlBranch is set to "patron's home library" we compare the patron's branch to >the patron's branch, this is useless and means we pass the check for all branches all of the time > >We should instead compare the patron's branch to the item's branch, and only fetch the rule using ReservesControlBranch > >To test: > 1 - Have a record with an item from library A and library B > 2 - Set the 'Default checkout, hold and return policy'->Hold policy->From home library for all libraries > and ensure you have no branch specific/itemtype specific rules set > 3 - Attempt to place a hold on the record for a patron from library B > 4 - Note that only the library B item is holdable - place a title level hold (do not choose an item) > 5 - Check in the item from library A > 6 - It fills the hold - This is incorrect - ignore the hold > 7 - Apply patch > 8 - Restart all the things > 9 - Check in the item from library A >10 - No hold found >11 - Check in the item from library B >12 - Hold found, correctly > >Signed-off-by: David Nind <david@davidnind.com> >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > >Bug 28503: Clarify what ReservesControlBranch controls > >Signed-off-by: David Nind <david@davidnind.com> >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >(cherry picked from commit 19660a25fa9421373a41fb6aba71215d71c541be) >Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> > >Bug 28503: Unit tests > >Signed-off-by: David Nind <david@davidnind.com> >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >(cherry picked from commit a4cdeaae3f82ea47fe3fba5e79f419e6911fb524) >Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> > >Bug 28503: (follow-up) Get rid of tests warnings > >Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >(cherry picked from commit 1e677a8755dfa4b5df3ff8df8f2644aedf388eb3) >Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> > >Bug 28503: [20.11.x] fix unit tests > >Impact of Bug 27069 >https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27069 > >Bug 28503: [20.11.x] Adjust bad test assumption > >The tests set holdpolicy to 'from home library', this is the case we were fixing > >Adjusted test to set policy to 'any library' and tests pass >--- > C4/Reserves.pm | 2 +- > .../admin/preferences/circulation.pref | 2 +- > misc/cronjobs/update_patrons_category.pl | 1 + > t/db_dependent/Circulation.t | 2 +- > t/db_dependent/Reserves.t | 74 ++++++++++++++++++- > 5 files changed, 74 insertions(+), 7 deletions(-) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 0e20c77ae5..0d8f4aac7e 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -894,7 +894,7 @@ sub CheckReserves { > my $branch = GetReservesControlBranch( $item->unblessed, $patron->unblessed ); > my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$item->effective_itemtype); > next if ($branchitemrule->{'holdallowed'} == 0); >- next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $patron->branchcode)); >+ next if (($branchitemrule->{'holdallowed'} == 1) && ($item->homebranch ne $patron->branchcode)); > my $library = Koha::Libraries->find({branchcode=>$item->homebranch}); > next if (($branchitemrule->{'holdallowed'} == 3) && (!$library->validate_hold_sibling({branchcode => $patron->branchcode}) )); > my $hold_fulfillment_policy = $branchitemrule->{hold_fulfillment_policy}; >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >index d290cabfa4..9742cd468c 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref >@@ -716,7 +716,7 @@ Circulation: > class: integer > - days from now) at checkin time. Note that this number of days will be used too in calculating the default end date for the Holds to pull-report. But it does not interfere with issuing, renewing or transferring items. > - >- - Check the >+ - Check the rule from the > - pref: ReservesControlBranch > choices: > ItemHomeLibrary: "item's home library" >diff --git a/misc/cronjobs/update_patrons_category.pl b/misc/cronjobs/update_patrons_category.pl >index 86541a818d..fd4bc4a7ea 100755 >--- a/misc/cronjobs/update_patrons_category.pl >+++ b/misc/cronjobs/update_patrons_category.pl >@@ -47,6 +47,7 @@ update_patrons_category.pl -f=categorycode -t=categorycode > update_patrons_category.pl --help | --man > > Options: >+ > --help brief help message > --man full documentation > -too_old update if over maximum age for current category >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index 46d629d65f..23c034bd44 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -291,7 +291,7 @@ subtest "CanBookBeRenewed AllowRenewalIfOtherItemsAvailable multiple borrowers a > branchcode => undef, > itemtype => undef, > rule_name => 'holdallowed', >- rule_value => 1 >+ rule_value => 2 > } > ); > Koha::CirculationRules->set_rule( >diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t >index 5ff4e2aacd..a0c502a574 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -1076,9 +1076,9 @@ subtest 'RevertWaitingStatus' => sub { > ); > }; > >-subtest 'CheckReserves additional test' => sub { >+subtest 'CheckReserves additional tests' => sub { > >- plan tests => 3; >+ plan tests => 8; > > my $item = $builder->build_sample_item; > my $reserve1 = $builder->build_object( >@@ -1151,6 +1151,68 @@ subtest 'CheckReserves additional test' => sub { > is( $matched_reserve->{reserve_id}, > $reserve1->reserve_id, "We got the Transit reserve" ); > is( scalar @$possible_reserves, 2, 'We do get both reserves' ); >+ >+ my $patron_B = $builder->build_object({ class => "Koha::Patrons" }); >+ my $item_A = $builder->build_sample_item; >+ my $item_B = $builder->build_sample_item({ >+ homebranch => $patron_B->branchcode, >+ biblionumber => $item_A->biblionumber, >+ itype => $item_A->itype >+ }); >+ Koha::CirculationRules->set_rules( >+ { >+ branchcode => undef, >+ categorycode => undef, >+ itemtype => $item_A->itype, >+ rules => { >+ reservesallowed => 25, >+ holds_per_record => 1, >+ } >+ } >+ ); >+ Koha::CirculationRules->set_rule({ >+ branchcode => undef, >+ itemtype => $item_A->itype, >+ rule_name => 'holdallowed', >+ rule_value => 1 >+ }); >+ my $reserve_id = AddReserve( >+ { >+ branchcode => $patron_B->branchcode, >+ borrowernumber => $patron_B->borrowernumber, >+ biblionumber => $item_A->biblionumber, >+ priority => 1, >+ itemnumber => undef, >+ } >+ ); >+ >+ ok( $reserve_id, "We can place a record level hold because one item is owned by patron's home library"); >+ t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); >+ ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); >+ is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch"); >+ Koha::CirculationRules->set_rule({ >+ branchcode => $item_A->homebranch, >+ itemtype => $item_A->itype, >+ rule_name => 'holdallowed', >+ rule_value => 2 >+ }); >+ ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); >+ is( $status, "Reserved", "We fill the hold with item A because item's branch rule says allow any"); >+ >+ >+ # Changing the control branch should change only the rule we get >+ t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); >+ ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); >+ is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch"); >+ Koha::CirculationRules->set_rule({ >+ branchcode => $patron_B->branchcode, >+ itemtype => $item_A->itype, >+ rule_name => 'holdallowed', >+ rule_value => 2 >+ }); >+ ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); >+ is( $status, "Reserved", "We fill the hold with item A because patron's branch rule says allow any"); >+ > }; > > subtest 'AllowHoldOnPatronPossession test' => sub { >@@ -1250,11 +1312,15 @@ subtest 'ModReserveAffect logging' => sub { > $hold = Koha::Holds->find($reserve_id); > is( $hold->timestamp, $previous_timestamp, 'Make sure the previous timestamp has been used' ); > >+ # Avoid warnings >+ my $reserve_mock = Test::MockModule->new('C4::Reserves'); >+ $reserve_mock->mock( '_koha_notify_reserve', undef ); >+ > # Mark it waiting > ModReserveAffect( $item->itemnumber, $patron->borrowernumber ); > >- $hold = Koha::Holds->find($reserve_id); >- is( $hold->found, 'W', 'Hold has been set waiting' ); >+ $hold->discard_changes; >+ ok( $hold->is_waiting, 'Hold has been set waiting' ); > isnt( $hold->timestamp, $previous_timestamp, 'The timestamp has been modified' ); > > my $log = Koha::ActionLogs->search({ module => 'HOLDS', action => 'MODIFY', object => $hold->reserve_id })->next; >-- >2.20.1
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 28503
:
121542
|
121543
|
121544
|
121614
|
121615
|
121616
|
121991
|
121992
|
121993
|
121994
| 122282