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.
Created attachment 108016 [details] [review] Bug 26181: regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
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?
Created attachment 116720 [details] [review] Bug 26181: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
(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?
QAing
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>
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>
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.03
Pushed to 20.05.x for 20.05.09
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.