Bug 28254 - Make it possible to override rules in PUT /holds/:hold_id/pickup_location
Summary: Make it possible to override rules in PUT /holds/:hold_id/pickup_location
Status: Pushed to master
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:
URL:
Keywords:
Depends on: 27898
Blocks: 27865
  Show dependency treegraph
 
Reported: 2021-04-28 17:16 UTC by Tomás Cohen Arazi
Modified: 2021-05-11 12:06 UTC (History)
5 users (show)

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


Attachments
Bug 28254: Add 'override' parameter to Koha::Hold->set_pickup_location (2.92 KB, patch)
2021-04-28 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28254: Unit tests (1.63 KB, patch)
2021-04-28 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28254: Make PUT /holds/:hold_id/pickup_location honour x-koha-override (2.10 KB, patch)
2021-04-28 18:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28254: Add 'override' parameter to Koha::Hold->set_pickup_location (2.98 KB, patch)
2021-04-29 10:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28254: Unit tests (1.70 KB, patch)
2021-04-29 10:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28254: Make PUT /holds/:hold_id/pickup_location honour x-koha-override (2.17 KB, patch)
2021-04-29 10:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28254: Add 'override' parameter to Koha::Hold->set_pickup_location (3.03 KB, patch)
2021-04-30 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28254: Unit tests (1.75 KB, patch)
2021-04-30 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28254: Make PUT /holds/:hold_id/pickup_location honour x-koha-override (2.21 KB, patch)
2021-04-30 13:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28254: (QA follow-up) override -> force (2.85 KB, patch)
2021-04-30 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28254: Simplify tests (3.80 KB, patch)
2021-05-04 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28254: Add missing tests for PUT and POST (2.48 KB, patch)
2021-05-04 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-04-28 17:16:42 UTC
Bug 27898 made PUT /holds/:hold_id handle x-koha-override correctly, but we didn't do that for the specific route.
Comment 1 Tomás Cohen Arazi 2021-04-28 17:17:56 UTC
Making it dependent on bug 27898 because of the test files.
Comment 2 Tomás Cohen Arazi 2021-04-28 18:01:44 UTC
Created attachment 120281 [details] [review]
Bug 28254: Add 'override' parameter to Koha::Hold->set_pickup_location

This patch adds a new parameter to the method. If passed+true it makes
the method skip the pickup validation test.

Tests and POD added for this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 3 Tomás Cohen Arazi 2021-04-28 18:01:50 UTC
Created attachment 120282 [details] [review]
Bug 28254: Unit tests
Comment 4 Tomás Cohen Arazi 2021-04-28 18:01:56 UTC
Created attachment 120283 [details] [review]
Bug 28254: Make PUT /holds/:hold_id/pickup_location honour x-koha-override

This patch adds a new parameter (x-koha-override header) to the route,
and makes the controller pass this information (override requested) when
the AllowHoldPolicyOverride syspref is set, to
Koha::Hold->set_pickup_location.

This way, under certain conditions, we can be sure the update wont'
fail.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 5 Kyle M Hall 2021-04-29 10:48:31 UTC
Created attachment 120305 [details] [review]
Bug 28254: Add 'override' parameter to Koha::Hold->set_pickup_location

This patch adds a new parameter to the method. If passed+true it makes
the method skip the pickup validation test.

Tests and POD added for this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2021-04-29 10:48:49 UTC
Created attachment 120306 [details] [review]
Bug 28254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2021-04-29 10:48:54 UTC
Created attachment 120307 [details] [review]
Bug 28254: Make PUT /holds/:hold_id/pickup_location honour x-koha-override

This patch adds a new parameter (x-koha-override header) to the route,
and makes the controller pass this information (override requested) when
the AllowHoldPolicyOverride syspref is set, to
Koha::Hold->set_pickup_location.

This way, under certain conditions, we can be sure the update wont'
fail.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Martin Renvoize 2021-04-30 13:30:56 UTC
In the Transfers work I ended up with a 'force' parameter in a few places.. which was used to force an action that would normally be prevented by policy.. often with a check in the caller prior to calling with force.

I wonder if we should converge on that term or 'override' as implemented here.  Just some food for thought.

The code looks good.
Comment 9 Martin Renvoize 2021-04-30 13:36:37 UTC
Created attachment 120337 [details] [review]
Bug 28254: Add 'override' parameter to Koha::Hold->set_pickup_location

This patch adds a new parameter to the method. If passed+true it makes
the method skip the pickup validation test.

Tests and POD added for this change.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Hold.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-04-30 13:36:41 UTC
Created attachment 120338 [details] [review]
Bug 28254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2021-04-30 13:36:45 UTC
Created attachment 120339 [details] [review]
Bug 28254: Make PUT /holds/:hold_id/pickup_location honour x-koha-override

This patch adds a new parameter (x-koha-override header) to the route,
and makes the controller pass this information (override requested) when
the AllowHoldPolicyOverride syspref is set, to
Koha::Hold->set_pickup_location.

This way, under certain conditions, we can be sure the update wont'
fail.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-04-30 13:44:05 UTC
Created attachment 120340 [details] [review]
Bug 28254: (QA follow-up) override -> force

After some discussion we decided that the parameter should be called
'force' for pushign through an action against policy. (as per the
Transfers work).

We decided to stick to 'override' at the API level however as it
highlights the 'human interaction' element of the call.
Comment 13 Martin Renvoize 2021-04-30 13:45:59 UTC
All works well and passes QA scripts.  I added a followup for the consistency of parameter names after discussing it with Tomas.

Passing QA
Comment 14 Jonathan Druart 2021-05-04 13:06:09 UTC
Created attachment 120457 [details] [review]
Bug 28254: Simplify tests

This is only cosmetic, and maybe personal preferences.
It seems easier to create the library object with the pickup_library we
want to use.
Comment 15 Jonathan Druart 2021-05-04 13:06:30 UTC
Created attachment 120458 [details] [review]
Bug 28254: Add missing tests for PUT and POST
Comment 16 Jonathan Druart 2021-05-04 13:25:12 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 17 Tomás Cohen Arazi 2021-05-04 13:48:11 UTC
(In reply to Jonathan Druart from comment #16)
> Pushed to master for 21.05, thanks to everybody involved!

Thanks, and many thanks for the follow-ups!
Comment 18 Fridolin Somers 2021-05-11 12:06:10 UTC
Depends on Bug 27898 not in 20.11.x