Bug 27760 - Add handling for x-koha-override
Summary: Add handling for x-koha-override
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: Kyle M Hall
URL:
Keywords:
Depends on: 26181 28157
Blocks: 27797
  Show dependency treegraph
 
Reported: 2021-02-23 12:19 UTC by Tomás Cohen Arazi
Modified: 2021-12-13 21:12 UTC (History)
5 users (show)

See Also:
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


Attachments
Bug 27760: Add stash_overrides helper (2.91 KB, patch)
2021-02-25 12:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27760: Make auhtenticate_api_request call stash_overrides (2.89 KB, patch)
2021-02-25 12:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27760: Add stash_overrides helper (2.93 KB, patch)
2021-02-25 14:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 27760: Make auhtenticate_api_request call stash_overrides (2.94 KB, patch)
2021-02-25 14:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 27760: Add stash_overrides helper (3.03 KB, patch)
2021-02-25 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27760: Make auhtenticate_api_request call stash_overrides (3.00 KB, patch)
2021-02-25 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27760: Make authenticate_api_request call stash_overrides (3.00 KB, patch)
2021-02-25 15:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27760: Improve readabiliity using map (1.86 KB, patch)
2021-03-11 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27760: (QA follow-up) Improve POD (1.48 KB, patch)
2021-03-11 11:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27760: Add stash_overrides helper (3.02 KB, patch)
2021-04-23 15:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27760: Make authenticate_api_request call stash_overrides (2.98 KB, patch)
2021-04-23 15:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27760: Improve readabiliity using map (1.87 KB, patch)
2021-04-23 15:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27760: (QA follow-up) Improve POD (1.48 KB, patch)
2021-04-23 15:22 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-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