Bug 27797 - Make POST /holds use the stashed koha.overrides
Summary: Make POST /holds use the stashed koha.overrides
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Nick Clemens
URL:
Keywords:
Depends on: 26181 27760
Blocks: 27898
  Show dependency treegraph
 
Reported: 2021-02-25 17:57 UTC by Tomás Cohen Arazi
Modified: 2021-12-13 21:13 UTC (History)
6 users (show)

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


Attachments
Bug 27797: Unit tests (3.99 KB, patch)
2021-03-02 10:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: Make POST /holds use the stashed koha.overrides (3.22 KB, patch)
2021-03-02 10:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: Unit tests (4.05 KB, patch)
2021-03-02 17:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27797: Make POST /holds use the stashed koha.overrides (3.28 KB, patch)
2021-03-02 17:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27797: Unit tests (4.11 KB, patch)
2021-03-04 19:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27797: Make POST /holds use the stashed koha.overrides (3.34 KB, patch)
2021-03-04 19:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27797: (QA follow-up) Additional tests and operator change (2.66 KB, patch)
2021-03-04 19:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27797: (follow-up): Remove stray x-koha-override definition (763 bytes, patch)
2021-03-08 13:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: (QA follow-up) Make tests more robust (1.49 KB, patch)
2021-03-08 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: (QA follow-up) Pickup locations can be overridden too (4.09 KB, patch)
2021-03-08 15:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: Unit tests (4.12 KB, patch)
2021-04-28 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: Make POST /holds use the stashed koha.overrides (3.34 KB, patch)
2021-04-28 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: (QA follow-up) Additional tests and operator change (2.67 KB, patch)
2021-04-28 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: (follow-up): Remove stray x-koha-override definition (763 bytes, patch)
2021-04-28 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: (QA follow-up) Make tests more robust (1.49 KB, patch)
2021-04-28 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27797: (QA follow-up) Pickup locations can be overridden too (4.09 KB, patch)
2021-04-28 18:59 UTC, Tomás Cohen Arazi
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-02-25 17:57:23 UTC
Bug 26181 introduced a way to override behavior through a new header, x-koha-override. It was being discussed outside BZ, and it got pushed.

The agreement Martin and I reached while it was getting through QA was that we could take the chance and do not override a specific syspref, but use codes to designate what behavior would be overriden. And specifically about 26181, we decided 'any' was better than explicitly putting the syspref on the header.

This bug will use bug 27760 to change how it is done, and will rename the value to be passed on the header, while retaining the current behavior, of failing even if Can*BeReserved fails even if AllowHoldPolicyOverride is set, on the API, unless the header is passed.
Comment 1 Tomás Cohen Arazi 2021-03-01 19:49:50 UTC
Ok, on doing this, I discovered the versions of the JSON::Validator we use don't handle array type of headers, so I will need to make the header a string instead of an array with collectionFormat: csv as we want.

Will keep poking at upgrading the dependency and making better use of the capabilities (coercing the header into an array, for example).
Comment 2 Tomás Cohen Arazi 2021-03-02 10:05:37 UTC
Created attachment 117498 [details] [review]
Bug 27797: Unit tests
Comment 3 Tomás Cohen Arazi 2021-03-02 10:05:40 UTC
Created attachment 117499 [details] [review]
Bug 27797: Make POST /holds use the stashed koha.overrides

Bug 26181 introduced a way to override behavior through a new header,
x-koha-override. And bug 27760 introduced a generic approach to handle
x-koha-override.

This patchset makes the POST /api/v1/holds route rely on this new way of
handling x-koha-override instead of doing it manually.

The header is added to the spec as a parameter.

Note: the header should be defined as:

    "type": "array",
    "collectionFormat": "csv",
    "items": {
        "type": "string",
        "enum": [
            "any"
        ]
    }

but the versions of JSON::Validator we use have lots of bugs related to
header type coercion, so it just doesn't work. The changelog for
JSON::Validator is fairly elocuent about it.

The override now takes a list of valid keywords (right now only 'any'
but it would be fairly straight-forward to add a fairly granular set of
options based on each possible AddReserve failure.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!
4. Sign off!
Comment 4 Tomás Cohen Arazi 2021-03-02 10:10:52 UTC
If we agree on this patches, I suggest this should be backported down to 20.05 so anyone relying on the feature/behavior change it introduced, doesn't need to change their code when they upgrade.
Comment 5 Martin Renvoize 2021-03-02 17:14:19 UTC
Created attachment 117540 [details] [review]
Bug 27797: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-03-02 17:14:22 UTC
Created attachment 117541 [details] [review]
Bug 27797: Make POST /holds use the stashed koha.overrides

Bug 26181 introduced a way to override behavior through a new header,
x-koha-override. And bug 27760 introduced a generic approach to handle
x-koha-override.

This patchset makes the POST /api/v1/holds route rely on this new way of
handling x-koha-override instead of doing it manually.

The header is added to the spec as a parameter.

Note: the header should be defined as:

    "type": "array",
    "collectionFormat": "csv",
    "items": {
        "type": "string",
        "enum": [
            "any"
        ]
    }

but the versions of JSON::Validator we use have lots of bugs related to
header type coercion, so it just doesn't work. The changelog for
JSON::Validator is fairly elocuent about it.

The override now takes a list of valid keywords (right now only 'any'
but it would be fairly straight-forward to add a fairly granular set of
options based on each possible AddReserve failure.

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-03-02 17:15:01 UTC
Works well, tests pass, the feature works.

Signing off, thanks for doing this Tomas!
Comment 8 Nick Clemens 2021-03-04 19:52:27 UTC
Created attachment 117761 [details] [review]
Bug 27797: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2021-03-04 19:52:30 UTC
Created attachment 117762 [details] [review]
Bug 27797: Make POST /holds use the stashed koha.overrides

Bug 26181 introduced a way to override behavior through a new header,
x-koha-override. And bug 27760 introduced a generic approach to handle
x-koha-override.

This patchset makes the POST /api/v1/holds route rely on this new way of
handling x-koha-override instead of doing it manually.

The header is added to the spec as a parameter.

Note: the header should be defined as:

    "type": "array",
    "collectionFormat": "csv",
    "items": {
        "type": "string",
        "enum": [
            "any"
        ]
    }

but the versions of JSON::Validator we use have lots of bugs related to
header type coercion, so it just doesn't work. The changelog for
JSON::Validator is fairly elocuent about it.

The override now takes a list of valid keywords (right now only 'any'
but it would be fairly straight-forward to add a fairly granular set of
options based on each possible AddReserve failure.

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2021-03-04 19:52:33 UTC
Created attachment 117763 [details] [review]
Bug 27797: (QA follow-up) Additional tests and operator change

I added a few tests to cover additional cases and found the operator precedence
was failing, switched 'and' to '&&'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Tomás Cohen Arazi 2021-03-08 13:25:31 UTC
Created attachment 117920 [details] [review]
Bug 27797: (follow-up): Remove stray x-koha-override definition

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2021-03-08 15:36:04 UTC
Created attachment 117929 [details] [review]
Bug 27797: (QA follow-up) Make tests more robust

This patch makes the tests mock things so the complex holds scenarios
where existing data could interfere, don't get in the middle with what
we need to test.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Nothing broke

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2021-03-08 15:36:08 UTC
Created attachment 117930 [details] [review]
Bug 27797: (QA follow-up) Pickup locations can be overridden too

This patch adds regression tests for overridding pickup locations, which
was inadvertedly not covered by the original dev. What this does, is
moving the $can_override variable definition above, and avoid returning
400 if conditions are not met for the passed pickup_library_id, when
$can_override is true.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2021-03-08 15:37:29 UTC
Nick, can you give this a new QA round? I noticed pickup locations couldn't be overridden, and added a follow-up.
Comment 15 Jonathan Druart 2021-03-11 10:43:47 UTC
Why don't we actually enforce AllowHoldPolicyOverride in AddReserve?
Comment 16 Jonathan Druart 2021-03-11 10:46:35 UTC
(In reply to Jonathan Druart from comment #15)
> Why don't we actually enforce AllowHoldPolicyOverride in AddReserve?

Hum, forget that. It would mean we will have to call CanBeReserve* from AddReserve.
Comment 17 Jonathan Druart 2021-03-11 10:48:21 UTC
Then maybe it should be part of Can*BeReserved return.

(yes it's out of the scope, I am just thinking loud here)
Comment 18 Tomás Cohen Arazi 2021-03-11 10:51:38 UTC
(In reply to Jonathan Druart from comment #17)
> Then maybe it should be part of Can*BeReserved return.
> 
> (yes it's out of the scope, I am just thinking loud here)

(In reply to Jonathan Druart from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > Why don't we actually enforce AllowHoldPolicyOverride in AddReserve?
> 
> Hum, forget that. It would mean we will have to call CanBeReserve* from
> AddReserve.

My bet is we can deal with such change once we add

Koha::Item->add_hold
Koha::Biblio->add_hold

to replace AddReserve.
Comment 19 Tomás Cohen Arazi 2021-04-27 18:47:12 UTC
(In reply to Jonathan Druart from comment #17)
> Then maybe it should be part of Can*BeReserved return.

We faced this with the API and pickup locations: we really need a way to notify the caller that * can be reserved, but because we are overriding X situation because AllowHoldsPolicyOverride is enabled and we have permissions, etc...

As I said in a previous comment, I see us adding

Koha::{Biblio|Item}->add_hold

and probably rely on Koha::Object::Message to pass this information to the controllers/callers.
Comment 20 Tomás Cohen Arazi 2021-04-28 11:10:56 UTC
The trivial change is covered by tests and this was already PQA.
Comment 21 Tomás Cohen Arazi 2021-04-28 18:58:52 UTC
Created attachment 120285 [details] [review]
Bug 27797: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Tomás Cohen Arazi 2021-04-28 18:59:02 UTC
Created attachment 120286 [details] [review]
Bug 27797: Make POST /holds use the stashed koha.overrides

Bug 26181 introduced a way to override behavior through a new header,
x-koha-override. And bug 27760 introduced a generic approach to handle
x-koha-override.

This patchset makes the POST /api/v1/holds route rely on this new way of
handling x-koha-override instead of doing it manually.

The header is added to the spec as a parameter.

Note: the header should be defined as:

    "type": "array",
    "collectionFormat": "csv",
    "items": {
        "type": "string",
        "enum": [
            "any"
        ]
    }

but the versions of JSON::Validator we use have lots of bugs related to
header type coercion, so it just doesn't work. The changelog for
JSON::Validator is fairly elocuent about it.

The override now takes a list of valid keywords (right now only 'any'
but it would be fairly straight-forward to add a fairly granular set of
options based on each possible AddReserve failure.

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Tomás Cohen Arazi 2021-04-28 18:59:15 UTC
Created attachment 120287 [details] [review]
Bug 27797: (QA follow-up) Additional tests and operator change

I added a few tests to cover additional cases and found the operator precedence
was failing, switched 'and' to '&&'

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Tomás Cohen Arazi 2021-04-28 18:59:23 UTC
Created attachment 120288 [details] [review]
Bug 27797: (follow-up): Remove stray x-koha-override definition

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2021-04-28 18:59:31 UTC
Created attachment 120289 [details] [review]
Bug 27797: (QA follow-up) Make tests more robust

This patch makes the tests mock things so the complex holds scenarios
where existing data could interfere, don't get in the middle with what
we need to test.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Nothing broke

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2021-04-28 18:59:46 UTC
Created attachment 120290 [details] [review]
Bug 27797: (QA follow-up) Pickup locations can be overridden too

This patch adds regression tests for overridding pickup locations, which
was inadvertedly not covered by the original dev. What this does, is
moving the $can_override variable definition above, and avoid returning
400 if conditions are not met for the passed pickup_library_id, when
$can_override is true.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/holds.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Jonathan Druart 2021-05-04 13:24:50 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 28 Fridolin Somers 2021-05-11 12:04:50 UTC
Enhancement not pushed to 20.11.x