In C4::Reserves::CheckReserves we have: next if (($branchitemrule->{'holdallowed'} eq 'from_home_library') && ($branch ne $patron->branchcode)); Where $branch is the return of my $branch = GetReservesControlBranch( $item->unblessed, $patron->unblessed ); When ReservesControlBranch is set to "patron's home library" this boils down to: $patron->branchcode ne $patron->branchcode which will never be true In all other locations we ue ReservesControlBranch to fetch the rule, but always compare the item's homebranch to the patron's branchcode: 543 if ( $branchitemrule->{holdallowed} eq 'from_home_library' 544 && $borrower->{branchcode} ne $item->homebranch ) 545 { 546 return { status => 'cannotReserveFromOtherBranches' }; 547 } 1409 || $branchitemrule->{holdallowed} eq 'from_home_library' && $param->{patron}->branchcode ne $i->homebranch
Created attachment 121542 [details] [review] Bug 28503: Unit tests
Created attachment 121543 [details] [review] 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
Created attachment 121544 [details] [review] Bug 28503: Clarify what ReservesControlBranch controls
Lowering severity, if I understand correctly it's an old issue (not a recent regression).
Created attachment 121614 [details] [review] Bug 28503: Unit tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 121615 [details] [review] 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>
Created attachment 121616 [details] [review] Bug 28503: Clarify what ReservesControlBranch controls Signed-off-by: David Nind <david@davidnind.com>
Tomas, this is missing your signed-off-by patches!
Created attachment 121991 [details] [review] Bug 28503: Unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 121992 [details] [review] 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>
Created attachment 121993 [details] [review] Bug 28503: Clarify what ReservesControlBranch controls [squashed] Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 121994 [details] [review] Bug 28503: (follow-up) Get rid of tests warnings Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #8) > Tomas, this is missing your signed-off-by patches! Oops :-D
Comment on attachment 121993 [details] [review] Bug 28503: Clarify what ReservesControlBranch controls [squashed] Patch will be squashed when pushed.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.01
Pushed to 20.11.x for 20.11.07 I added a followup on tests because 20.11.x does not contain Bug 27069
Backported: Pushed to 20.05.x branch for 20.05.13
Nope. t/db_dependent/Circulation.t .. 1/50 # Failed test 'Cannot renew, reserved' # at t/db_dependent/Circulation.t line 353. # got: '1' # expected: '0' # Failed test 'Cannot renew, reserved (returned error is on_reserve)' # at t/db_dependent/Circulation.t line 354. # got: undef # expected: 'on_reserve' # Looks like you failed 2 tests of 5. t/db_dependent/Circulation.t .. 15/50 # Failed test 'CanBookBeRenewed AllowRenewalIfOtherItemsAvailable multiple borrowers and items tests' -------------- Please investigate this failure if these patches are needed for 20.05.x
I revert from 20.11.x in order to have a safe release 20.11.07 I could not find a good reason of this failure. If needed, please provide patches for 20.11.x
Created attachment 122282 [details] [review] 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
BTW changes misc/cronjobs/update_patrons_category.pl : Options: + --help brief help message Minor but maybe not on purpose.
so in 20.11.x there is also a change in t/db_dependent/Circulation.t : 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( Can we explain here why ?
I will release 20.11.07 without this so we can have time to validate
Should this be backported to 19.11.x? For 19.11.20 (next release in July)?
Should this be backported to 19.11.x for 19.11.20?
I would say that all >= to major should. But if it conflicts and is hard to test then just leave it until someone says they need it. In that case they should help test it ^^
(In reply to Victor Grousset/tuxayo from comment #27) > I would say that all >= to major should. But if it conflicts and is hard to > test then just leave it until someone says they need it. In that case they > should help test it ^^ Thanks Victor, there were some conflicts so I won't backport to 19.11.x this unless someone requests it.