[Bug 27797] New: Make POST /holds use the stashed koha.overrides
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Bug ID: 27797 Summary: Make POST /holds use the stashed koha.overrides Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Depends on| |26181, 27760 Status|NEW |ASSIGNED CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |martin.renvoize@ptfs-europe | |.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26181 [Bug 26181] Holds placed via the REST API should not be forced by default even if AllowHoldPolicyOverride is enabled https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27760 [Bug 27760] Add handling for x-koha-override -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117498 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117498&action=edit Bug 27797: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117499&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117498|0 |1 is obsolete| | --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117540 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117540&action=edit Bug 27797: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117499|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 117541 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117541&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Works well, tests pass, the feature works. Signing off, thanks for doing this Tomas! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117540|0 |1 is obsolete| | --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 117761 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117761&action=edit Bug 27797: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117541|0 |1 is obsolete| | --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 117762 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117762&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #10 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 117763 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117763&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117920&action=edit Bug 27797: (follow-up): Remove stray x-koha-override definition Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117929 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117929&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 117930 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117930&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Signed Off --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Nick, can you give this a new QA round? I noticed pickup locations couldn't be overridden, and added a follow-up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27898 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27898 [Bug 27898] Make PUT /holds/:hold_id handle x-koha-override for pickup locations -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Why don't we actually enforce AllowHoldPolicyOverride in AddReserve? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Then maybe it should be part of Can*BeReserved return. (yes it's out of the scope, I am just thinking loud here) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- The trivial change is covered by tests and this was already PQA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117920|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117761|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117762|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117763|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117929|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117930|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120285 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120285&action=edit Bug 27797: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120286 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120286&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #23 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120287&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #24 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120288 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120288&action=edit Bug 27797: (follow-up): Remove stray x-koha-override definition Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #25 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120289 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120289&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #26 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 120290 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=120290&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 --- Comment #27 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27797 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #28 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 20.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org