Bug 26181 - Holds placed via the REST API should not be forced by default even if AllowHoldPolicyOverride is enabled
Summary: Holds placed via the REST API should not be forced by default even if AllowHo...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 27760 27797
  Show dependency treegraph
 
Reported: 2020-08-07 16:07 UTC by Kyle M Hall
Modified: 2021-02-25 17:58 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This patch disables AllowHoldPolicyOverride by default in the /holds REST API. It also adds tests for this behaviour, and adds a header that can be used to request the override explicitly.
Version(s) released in:
21.05.00,20.11.03,20.05.09


Attachments
Bug 26181: regression tests (2.04 KB, patch)
2020-08-10 20:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26181: Disable override by default in /holds (2.16 KB, patch)
2020-08-10 20:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26181: Regression tests (1.74 KB, patch)
2021-02-11 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26181: Disable override by default in /holds (2.09 KB, patch)
2021-02-11 13:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26181: Regression tests (1.81 KB, patch)
2021-02-11 14:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26181: Disable override by default in /holds (2.15 KB, patch)
2021-02-11 14:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26181: Regression tests (1.89 KB, patch)
2021-02-12 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26181: Disable override by default in /holds (2.23 KB, patch)
2021-02-12 08:20 UTC, Marcel de Rooy
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 2020-08-07 16:07:17 UTC
Bug 23710 inadvertently introduced automatic placing of holds that would normally not be allowed, if AllowHoldPolicyOverride is enabled. This was not a specific intention of bug 23710 and this behavior should be reverted.

To retain the new ability to place override holds we should add the ability to send a header specifying the we should allow holds to be forcefully placed.
Comment 1 Tomás Cohen Arazi 2020-08-10 20:35:09 UTC
Created attachment 108016 [details] [review]
Bug 26181: regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-08-10 20:35:12 UTC
Created attachment 108017 [details] [review]
Bug 26181: Disable override by default in /holds

This patch disables AllowHoldPolicyOverride by default in /holds. It
also adds a header that can be used to request the override explicitly.

Tests are added for this behaviour

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail because the behaviour is not implemented
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-08-10 20:36:33 UTC
I left the AllowHoldPolicyOverride name in the x-koha-override header on purpose. To have less friction on this patches.
But I really preferred something like 'holds-policy'. Food for thought.
Comment 4 David Nind 2020-09-19 06:22:46 UTC
Patch no longer applies 8-( :

root@kohadevbox:koha(bz26181)$ git bz apply 26181
Bug 26181 - Holds placed via the REST API should not be forced by default even if AllowHoldPolicyOverride is enabled

108016 - Bug 26181: regression tests
108017 - Bug 26181: Disable override by default in /holds

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 26181: regression tests
Using index info to reconstruct a base tree...
M	t/db_dependent/api/v1/holds.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/api/v1/holds.t
CONFLICT (content): Merge conflict in t/db_dependent/api/v1/holds.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26181: regression tests
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-26181-regression-tests-FLFBjo.patch

Question: Do I need to post the message for the patch failing, or is it okay to just say that the patch no longer applies?
Comment 5 Tomás Cohen Arazi 2021-02-11 13:35:53 UTC
Created attachment 116720 [details] [review]
Bug 26181: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-02-11 13:35:58 UTC
Created attachment 116721 [details] [review]
Bug 26181: Disable override by default in /holds

This patch disables AllowHoldPolicyOverride by default in /holds. It
also adds a header that can be used to request the override explicitly.

Tests are added for this behaviour

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail because the behaviour is not implemented
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Kyle M Hall 2021-02-11 14:51:44 UTC
Created attachment 116757 [details] [review]
Bug 26181: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2021-02-11 14:51:57 UTC
Created attachment 116758 [details] [review]
Bug 26181: Disable override by default in /holds

This patch disables AllowHoldPolicyOverride by default in /holds. It
also adds a header that can be used to request the override explicitly.

Tests are added for this behaviour

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail because the behaviour is not implemented
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2021-02-11 14:53:39 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I left the AllowHoldPolicyOverride name in the x-koha-override header on
> purpose. To have less friction on this patches.
> But I really preferred something like 'holds-policy'. Food for thought.

I'm tempted by the actual rule/policy failure names being in x-koha-override.. and in this case as the override is only available as a 'all or nothing' we should start with a special 'any' rule.


So

x-koha-override: [ any ] # Meaning, override whatever we failed for

in the future

x-koha-override: [ any, damaged_item, transfer_restriction, age_restriction, etc, etc ] # An override per policy violation error that can be thrown and individually override.

Thoughts?
Comment 10 Marcel de Rooy 2021-02-12 08:02:54 UTC
QAing
Comment 11 Marcel de Rooy 2021-02-12 08:20:22 UTC
Created attachment 116786 [details] [review]
Bug 26181: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2021-02-12 08:20:25 UTC
Created attachment 116787 [details] [review]
Bug 26181: Disable override by default in /holds

This patch disables AllowHoldPolicyOverride by default in /holds. It
also adds a header that can be used to request the override explicitly.

Tests are added for this behaviour

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests fail because the behaviour is not implemented
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Jonathan Druart 2021-02-12 11:18:50 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2021-02-19 14:50:03 UTC
Pushed to 20.11.x for 20.11.03
Comment 15 Andrew Fuerste-Henry 2021-02-22 22:50:15 UTC
Pushed to 20.05.x for 20.05.09
Comment 16 Victor Grousset/tuxayo 2021-02-23 16:04:34 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.