Bug 19370

Summary: Add a helper function for translating order_by params into SQL::Abstract
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: kyle, lari.taskula, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19234    
Bug Blocks: 18731, 19410    
Attachments: Bug 19370: Unit tests
Bug 19370: Add helper function for order_by attribute generation
Bug 19370: Unit tests
Bug 19370: Add helper function for order_by attribute generation
Bug 19369: (followup) Rename params for consitency with 19370
Bug 19370: Remove undefined variable warning
Bug 19370: Unit tests
Bug 19370: Add helper function for order_by attribute generation
Bug 19370: Remove undefined variable warning
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values
Bug 19370: Unit tests
Bug 19370: Add helper function for order_by attribute generation
Bug 19370: Remove undefined variable warning
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values

Description Tomás Cohen Arazi 2017-09-26 18:08:30 UTC
It would be handy to have a helper function in Koha::REST::Plugin::Query generating the 'order_by' attribute out of the query params, to be used when listing objects on the REST api endpoints.
Comment 1 Tomás Cohen Arazi 2017-09-26 18:29:39 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-09-26 18:29:45 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-09-27 16:30:00 UTC
Created attachment 67402 [details] [review]
Bug 19370: Unit tests
Comment 4 Tomás Cohen Arazi 2017-09-27 16:30:06 UTC
Created attachment 67403 [details] [review]
Bug 19370: Add helper function for order_by attribute generation

This patch introduces a helper function called 'dbic_merge_sorting'
to Koha::REST::Plugin::Query.

This simple function adds SQL::Abstract order_by attribute to the passed
$filter hashref, as explained in the POD.

It introduces a syntax for passing sorting params on the request to the REST api.
The proposed syntax has been found in the wild, and is pretty trivial to parse/work with:

    GET /api/v1/<endpoint>?order_by=+column_1|-column_2|column_3

As explained on the POD, + stands for 'asc' and - for 'desc'. If ommited, it defaults to the
DB engine default (usually asc).

To test:
- Apply this patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! And they make sense! :-P
- Sign off :-D

Edit: renamed params to match DBIC terminology. My bad :-D (tcohen)

Sponsored-by: Camden County
Comment 5 Tomás Cohen Arazi 2017-10-02 16:22:27 UTC
Created attachment 67525 [details] [review]
Bug 19369: (followup) Rename params for consitency with 19370
Comment 6 Lari Taskula 2017-10-03 10:55:03 UTC
Is the patch "Bug 19369: (followup) Rename params for consitency with 19370" accidentally attached into this Bug?
Comment 7 Tomás Cohen Arazi 2017-10-03 11:49:37 UTC
(In reply to Lari Taskula from comment #6)
> Is the patch "Bug 19369: (followup) Rename params for consitency with 19370"
> accidentally attached into this Bug?

Indeed :-D
Comment 8 Tomás Cohen Arazi 2017-10-03 20:05:58 UTC
Created attachment 67576 [details] [review]
Bug 19370: Remove undefined variable warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Lari Taskula 2017-10-07 12:43:23 UTC
Created attachment 67782 [details] [review]
Bug 19370: Unit tests

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 10 Lari Taskula 2017-10-07 12:43:33 UTC
Created attachment 67783 [details] [review]
Bug 19370: Add helper function for order_by attribute generation

This patch introduces a helper function called 'dbic_merge_sorting'
to Koha::REST::Plugin::Query.

This simple function adds SQL::Abstract order_by attribute to the passed
$filter hashref, as explained in the POD.

It introduces a syntax for passing sorting params on the request to the REST api.
The proposed syntax has been found in the wild, and is pretty trivial to parse/work with:

    GET /api/v1/<endpoint>?order_by=+column_1|-column_2|column_3

As explained on the POD, + stands for 'asc' and - for 'desc'. If ommited, it defaults to the
DB engine default (usually asc).

To test:
- Apply this patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! And they make sense! :-P
- Sign off :-D

Edit: renamed params to match DBIC terminology. My bad :-D (tcohen)

Sponsored-by: Camden County

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 11 Lari Taskula 2017-10-07 12:43:48 UTC
Created attachment 67784 [details] [review]
Bug 19370: Remove undefined variable warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 12 Tomás Cohen Arazi 2017-11-23 13:55:08 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2017-11-23 14:03:14 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2017-11-23 14:06:18 UTC
Created attachment 69307 [details] [review]
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values

This patch makes the helper handling _order_by params expect a list of
values instead of a (to-be-splitted) string.

The idea is that the OpenAPI plugin will take care of splitting
pipe-delimited values if the spec is correctly defined.

Note: In the process I noticed + on the URL represents a space, so the
helper function is updated to handle both + and %2B as ascending.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2017-11-27 14:38:16 UTC
Created attachment 69383 [details] [review]
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values

This patch makes the helper handling _order_by params expect a list of
values instead of a (to-be-splitted) string.

The idea is that the OpenAPI plugin will take care of splitting
pipe-delimited values if the spec is correctly defined.

Note: In the process I noticed + on the URL represents a space, so the
helper function is updated to handle both + and %2B as ascending.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Edit: Removed rebasing leftover making the tests fail.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2017-11-27 14:40:35 UTC
Created attachment 69384 [details] [review]
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values

This patch makes the helper handling _order_by params expect a list of
values instead of a (to-be-splitted) string.

The idea is that the OpenAPI plugin will take care of splitting
pipe-delimited values if the spec is correctly defined.

Note: In the process I noticed + on the URL represents a space, so the
helper function is updated to handle both + and %2B as ascending.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Edit: Removed rebasing leftover making the tests fail.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Kyle M Hall 2017-11-27 16:46:13 UTC
Created attachment 69391 [details] [review]
Bug 19370: Unit tests

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2017-11-27 16:46:19 UTC
Created attachment 69392 [details] [review]
Bug 19370: Add helper function for order_by attribute generation

This patch introduces a helper function called 'dbic_merge_sorting'
to Koha::REST::Plugin::Query.

This simple function adds SQL::Abstract order_by attribute to the passed
$filter hashref, as explained in the POD.

It introduces a syntax for passing sorting params on the request to the REST api.
The proposed syntax has been found in the wild, and is pretty trivial to parse/work with:

    GET /api/v1/<endpoint>?order_by=+column_1|-column_2|column_3

As explained on the POD, + stands for 'asc' and - for 'desc'. If ommited, it defaults to the
DB engine default (usually asc).

To test:
- Apply this patches
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass! And they make sense! :-P
- Sign off :-D

Edit: renamed params to match DBIC terminology. My bad :-D (tcohen)

Sponsored-by: Camden County

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2017-11-27 16:46:22 UTC
Created attachment 69393 [details] [review]
Bug 19370: Remove undefined variable warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2017-11-27 16:46:25 UTC
Created attachment 69394 [details] [review]
Bug 19370: (QA followup) Use OpenAPI's handling of pipe separated values

This patch makes the helper handling _order_by params expect a list of
values instead of a (to-be-splitted) string.

The idea is that the OpenAPI plugin will take care of splitting
pipe-delimited values if the spec is correctly defined.

Note: In the process I noticed + on the URL represents a space, so the
helper function is updated to handle both + and %2B as ascending.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/REST/Plugin/Query.t
=> SUCCESS: Tests pass!
- Sign off :-D

Edit: Removed rebasing leftover making the tests fail.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Jonathan Druart 2017-12-11 20:47:42 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 22 Nick Clemens 2017-12-20 11:42:35 UTC
Awesome work all! Enhancement, won't backport for for 17.11.01