Description
Nick Clemens (kidclamp)
2018-05-30 17:22:27 UTC
Created attachment 75684 [details] [review] Bug 20837: Unit tests To test: prove -v t/db_dependent/Holds.t 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 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 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 Created attachment 87021 [details] [review] Bug 20837: Unit tests To test: prove -v t/db_dependent/Holds.t 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 Created attachment 87023 [details] [review] Bug 20837: Unit tests 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 Created attachment 87025 [details] [review] Bug 20837: (follow-up) Rename variables 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. 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 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 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 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 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 Created attachment 87675 [details] [review] Bug 20837: (QA Follow-up) adding missing filter to make QA tool happy 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 ;) 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 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> 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> 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> Nick, could you check my release notes make sense? Otherwise, looks good and Passing QA. Awesome work all! Pushed to master for 19.05 This could be a rather confusing change for a stable branch.. opting not to backport to 18.11.x series. |