Bug 24432

Summary: order_by broken for date columns
Product: Koha Reporter: Agustín Moyano <agustinmoyano>
Component: REST APIAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact:
Severity: blocker    
Priority: P5 - low CC: joonas.kylmala, josef.moravec, joy, julian.maurice, kyle, martin.renvoize, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 23893    
Bug Blocks:    
Attachments: Bug 24432: Regresion test
Bug 24432: Regresion test
Bug 24432: add Koha::Objects->from_api_mapping
Bug 24432: Use Koha::Objects->from_api_mapping instead of attributes_from_api to get the column name in Koha::REST::Plugins::Query->_build_order_atom
Bug 24432: Regresion test
Bug 24432: add Koha::Objects->from_api_mapping
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom
Bug 24432: Regresion test
Bug 24432: add Koha::Objects->from_api_mapping
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom

Description Agustín Moyano 2020-01-15 21:39:06 UTC
When you request an endpoint in the API with a date as orderby, it throws Koha::Exceptions::BadParameter.

Koha::REST::Plugin::Query in _build_order_atom, in order to get the model parameter for the order by, it requests 

{instance of Koha::Objects}->attributes_from_api({ <api parameter name> => 1 });

to get that hash translated, and use the resulting key as the model column to do the order by.

This works ok for all datatypes except date types, because it's trying to transate the value of "1" as a date, throwig the mentioned exception.

In my opinion, _build_order_atom should not use attributes_from_api, but should use the mapping in Koha::Object to get the column name.
Comment 1 Agustín Moyano 2020-01-15 22:54:25 UTC
Created attachment 97421 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.
Comment 2 Agustín Moyano 2020-01-15 23:18:35 UTC
Created attachment 97424 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.
Comment 3 Agustín Moyano 2020-01-15 23:18:38 UTC
Created attachment 97425 [details] [review]
Bug 24432: add Koha::Objects->from_api_mapping

This patch adds from_api_mapping to Koha::Objects, in order to be able to get the mapping from a result set.

To test:
1. apply this patch
2. prove t/db_dependent/Koha/Objects.t
3. sign off
Comment 4 Agustín Moyano 2020-01-15 23:18:42 UTC
Created attachment 97426 [details] [review]
Bug 24432: Use Koha::Objects->from_api_mapping instead of attributes_from_api to get the column name in Koha::REST::Plugins::Query->_build_order_atom

Use the mapping instead of building a fake hash only to get the key name.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t
3. sign off
Comment 5 Tomás Cohen Arazi 2020-01-17 14:31:42 UTC
Great catch!
Comment 6 Tomás Cohen Arazi 2020-01-17 14:37:03 UTC
Created attachment 97528 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2020-01-17 14:37:07 UTC
Created attachment 97529 [details] [review]
Bug 24432: add Koha::Objects->from_api_mapping

This patch adds from_api_mapping to Koha::Objects, in order to be able to get the mapping from a result set.

To test:
1. apply this patch
2. prove t/db_dependent/Koha/Objects.t
3. sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2020-01-17 14:37:10 UTC
Created attachment 97530 [details] [review]
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom

Use the mapping instead of building a fake hash only to get the key name.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t
3. sign off

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Nick Clemens 2020-01-21 13:21:52 UTC
Created attachment 97660 [details] [review]
Bug 24432: Regresion test

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t

Test should fail at this point.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2020-01-21 13:21:56 UTC
Created attachment 97661 [details] [review]
Bug 24432: add Koha::Objects->from_api_mapping

This patch adds from_api_mapping to Koha::Objects, in order to be able to get the mapping from a result set.

To test:
1. apply this patch
2. prove t/db_dependent/Koha/Objects.t
3. sign off

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2020-01-21 13:22:00 UTC
Created attachment 97662 [details] [review]
Bug 24432: Use from_api_mapping to translate column name in _build_order_atom

Use the mapping instead of building a fake hash only to get the key name.

To test:
1. apply this patch
2. prove t/Koha/REST/Plugin/Query.t
3. sign off

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Martin Renvoize 2020-01-23 08:53:40 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Joy Nelson 2020-02-06 22:50:48 UTC
does not apply to 19.11.x
not backported