Bug 20837 - CanItemBeReserved should follow ReservesControlBranch and not CircControl
Summary: CanItemBeReserved should follow ReservesControlBranch and not CircControl
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords: Manual
Depends on:
Blocks:
 
Reported: 2018-05-30 17:22 UTC by Nick Clemens
Modified: 2019-04-26 15:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
WARNING: This patch corrects the behaviour of reserve rules such that they match the system preference descriptions. This may initial lead to confusion as prior to this the CircControl branches were used incorrectly. Settings for ReservesControlBranch and CircControl should be reviewed to ensure proper behaviour is enforced.
Version(s) released in:
19.05.00


Attachments
Bug 20837: Unit tests (1.70 KB, patch)
2018-05-30 17:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Use ReservesControlBranch to determine holdability of items (2.68 KB, patch)
2018-05-30 17:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Unit tests (1.79 KB, patch)
2019-03-26 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Use ReservesControlBranch to determine holdability of items (2.75 KB, patch)
2019-03-26 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Unit tests (8.85 KB, patch)
2019-03-26 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Alter IsAvailableForItemForItemLevelRequest to use ReservesControlBranch (1.67 KB, patch)
2019-03-26 14:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: (follow-up) Rename variables (1.69 KB, patch)
2019-03-26 14:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Unit tests (10.46 KB, patch)
2019-04-04 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Use ReservesControlBranch to determine holdability of items (4.27 KB, patch)
2019-04-04 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20837: Unit tests (10.48 KB, patch)
2019-04-09 19:36 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20837: Use ReservesControlBranch to determine holdability of items (4.28 KB, patch)
2019-04-09 19:36 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20837: (QA Follow-up) adding missing filter to make QA tool happy (1.58 KB, patch)
2019-04-09 19:38 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20837: Unit tests (10.42 KB, patch)
2019-04-25 07:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20837: Use ReservesControlBranch to determine holdability of items (4.37 KB, patch)
2019-04-25 07:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20837: (QA follow-up) adding missing filter to make QA tool happy (1.68 KB, patch)
2019-04-25 07:41 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 2018-05-30 17:22:27 UTC
According to the pref descriptions CircControl shouldn't affect reserves, we should check the other pref.

Also, the important note 'Default holds policy by item type' admin/smart-rules.pl is misleading:
"Important: The policies are based on the patron's home library, not the library where the hold is being placed."
Comment 1 Nick Clemens 2018-05-30 17:31:58 UTC
Created attachment 75684 [details] [review]
Bug 20837: Unit tests

To test:
prove -v t/db_dependent/Holds.t
Comment 2 Nick Clemens 2018-05-30 17:32:01 UTC
Created attachment 75685 [details] [review]
Bug 20837: Use ReservesControlBranch to determine holdability of items

To test:
 1 - Set a default holds plicy for an item type as 'from home library'
 2 - Set CircControl to 'logged in library'
 3 - Log in as staff from a library without the rule set
 4 - Note you can set holds for patrons of any library on items of type
above
 5 - Toggle ReservesControlBranch and note that nothing changes
 6 - Toggle CircControl and note some combinations don't allow holds
 7 - Apply patch
 8 - Toggle ReservesControlBranch and note that holdability is affected
 9 - Toggle CircControl and note holdability does not change
10 - Read the note on admin/smartrules.pl and confirm it makes sense
Comment 3 Jonathan Druart 2018-05-31 18:55:04 UTC
It makes sense to me yes. Note that there is another occurrence of C4::Circulation::_GetCircControlBranch in IsAvailableForItemLevelRequest

We should also rename the variable name ($circ_control_branch)

For discussion, from hea:
ReservesControlBranch
  ItemHomeLibrary - 115
  PatronLibrary   - 944 (default)
CircControl
  ItemHomeLibrary - 962 (default)
  PatronLibrary   - 34
  PickupLibrary   - 65
Comment 4 Liz Rea 2019-03-25 18:09:19 UTC
Hi Nick, 

Terribly sorry, the patches don't seem to apply:

CONFLICT (content): Merge conflict in t/db_dependent/Holds.t
error: Failed to merge in the changes.
- This was a conflict on number of tests, easy to resolve so I did that, HOWEVER have also got...

Applying: Bug 20837: Use ReservesControlBranch to determine holdability of items
error: sha1 information is lacking or useless (C4/Reserves.pm).
error: could not build fake ancestor

A new patch set would be nice. :)

Liz
Comment 5 Nick Clemens 2019-03-26 14:52:15 UTC
Created attachment 87021 [details] [review]
Bug 20837: Unit tests

To test:
prove -v t/db_dependent/Holds.t
Comment 6 Nick Clemens 2019-03-26 14:52:18 UTC
Created attachment 87022 [details] [review]
Bug 20837: Use ReservesControlBranch to determine holdability of items

To test:
 1 - Set a default holds plicy for an item type as 'from home library'
 2 - Set CircControl to 'logged in library'
 3 - Log in as staff from a library without the rule set
 4 - Note you can set holds for patrons of any library on items of type
above
 5 - Toggle ReservesControlBranch and note that nothing changes
 6 - Toggle CircControl and note some combinations don't allow holds
 7 - Apply patch
 8 - Toggle ReservesControlBranch and note that holdability is affected
 9 - Toggle CircControl and note holdability does not change
10 - Read the note on admin/smartrules.pl and confirm it makes sense
Comment 7 Nick Clemens 2019-03-26 14:52:21 UTC
Created attachment 87023 [details] [review]
Bug 20837: Unit tests
Comment 8 Nick Clemens 2019-03-26 14:52:24 UTC
Created attachment 87024 [details] [review]
Bug 20837: Alter IsAvailableForItemForItemLevelRequest to use ReservesControlBranch

Also use Koha objects for fetching itemtype info

To test:
Same test plan as above, ensure item availability is affected the the system preference
prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Comment 9 Nick Clemens 2019-03-26 14:54:26 UTC
Created attachment 87025 [details] [review]
Bug 20837: (follow-up) Rename variables
Comment 10 Liz Rea 2019-04-03 17:39:52 UTC
Hi Nick, 

It looks like Holds.t isn't currently passing with all of the patches applied: 

not ok 52 - CanItemBeReserved should use item home library rule when RsservesControlBranch set to 'ItemsHomeLibrary'

Arg.
Comment 11 Nick Clemens 2019-04-04 12:58:14 UTC
Created attachment 87409 [details] [review]
Bug 20837: Unit tests

To test:
prove -v t/db_dependent/Holds.t
prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Comment 12 Nick Clemens 2019-04-04 12:58:18 UTC
Created attachment 87410 [details] [review]
Bug 20837: Use ReservesControlBranch to determine holdability of items

To test:
 1 - Set a default holds plicy for an item type as 'from home library'
 2 - Set CircControl to 'logged in library'
 3 - Log in as staff from a library without the rule set
 4 - Note you can set holds for patrons of any library on items of type
above
 5 - Toggle ReservesControlBranch and note that nothing changes
 6 - Toggle CircControl and note some combinations don't allow holds
 7 - Apply patch
 8 - Toggle ReservesControlBranch and note that holdability is affected
 9 - Toggle CircControl and note holdability does not change
10 - Read the note on admin/smartrules.pl and confirm it makes sense
Comment 13 Liz Rea 2019-04-09 19:34:28 UTC
I have questions about how this could affect current users and might cause some nasty surprises for existing users, but the behaviour is correct now based on the tooltips so new libraries might find this easier to understand so the change is probably in the better long term interest.

Signing off.

Liz
Comment 14 Liz Rea 2019-04-09 19:36:07 UTC
Created attachment 87673 [details] [review]
Bug 20837: Unit tests

To test:
prove -v t/db_dependent/Holds.t
prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Comment 15 Liz Rea 2019-04-09 19:36:10 UTC
Created attachment 87674 [details] [review]
Bug 20837: Use ReservesControlBranch to determine holdability of items

To test:
 1 - Set a default holds plicy for an item type as 'from home library'
 2 - Set CircControl to 'logged in library'
 3 - Log in as staff from a library without the rule set
 4 - Note you can set holds for patrons of any library on items of type
above
 5 - Toggle ReservesControlBranch and note that nothing changes
 6 - Toggle CircControl and note some combinations don't allow holds
 7 - Apply patch
 8 - Toggle ReservesControlBranch and note that holdability is affected
 9 - Toggle CircControl and note holdability does not change
10 - Read the note on admin/smartrules.pl and confirm it makes sense
Comment 16 Liz Rea 2019-04-09 19:38:52 UTC
Created attachment 87675 [details] [review]
Bug 20837: (QA Follow-up) adding missing filter to make QA tool happy
Comment 17 Martin Renvoize 2019-04-25 07:36:05 UTC
Haha.. thanks for the follow-up Liz.. but you fell into the whole I nearly always also fall into 'follow-up' vs 'Follow-up'... QA script complained.. I've fixed it ;)
Comment 18 Martin Renvoize 2019-04-25 07:37:46 UTC
ooh.. also you forgot your signoff lines.. must have been a late night for you aye... so glad it's not just me who does that
Comment 19 Martin Renvoize 2019-04-25 07:41:02 UTC
Created attachment 88699 [details] [review]
Bug 20837: Unit tests

To test:
prove -v t/db_dependent/Holds.t
prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2019-04-25 07:41:05 UTC
Created attachment 88700 [details] [review]
Bug 20837: Use ReservesControlBranch to determine holdability of items

To test:
 1 - Set a default holds plicy for an item type as 'from home library'
 2 - Set CircControl to 'logged in library'
 3 - Log in as staff from a library without the rule set
 4 - Note you can set holds for patrons of any library on items of type
above
 5 - Toggle ReservesControlBranch and note that nothing changes
 6 - Toggle CircControl and note some combinations don't allow holds
 7 - Apply patch
 8 - Toggle ReservesControlBranch and note that holdability is affected
 9 - Toggle CircControl and note holdability does not change
10 - Read the note on admin/smartrules.pl and confirm it makes sense

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2019-04-25 07:41:08 UTC
Created attachment 88701 [details] [review]
Bug 20837: (QA follow-up) adding missing filter to make QA tool happy

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-04-25 07:44:05 UTC
Nick, could you check my release notes make sense?

Otherwise, looks good and Passing QA.
Comment 23 Nick Clemens 2019-04-25 11:52:25 UTC
Awesome work all!

Pushed to master for 19.05
Comment 24 Martin Renvoize 2019-04-26 15:18:44 UTC
This could be a rather confusing change for a stable branch.. opting not to backport to 18.11.x series.