Bug 23732 - Hold rules checker: show matched rules and syspref values to help understand why a hold is possible or not
Summary: Hold rules checker: show matched rules and syspref values to help understand ...
Status: BLOCKED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Victor Grousset/tuxayo
QA Contact: Testopia
URL: https://gitlab.com/tuxayo/Koha/-/tree...
Keywords:
Depends on: 33166
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-03 13:32 UTC by Victor Grousset/tuxayo
Modified: 2023-10-30 21:57 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23732: Hold rules checker (23.14 KB, patch)
2019-11-14 16:44 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23732: Add unit tests (4.52 KB, patch)
2019-11-14 16:44 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Screen capture - Check this out :) (94.79 KB, image/png)
2019-11-14 16:52 UTC, Arthur Suzuki
Details
Screen capture - position of "hold rule checker" button (89.06 KB, image/png)
2019-11-14 16:55 UTC, Arthur Suzuki
Details
Bug 23732: Add unit tests (4.67 KB, patch)
2019-11-14 17:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23732: Renamed TODOs and added translation capabilities (7.99 KB, patch)
2019-12-03 01:10 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23732: Hold rules checker (33.56 KB, patch)
2023-02-28 16:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Screenshot (110.26 KB, image/png)
2023-02-28 16:19 UTC, Julian Maurice
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Grousset/tuxayo 2019-10-03 13:32:08 UTC

    
Comment 1 Arthur Suzuki 2019-11-14 16:44:08 UTC
Created attachment 95435 [details] [review]
Bug 23732: Hold rules checker

== Test plan ==
- apply
- yarn build
- generate translations
- try to place a hold, on the Hold details page you should see a "Holding rules" blue button
- click it
- check that the info if consistent with the holding rules, the syspref and the current and future number of holds of the patron
- place the hold
- set maxreserves syspref to 1
- try to place a hold
- see in the holds rules checker that the maxreserves is highlighted in red (it the only thing supporting highlighting)
- restore maxreserves syspref
- test that sabotaging the matched circulation rule or hold policy is reflected (the limits) in the hold rules checker
Comment 2 Arthur Suzuki 2019-11-14 16:44:20 UTC
Created attachment 95436 [details] [review]
Bug 23732: Add unit tests
Comment 3 Arthur Suzuki 2019-11-14 16:52:23 UTC
Created attachment 95443 [details]
Screen capture - Check this out :)

Screen capture of our nice hold rules checker tool :)
Comment 4 Arthur Suzuki 2019-11-14 16:55:07 UTC
Created attachment 95444 [details]
Screen capture - position of "hold rule checker" button
Comment 5 Victor Grousset/tuxayo 2019-11-14 17:01:09 UTC
Created attachment 95445 [details] [review]
Bug 23732: Add unit tests

== test plan ==
1. prove -r t/db_dependent/Reserves
2. Apply bugzilla
3. prove t/db_dependent/Reserves/HoldRulesChecker.t
4. prove -r t/db_dependent/Reserves
Comment 6 Victor Grousset/tuxayo 2019-11-14 17:14:31 UTC
screenshots: Not that "Check this out" is the name of the record.
A very great also not so great name for examples ^_^
Comment 7 Victor Grousset/tuxayo 2019-11-15 14:51:53 UTC
*Note that
Comment 8 Owen Leonard 2019-11-21 00:59:31 UTC
Overall this looks really good--and really useful! I see a few instances of "TODO" in the changes. Are these things which need a follow-up?
Comment 9 Victor Grousset/tuxayo 2019-11-21 08:29:56 UTC
Indeed I spotted these, I though I had fixed these but can't find the commit anywhere :o

Planned to take care of these, at the same time of 18.11.x rebase.
Comment 10 Arthur Suzuki 2019-12-03 01:10:36 UTC
Created attachment 95934 [details] [review]
Bug 23732: Renamed TODOs and added translation capabilities
Comment 11 Arthur Suzuki 2019-12-03 14:25:53 UTC
When backporting on 18.11 I got this error (when only circulation used):
Template process failed: undef error - Can't call method "description" on an undefined value at /home/koha/src/Koha/Template/Plugin/Categories.pm line 33.

To debug with :
https://wiki.koha-community.org/wiki/User:Victor_Grousset_-_tuxayo:Debugging#print_a_stack_backtrace
Comment 12 Bernardo Gonzalez Kriegel 2020-02-29 19:32:12 UTC
Sorry, conflicts

CONFLICT (content): Merge conflict in reserve/request.pl
CONFLICT (content): Merge conflict in C4/Reserves.pm
Comment 13 Julian Maurice 2023-02-28 16:18:40 UTC
Created attachment 147501 [details] [review]
Bug 23732: Hold rules checker

Test plan:
- apply patch
- try to place a hold, on the Hold details page you should see a
  "Holding rules" button. Click on it
- check that the info is consistent with the holding rules, the syspref
  and the current and future number of holds of the patron
- place the hold
- set maxreserves syspref to 1
- try to place a hold
- see in the holds rules checker that the maxreserves is highlighted in
  red
- restore maxreserves syspref
- test that sabotaging the matched circulation rule or hold policy is
  reflected in the hold rules checker
Comment 14 Julian Maurice 2023-02-28 16:19:56 UTC
Created attachment 147504 [details]
Screenshot
Comment 15 Julian Maurice 2023-02-28 16:26:14 UTC
Trying to revive this bug.

There is one significant change: instead of a single circulation rule, the modal now shows all circulation rules that have been checked, in the order they have been checked.
So if no hold is possible, the last rule should be the culprit.
Comment 16 Owen Leonard 2023-03-02 15:42:47 UTC
I tested with "On shelf holds allowed" set to "If all unavailable" and the modal didn't give that as a reason why. Are there some rules which are not checked by this feature?
Comment 17 Laurence Rault 2023-03-03 13:17:52 UTC
Tried to test on sandbox biblibre : 
When I choose a borrower during the hold process, it fails with :  
Can't call method "notforloan" on an undefined value at /kohadevbox/koha/C4/Reserves.pm line 1570
Comment 18 Julian Maurice 2023-03-08 07:55:29 UTC
(In reply to Owen Leonard from comment #16)
> I tested with "On shelf holds allowed" set to "If all unavailable" and the
> modal didn't give that as a reason why. Are there some rules which are not
> checked by this feature?

Unfortunately yes. Only the rules checked by CanBookBeReserved are displayed.
The 'on shelf holds' rule is checked in IsAvailableForItemLevelRequest so it's not displayed.
I don't know how to fix that without a significant rewrite... :/
Comment 19 Julian Maurice 2023-03-08 08:46:35 UTC
(In reply to Laurence Rault from comment #17)
> Tried to test on sandbox biblibre : 
> When I choose a borrower during the hold process, it fails with :  
> Can't call method "notforloan" on an undefined value at
> /kohadevbox/koha/C4/Reserves.pm line 1570

This error is not caused by this patch. I will open a new bug report
Comment 20 Julian Maurice 2023-03-08 09:07:55 UTC
> I will open a new bug report
=> Bug 33166