Bug 20212 - Slowness in receiving in acquisitions
Summary: Slowness in receiving in acquisitions
Status: Signed Off
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: Testopia
URL:
Keywords:
: 10959 (view as bug list)
Depends on: 24461 24561 24830 24918 25297 25670 18731 24366 24418 24419 24430 24435 24440 24448 24457 24462 24467 24528 24554
Blocks: 8179
  Show dependency treegraph
 
Reported: 2018-02-15 13:44 UTC by Kyle M Hall
Modified: 2020-11-06 19:54 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
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

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?