Bug 26634 - Hold rules applied incorrectly when All Libraries rules are more specific than branch rules
Summary: Hold rules applied incorrectly when All Libraries rules are more specific tha...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-08 15:19 UTC by Andrew Fuerste-Henry
Modified: 2021-06-14 21:34 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.09


Attachments
Bug 26634: Do not reorder effective rules (1.76 KB, patch)
2020-10-08 15:36 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26634: Do not reorder effective rules (1.82 KB, patch)
2020-10-08 20:39 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 26634: Do not reorder effective rules (1.88 KB, patch)
2020-10-09 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26634: Remove GetHoldRule subroutine in C4::Reserves (14.03 KB, patch)
2020-11-24 20:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26634: Remove GetHoldRule subroutine in C4::Reserves (14.07 KB, patch)
2020-11-26 15:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26634: Remove GetHoldRule subroutine in C4::Reserves (14.13 KB, patch)
2020-12-08 15:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26634: Remove GetHoldRule subroutine in C4::Reserves (14.21 KB, patch)
2020-12-16 14:42 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 26634: [20.05.x] Remove GetHoldRule subroutine in C4::Reserves (14.24 KB, patch)
2021-02-08 19:52 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-10-08 15:19:13 UTC
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.
Comment 1 Andrew Fuerste-Henry 2020-10-08 15:36:41 UTC
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!
Comment 2 ByWater Sandboxes 2020-10-08 20:39:13 UTC
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>
Comment 3 Martin Renvoize 2020-10-09 13:11:33 UTC
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.
Comment 4 Martin Renvoize 2020-10-09 13:14:15 UTC
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>
Comment 5 Martin Renvoize 2020-10-09 13:15:17 UTC
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?
Comment 6 Andrew Fuerste-Henry 2020-10-09 13:23:23 UTC
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 :)
Comment 7 Jonathan Druart 2020-10-09 13:45:18 UTC
(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";
Comment 8 David Cook 2020-10-11 23:03:42 UTC
I think I had sometimes similar happen recently with loan period...
Comment 9 Nick Clemens 2020-11-24 20:23:35 UTC
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!
Comment 10 Martin Renvoize 2020-11-25 17:37:20 UTC
Working through testing this one now.
Comment 11 Martin Renvoize 2020-11-26 15:40:07 UTC
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>
Comment 12 Martin Renvoize 2020-11-26 15:41:56 UTC
Re-tested and it's working well and provides a more comprehensive cleanup.

Signing off.
Comment 13 Martin Renvoize 2020-11-26 15:42:09 UTC
Re-tested and it's working well and provides a more comprehensive cleanup.

Signing off.
Comment 14 Victor Grousset/tuxayo 2020-12-08 15:04:36 UTC
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>
Comment 15 Victor Grousset/tuxayo 2020-12-08 15:09:32 UTC
As a step towards QA, here is another signoff. And QA script ok
Comment 16 Josef Moravec 2020-12-16 14:42:32 UTC
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>
Comment 17 Josef Moravec 2020-12-16 14:43:44 UTC
QA script passed
tests passed

code is readable and does have test

Passing QA
Comment 18 Jonathan Druart 2020-12-21 09:20:05 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2020-12-28 13:34:47 UTC
Pushed to 20.11.x for 20.11.01
Comment 20 Andrew Fuerste-Henry 2020-12-28 14:42:24 UTC
Doesn't apply cleanly to 20.05.x, please rebase for backport
Comment 21 Nick Clemens 2021-02-08 19:52:02 UTC
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>
Comment 22 Andrew Fuerste-Henry 2021-02-08 20:11:17 UTC
Pushed to 20.05.x for 20.05.09
Comment 23 Victor Grousset/tuxayo 2021-02-10 00:52:04 UTC
Can't backport to 19.11.x: can't solve the many conflicts. 💥💥💥
If there is an interest in having this backported, please submit a patch for 19.11.