Bug 27529 - Cannot place hold on OPAC if hold_fullfillment_policy is set to group and OPACAllowUserToChooseBranch not allowed
Summary: Cannot place hold on OPAC if hold_fullfillment_policy is set to group and OP...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 22284
Blocks:
  Show dependency treegraph
 
Reported: 2021-01-22 15:22 UTC by Nick Clemens
Modified: 2021-04-25 20:46 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11


Attachments
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch (1.91 KB, patch)
2021-01-25 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch (1.91 KB, patch)
2021-03-29 18:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch (1.96 KB, patch)
2021-04-03 17:58 UTC, David Nind
Details | Diff | Splinter Review
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch (2.03 KB, patch)
2021-04-16 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-01-22 15:22:34 UTC
In opac/opac-reserve.pl:
243         # When choosing a specific item, the default pickup library should be dictated by the default hold policy
244         if ( ! C4::Context->preference("OPACAllowUserToChooseBranch") && $itemNum ) {
245             my $item = Koha::Items->find( $itemNum );
246             my $type = $item->effective_itemtype;
247             my $rule = GetBranchItemRule( $patron->branchcode, $type );
248             if ( $rule->{hold_fulfillment_policy} eq 'any' ) {
249                 $branch = $patron->branchcode;
250             } else {
251                 my $policy = $rule->{hold_fulfillment_policy};
252                 $branch = $item->$policy;
253             }   
254         }
255 

$branch = $item->$policy; is problematic for group values


To test:
1 - Set 'Default checkout, hold and return policy' -> 'Hold pickup library match' to item's hold group or patron's hold group 
2 - Set  OPACAllowUserToChooseBranch to 'Don't allow'
3 - Try to place an item level hold on the opac
4 - Ka-boom
    The method Koha::Item->patrongroup is not covered by tests!
    The method Koha::Item->itemgroup is not covered by tests!
Comment 1 Andrew Fuerste-Henry 2021-01-22 15:38:43 UTC
The behavior with hold groups should mirror the behavior without them. If we set hold_fulfillment_policy to Any Library and OPACAllowUserToChooseBranch to not allowed, Koha defaults OPAC holds to the patron's home library for the pickup location. So when hold_fulfillment_policy is set to patron's hold group, we should do the same and set to the pickup location to the patron's home library, which will by definition be in the patron's hold group and therefore a valid pickup location.
Comment 2 Lisette Scheer 2021-01-22 16:18:20 UTC
(In reply to Andrew Fuerste-Henry from comment #1)
> The behavior with hold groups should mirror the behavior without them. If we
> set hold_fulfillment_policy to Any Library and OPACAllowUserToChooseBranch
> to not allowed, Koha defaults OPAC holds to the patron's home library for
> the pickup location. So when hold_fulfillment_policy is set to patron's hold
> group, we should do the same and set to the pickup location to the patron's
> home library, which will by definition be in the patron's hold group and
> therefore a valid pickup location.

Agreed, this is what we expected when we set up hold groups.
Comment 3 Nick Clemens 2021-01-25 15:37:46 UTC
Created attachment 115782 [details] [review]
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch

This patch defaults the holds pickup location to the items homebranch or the patron's branch when a group option is selected
for the hold fulfillment policy and the patron is not allowed to choose the branch on the OPAC

To test:
1 - Set 'Default checkout, hold and return policy' -> 'Hold pickup library match' to item's hold group or patron's hold group
2 - Set  OPACAllowUserToChooseBranch to 'Don't allow'
3 - Try to place an item level hold on the opac
4 - Ka-boom, etiher:
    The method Koha::Item->patrongroup is not covered by tests!
    The method Koha::Item->itemgroup is not covered by tests!
5 - Apply patch
6 - Repeat
7 - Note the pickup location is set to either the patron's branch or the items homebranch
8 - Repeat plan with the other group setting
Comment 4 Andrew Fuerste-Henry 2021-03-29 18:07:25 UTC
I'm still getting an error after applying the patch:
The method Koha::Item->holdgroup is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Object.pm line 827
Koha::Object::AUTOLOAD('Koha::Item=HASH(0x55b488f14d30)') called at /kohadevbox/koha/opac/opac-reserve.pl line 255
eval {...} at /kohadevbox/koha/opac/opac-reserve.pl line 2

That's with Hold Pickup Library Match set to item's hold group, reservescontrol set to patron's library.
Comment 5 Nick Clemens 2021-03-29 18:34:19 UTC
Created attachment 118975 [details] [review]
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch

This patch defaults the holds pickup location to the items homebranch or the patron's branch when a group option is selected
for the hold fulfillment policy and the patron is not allowed to choose the branch on the OPAC

To test:
1 - Set 'Default checkout, hold and return policy' -> 'Hold pickup library match' to item's hold group or patron's hold group
2 - Set  OPACAllowUserToChooseBranch to 'Don't allow'
3 - Try to place an item level hold on the opac
4 - Ka-boom, etiher:
    The method Koha::Item->patrongroup is not covered by tests!
    The method Koha::Item->itemgroup is not covered by tests!
5 - Apply patch
6 - Repeat
7 - Note the pickup location is set to either the patron's branch or the items homebranch
8 - Repeat plan with the other group setting
Comment 6 David Nind 2021-04-03 17:58:44 UTC
Created attachment 119162 [details] [review]
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch

This patch defaults the holds pickup location to the items homebranch or the patron's branch when a group option is selected
for the hold fulfillment policy and the patron is not allowed to choose the branch on the OPAC

To test:
1 - Set 'Default checkout, hold and return policy' -> 'Hold pickup library match' to item's hold group or patron's hold group
2 - Set  OPACAllowUserToChooseBranch to 'Don't allow'
3 - Try to place an item level hold on the opac
4 - Ka-boom, etiher:
    The method Koha::Item->patrongroup is not covered by tests!
    The method Koha::Item->itemgroup is not covered by tests!
5 - Apply patch
6 - Repeat
7 - Note the pickup location is set to either the patron's branch or the items homebranch
8 - Repeat plan with the other group setting

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Martin Renvoize 2021-04-16 11:42:55 UTC
Created attachment 119708 [details] [review]
Bug 27529: Choose patron's branch or item's homebranch if following group rules and patron cannot choose branch

This patch defaults the holds pickup location to the items homebranch or the patron's branch when a group option is selected
for the hold fulfillment policy and the patron is not allowed to choose the branch on the OPAC

To test:
1 - Set 'Default checkout, hold and return policy' -> 'Hold pickup library match' to item's hold group or patron's hold group
2 - Set  OPACAllowUserToChooseBranch to 'Don't allow'
3 - Try to place an item level hold on the opac
4 - Ka-boom, etiher:
    The method Koha::Item->patrongroup is not covered by tests!
    The method Koha::Item->itemgroup is not covered by tests!
5 - Apply patch
6 - Repeat
7 - Note the pickup location is set to either the patron's branch or the items homebranch
8 - Repeat plan with the other group setting

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2021-04-16 11:43:41 UTC
Trivial patch works as expected, no QA script failures.

Passing QA
Comment 9 Jonathan Druart 2021-04-16 13:12:12 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Fridolin Somers 2021-04-22 14:22:16 UTC
Pushed to 20.11.x for 20.11.05
Comment 11 Andrew Fuerste-Henry 2021-04-25 16:37:26 UTC
Pushed to 20.05.x for 20.05.11
Comment 12 Victor Grousset/tuxayo 2021-04-25 20:46:02 UTC
Missing dependencies for 19.11.x, it shouldn't be affected, no backport.