We can make a single column contain data from multiple 'data' points in the ajax response using a mixture of a render function and concatenating the data points with ':' in the data field. This allows for a nice display and searching to work.. but the ordering call fails :( The final patch on bug 26273 highlights this: { "data": "manager.firstname:manager.surname", "searchable": true, "orderable": true, "render": function(data, type, row, meta) { var fullname = row.manager.firstname + " " + row.manager.surname; return fullname; } }, Assuming manager.firstname = 'Bob' and manager.surname = 'Geldoff'.. we will display "Bob Geldoff". Searching for 'Bob' will add a suitable search header searching for 'Bob' in both 'manager.firstname' and 'manager.surname' fields.. but hitting order by on this column will result in a 500 error as we add '_order_by: +manager.firstname:manager.surname' to the query parameters.
Created attachment 116794 [details] [review] :x Bug 27680: Take first order column for ordering
Created attachment 116795 [details] [review] Bug 27680: [DO NOT PUSH] Example patch on cities
Created attachment 116796 [details] [review] Bug 27680: Take first order column for ordering
(In reply to Jonathan Druart from comment #3) > Created attachment 116796 [details] [review] [review] > Bug 27680: Take first order column for ordering We can go with this patch for now. Are the REST API routes support ordering on several attributes?
(In reply to Jonathan Druart from comment #4) > (In reply to Jonathan Druart from comment #3) > > Created attachment 116796 [details] [review] [review] [review] > > Bug 27680: Take first order column for ordering > > We can go with this patch for now. > > Are the REST API routes support ordering on several attributes? It appears to: https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/REST/Plugin/Query.pm#L87 :)
(In reply to Jonathan Druart from comment #4) > (In reply to Jonathan Druart from comment #3) > > Created attachment 116796 [details] [review] [review] [review] > > Bug 27680: Take first order column for ordering > > We can go with this patch for now. > > Are the REST API routes support ordering on several attributes? Looks like we do 95 if ( reftype($order_by) and reftype($order_by) eq 'ARRAY' ) { So either we go first with this quick fix, or we need to implement it for the DT API wrapper. Currently it does not work if you select more than 1 column.
Created attachment 116829 [details] [review] Bug 27680: Add multi-field support
Still working here.. I found a series of little bugs and am writing some extra unit tests
Created attachment 116831 [details] [review] Bug 27680: Take first order column for ordering
Created attachment 116832 [details] [review] Bug 27680: Add multi-field support
Created attachment 116833 [details] [review] Bug 27680: Make collectionFormat open for order_by
Created attachment 116834 [details] [review] Bug 27680: Add tests for various ways of passing multi-params
As the final tests expose.. this raises a question. What form of 'multi-param' sending do we actually want to support? Prior to my patches we were "in theory" limiting to pipe delimited strings.. but the tests weren't passing any pipe delimited parameters and were passing for 'traditional multiple params' (i.e ?param1=+thingy¶m2=+thangy). I've added tests to cover the following.. but clearly many of them currently fail. 1. Multi param traditional `?param=+this¶m=+that` - Passing 2. Multi param PHP `?param[]=+this¶m[]=+that` - Failing 3. CSV param `?param=+this,+that` - Failing I like option 1, but feel we should fail in a more formal way for any/all other options.. right now we just explode horribly.
It does not work for me. https://snipboard.io/tzlfnE.jpg generates: /api/v1/cities?&_page=1&_per_page=20&q=%5B%5D&_match=contains&_order_by=-me.city_id
Actually I forgot to restart_all. Now the table does not display at all OpenAPI <<< GET api/v1/cities [{"message":"Expected array - got string.","path":"\/_order_by"}]
Created attachment 116843 [details] [review] Bug 27680: Take first order column for ordering
Created attachment 116844 [details] [review] Bug 27680: Add support for sorting fields with multiple data points This patch adds proper handling for sorting a single column that is constructed of multiple data entities.. i.e `"data": "string1:string2"` It does NOT add support for filtering on multiple columns yet.
Created attachment 116845 [details] [review] Bug 27680: Add tests for various ways of passing multi-params This patch adds unit test to increase the coverage of parameter that accept multiple values. There are a number of different ways end users can send such parameters and we should test to ensure we are recieving the correct option. Options `?param1=this¶m1=that` - traditional multiple pass params `?param1[]=this¶m1[]=that` - php multiple pass params `?param1=this,that` - comma delimited list param `?param1=this|that` - pipe delimited list param
Created attachment 116846 [details] [review] Bug 27680: Allow traditional multi-params to work This patch allows the preceeding test to pass. Strictly, we want a comma delimited string for our _order_by parameter, but we cannot easily block a traditional multi-passed parameter. As such the 'nice' thing to do is handle it when such a thing is passed as it will pass through validation regardless.
This is the most pragmatic solution I could come up with. * We set the datatable wrapper to send _order_by as a comma delimited string of fields. * We update the collectionFormat spec to 'csv' from 'pipes'.. pipes was a bad value to start with as it's 'pipe' and 'csv' is more common. * We add handling to allow for traditional multiple passed params by name (as we can't rule them out via validation for some reason so it's nicer to handle them than it is to throw a nasty serverside error. * We add notes to some tests that are expected to result in a nasty 500 error. We cannot prevent arbitrary parameters from being sent to the API (which includes the _order_by[] PHP style multiple pass option, and non-csv delimited string options). Looking for feedback as to whether this is enough.. I'm thinking perhaps we need to throw a better Koha level exception when catching bad column names in our query string perhaps?
Created attachment 117335 [details] [review] Bug 27680: [DO NOT PUSH] Example patch on cities
Created attachment 117336 [details] [review] Bug 27680: Take first order column for ordering
Created attachment 117337 [details] [review] Bug 27680: Add support for sorting fields with multiple data points This patch adds proper handling for sorting a single column that is constructed of multiple data entities.. i.e `"data": "string1:string2"` It does NOT add support for filtering on multiple columns yet.
Created attachment 117338 [details] [review] Bug 27680: Add tests for various ways of passing multi-params This patch adds unit test to increase the coverage of parameter that accept multiple values. There are a number of different ways end users can send such parameters and we should test to ensure we are recieving the correct option. Options `?param1=this¶m1=that` - traditional multiple pass params `?param1[]=this¶m1[]=that` - php multiple pass params `?param1=this,that` - comma delimited list param `?param1=this|that` - pipe delimited list param
Created attachment 117339 [details] [review] Bug 27680: Allow traditional multi-params to work This patch allows the preceeding test to pass. Strictly, we want a comma delimited string for our _order_by parameter, but we cannot easily block a traditional multi-passed parameter. As such the 'nice' thing to do is handle it when such a thing is passed as it will pass through validation regardless.
Created attachment 117340 [details] [review] Bug 27680: (follow-up) Add Koha::Exception for bad queries This patch adds a PropertyNotFound exception tothe 'as_list' method to catch DBIx::Class excpetions caused by passing bad column names into a query.
Created attachment 117341 [details] [review] Bug 27680: (follow-up) Attempt to catch the exception Help: I'm struggling to catch the point at which the exception should be thrown to convert it to a 400 error response in the API. '->search' does not actually trigger the DB hit until we reference some data from it.. so we can't use that.. so I tried '->count', but of course.. that drops 'order_by' which is the failure in this case and so we pass through.
Created attachment 117375 [details] [review] Bug 27680: (follow-up) Attempt to catch the exception Help: I'm struggling to catch the point at which the exception should be thrown to convert it to a 400 error response in the API. '->search' does not actually trigger the DB hit until we reference some data from it.. so we can't use that.. so I tried '->count', but of course.. that drops 'order_by' which is the failure in this case and so we pass through.
I've moved the followup bugs to try and render exceptions more cleanly to another bug now.. I'd really like to get this bug unblocked so the dependant cash management bug can start moving again.
Created attachment 117388 [details] [review] Bug 27680: [DO NOT PUSH] Example patch on cities Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117389 [details] [review] Bug 27680: Take first order column for ordering Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117390 [details] [review] Bug 27680: Add support for sorting fields with multiple data points This patch adds proper handling for sorting a single column that is constructed of multiple data entities.. i.e `"data": "string1:string2"` It does NOT add support for filtering on multiple columns yet. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117391 [details] [review] Bug 27680: Add tests for various ways of passing multi-params This patch adds unit test to increase the coverage of parameter that accept multiple values. There are a number of different ways end users can send such parameters and we should test to ensure we are recieving the correct option. Options `?param1=this¶m1=that` - traditional multiple pass params `?param1[]=this¶m1[]=that` - php multiple pass params `?param1=this,that` - comma delimited list param `?param1=this|that` - pipe delimited list param Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117392 [details] [review] Bug 27680: Allow traditional multi-params to work This patch allows the preceeding test to pass. Strictly, we want a comma delimited string for our _order_by parameter, but we cannot easily block a traditional multi-passed parameter. As such the 'nice' thing to do is handle it when such a thing is passed as it will pass through validation regardless. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117393 [details] [review] Bug 27680: Add support for param[] syntax While not that common nowadays, it is the syntax PHP uses, and DataTables also generates such thing in 'traditional' mode. We should support it as well. This patch adds support for that. It does so by adding _order_by[] to the reserved param names, and proper handling on the dbic_merge_sorting helper. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 3- Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117394 [details] [review] Bug 27680: (QA follow-up) Minor perlcritic issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117438 [details] [review] Bug 27680: Take first order column for ordering Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 117439 [details] [review] Bug 27680: Add support for sorting fields with multiple data points This patch adds proper handling for sorting a single column that is constructed of multiple data entities.. i.e `"data": "string1:string2"` It does NOT add support for filtering on multiple columns yet. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 117440 [details] [review] Bug 27680: Add tests for various ways of passing multi-params This patch adds unit test to increase the coverage of parameter that accept multiple values. There are a number of different ways end users can send such parameters and we should test to ensure we are recieving the correct option. Options `?param1=this¶m1=that` - traditional multiple pass params `?param1[]=this¶m1[]=that` - php multiple pass params `?param1=this,that` - comma delimited list param `?param1=this|that` - pipe delimited list param Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 117441 [details] [review] Bug 27680: Allow traditional multi-params to work This patch allows the preceeding test to pass. Strictly, we want a comma delimited string for our _order_by parameter, but we cannot easily block a traditional multi-passed parameter. As such the 'nice' thing to do is handle it when such a thing is passed as it will pass through validation regardless. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 117442 [details] [review] Bug 27680: Add support for param[] syntax While not that common nowadays, it is the syntax PHP uses, and DataTables also generates such thing in 'traditional' mode. We should support it as well. This patch adds support for that. It does so by adding _order_by[] to the reserved param names, and proper handling on the dbic_merge_sorting helper. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 3- Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 27680: (QA follow-up) Minor perlcritic issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 21.05, thanks to everybody involved!
We have a test that is failing: # Failed test 'exact match for JSON Pointer "/order_by"' # at t/Koha/REST/Plugin/Query.t line 338. # Structures begin differing at: # $got = ARRAY(0x55702f09b360) # $expected = HASH(0x55702f094400) # Looks like you failed 1 test of 20. # Failed test 'dbic_merge_sorting() tests' # at t/Koha/REST/Plugin/Query.t line 343.
Created attachment 117476 [details] [review] Bug 27680: (QA follow-up) Update test for dbic_merge_sorting The order_by handling will now always return an arrayref of ordering hashrefs even when only one ordering hash is present. This is interpreted by dbic as equivilent so I just update the test to pass here.
follow-up pushed to master
Pushed to 20.11.x for 20.11.04
Pushed to 20.05.x for 20.05.10
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.