Bug 31575 - Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed
Summary: Missing warning for holds where AllowHoldPolicyOverride can be used to force ...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
: 26841 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-09-16 14:42 UTC by Kyle M Hall
Modified: 2023-12-01 06:55 UTC (History)
7 users (show)

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


Attachments
Bug 31575 - Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed (1.71 KB, patch)
2022-09-16 14:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31575 - Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed (1.76 KB, patch)
2022-09-18 20:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 31575: Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed (1.82 KB, patch)
2022-10-07 10:20 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 Kyle M Hall 2022-09-16 14:42:13 UTC
If some holds are overridable, and AllowHoldPolicyOverride is enabled, Koha used to "flag" each holdable item to let librarians know the hold could not normally be placed. In 22.05 up to current master as of this writing, no warning is displayed.

Test Plan:
1) Set one circ rule to allow 0 holds
2) For Library A, set "Hold pickup library match" to "item's hold group"
3) Enable AllowHoldPolicyOverride
4) Set ReservesControlBranch to "item's home branch"
5) Create a record with an item for Library A, one for Library B, and one for Library C
   This should create a scenario where A's item cannot be held, and B and C's items must be overridden
6) Note you get the "Cannot place hold" but can place those holds anyway without a "warning" sign
Comment 1 Kyle M Hall 2022-09-16 14:49:35 UTC
Created attachment 140710 [details] [review]
Bug 31575 - Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed

If some holds are overridable, and AllowHoldPolicyOverride is enabled, Koha used to "flag" each holdable item to let librarians know the hold could not normally be placed. In 22.05 up to current master as of this writing, no warning is displayed.

Test Plan:
1) Set one circ rule to allow 0 holds
2) For Library A, set "Hold pickup library match" to "item's hold group"
3) Enable AllowHoldPolicyOverride
4) Set ReservesControlBranch to "item's home branch"
5) Create a record with an item for Library A, one for Library B, and one for Library C
   This should create a scenario where A's item cannot be held, and B and C's items must be overridden
6) Note you get the "Cannot place hold" but can place those holds anyway without a "warning" sign
7) Apply this patch
8) Restart all the things!
9) Reload the page, you should now see the warning icons!
Comment 2 David Nind 2022-09-18 20:35:46 UTC
Created attachment 140737 [details] [review]
Bug 31575 - Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed

If some holds are overridable, and AllowHoldPolicyOverride is enabled, Koha used to "flag" each holdable item to let librarians know the hold could not normally be placed. In 22.05 up to current master as of this writing, no warning is displayed.

Test Plan:
1) Set one circ rule to allow 0 holds
2) For Library A, set "Hold pickup library match" to "item's hold group"
3) Enable AllowHoldPolicyOverride
4) Set ReservesControlBranch to "item's home branch"
5) Create a record with an item for Library A, one for Library B, and one for Library C
   This should create a scenario where A's item cannot be held, and B and C's items must be overridden
6) Note you get the "Cannot place hold" but can place those holds anyway without a "warning" sign
7) Apply this patch
8) Restart all the things!
9) Reload the page, you should now see the warning icons!

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Martin Renvoize 2022-10-07 10:20:27 UTC
Created attachment 141489 [details] [review]
Bug 31575: Missing warning for holds where AllowHoldPolicyOverride can be used to force a hold to be placed

If some holds are overridable, and AllowHoldPolicyOverride is enabled, Koha
used to "flag" each holdable item to let librarians know the hold could not
normally be placed. In 22.05 up to current master as of this writing, no
warning is displayed.

Test Plan:
1) Set one circ rule to allow 0 holds
2) For Library A, set "Hold pickup library match" to "item's hold group"
3) Enable AllowHoldPolicyOverride
4) Set ReservesControlBranch to "item's home branch"
5) Create a record with an item for Library A, one for Library B, and one for Library C
   This should create a scenario where A's item cannot be held, and B and C's items must be overridden
6) Note you get the "Cannot place hold" but can place those holds anyway without a "warning" sign
7) Apply this patch
8) Restart all the things!
9) Reload the page, you should now see the warning icons!

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2022-10-07 10:21:32 UTC
Working as expected now.. another one where perhaps moving things up a level would mean we had unit tests to prevent such a regression.. but nothing to prevent a QA pass here.

Passed.
Comment 5 Tomás Cohen Arazi 2022-10-11 13:23:52 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 6 Lucas Gass 2022-12-05 18:07:32 UTC
Backported to 22.05.x for upcoming 22.05.08
Comment 7 Arthur Suzuki 2022-12-14 14:11:12 UTC
applied to 21.11 for 21.11.15
Comment 8 wainuiwitikapark 2023-03-15 00:50:28 UTC
Not backported to 21.05.x
Comment 9 Emily Lamancusa 2023-05-09 18:15:14 UTC
*** Bug 26841 has been marked as a duplicate of this bug. ***
Comment 10 Jonathan Druart 2023-12-01 06:55:01 UTC
I have been git bisect a selenium failure on 21.11.x

STRACE: /usr/share/perl5/Try/Tiny.pm:123 in Selenium::Remote::Driver::catch {...} 
        /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm:361 in Try::Tiny::try
        (eval 1693):1 in Selenium::Remote::Driver::__ANON__
        (eval 1695):2 in Selenium::Remote::Driver::__ANON__
        /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm:1335 in Selenium::Remote::Driver::_execute_command
        /usr/local/share/perl/5.32.1/Selenium/Remote/Driver.pm:1335 in (eval)
        t/db_dependent/selenium/basic_workflow.t:263 in Selenium::Remote::Driver::find_element

Error while executing command: unexpected alert open: Dismissed user prompt dialog: 


And found this commit

7a88155fe13c3a4f71f1a8d09b170738e7d8cbb8 is the first bad commit

It's crazy that the failure has been ignored for so long...