|
Description
Tomás Cohen Arazi (tcohen)
2021-02-25 17:57:23 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). Created attachment 117498 [details] [review] Bug 27797: Unit tests 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! 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. Created attachment 117540 [details] [review] Bug 27797: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> Works well, tests pass, the feature works. Signing off, thanks for doing this Tomas! 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> 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> 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> Created attachment 117920 [details] [review] Bug 27797: (follow-up): Remove stray x-koha-override definition Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> Nick, can you give this a new QA round? I noticed pickup locations couldn't be overridden, and added a follow-up. Why don't we actually enforce AllowHoldPolicyOverride in AddReserve? (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. 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 #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. (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. The trivial change is covered by tests and this was already PQA. 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> 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> 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> Created attachment 120288 [details] [review] Bug 27797: (follow-up): Remove stray x-koha-override definition Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x |