[Bug 24191] New: Sorting doesn't use to_model
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Bug ID: 24191 Summary: Sorting doesn't use to_model Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com There are no tests for sorting the results from objects.search (there are unit test for the query building code only) and it is not working as expected, as objects.search is not using to_model to map the attribute names. To reproduce: - Perform the following with your favourite tool: GET http://kohadev-intra.myDNSname.org:8081/api/v1/cities?_order_by=+name => FAIL: You get a DBIC error about 'name' not being a valid column name -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |josef.moravec@gmail.com, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 96114 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96114&action=edit Bug 24191: Regression tests This patch adds missing tests for calling objects.search with non-existent column names for sorting, that should be mapped using to_model for that matter. Tests should fail as there's no current use of to_model for building the order_by portion of the query. Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 96115 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96115&action=edit Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting This patch adds a to_model parameter to dbic_merge_sorting so it is passed when used (for example from objects.search). The to_model param is passed along to the _build_order_atom method where it is finally used. In the process I wrote tests that reflected some problems in the current code: - Mojolicious automatically returns a scalar if a query parameter only happens once on a request. The code expected an arrayref in every case. - There's a design issue that forced me to use some hacky code in _build_order_atom. The first issue is dealth with, by using Scalar::Util::reftype as the Perl docs recommend. The second issue, I don't plan to clean it here, as there's ongoing work on a Koha::Objects->search_from_api method that will obsolete this code most probably (see bug 23893 for a better picture of where the mappings will be living soon). To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t => SUCCESS: Tests pass!! 3. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 96116 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96116&action=edit Bug 24191: Make objects.search pass to_model to dbic_merge_sorting This patch makes objects.search pass the to_mode method reference so it can be used to map API attributes to column names when building the sorting portion of the query. To test: 1. Apply the regression tests on this patchset 2. Run: $ kshell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => FAIL: Tests fail! 3. Apply the rest of the patchset 4. Run: k$ prove t/Koha/REST/Plugin/Query.t => SUCCESS: Tests pass! changes to the methods work as expected! 5. Run: k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 6. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- @RM I set this to major as it is a blocker for serious uses of the API pagination and sorting. Please feel free to set it back to normal! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96114|0 |1 is obsolete| | Attachment #96115|0 |1 is obsolete| | Attachment #96116|0 |1 is obsolete| | --- Comment #5 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 96117 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96117&action=edit Bug 24191: Regression tests This patch adds missing tests for calling objects.search with non-existent column names for sorting, that should be mapped using to_model for that matter. Tests should fail as there's no current use of to_model for building the order_by portion of the query. Sponsored-by: ByWater Solutions Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #6 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 96118 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96118&action=edit Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting This patch adds a to_model parameter to dbic_merge_sorting so it is passed when used (for example from objects.search). The to_model param is passed along to the _build_order_atom method where it is finally used. In the process I wrote tests that reflected some problems in the current code: - Mojolicious automatically returns a scalar if a query parameter only happens once on a request. The code expected an arrayref in every case. - There's a design issue that forced me to use some hacky code in _build_order_atom. The first issue is dealth with, by using Scalar::Util::reftype as the Perl docs recommend. The second issue, I don't plan to clean it here, as there's ongoing work on a Koha::Objects->search_from_api method that will obsolete this code most probably (see bug 23893 for a better picture of where the mappings will be living soon). To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t => SUCCESS: Tests pass!! 3. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 96119 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96119&action=edit Bug 24191: Make objects.search pass to_model to dbic_merge_sorting This patch makes objects.search pass the to_mode method reference so it can be used to map API attributes to column names when building the sorting portion of the query. To test: 1. Apply the regression tests on this patchset 2. Run: $ kshell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => FAIL: Tests fail! 3. Apply the rest of the patchset 4. Run: k$ prove t/Koha/REST/Plugin/Query.t => SUCCESS: Tests pass! changes to the methods work as expected! 5. Run: k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 6. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96117|0 |1 is obsolete| | --- Comment #8 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 96133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96133&action=edit Bug 24191: Regression tests This patch adds missing tests for calling objects.search with non-existent column names for sorting, that should be mapped using to_model for that matter. Tests should fail as there's no current use of to_model for building the order_by portion of the query. Sponsored-by: ByWater Solutions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96118|0 |1 is obsolete| | --- Comment #9 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 96134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96134&action=edit Bug 24191: Add to_model param to _build_order_atom and dbic_merge_sorting This patch adds a to_model parameter to dbic_merge_sorting so it is passed when used (for example from objects.search). The to_model param is passed along to the _build_order_atom method where it is finally used. In the process I wrote tests that reflected some problems in the current code: - Mojolicious automatically returns a scalar if a query parameter only happens once on a request. The code expected an arrayref in every case. - There's a design issue that forced me to use some hacky code in _build_order_atom. The first issue is dealth with, by using Scalar::Util::reftype as the Perl docs recommend. The second issue, I don't plan to clean it here, as there's ongoing work on a Koha::Objects->search_from_api method that will obsolete this code most probably (see bug 23893 for a better picture of where the mappings will be living soon). To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/Koha/REST/Plugin/Query.t => SUCCESS: Tests pass!! 3. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96119|0 |1 is obsolete| | --- Comment #10 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 96135 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96135&action=edit Bug 24191: Make objects.search pass to_model to dbic_merge_sorting This patch makes objects.search pass the to_mode method reference so it can be used to map API attributes to column names when building the sorting portion of the query. To test: 1. Apply the regression tests on this patchset 2. Run: $ kshell k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => FAIL: Tests fail! 3. Apply the rest of the patchset 4. Run: k$ prove t/Koha/REST/Plugin/Query.t => SUCCESS: Tests pass! changes to the methods work as expected! 5. Run: k$ prove t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 6. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |19.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|19.05.00 |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |joy@bywatersolutions.com Version(s)|20.05.00 |20.05.00, 19.11.01 released in| | --- Comment #12 from Joy Nelson <joy@bywatersolutions.com> --- Pushed to 19.11.01 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable --- Comment #13 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 19.05.x for 19.05.07 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Pushed to oldoldstable CC| |hayleymapley@catalyst.net.n | |z Version(s)|20.05.00, 19.11.01 |20.05.00, 19.11.01, released in| |18.11.13 --- Comment #14 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Backported to 18.11.x for 18.11.13 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24191 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.05.00, 19.11.01, |20.05.00, 19.11.01, released in|18.11.13 |19.05.07, 18.11.13 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org