Bug 28503

Summary: When ReservesControlBranch = "patron's home library" and Hold policy = "From home library" all holds are allowed
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: Pushed to stable --- QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart+koha, joonas.kylmala, katrin.fischer, kyle, martin.renvoize, tomascohen, victor, wainuiwitikapark
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5800
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01
Attachments: Bug 28503: Unit tests
Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library'
Bug 28503: Clarify what ReservesControlBranch controls
Bug 28503: Unit tests
Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library'
Bug 28503: Clarify what ReservesControlBranch controls
Bug 28503: Unit tests
Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library'
Bug 28503: Clarify what ReservesControlBranch controls [squashed]
Bug 28503: (follow-up) Get rid of tests warnings
Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library'

Description Nick Clemens 2021-06-02 15:04:48 UTC
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
Comment 1 Nick Clemens 2021-06-02 15:28:58 UTC
Created attachment 121542 [details] [review]
Bug 28503: Unit tests
Comment 2 Nick Clemens 2021-06-02 15:29:01 UTC
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
Comment 3 Nick Clemens 2021-06-02 15:52:12 UTC
Created attachment 121544 [details] [review]
Bug 28503: Clarify what ReservesControlBranch controls
Comment 4 Jonathan Druart 2021-06-03 07:41:34 UTC
Lowering severity, if I understand correctly it's an old issue (not a recent regression).
Comment 5 David Nind 2021-06-03 22:58:29 UTC
Created attachment 121614 [details] [review]
Bug 28503: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2021-06-03 22:58:36 UTC
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>
Comment 7 David Nind 2021-06-03 22:58:44 UTC
Created attachment 121616 [details] [review]
Bug 28503: Clarify what ReservesControlBranch controls

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Jonathan Druart 2021-06-15 09:47:50 UTC
Tomas, this is missing your signed-off-by patches!
Comment 9 Tomás Cohen Arazi 2021-06-15 12:32:49 UTC
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>
Comment 10 Tomás Cohen Arazi 2021-06-15 12:32:54 UTC
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>
Comment 11 Tomás Cohen Arazi 2021-06-15 12:33:00 UTC
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>
Comment 12 Tomás Cohen Arazi 2021-06-15 12:33:06 UTC
Created attachment 121994 [details] [review]
Bug 28503: (follow-up) Get rid of tests warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2021-06-15 12:34:08 UTC
(In reply to Jonathan Druart from comment #8)
> Tomas, this is missing your signed-off-by patches!

Oops :-D
Comment 14 Jonathan Druart 2021-06-15 13:52:16 UTC
Comment on attachment 121993 [details] [review]
Bug 28503: Clarify what ReservesControlBranch controls [squashed]

Patch will be squashed when pushed.
Comment 15 Jonathan Druart 2021-06-15 14:29:02 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Kyle M Hall 2021-06-18 15:18:08 UTC
Pushed to 21.05.x for 21.05.01
Comment 17 Fridolin Somers 2021-06-21 13:43:04 UTC
Pushed to 20.11.x for 20.11.07

I added a followup on tests because 20.11.x does not contain Bug 27069
Comment 18 Victor Grousset/tuxayo 2021-06-22 02:43:02 UTC
Backported: Pushed to 20.05.x branch for 20.05.13
Comment 19 Victor Grousset/tuxayo 2021-06-22 04:31:33 UTC
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
Comment 20 Fridolin Somers 2021-06-22 12:23:00 UTC
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
Comment 21 Nick Clemens 2021-06-22 13:27:51 UTC
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
Comment 22 Fridolin Somers 2021-06-22 13:54:24 UTC
BTW changes misc/cronjobs/update_patrons_category.pl :

 Options:
+
    --help                   brief help message

Minor but maybe not on purpose.
Comment 23 Fridolin Somers 2021-06-22 14:36:17 UTC
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 ?
Comment 24 Fridolin Somers 2021-06-22 14:51:04 UTC
I will release 20.11.07 without this so we can have time to validate
Comment 25 wainuiwitikapark 2021-06-24 02:29:49 UTC
Should this be backported to 19.11.x? For 19.11.20 (next release in July)?
Comment 26 wainuiwitikapark 2021-07-07 02:10:45 UTC
Should this be backported to 19.11.x for 19.11.20?
Comment 27 Victor Grousset/tuxayo 2021-07-07 02:51:37 UTC
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 ^^
Comment 28 wainuiwitikapark 2021-07-08 02:05:54 UTC
(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.