To recreate: - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds - confirm ReservesControll is set to patron's library - find a patron from Branch A of the category for which you made your rule - find two bibs with items of the itype got which you made your rule - place a hold on one bib. success! - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold We should be enforcing the All/All rule for Branch A. If we put in a rule for the category and itype for Branch A, that rule will be enforced and we'll be allowed a second hold. In GetHoldRule, we tell get_effective_rules to return those rules ordered by 'categorycode', 'itemtype', 'branchcode.' We should just let get_effective_rule handle that ordering on its own.
Created attachment 111379 [details] [review] Bug 26634: Do not reorder effective rules To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success!
Created attachment 111383 [details] [review] Bug 26634: Do not reorder effective rules To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Hmmm, so the ordering came from bug 14695 in the first place.. I'm wondering if it was a deliberate diversion from the norm or a mistake in the first place. It does make sense to make it consistent with the rest of the system however.
Created attachment 111398 [details] [review] Bug 26634: Do not reorder effective rules To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
So.. it appears the original unit tests don't test for this scenario.. any chance of adding such a test to db_dependent/Reserves/MultiplePerRecord.t Andrew?
Kyle, Nick, and I couldn't find or remember any reason for this grouping from that original bug. If anyone can identify a reason for it, I'm open to keeping it, but then we need to change a bunch of documentation to reflect it. I very much don't know how to write new tests, but I'll bug Kyle about it :)
(In reply to Martin Renvoize from comment #3) > Hmmm, so the ordering came from bug 14695 in the first place.. I'm wondering > if it was a deliberate diversion from the norm or a mistake in the first > place. Nope, it existed prior to 14695: commit 3c741d2376e939dea0554a05eddd4f9e9b2d9449 Date: Mon Aug 24 22:10:21 2009 +0200 Smart Rules enhancements + $rightsquery .= " ORDER BY categorycode DESC, itemtype DESC, branchcode DESC";
I think I had sometimes similar happen recently with loan period...
Created attachment 113966 [details] [review] Bug 26634: Remove GetHoldRule subroutine in C4::Reserves This routine is only used internally and incorrectly overrides the precedence of holds rules - it should be removed This patch removes the routine, adjusts tests, and adds test to confirm correct precedence is followed To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success!
Working through testing this one now.
Created attachment 114017 [details] [review] Bug 26634: Remove GetHoldRule subroutine in C4::Reserves This routine is only used internally and incorrectly overrides the precedence of holds rules - it should be removed This patch removes the routine, adjusts tests, and adds test to confirm correct precedence is followed To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Re-tested and it's working well and provides a more comprehensive cleanup. Signing off.
Created attachment 114260 [details] [review] Bug 26634: Remove GetHoldRule subroutine in C4::Reserves This routine is only used internally and incorrectly overrides the precedence of holds rules - it should be removed This patch removes the routine, adjusts tests, and adds test to confirm correct precedence is followed To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
As a step towards QA, here is another signoff. And QA script ok
Created attachment 114439 [details] [review] Bug 26634: Remove GetHoldRule subroutine in C4::Reserves This routine is only used internally and incorrectly overrides the precedence of holds rules - it should be removed This patch removes the routine, adjusts tests, and adds test to confirm correct precedence is followed To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
QA script passed tests passed code is readable and does have test Passing QA
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.01
Doesn't apply cleanly to 20.05.x, please rebase for backport
Created attachment 116519 [details] [review] Bug 26634: [20.05.x] Remove GetHoldRule subroutine in C4::Reserves This routine is only used internally and incorrectly overrides the precedence of holds rules - it should be removed This patch removes the routine, adjusts tests, and adds test to confirm correct precedence is followed To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Pushed to 20.05.x for 20.05.09
Can't backport to 19.11.x: can't solve the many conflicts. [U+1F4A5][U+1F4A5][U+1F4A5] If there is an interest in having this backported, please submit a patch for 19.11.