Bug 20212 - Slowness in receiving in acquisitions
Summary: Slowness in receiving in acquisitions
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 20 votes (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
: 10959 (view as bug list)
Depends on: 25670 27402 18731 24366 24418 24419 24430 24435 24440 24448 24457 24461 24462 24467 24528 24554 24561 24830 24918 25297
Blocks: 8179
  Show dependency treegraph
 
Reported: 2018-02-15 13:44 UTC by Kyle M Hall
Modified: 2021-01-13 12:06 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This development changes how the orders table on receiving in acquisitions is rendered. Before this patches all active orders data was fetched from the database, along with all the related data (patrons, biblio). This lead to significant delays because all orders matching the filtering criteria were loaded, even when only 20 rows are displayed on the UI. With this patch, the table is rendering using server-side pagination, provided by the REST API. This means (1) only the information to be rendered is fetched (faster drawing (2) the page doesn't reload anymore when a new filter is added: it just refreshes the table, very fastly.
Version(s) released in:


Attachments
Bug 20212: Extend datatables for API interaction (3.71 KB, patch)
2020-01-17 16:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.19 KB, patch)
2020-01-17 16:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (15.97 KB, patch)
2020-01-17 16:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.27 KB, patch)
2020-01-20 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (16.02 KB, patch)
2020-01-20 15:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.34 KB, patch)
2020-01-20 19:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (16.14 KB, patch)
2020-01-20 19:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (29.10 KB, patch)
2020-01-20 20:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Display correct number of items (1.19 KB, patch)
2020-01-22 16:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Use the API to render orders (31.22 KB, patch)
2020-01-31 12:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (32.26 KB, patch)
2020-01-31 13:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (32.27 KB, patch)
2020-01-31 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.34 KB, patch)
2020-02-27 23:53 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20212: Use the API to render orders (32.27 KB, patch)
2020-02-27 23:53 UTC, Agustín Moyano
Details | Diff | Splinter Review
[WIP] Bug 20212: Use the API to render orders (32.27 KB, patch)
2020-02-27 23:54 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20212: Use the API to render orders (31.70 KB, patch)
2020-03-04 18:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing entried on schema files (3.42 KB, patch)
2020-03-10 19:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.86 KB, patch)
2020-03-10 19:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Allow filtering orders on ISBN (6.04 KB, patch)
2020-03-10 19:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (37.72 KB, patch)
2020-03-10 19:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing entries on schema files (4.30 KB, patch)
2020-03-11 13:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.86 KB, patch)
2020-03-11 13:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Allow filtering orders on ISBN (6.04 KB, patch)
2020-03-11 13:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (24.68 KB, patch)
2020-03-11 13:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing entries on schema files (5.18 KB, patch)
2020-03-11 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.86 KB, patch)
2020-03-11 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Allow filtering orders on biblioitems fields (6.23 KB, patch)
2020-03-11 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (26.49 KB, patch)
2020-03-11 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (34.97 KB, patch)
2020-03-11 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.55 KB, patch)
2020-03-11 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (35.00 KB, patch)
2020-03-12 15:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing entries on schema files (4.88 KB, patch)
2020-03-20 19:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (2.86 KB, patch)
2020-04-24 13:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Allow filtering orders on biblioitems fields (6.23 KB, patch)
2020-04-24 13:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.56 KB, patch)
2020-04-24 13:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (35.00 KB, patch)
2020-04-24 13:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing entries on schema files (2.07 KB, patch)
2020-04-24 13:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (11.31 KB, patch)
2020-09-10 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.56 KB, patch)
2020-09-10 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (35.73 KB, patch)
2020-09-10 18:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing relation in Biblio (953 bytes, patch)
2020-09-10 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (35.76 KB, patch)
2020-09-10 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (11.31 KB, patch)
2020-09-10 19:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.56 KB, patch)
2020-09-10 19:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (35.76 KB, patch)
2020-09-10 19:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add missing relation in Biblio (953 bytes, patch)
2020-09-10 19:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Remove warning if no basket group (1.76 KB, patch)
2020-09-11 12:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Plack error when trying to receive w/ library data (9.96 KB, text/plain)
2020-09-16 19:30 UTC, Andrew Fuerste-Henry
Details
Bug 20212: Remove warning if no basket group (1.49 KB, patch)
2020-09-16 20:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (36.09 KB, patch)
2020-09-21 15:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (38.57 KB, patch)
2020-09-21 19:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Screenshot of table after patch (439.78 KB, image/png)
2020-10-13 16:35 UTC, Martin Renvoize
Details
Screenshot of table before patch (488.36 KB, image/png)
2020-10-13 16:35 UTC, Martin Renvoize
Details
Bug 20212: Use API to render orders datatable (38.35 KB, patch)
2020-10-23 20:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (11.43 KB, patch)
2020-10-26 16:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.71 KB, patch)
2020-10-26 16:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20212: Add missing relation in Biblio (1.07 KB, patch)
2020-10-26 16:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (38.50 KB, patch)
2020-10-26 16:37 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (39.01 KB, patch)
2020-10-29 23:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (39.04 KB, patch)
2020-10-30 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (11.45 KB, patch)
2020-10-30 13:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.71 KB, patch)
2020-10-30 13:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Add missing relation in Biblio (1.08 KB, patch)
2020-10-30 13:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (39.04 KB, patch)
2020-10-30 13:58 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (11.52 KB, patch)
2020-10-30 13:59 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.78 KB, patch)
2020-10-30 13:59 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Add missing relation in Biblio (1.14 KB, patch)
2020-10-30 14:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (39.11 KB, patch)
2020-10-30 14:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Remember current filter (9.27 KB, patch)
2020-11-02 20:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Controller changes regression tests (4.18 KB, patch)
2020-11-02 23:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Restore table filter sticky (1.05 KB, patch)
2020-11-04 12:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Resurrect column filtering (12.21 KB, patch)
2020-11-06 19:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Make all biblioitems.* fields searchable (3.27 KB, patch)
2020-11-06 19:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Add more embeddable objects to orders (11.59 KB, patch)
2020-12-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: Clean parcel.pl (5.83 KB, patch)
2020-12-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: Add missing relation in Biblio (1.20 KB, patch)
2020-12-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: Use API to render orders datatable (39.16 KB, patch)
2020-12-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Remember current filter (9.33 KB, patch)
2020-12-22 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: Controller changes regression tests (4.24 KB, patch)
2020-12-22 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Restore table filter sticky (1.11 KB, patch)
2020-12-22 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Resurrect column filtering (12.27 KB, patch)
2020-12-22 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: Make all biblioitems.* fields searchable (3.34 KB, patch)
2020-12-22 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Restore missing comma (1.22 KB, patch)
2020-12-22 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Spelling typo fix (1.03 KB, patch)
2020-12-22 16:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20212: (QA follow-up) Fix escaping (3.55 KB, patch)
2021-01-07 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Remove clone (1.17 KB, patch)
2021-01-08 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Fix test after 25670 follow-up (1.06 KB, patch)
2021-01-08 13:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Use functions to escape variables (3.65 KB, patch)
2021-01-08 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: (follow-up) Avoid pagination on full resultset test (1.14 KB, patch)
2021-01-08 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20212: Search on fund's name instead of id (1.07 KB, patch)
2021-01-08 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Remove fixedHeader (1.07 KB, patch)
2021-01-12 08:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Use the DT column filtering provided by the wrapper (28.07 KB, patch)
2021-01-12 09:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Use the DT column filtering provided by the wrapper (27.80 KB, patch)
2021-01-12 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Use the DT column filtering provided by the wrapper (28.21 KB, patch)
2021-01-12 09:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Use the DT column filtering provided by the wrapper (28.21 KB, patch)
2021-01-12 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Re-add EAN for UNIMARC (1.96 KB, patch)
2021-01-13 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Fix query for the global filter (2.96 KB, patch)
2021-01-13 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20212: Remove fix_query and assume ugliness (5.59 KB, patch)
2021-01-13 10:59 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-02-15 13:44:27 UTC
Convert order tables from pre-loading all order data to ajax tables that load order data on an as needed basis.
Comment 1 Katrin Fischer 2018-02-15 16:10:32 UTC
*** Bug 10959 has been marked as a duplicate of this bug. ***
Comment 2 Liz Rea 2018-07-03 01:45:36 UTC
Is this work in progress or are other solutions to this acceptable.
Comment 3 Kyle M Hall 2018-07-09 16:50:04 UTC
(In reply to Liz Rea from comment #2)
> Is this work in progress or are other solutions to this acceptable.

If you have thoughts or plans I'd love to hear them! As far as I'm aware, we haven't put pen to paper yet :)
Comment 4 Tomás Cohen Arazi 2019-02-20 15:13:18 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2019-02-20 15:13:43 UTC
And please help make bug 18731 into master ASAP!
Comment 6 Tomás Cohen Arazi 2020-01-13 14:52:44 UTC
Time to finish this as all dependencies are in master \o/
Comment 7 Tomás Cohen Arazi 2020-01-17 16:07:59 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2020-01-17 16:08:02 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2020-01-17 16:08:06 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2020-01-17 16:09:03 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2020-01-20 15:52:48 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2020-01-20 15:52:52 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2020-01-20 15:54:19 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2020-01-20 19:20:09 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2020-01-20 19:20:14 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2020-01-20 20:01:10 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2020-01-22 16:20:24 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2020-01-31 12:21:18 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2020-01-31 13:23:55 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2020-01-31 14:45:43 UTC Comment hidden (obsolete)
Comment 21 Agustín Moyano 2020-02-27 23:53:02 UTC Comment hidden (obsolete)
Comment 22 Agustín Moyano 2020-02-27 23:53:09 UTC Comment hidden (obsolete)
Comment 23 Agustín Moyano 2020-02-27 23:54:31 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2020-03-04 18:29:20 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2020-03-10 19:24:05 UTC
Created attachment 100475 [details] [review]
Bug 20212: Add missing entried on schema files
Comment 26 Tomás Cohen Arazi 2020-03-10 19:24:09 UTC
Created attachment 100476 [details] [review]
Bug 20212: Add more embeddable objects to orders
Comment 27 Tomás Cohen Arazi 2020-03-10 19:24:14 UTC
Created attachment 100477 [details] [review]
Bug 20212: Allow filtering orders on ISBN

This patch adapts the code from the list() sub so it manipulates the
query parameters and the embed header so:
- the biblioitem relationship is prefetch
- any queries on biblio.isbn are correctly translated into search on the
  biblioitems table.
Comment 28 Tomás Cohen Arazi 2020-03-10 19:24:19 UTC
Created attachment 100478 [details] [review]
Bug 20212: Use API to render orders datatable
Comment 29 Tomás Cohen Arazi 2020-03-10 19:28:01 UTC
I've set it NSO to gather some attention. This is not ready yet (cleanup is required for that). But The only way I found of testing it is displaying both tables (the original and the API-driven so the behaviour can be compared).

The patchset touches histsearch.pl but only the table displayed when adding an invoice is complete-ish.

The only missing feature is the in-column filtering, but the main technical challenge is solved (filtering on bilbioitems.isbn).

Please test and provide feedback!
Comment 30 Tomás Cohen Arazi 2020-03-11 13:37:52 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2020-03-11 13:37:56 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2020-03-11 13:38:01 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2020-03-11 13:38:05 UTC Comment hidden (obsolete)
Comment 34 Tomás Cohen Arazi 2020-03-11 18:49:23 UTC Comment hidden (obsolete)
Comment 35 Tomás Cohen Arazi 2020-03-11 18:49:26 UTC Comment hidden (obsolete)
Comment 36 Tomás Cohen Arazi 2020-03-11 18:49:30 UTC Comment hidden (obsolete)
Comment 37 Tomás Cohen Arazi 2020-03-11 18:49:35 UTC Comment hidden (obsolete)
Comment 38 Tomás Cohen Arazi 2020-03-11 20:11:01 UTC Comment hidden (obsolete)
Comment 39 Tomás Cohen Arazi 2020-03-11 20:11:06 UTC Comment hidden (obsolete)
Comment 40 Tomás Cohen Arazi 2020-03-11 20:13:26 UTC
So this is ready for real testing. I cleaned the code related to listing the pending orders from parcel.pl as well. If we agree on this approach, the same API and code can be reused in all places that display orders! YAY
Comment 41 Tomás Cohen Arazi 2020-03-12 15:25:51 UTC Comment hidden (obsolete)
Comment 42 Martin Renvoize 2020-03-18 09:02:19 UTC
Think I'm missing where this requires bug 24455?
Comment 43 Tomás Cohen Arazi 2020-03-19 12:12:53 UTC
(In reply to Martin Renvoize from comment #42)
> Think I'm missing where this requires bug 24455?

It doesn't, removed the dependency.
Comment 44 Tomás Cohen Arazi 2020-03-20 19:28:00 UTC Comment hidden (obsolete)
Comment 45 Tomás Cohen Arazi 2020-03-22 01:39:06 UTC
I've got an interesting DB with lots of real-life acq data, and:
- There are some use cases that I didn't consider
- Some combinations of the data produce error 500.
Comment 46 Tomás Cohen Arazi 2020-04-24 13:27:19 UTC Comment hidden (obsolete)
Comment 47 Tomás Cohen Arazi 2020-04-24 13:27:38 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2020-04-24 13:27:57 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2020-04-24 13:28:08 UTC Comment hidden (obsolete)
Comment 50 Tomás Cohen Arazi 2020-04-24 13:28:22 UTC Comment hidden (obsolete)
Comment 51 Tomás Cohen Arazi 2020-09-10 18:58:25 UTC Comment hidden (obsolete)
Comment 52 Tomás Cohen Arazi 2020-09-10 18:58:34 UTC Comment hidden (obsolete)
Comment 53 Tomás Cohen Arazi 2020-09-10 18:58:46 UTC Comment hidden (obsolete)
Comment 54 Tomás Cohen Arazi 2020-09-10 18:59:05 UTC Comment hidden (obsolete)
Comment 55 Tomás Cohen Arazi 2020-09-10 19:45:21 UTC Comment hidden (obsolete)
Comment 56 Tomás Cohen Arazi 2020-09-10 19:49:36 UTC
Created attachment 109867 [details] [review]
Bug 20212: Add more embeddable objects to orders

This patch adds options to embed more related objects based on the needs
by parcel.tt.

For filtering by biblioitems fields (ISBN and EAN) I had to make the
'list' method a modified version of the objects->search helper. I
thought of doing it in a more generic way but I didn't find any other
use cases and it would certainly make an already complex piece of code
even more complex.

So this is quite similar, but at some steps the biblio.<biblioitem
field> gets translated into the proper relation names, and the same
happens for prefetching.

A new parameter is also added: only_active. It makes the controller use
Koha::Acquisition::Orders->filter_by_active, avoiding the need to build
complex queries in the UI.

The same handling is done when the order_id parameter is passed (outside
the q= parameters). In this case using Koha::Acquisition::Orders->filter_by_id_including_transfers

This is all respecting the C4::Acquisitions::SearchOrders behaviour.

TL;DR:

This patch adapts the code from the list() sub so it manipulates the
query parameters and the embed header so:
- the biblioitem relationship is prefetch
- any queries on biblio.isbn and biblio.ean are correctly translated into search on the
  biblioitems table.
- Adds an only_active parameter to the /acquisitions/orders route to
  easily request only the active orders.
Comment 57 Tomás Cohen Arazi 2020-09-10 19:49:57 UTC
Created attachment 109868 [details] [review]
Bug 20212: Clean parcel.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 58 Tomás Cohen Arazi 2020-09-10 19:50:15 UTC Comment hidden (obsolete)
Comment 59 Tomás Cohen Arazi 2020-09-10 19:50:37 UTC Comment hidden (obsolete)
Comment 60 Tomás Cohen Arazi 2020-09-11 12:18:35 UTC Comment hidden (obsolete)
Comment 61 Andrew Fuerste-Henry 2020-09-16 19:29:16 UTC
I was able to successfully apply these patches to a fresh master site, but ran into trouble when I loaded an existing library's 19.11 data into my testing docker, updated the database to master, applied these patches and restarted services, and then tried to receive. When I go to parcel.pl, the pending orders list is empty when it should not be and I get a long db error. I'll attach the content of my plack error log in a text file.
Comment 62 Andrew Fuerste-Henry 2020-09-16 19:30:29 UTC
Created attachment 110229 [details]
Plack error when trying to receive w/ library data
Comment 63 Tomás Cohen Arazi 2020-09-16 20:59:34 UTC Comment hidden (obsolete)
Comment 64 Tomás Cohen Arazi 2020-09-21 15:49:56 UTC Comment hidden (obsolete)
Comment 65 Tomás Cohen Arazi 2020-09-21 19:01:59 UTC Comment hidden (obsolete)
Comment 66 Tomás Cohen Arazi 2020-09-21 19:03:14 UTC
Thanks Andrew, the last version of the patch should cover it.
Comment 67 Martin Renvoize 2020-10-13 16:34:23 UTC
Generally this looks like it's going in the right direction to me.

I do wonder if we could preload the first page of results using the template to give the appearance of better performance on initial page load whilst still getting the benefit of serverside processing for the heavy lifting. (As discussed on Slack, see https://datatables.net/examples/server_side/defer_loading.html for possibilities)

However.. I'm not sure the API's is returning the same results as expected... attaching before and after shots for the table with the exact same database.

The ajax version appears to have considerably more results and many of them seem to contain malformed 'null' data?
Comment 68 Martin Renvoize 2020-10-13 16:35:22 UTC
Created attachment 111568 [details]
Screenshot of table after patch
Comment 69 Martin Renvoize 2020-10-13 16:35:46 UTC
Created attachment 111569 [details]
Screenshot of table before patch
Comment 70 Tomás Cohen Arazi 2020-10-13 17:29:51 UTC
(In reply to Martin Renvoize from comment #67)
> Generally this looks like it's going in the right direction to me.
> 
> I do wonder if we could preload the first page of results using the template
> to give the appearance of better performance on initial page load whilst
> still getting the benefit of serverside processing for the heavy lifting.
> (As discussed on Slack, see
> https://datatables.net/examples/server_side/defer_loading.html for
> possibilities)

This is really interesting and I will take a look at it for a follow-up patch/bug depending on the size of it.

> However.. I'm not sure the API's is returning the same results as
> expected... attaching before and after shots for the table with the exact
> same database.
> 
> The ajax version appears to have considerably more results and many of them
> seem to contain malformed 'null' data?

Looking at this, it feels like the original version was hiding the bad rows (i.e. those orders without a basket, which is not something you can end-up with) . I consider it a feature LOL.
Comment 71 Martin Renvoize 2020-10-14 07:38:28 UTC
Hmm, I think you're right, it highlights bad data in my test set..  That's probably a good thing.
Comment 72 Tomás Cohen Arazi 2020-10-23 20:15:00 UTC
Created attachment 112310 [details] [review]
Bug 20212: Use API to render orders datatable

This patch makes the pending orders datatable render using the API
instead of hte old controller script. It implements native server-side
pagination thanks to the API and the datatables wrapper.

On polishing it, we found that data can contain many null values, and it
got dirtier in the process.

This code with all the checks that are done on the data is the result of
testing this dev with real-life production databases, huge ones with
thousands of orderlines.

To test:
1. You should compare the behaviour between this table, and the original
   one in master.
Comment 73 Tomás Cohen Arazi 2020-10-23 20:16:15 UTC
The last patch applies the lessons learned from the libraries page work. Escaping things, as generic as possible. Handle nulls more gracefully.

Hope you enjoy testing this!
Comment 74 Séverine Queune 2020-10-26 16:37:19 UTC
Created attachment 112521 [details] [review]
Bug 20212: Add more embeddable objects to orders

This patch adds options to embed more related objects based on the needs
by parcel.tt.

For filtering by biblioitems fields (ISBN and EAN) I had to make the
'list' method a modified version of the objects->search helper. I
thought of doing it in a more generic way but I didn't find any other
use cases and it would certainly make an already complex piece of code
even more complex.

So this is quite similar, but at some steps the biblio.<biblioitem
field> gets translated into the proper relation names, and the same
happens for prefetching.

A new parameter is also added: only_active. It makes the controller use
Koha::Acquisition::Orders->filter_by_active, avoiding the need to build
complex queries in the UI.

The same handling is done when the order_id parameter is passed (outside
the q= parameters). In this case using Koha::Acquisition::Orders->filter_by_id_including_transfers

This is all respecting the C4::Acquisitions::SearchOrders behaviour.

TL;DR:

This patch adapts the code from the list() sub so it manipulates the
query parameters and the embed header so:
- the biblioitem relationship is prefetch
- any queries on biblio.isbn and biblio.ean are correctly translated into search on the
  biblioitems table.
- Adds an only_active parameter to the /acquisitions/orders route to
  easily request only the active orders.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 75 Séverine Queune 2020-10-26 16:37:25 UTC
Created attachment 112522 [details] [review]
Bug 20212: Clean parcel.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 76 Séverine Queune 2020-10-26 16:37:32 UTC
Created attachment 112523 [details] [review]
Bug 20212: Add missing relation in Biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 77 Séverine Queune 2020-10-26 16:37:39 UTC
Created attachment 112524 [details] [review]
Bug 20212: Use API to render orders datatable

This patch makes the pending orders datatable render using the API
instead of hte old controller script. It implements native server-side
pagination thanks to the API and the datatables wrapper.

On polishing it, we found that data can contain many null values, and it
got dirtier in the process.

This code with all the checks that are done on the data is the result of
testing this dev with real-life production databases, huge ones with
thousands of orderlines.

To test:
1. You should compare the behaviour between this table, and the original
   one in master.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 78 Tomás Cohen Arazi 2020-10-29 23:42:33 UTC Comment hidden (obsolete)
Comment 79 Tomás Cohen Arazi 2020-10-30 13:41:59 UTC
Created attachment 112695 [details] [review]
Bug 20212: Use API to render orders datatable

This patch makes the pending orders datatable render using the API
instead of hte old controller script. It implements native server-side
pagination thanks to the API and the datatables wrapper.

On polishing it, we found that data can contain many null values, and it
got dirtier in the process.

This code with all the checks that are done on the data is the result of
testing this dev with real-life production databases, huge ones with
thousands of orderlines.

To test:
1. You should compare the behaviour between this table, and the original
   one in master.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 80 Andrew Fuerste-Henry 2020-10-30 13:58:13 UTC
Created attachment 112698 [details] [review]
Bug 20212: Add more embeddable objects to orders

This patch adds options to embed more related objects based on the needs
by parcel.tt.

For filtering by biblioitems fields (ISBN and EAN) I had to make the
'list' method a modified version of the objects->search helper. I
thought of doing it in a more generic way but I didn't find any other
use cases and it would certainly make an already complex piece of code
even more complex.

So this is quite similar, but at some steps the biblio.<biblioitem
field> gets translated into the proper relation names, and the same
happens for prefetching.

A new parameter is also added: only_active. It makes the controller use
Koha::Acquisition::Orders->filter_by_active, avoiding the need to build
complex queries in the UI.

The same handling is done when the order_id parameter is passed (outside
the q= parameters). In this case using Koha::Acquisition::Orders->filter_by_id_including_transfers

This is all respecting the C4::Acquisitions::SearchOrders behaviour.

TL;DR:

This patch adapts the code from the list() sub so it manipulates the
query parameters and the embed header so:
- the biblioitem relationship is prefetch
- any queries on biblio.isbn and biblio.ean are correctly translated into search on the
  biblioitems table.
- Adds an only_active parameter to the /acquisitions/orders route to
  easily request only the active orders.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 81 Andrew Fuerste-Henry 2020-10-30 13:58:17 UTC
Created attachment 112699 [details] [review]
Bug 20212: Clean parcel.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 82 Andrew Fuerste-Henry 2020-10-30 13:58:21 UTC
Created attachment 112700 [details] [review]
Bug 20212: Add missing relation in Biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 83 Andrew Fuerste-Henry 2020-10-30 13:58:26 UTC
Created attachment 112702 [details] [review]
Bug 20212: Use API to render orders datatable

This patch makes the pending orders datatable render using the API
instead of hte old controller script. It implements native server-side
pagination thanks to the API and the datatables wrapper.

On polishing it, we found that data can contain many null values, and it
got dirtier in the process.

This code with all the checks that are done on the data is the result of
testing this dev with real-life production databases, huge ones with
thousands of orderlines.

To test:
1. You should compare the behaviour between this table, and the original
   one in master.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 84 Andrew Fuerste-Henry 2020-10-30 13:59:52 UTC
Created attachment 112703 [details] [review]
Bug 20212: Add more embeddable objects to orders

This patch adds options to embed more related objects based on the needs
by parcel.tt.

For filtering by biblioitems fields (ISBN and EAN) I had to make the
'list' method a modified version of the objects->search helper. I
thought of doing it in a more generic way but I didn't find any other
use cases and it would certainly make an already complex piece of code
even more complex.

So this is quite similar, but at some steps the biblio.<biblioitem
field> gets translated into the proper relation names, and the same
happens for prefetching.

A new parameter is also added: only_active. It makes the controller use
Koha::Acquisition::Orders->filter_by_active, avoiding the need to build
complex queries in the UI.

The same handling is done when the order_id parameter is passed (outside
the q= parameters). In this case using Koha::Acquisition::Orders->filter_by_id_including_transfers

This is all respecting the C4::Acquisitions::SearchOrders behaviour.

TL;DR:

This patch adapts the code from the list() sub so it manipulates the
query parameters and the embed header so:
- the biblioitem relationship is prefetch
- any queries on biblio.isbn and biblio.ean are correctly translated into search on the
  biblioitems table.
- Adds an only_active parameter to the /acquisitions/orders route to
  easily request only the active orders.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 85 Andrew Fuerste-Henry 2020-10-30 13:59:56 UTC
Created attachment 112704 [details] [review]
Bug 20212: Clean parcel.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 86 Andrew Fuerste-Henry 2020-10-30 14:00:00 UTC
Created attachment 112705 [details] [review]
Bug 20212: Add missing relation in Biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 87 Andrew Fuerste-Henry 2020-10-30 14:00:05 UTC
Created attachment 112706 [details] [review]
Bug 20212: Use API to render orders datatable

This patch makes the pending orders datatable render using the API
instead of hte old controller script. It implements native server-side
pagination thanks to the API and the datatables wrapper.

On polishing it, we found that data can contain many null values, and it
got dirtier in the process.

This code with all the checks that are done on the data is the result of
testing this dev with real-life production databases, huge ones with
thousands of orderlines.

To test:
1. You should compare the behaviour between this table, and the original
   one in master.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 88 Tomás Cohen Arazi 2020-10-30 14:57:34 UTC
Thanks, Andrew. It's been a very nice testing session, and the things you found were really helpful to make this solid!
Comment 89 Katrin Fischer 2020-11-01 14:19:16 UTC
I am kind of missing unit tests here for the new and changed subs?

There is also an issue here in that the search terms from the left side search or the table search are no longer retained.

To test:
- Enter a search term in either of the search fields
- Add a note, receive or do another action
- The search term is kept
- Apply this patch set
- Repeat actions
- The search term is now lost

Especially when dealing with a long list of orders this is an important feature.
Comment 90 Tomás Cohen Arazi 2020-11-01 14:25:39 UTC
(In reply to Katrin Fischer from comment #89)
> I am kind of missing unit tests here for the new and changed subs?

Which ones?

> There is also an issue here in that the search terms from the left side
> search or the table search are no longer retained.
> 
> To test:
> - Enter a search term in either of the search fields
> - Add a note, receive or do another action
> - The search term is kept
> - Apply this patch set
> - Repeat actions
> - The search term is now lost
> 
> Especially when dealing with a long list of orders this is an important
> feature.

This is paired with the multiple receive patches (dependency) but I see this would be a blocker. I will provide a follow-up to retain that feature in the meantime.
Comment 91 Tomás Cohen Arazi 2020-11-02 20:20:38 UTC
Created attachment 112868 [details] [review]
Bug 20212: (QA follow-up) Remember current filter

This patch restores the original behaviour: if you jump into receiving
an order, and then go back to receiving, you want the page to remember
your filters.

This feature was overlooked. On fixing it, I wrapped some JS code in a
function for reusing it and simplified it a bit as well.

To test:
1. Enter a search term in either of the search fields
2. Add a note, receive or do another action
=> SUCCESS: The search term is kept
3. Apply this patch set
4. Repeat 2
=> SUCCESS: The search term is kept

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Tomás Cohen Arazi 2020-11-02 20:26:44 UTC
Katrin, I've submitted a follow-up for fixing the problem with remembering searches.

As for the regression tests, what I did was embedding the objects->search code in the controller, so I can tweak the handling for biblioitems.* fields in the query, because biblio objects include the biblioitems columns, so a special handling is required until we merge those tables (or not). So my feeling was that the fact that running:

    $ kshell
   k$ prove t/db_dependent/api/v1/acquisitions_orders.t

was passing, was enough.

I can try adding tests for filtering on biblioitems.* fields, but the general tests cover the routes already.
Comment 93 Tomás Cohen Arazi 2020-11-02 23:00:13 UTC
Created attachment 112882 [details] [review]
Bug 20212: Controller changes regression tests

This patchset did a copy and paste of the objects->search helper inside
the controller, to handle a very special case that wasn't worth
generalizing: the fact that the biblio object (that this patchset makes
embeddable) is actually built from two tables, and we want to filter by
fields in both tables.

This requires us to translate:
- Any filter reference to fields in biblioitems we allow to search on, through
  an allow list.
- For filtering to work, we need to tweak the stashed 'embed' object so
  the prefetch is done correctly

This patchset also adds a new query parameter: only_active. It is used
to only request active orders (see the spec for more details).

This patch adds tests for:
- Filtering by a column in bilbioitems (this implies correctly
  prefetching the bilbio => biblioitems relation, and translating the
  filtering parameter.
- Using the only_active query parameter to retrieve only active orders

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> SUCCESS: Tests pass!
3. Verify the tests check for the explained behaviours.
=> SUCCESS: They do!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 94 Katrin Fischer 2020-11-03 22:23:33 UTC
There still appears to be some regression :( 

- The filters on the datatable are no longer kept 
- and the column filters have vanished altogether
Comment 95 Tomás Cohen Arazi 2020-11-03 22:33:54 UTC
(In reply to Katrin Fischer from comment #94)
> There still appears to be some regression :( 
> 
> - The filters on the datatable are no longer kept

Is that a thing in master?? I don't recall seeing code related to that.
 
> - and the column filters have vanished altogether

Do we miss them?
Comment 96 Katrin Fischer 2020-11-03 22:40:14 UTC
(In reply to Tomás Cohen Arazi from comment #95)
> (In reply to Katrin Fischer from comment #94)
> > There still appears to be some regression :( 
> > 
> > - The filters on the datatable are no longer kept
> 
> Is that a thing in master?? I don't recall seeing code related to that.

I tested and yes, it worked in my tests. I had also noted it in comment#89 "or the table search".

>  
> > - and the column filters have vanished altogether
> 
> Do we miss them?

They allow filtering on aspects that cannot be searched otherwise - I believe they would be missed.
Comment 97 Tomás Cohen Arazi 2020-11-04 12:34:02 UTC
Created attachment 112987 [details] [review]
Bug 20212: (QA follow-up) Restore table filter sticky

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 98 Martin Renvoize 2020-11-04 13:07:38 UTC
I'm sure we can restore the column filters as a followup.. I was hoping to see this in the queue for this release.  Getting stuck this close to the deadline is plain disheartening :(
Comment 99 Andrew Fuerste-Henry 2020-11-04 13:14:39 UTC
> They allow filtering on aspects that cannot be searched otherwise - I
> believe they would be missed.

Testing this, it seems that on current code the main search box above the pending orders table is capable of searching every column in that table except the fund.
Comment 100 Tomás Cohen Arazi 2020-11-04 15:56:30 UTC
(In reply to Andrew Fuerste-Henry from comment #99)
> > They allow filtering on aspects that cannot be searched otherwise - I
> > believe they would be missed.
> 
> Testing this, it seems that on current code the main search box above the
> pending orders table is capable of searching every column in that table
> except the fund.

Good catch!
Comment 101 Tomás Cohen Arazi 2020-11-04 15:58:25 UTC
I have a question about column filtering.

The current code fetches all orders and then provides a 'string search' on the columns, based on the data there's on them.

How would it work for numbers? If we translate that to a DB query, how would you say it should be done? Treat them as strings and '%<input>%'?

I ask this because I don't know what is expected.
Comment 102 Tomás Cohen Arazi 2020-11-06 19:47:12 UTC
Created attachment 113278 [details] [review]
Bug 20212: (QA follow-up) Resurrect column filtering

This patch adds per-column filtering to the order receiving table.

The order cost column proved difficult to filter, because of the syntax
DBIC requires for comparing replacement_cost * quantity, and passing
that all the way down from the API request to DBIC itself.

I'm still looking for options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 103 Tomás Cohen Arazi 2020-11-06 19:47:39 UTC
Created attachment 113279 [details] [review]
Bug 20212: Make all biblioitems.* fields searchable

Prior to this bug, only a few bilbio fields were actually searchable through
the orders API. This patch adds all of them to the mapping, so other
pages can take advantage of them.

My first approach was to add things as needed. But this is trivial to
make complete.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 104 Tomás Cohen Arazi 2020-11-06 19:54:28 UTC
(In reply to Andrew Fuerste-Henry from comment #99)
> > They allow filtering on aspects that cannot be searched otherwise - I
> > believe they would be missed.
> 
> Testing this, it seems that on current code the main search box above the
> pending orders table is capable of searching every column in that table
> except the fund.

I also noticed the main search box doesn't search on:
- internal_note
- vendor_note
- isbn
- publisher
- publication_date
- copyright_date
- suggester.surname
- suggester.firtname

Do we need to add them?
Comment 105 Martin Renvoize 2020-12-22 16:08:39 UTC
Created attachment 114607 [details] [review]
Bug 20212: Add more embeddable objects to orders

This patch adds options to embed more related objects based on the needs
by parcel.tt.

For filtering by biblioitems fields (ISBN and EAN) I had to make the
'list' method a modified version of the objects->search helper. I
thought of doing it in a more generic way but I didn't find any other
use cases and it would certainly make an already complex piece of code
even more complex.

So this is quite similar, but at some steps the biblio.<biblioitem
field> gets translated into the proper relation names, and the same
happens for prefetching.

A new parameter is also added: only_active. It makes the controller use
Koha::Acquisition::Orders->filter_by_active, avoiding the need to build
complex queries in the UI.

The same handling is done when the order_id parameter is passed (outside
the q= parameters). In this case using Koha::Acquisition::Orders->filter_by_id_including_transfers

This is all respecting the C4::Acquisitions::SearchOrders behaviour.

TL;DR:

This patch adapts the code from the list() sub so it manipulates the
query parameters and the embed header so:
- the biblioitem relationship is prefetch
- any queries on biblio.isbn and biblio.ean are correctly translated into search on the
  biblioitems table.
- Adds an only_active parameter to the /acquisitions/orders route to
  easily request only the active orders.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 106 Martin Renvoize 2020-12-22 16:08:44 UTC
Created attachment 114608 [details] [review]
Bug 20212: Clean parcel.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 107 Martin Renvoize 2020-12-22 16:08:48 UTC
Created attachment 114609 [details] [review]
Bug 20212: Add missing relation in Biblio

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 108 Martin Renvoize 2020-12-22 16:08:53 UTC
Created attachment 114610 [details] [review]
Bug 20212: Use API to render orders datatable

This patch makes the pending orders datatable render using the API
instead of hte old controller script. It implements native server-side
pagination thanks to the API and the datatables wrapper.

On polishing it, we found that data can contain many null values, and it
got dirtier in the process.

This code with all the checks that are done on the data is the result of
testing this dev with real-life production databases, huge ones with
thousands of orderlines.

To test:
1. You should compare the behaviour between this table, and the original
   one in master.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 109 Martin Renvoize 2020-12-22 16:08:57 UTC
Created attachment 114611 [details] [review]
Bug 20212: (QA follow-up) Remember current filter

This patch restores the original behaviour: if you jump into receiving
an order, and then go back to receiving, you want the page to remember
your filters.

This feature was overlooked. On fixing it, I wrapped some JS code in a
function for reusing it and simplified it a bit as well.

To test:
1. Enter a search term in either of the search fields
2. Add a note, receive or do another action
=> SUCCESS: The search term is kept
3. Apply this patch set
4. Repeat 2
=> SUCCESS: The search term is kept

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 110 Martin Renvoize 2020-12-22 16:09:02 UTC
Created attachment 114612 [details] [review]
Bug 20212: Controller changes regression tests

This patchset did a copy and paste of the objects->search helper inside
the controller, to handle a very special case that wasn't worth
generalizing: the fact that the biblio object (that this patchset makes
embeddable) is actually built from two tables, and we want to filter by
fields in both tables.

This requires us to translate:
- Any filter reference to fields in biblioitems we allow to search on, through
  an allow list.
- For filtering to work, we need to tweak the stashed 'embed' object so
  the prefetch is done correctly

This patchset also adds a new query parameter: only_active. It is used
to only request active orders (see the spec for more details).

This patch adds tests for:
- Filtering by a column in bilbioitems (this implies correctly
  prefetching the bilbio => biblioitems relation, and translating the
  filtering parameter.
- Using the only_active query parameter to retrieve only active orders

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> SUCCESS: Tests pass!
3. Verify the tests check for the explained behaviours.
=> SUCCESS: They do!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 111 Martin Renvoize 2020-12-22 16:09:06 UTC
Created attachment 114613 [details] [review]
Bug 20212: (QA follow-up) Restore table filter sticky

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 112 Martin Renvoize 2020-12-22 16:09:11 UTC
Created attachment 114614 [details] [review]
Bug 20212: (QA follow-up) Resurrect column filtering

This patch adds per-column filtering to the order receiving table.

The order cost column proved difficult to filter, because of the syntax
DBIC requires for comparing replacement_cost * quantity, and passing
that all the way down from the API request to DBIC itself.

I'm still looking for options.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 113 Martin Renvoize 2020-12-22 16:09:15 UTC
Created attachment 114615 [details] [review]
Bug 20212: Make all biblioitems.* fields searchable

Prior to this bug, only a few bilbio fields were actually searchable through
the orders API. This patch adds all of them to the mapping, so other
pages can take advantage of them.

My first approach was to add things as needed. But this is trivial to
make complete.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 114 Martin Renvoize 2020-12-22 16:09:20 UTC
Created attachment 114616 [details] [review]
Bug 20212: (QA follow-up) Restore missing comma

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 115 Martin Renvoize 2020-12-22 16:09:24 UTC
Created attachment 114617 [details] [review]
Bug 20212: (QA follow-up) Spelling typo fix

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 116 Martin Renvoize 2020-12-22 16:10:52 UTC
Works as expected. No regressions found and QA script is happy.

Passing QA
Comment 117 Jonathan Druart 2021-01-07 17:03:37 UTC
1. We need a delay between the different AJAX request when filtering the table, otherwise we face weird behaviour and lag (server and browser)

2. Prior to this patch, the "pending orders" table contained orders with status (orderstatus) set to "ordered" or "partial". Now we see them all.
See 'pending' and 'ordered' flag of SearchOrders

3. Not blocker but worth noting, we lost the column filter on "Order cost"

4. I don't understand how works the column filters. It's the first view where we introduce it. It would have been interesting to have it on a simpler view (cities?) to understand how it is working.

There is this code:
793             // column filter events handling
794             $(".column-filter").each(function () {
795                 $(this).on( 'keyup change', function () {

It's like we are not using the DT standard way of using column filtering.

5. Code to build the URI is c/p in
794             $(".column-filter").each(function () {
and
809             $("#filterform").on("submit", function(e) {

6. In the schema:

+__PACKAGE__->has_many(
+  "biblioitem",
+  "Koha::Schema::Result::Biblioitem",
+  { "foreign.biblionumber" => "self.biblionumber" },
+  { cascade_copy => 0, cascade_delete => 0 },
+);

Why has_many? Why cascade_delete => 0?

7. Not blocker for now, but I think we need some JS functions to display variables coming from the REST API.
For instance this code appears a lot:
  if ( data != null ) {
    return data.escapeHtml().format_price();
  }
  else {
    return "";
  }

or

  if ( data == null ) {
    return "";
  }
  else {
    return data;
  }

8. In the "render" you have
  if ( type != 'display' ) {
Why are you testing that?
The different values are sorting, filtering and display. According to the doc 
https://datatables.net/reference/option/columns.render

9. There are several places where strings are not escaped

At least 2 examples:

+                            return "<a href=\"/cgi-bin/koha/acqui/basketgroup.pl?op=add&amp;booksellerid="
+                                    + row.basket.vendor_id + "&amp;basketgroupid="
+                                    + row.basket.basket_group_id + "\">"
+                                    + row.basket.basket_group.name + " (" + row.basket.basket_group_id + ")</a>";


+                        return "<a href=\"/cgi-bin/koha/acqui/basket.pl?basketno=" + row.basket.basket_id + "\">" + data + " (" + row.basket.basket_id + ")</a>";


Side notes (and my opinion), I don't think we are going into the right direction. The JS code we are adding in our template is not robust, hard to read, and so hard to maintain.
Also we will have to rewrite things we have done in TT, like display of patron's name for instance (patron-title.inc).
I really would like to see some efforts in improving the readability of the code we have already before adding more.
Comment 118 Tomás Cohen Arazi 2021-01-07 18:03:48 UTC
(In reply to Jonathan Druart from comment #117)
> 1. We need a delay between the different AJAX request when filtering the
> table, otherwise we face weird behaviour and lag (server and browser)

I will look into that. I understand you're referring to the columns filters.

> 2. Prior to this patch, the "pending orders" table contained orders with
> status (orderstatus) set to "ordered" or "partial". Now we see them all.
> See 'pending' and 'ordered' flag of SearchOrders

That's odd, actually. What we do in the controller:

+        if ( $only_active ) {
+            $orders_rs = Koha::Acquisition::Orders->filter_by_active;
+        }

which in turn

+sub filter_by_active {
+    my ($self) = @_;
+    return $self->search(
+        {
+            orderstatus => [ 'new', 'ordered', 'partial' ]
+        }
+    );
+}

should take care of that. Maybe we are loosing the only_active query param somewhere. Worth checking.

Also, maybe there's something wrong when it comes to partially received orders. It would be a problem with bug 25670.

> 3. Not blocker but worth noting, we lost the column filter on "Order cost"

That worked just fine because all data was loaded on the DataTable. If this is a problem, we could add a special munging to the query parameters so it builds the multiply on the query. I didn't consider it that important.

> 4. I don't understand how works the column filters. It's the first view
> where we introduce it. It would have been interesting to have it on a
> simpler view (cities?) to understand how it is working.
> 
> There is this code:
> 793             // column filter events handling
> 794             $(".column-filter").each(function () {
> 795                 $(this).on( 'keyup change', function () {
> 
> It's like we are not using the DT standard way of using column filtering.

This is related to (1). Will take a look.

> 5. Code to build the URI is c/p in
> 794             $(".column-filter").each(function () {
> and
> 809             $("#filterform").on("submit", function(e) {

It is not exactly the same. I think it is fine as-is.

> 6. In the schema:
> 
> +__PACKAGE__->has_many(
> +  "biblioitem",
> +  "Koha::Schema::Result::Biblioitem",
> +  { "foreign.biblionumber" => "self.biblionumber" },
> +  { cascade_copy => 0, cascade_delete => 0 },
> +);
> 
> Why has_many? Why cascade_delete => 0?

This is just having an alias for the already existing relationship. I consider it correct as it is what we have already defined in the schema. I don't mind changing it anyway.

> 7. Not blocker for now, but I think we need some JS functions to display
> variables coming from the REST API.
> For instance this code appears a lot:
>   if ( data != null ) {
>     return data.escapeHtml().format_price();
>   }
>   else {
>     return "";
>   }
> 
> or
> 
>   if ( data == null ) {
>     return "";
>   }
>   else {
>     return data;
>   }

I agree. But it is small enough anyway. Truth is, we do the same in Perl. Specially when it comes to rendering undef.

> 8. In the "render" you have
>   if ( type != 'display' ) {
> Why are you testing that?
> The different values are sorting, filtering and display. According to the
> doc 
> https://datatables.net/reference/option/columns.render

Usually, it is to leave the data untouched if is not for display, and tweak it for display. Not sure about each specific case you're referring to.

> 9. There are several places where strings are not escaped
> 
> At least 2 examples:
> 
> +                            return "<a
> href=\"/cgi-bin/koha/acqui/basketgroup.pl?op=add&amp;booksellerid="
> +                                    + row.basket.vendor_id +
> "&amp;basketgroupid="
> +                                    + row.basket.basket_group_id + "\">"
> +                                    + row.basket.basket_group.name + " (" +
> row.basket.basket_group_id + ")</a>";
> 
> 
> +                        return "<a
> href=\"/cgi-bin/koha/acqui/basket.pl?basketno=" + row.basket.basket_id +
> "\">" + data + " (" + row.basket.basket_id + ")</a>";

I'm not sure why those slipped in, but is probably a bad rebase. I'll fix them.

> Side notes (and my opinion), I don't think we are going into the right
> direction. The JS code we are adding in our template is not robust, hard to
> read, and so hard to maintain.
> Also we will have to rewrite things we have done in TT, like display of
> patron's name for instance (patron-title.inc).
> I really would like to see some efforts in improving the readability of the
> code we have already before adding more.

We really need to adopt a JS framework for dealing with the UI. Modern frameworks fit well with APIs like the one we are writing.

I find myself (again) in the chicken-egg situation regarding this bug. When the first routes were pushed, we just built hashrefs on the controllers and returned them. We've come a long way from that point, with magic stuffs happening under the hood to prevent working more than needed, and having clean new code. The quest for optimal, easily reusable code and I'm very happy with the results. We have a hell of a framework written. With its caveats of course.

But at some point, years are passing, and not being a bit more pragmatic is holding us from lots of cool stuffs we could be doing.
Comment 119 Katrin Fischer 2021-01-07 18:19:36 UTC
For the status question... I believe new orders should only show if they are standing orders. I haven't tracked if there was a change, but we used to only show ordered and partial - must have changed with standing orders or for them.
Comment 120 Tomás Cohen Arazi 2021-01-07 18:22:09 UTC
(In reply to Katrin Fischer from comment #119)
> For the status question... I believe new orders should only show if they are
> standing orders. I haven't tracked if there was a change, but we used to
> only show ordered and partial - must have changed with standing orders or
> for them.

Ha! That's it. I'll try to solve that on bug 25670.
Comment 121 Tomás Cohen Arazi 2021-01-07 19:35:39 UTC
(In reply to Katrin Fischer from comment #119)
> For the status question... I believe new orders should only show if they are
> standing orders. I haven't tracked if there was a change, but we used to
> only show ordered and partial - must have changed with standing orders or
> for them.

Katrina, thanks for that explanation. I've just submitted a follow-up on bug 25670 capturing this behavior, which was actually clear in SearchOrders. I added regression tests as well.
Comment 122 Tomás Cohen Arazi 2021-01-07 19:46:01 UTC
Created attachment 114942 [details] [review]
Bug 20212: (QA follow-up) Fix escaping

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 123 Tomás Cohen Arazi 2021-01-07 19:56:45 UTC
(In reply to Jonathan Druart from comment #117)
> 1. We need a delay between the different AJAX request when filtering the
> table, otherwise we face weird behaviour and lag (server and browser)

I didn't manage to reproduce the problem.

> 2. Prior to this patch, the "pending orders" table contained orders with
> status (orderstatus) set to "ordered" or "partial". Now we see them all.
> See 'pending' and 'ordered' flag of SearchOrders

This got fixed on the corresponding bug.

> 9. There are several places where strings are not escaped

Fixed them all with the follow-up.

> Side notes (and my opinion), I don't think we are going into the right
> direction. The JS code we are adding in our template is not robust, hard to
> read, and so hard to maintain.

I believe we need to pick a JS framework, and start using it. In the meantime, we should do our best in terms of code reliability and maintainability, as we do. I'd add, that with the complexity this table has, it will look bad no matter what. It contains many layers of nested data, that are now (with the API use) readable, intuitively organized and fully covered by tests. As opposed to the lengthy and hard to maintain .pl files.

> Also we will have to rewrite things we have done in TT, like display of
> patron's name for instance (patron-title.inc).
> I really would like to see some efforts in improving the readability of the
> code we have already before adding more.

Yeah, I would love the same. There are many good templating tools for using out there, to pair with the new APIs.
Comment 124 Jonathan Druart 2021-01-08 13:02:14 UTC
There is something wrong with encoding, search for "❤"

[2021/01/08 13:01:36] [ERROR] GET /api/v1/acquisitions/orders: unhandled exception (Mojo::Exception)<<Wide character in subroutine entry at /kohadevbox/koha/Koha/REST/V1/Acquisitions/Orders.pm line 125.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)
Comment 125 Jonathan Druart 2021-01-08 13:04:30 UTC
Tests are failing on fresh DB:

t/db_dependent/api/v1/acquisitions_orders.t ..
    #   Failed test 'Only one order is active'
    #   at t/db_dependent/api/v1/acquisitions_orders.t line 130.
    #          got: '0'
    #     expected: '1'


And this one if there are orders in the DB:
    #   Failed test 'has value for JSON Pointer "/43/order_id"'
    #   at t/db_dependent/api/v1/acquisitions_orders.t line 76.
Comment 126 Jonathan Druart 2021-01-08 13:07:00 UTC
123             if ( exists $reserved_params->{query} and defined $reserved_params->{query} ) {          
124                 push @query_params_array, fix_query({ query => $reserved_params->{query} });         
125             }   
126             if ( exists $reserved_params->{q} and defined $reserved_params->{q}) {                   
127                 push @query_params_array, fix_query({ query => decode_json($reserved_params->{q}) });
128             }   
129             if ( exists $reserved_params->{'x-koha-query'} and defined $reserved_params->{'x-koha-query'} ) {     
130                 push @query_params_array, fix_query({ query => decode_json($reserved_params->{'x-koha-query'}) });;
131             }      

Cannot we replace it with a loop?

123             for my $q ( qw( query q x-koha-query ) ) {
124                 next unless exists $reserved_params->{$q} or defined $reserved_params->{$q};
125                 push @query_params_array, fix_query({ query => decode_json $reserved_params->{$q} }); 
126             }  

It's adding a decode_json for query, but I think it shouldn't hurt.
Comment 127 Jonathan Druart 2021-01-08 13:07:29 UTC
Created attachment 114953 [details] [review]
Bug 20212: Remove clone

We don't need it here.
Comment 128 Tomás Cohen Arazi 2021-01-08 13:13:25 UTC
Created attachment 114954 [details] [review]
Bug 20212: Fix test after 25670 follow-up

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 129 Jonathan Druart 2021-01-08 13:15:22 UTC
Created attachment 114956 [details] [review]
Bug 20212: Use functions to escape variables
Comment 130 Tomás Cohen Arazi 2021-01-08 13:34:04 UTC
Created attachment 114959 [details] [review]
Bug 20212: (follow-up) Avoid pagination on full resultset test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 131 Jonathan Druart 2021-01-08 14:04:47 UTC
Created attachment 114963 [details] [review]
Bug 20212: Search on fund's name instead of id
Comment 132 Jonathan Druart 2021-01-11 15:14:02 UTC
Orders from all the vendors are displayed.

I am working on this, I am afraid that a full retest will be needed.
Comment 133 Jonathan Druart 2021-01-12 08:56:12 UTC
Created attachment 115050 [details] [review]
Bug 20212: Remove fixedHeader

It's an extension that needs to be added.
Comment 134 Jonathan Druart 2021-01-12 09:29:12 UTC
(In reply to Jonathan Druart from comment #133)
> Created attachment 115050 [details] [review] [review]
> Bug 20212: Remove fixedHeader
> 
> It's an extension that needs to be added.

See bug 27403.
Comment 135 Jonathan Druart 2021-01-12 09:35:07 UTC
Created attachment 115056 [details] [review]
Bug 20212: Use the DT column filtering provided by the wrapper

Bug 27402 is adding it, we do not longer need the query_from_filters JS
function.

This patch also remove the filters on the left. As we have DT
remembering the filter on the table we don't need them anymore.

Technical note:
Prior to this patch, the search on biblio.author, biblio.title and
biblio.ean was done by adding hidden columns. Now we are using:
  "data": "biblio.author:biblio.title:biblio.isbn"
to tell the wrapper we are going to build a search on these 3
attributes.

The change in Orders.pm is only formatting to match what's done in
  Bug 27353: Set X-Base-Total-Count header for REST API
Comment 136 Jonathan Druart 2021-01-12 09:36:43 UTC
Note that with bug 27353 we are going to display the wrong number of non-filtering rows. We should not use /acq/orders?vendor_id=42 but /acq/vendor/42/orders instead (which does not exist yet).
Comment 137 Jonathan Druart 2021-01-12 09:40:07 UTC
Created attachment 115057 [details] [review]
Bug 20212: Use the DT column filtering provided by the wrapper

Bug 27402 is adding it, we do not longer need the query_from_filters JS
function.

This patch also remove the filters on the left. As we have DT
remembering the filter on the table we don't need them anymore.

Technical note:
Prior to this patch, the search on biblio.author, biblio.title and
biblio.ean was done by adding hidden columns. Now we are using:
  "data": "biblio.author:biblio.title:biblio.isbn"
to tell the wrapper we are going to build a search on these 3
attributes.

The change in Orders.pm is only formatting to match what's done in
  Bug 27353: Set X-Base-Total-Count header for REST API
Comment 138 Jonathan Druart 2021-01-12 09:52:38 UTC
Created attachment 115060 [details] [review]
Bug 20212: Use the DT column filtering provided by the wrapper

Bug 27402 is adding it, we do not longer need the query_from_filters JS
function.

This patch also remove the filters on the left. As we have DT
remembering the filter on the table we don't need them anymore.

Technical note:
Prior to this patch, the search on biblio.author, biblio.title and
biblio.ean was done by adding hidden columns. Now we are using:
  "data": "biblio.author:biblio.title:biblio.isbn"
to tell the wrapper we are going to build a search on these 3
attributes.

Another trick is to pass a default_filters parameters to the wrapper, to
tell it we want to filter on the orders of a given vendor (this is a
bugfix, the original implementation was returning all the orders).
However We should not use /acq/orders?vendor_id=42 but /acq/vendor/42/orders instead (which does not exist yet),
otherwise (with bug 27353 ) we are going to display the wrong number of non-filtering rows.

The change in Orders.pm is only formatting to match what's done in
  Bug 27353: Set X-Base-Total-Count header for REST API
Comment 139 Jonathan Druart 2021-01-12 10:10:37 UTC
Created attachment 115062 [details] [review]
Bug 20212: Use the DT column filtering provided by the wrapper

Bug 27402 is adding it, we do not longer need the query_from_filters JS
function.

This patch also remove the filters on the left. As we have DT
remembering the filter on the table we don't need them anymore.

Technical note:
Prior to this patch, the search on biblio.author, biblio.title and
biblio.isbn was done by adding hidden columns. Now we are using:
  "data": "biblio.author:biblio.title:biblio.isbn"
to tell the wrapper we are going to build a search on these 3
attributes.

Another trick is to pass a default_filters parameters to the wrapper, to
tell it we want to filter on the orders of a given vendor (this is a
bugfix, the original implementation was returning all the orders).
However We should not use /acq/orders?vendor_id=42 but /acq/vendor/42/orders instead (which does not exist yet),
otherwise (with bug 27353 ) we are going to display the wrong number of non-filtering rows.

The change in Orders.pm is only formatting to match what's done in
  Bug 27353: Set X-Base-Total-Count header for REST API
Comment 140 Jonathan Druart 2021-01-12 10:15:45 UTC
I am sorry but I cannot persuade myself that pushing the "fix_query" trick is a good idea.

We are doing it for only one purpose: search on isbn. In that case why don't we have a hidden feature to allow that (instead of having ton of code that is dealing with all biblioitem attributes)?
Tomas told me that this fix_query won't be reused somewhere else, but actually IMO we will need to implement it correctly everywhere we search on biblio.
Like a simple route GET /biblios will need it :)

If we push it "as it", what would be the next step? Is there plan to have a GET /biblios route?


I've just noticed I remove the search by ean, that was using the same trick. We could re-add it easily but we should then display it in the summary column, for consistency.
Comment 141 Tomás Cohen Arazi 2021-01-12 14:47:39 UTC
(In reply to Jonathan Druart from comment #140)
> I am sorry but I cannot persuade myself that pushing the "fix_query" trick
> is a good idea.
> 
> We are doing it for only one purpose: search on isbn. In that case why don't
> we have a hidden feature to allow that (instead of having ton of code that
> is dealing with all biblioitem attributes)?
> Tomas told me that this fix_query won't be reused somewhere else, but
> actually IMO we will need to implement it correctly everywhere we search on
> biblio.
> Like a simple route GET /biblios will need it :)
> 
> If we push it "as it", what would be the next step?

If we push it, and it makes sense (i.e. we really need it in other places) we need a second loop similar to that of ->to_api to gather information about the bestest objects  with each of them specifying the transformations required for the query. And pass that to the build_query helper.

It felt too much for a single case, but can be done if course.

> Is there plan to have a
> GET /biblios route?

Development shifted to routes we need to implement things. There's no plan for that route, still.

> I've just noticed I remove the search by ean, that was using the same trick.
> We could re-add it easily but we should then display it in the summary
> column, for consistency.

The EAN filter is dependent on UNIMARC being set. Keep it in mind.
Comment 142 Jonathan Druart 2021-01-12 15:53:28 UTC
What about the "make it dirty but simple" suggestion?


(In reply to Jonathan Druart from comment #140)
> We are doing it for only one purpose: search on isbn. In that case why don't
> we have a hidden feature to allow that (instead of having ton of code that
> is dealing with all biblioitem attributes)?
Comment 143 Tomás Cohen Arazi 2021-01-12 15:58:10 UTC
(In reply to Jonathan Druart from comment #142)
> What about the "make it dirty but simple" suggestion?

I didn't manage to, because that's not how merge_dbic_query is designed to work. It felt 'simpler' to just use the fix_query function to munge the query parameters and let the tool do its job.

I'm open to ideas. Not attached to this Implementation

> (In reply to Jonathan Druart from comment #140)
> > We are doing it for only one purpose: search on isbn. In that case why don't
> > we have a hidden feature to allow that (instead of having ton of code that
> > is dealing with all biblioitem attributes)?
Comment 144 Jonathan Druart 2021-01-13 10:59:11 UTC
Created attachment 115107 [details] [review]
Bug 20212: Re-add EAN for UNIMARC
Comment 145 Jonathan Druart 2021-01-13 10:59:15 UTC
Created attachment 115108 [details] [review]
Bug 20212: Fix query for the global filter
Comment 146 Jonathan Druart 2021-01-13 10:59:21 UTC
Created attachment 115109 [details] [review]
Bug 20212: Remove fix_query and assume ugliness

It seems better to do things in an ugly way but simple than complicated.
This patch removes the fix_query method that deals with ean and isbn and
explicitly tell that it's a trick that must not be reused somewhere
else.
It should only be used by Koha internally and we should not advertised
it until it's ready.
This problem will have to be fixed properly when the GET /biblios route
will be implemented.
Comment 147 Jonathan Druart 2021-01-13 10:59:45 UTC
Tomas, if you agree with this last patch please reset the status to NSO.
Comment 148 Tomás Cohen Arazi 2021-01-13 11:33:12 UTC
(In reply to Jonathan Druart from comment #147)
> Tomas, if you agree with this last patch please reset the status to NSO.

A proper solution should be implemented in Koha::REST::Plugin::Query, in the merge_q_params and dbic_merge_prefetch helpers. They both have the resultset object available, so if we added the mapping at Koha::Object level, we could make the translations there, and thus have a 'global' solution in a fairly easy way.

To do:

- Add the attribute mapping hash in Koha::Biblio and make it available in Koha::Biblios too. A sample mapping would be isbn => biblioitem.isbn. It could be hardcoded (as the table doesn't change, or calculated.

- Add a way for Koha::Biblio(s) to tell 'biblioitem' needs to be prefetch (a way for a Koha::Object to tell which other tables need to be prefect).

- Make Koha::REST::Plugin::Query::_parse_prefetch add those prefetches using the above. Note that the 'biblio' name in this case is used because x-koha-embed calls it 'biblio' but it could (a) not exist, as in /biblios, where the biblio is top level (b) it could be named in a different way, as happens to (say) 'biblio.suggestions.suggester' which is a Koha::Patron object, but it is not called 'patron'.

- Make Koha::REST::Plugin::Query::_parse_dbic_query accumulate the translations to be done while building the query. Then merge_q_params should call a new helper that applies those translations to the already created DBIC query (pretty much a copy and paste of fix_query).

That said, we could move forward with the ugly-but-simple implementation. And once I'm back I can work on the above, unless someone does it first ;-)