Bug 27760

Summary: Add handling for x-koha-override
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patches add a new Mojolicious helper method that takes care of reading 'x-koha-override' headers that contain comma-separated overrides (e.g. 'pickup_location,too_many_holds') and stashes them for later use in controller methods.
Version(s) released in:
21.05.00
Bug Depends on: 26181, 28157    
Bug Blocks: 27797    
Attachments: Bug 27760: Add stash_overrides helper
Bug 27760: Make auhtenticate_api_request call stash_overrides
Bug 27760: Add stash_overrides helper
Bug 27760: Make auhtenticate_api_request call stash_overrides
Bug 27760: Add stash_overrides helper
Bug 27760: Make auhtenticate_api_request call stash_overrides
Bug 27760: Make authenticate_api_request call stash_overrides
Bug 27760: Improve readabiliity using map
Bug 27760: (QA follow-up) Improve POD
Bug 27760: Add stash_overrides helper
Bug 27760: Make authenticate_api_request call stash_overrides
Bug 27760: Improve readabiliity using map
Bug 27760: (QA follow-up) Improve POD

Description Tomás Cohen Arazi 2021-02-23 12:19:53 UTC
On the development of bug 26181, Martin and I were discussing a more generic approach, when suddenly the bug went SO => QA => Pushed to master => Backported to all stable branches :-D. My fault for not setting it 'In discussion'.

We should use a follow-up to adjust the pushed implementation in line with bug 26181 comment 9.
Comment 1 Tomás Cohen Arazi 2021-02-25 12:47:14 UTC
Created attachment 117308 [details] [review]
Bug 27760: Add stash_overrides helper

This patch adds a simple helper that reads the x-koha-override request
header, and processes it to stash a hashref with the passed overrides.

No check on the overrides themselves is done, as they should be
validated using the OpenAPI spec.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 2 Tomás Cohen Arazi 2021-02-25 12:47:18 UTC
Created attachment 117309 [details] [review]
Bug 27760: Make auhtenticate_api_request call stash_overrides

By making this method call the helper, we make sure the overrides
hashref is available to any controller to make us of it.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! The helper works as expected and the
authenticate_api_request method invokes it!
3. Sign off :-D
Comment 3 David Nind 2021-02-25 14:12:17 UTC
Created attachment 117326 [details] [review]
Bug 27760: Add stash_overrides helper

This patch adds a simple helper that reads the x-koha-override request
header, and processes it to stash a hashref with the passed overrides.

No check on the overrides themselves is done, as they should be
validated using the OpenAPI spec.

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-02-25 14:12:22 UTC
Created attachment 117327 [details] [review]
Bug 27760: Make auhtenticate_api_request call stash_overrides

By making this method call the helper, we make sure the overrides
hashref is available to any controller to make us of it.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! The helper works as expected and the
authenticate_api_request method invokes it!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Tomás Cohen Arazi 2021-02-25 15:02:25 UTC
I wanted to mention that it feels the right way to retrieve the headers in the code should use the validated data (by the OpenAPI plugin) so we could take advantage of data coercion and such. But I couldn't find a way to get the Mojolicious::Validator::Validation object ($c->validation) to give us the headers. So skipping that for now.

I will follow-up on this once I get answers from the mojo/openapi devs on IRC.
Comment 6 Kyle M Hall 2021-02-25 15:30:26 UTC
Created attachment 117342 [details] [review]
Bug 27760: Add stash_overrides helper

This patch adds a simple helper that reads the x-koha-override request
header, and processes it to stash a hashref with the passed overrides.

No check on the overrides themselves is done, as they should be
validated using the OpenAPI spec.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2021-02-25 15:30:37 UTC
Created attachment 117343 [details] [review]
Bug 27760: Make auhtenticate_api_request call stash_overrides

By making this method call the helper, we make sure the overrides
hashref is available to any controller to make us of it.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! The helper works as expected and the
authenticate_api_request method invokes it!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2021-02-25 15:32:49 UTC
Created attachment 117344 [details] [review]
Bug 27760: Make authenticate_api_request call stash_overrides

By making this method call the helper, we make sure the overrides
hashref is available to any controller to make us of it.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! The helper works as expected and the
authenticate_api_request method invokes it!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2021-02-25 15:38:18 UTC
Blimey.. I was about to SO this and by the time I'd read the code in splinter review it had been SO'd and QA'd!.. Impressive work chaps.
Comment 10 Jonathan Druart 2021-03-11 10:16:33 UTC
1. Missing POD

2. (In reply to Tomás Cohen Arazi from comment #5)
> I will follow-up on this once I get answers from the mojo/openapi devs on
> IRC.

Any news?
Comment 11 Jonathan Druart 2021-03-11 10:16:50 UTC
Created attachment 118108 [details] [review]
Bug 27760: Improve readabiliity using map
Comment 12 Tomás Cohen Arazi 2021-03-11 10:24:09 UTC
(In reply to Jonathan Druart from comment #10)
> 1. Missing POD

Well check.

> 2. (In reply to Tomás Cohen Arazi from comment #5)
> > I will follow-up on this once I get answers from the mojo/openapi devs on
> > IRC.
> 
> Any news?

We need to upgrade our JSON::Validator.
Comment 13 Tomás Cohen Arazi 2021-03-11 11:05:25 UTC
Created attachment 118110 [details] [review]
Bug 27760: (QA follow-up) Improve POD

This patch improves the POD for the added helper, and also adds
references to the JSON::Validator issue that explains the current
implementation. A FIXME is added for when we upgrade our used
JSON::Validator version.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2021-04-23 15:20:45 UTC
Rebased on top of bug 28157.
Comment 15 Tomás Cohen Arazi 2021-04-23 15:22:20 UTC
Created attachment 120112 [details] [review]
Bug 27760: Add stash_overrides helper

This patch adds a simple helper that reads the x-koha-override request
header, and processes it to stash a hashref with the passed overrides.

No check on the overrides themselves is done, as they should be
validated using the OpenAPI spec.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Tomás Cohen Arazi 2021-04-23 15:22:25 UTC
Created attachment 120113 [details] [review]
Bug 27760: Make authenticate_api_request call stash_overrides

By making this method call the helper, we make sure the overrides
hashref is available to any controller to make us of it.

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! The helper works as expected and the
authenticate_api_request method invokes it!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Tomás Cohen Arazi 2021-04-23 15:22:31 UTC
Created attachment 120114 [details] [review]
Bug 27760: Improve readabiliity using map
Comment 18 Tomás Cohen Arazi 2021-04-23 15:22:36 UTC
Created attachment 120115 [details] [review]
Bug 27760: (QA follow-up) Improve POD

This patch improves the POD for the added helper, and also adds
references to the JSON::Validator issue that explains the current
implementation. A FIXME is added for when we upgrade our used
JSON::Validator version.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 David Cook 2021-04-25 23:32:07 UTC
Looks like upgrading JSON::Validator would give us a few different wins across the board...
Comment 20 Tomás Cohen Arazi 2021-04-25 23:50:41 UTC
(In reply to David Cook from comment #19)
> Looks like upgrading JSON::Validator would give us a few different wins
> across the board...

Oh yeah. But this is the very latest version. Would love to, though :-)
Comment 21 David Cook 2021-04-26 00:45:03 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> (In reply to David Cook from comment #19)
> > Looks like upgrading JSON::Validator would give us a few different wins
> > across the board...
> 
> Oh yeah. But this is the very latest version. Would love to, though :-)

Ahh that's harder :(
Comment 22 Jonathan Druart 2021-05-04 13:24:45 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 23 Fridolin Somers 2021-05-11 12:04:23 UTC
Enhancement not pushed to 20.11.x