Bug 20212

Summary: Improve performance of acquisitions receive page
Product: Koha Reporter: Kyle M Hall <kyle>
Component: AcquisitionsAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: new feature    
Priority: P5 - low CC: agustinmoyano, aleisha, andrewfh, caroline.cyr-la-rose, cslone, fridolin.somers, jonathan.druart, katrin.fischer, liz, martin.renvoize, nick, nicolas.legrand, olli-antti.kivilahti, p.thrasher, severine.queune, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27403
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28248
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29383
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30117
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This development changes how the orders table on receiving in acquisitions is rendered. Before these patches all active orders data was fetched from the database, along with all the related data (patrons, biblio). This leads to significant delays because all orders matching the filtering criteria were loaded, even when only 20 rows were 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. Sponsored by: Camden County Library System Sponsored by: ByWater Solutions
Version(s) released in:
21.05.00
Bug Depends on: 18731, 24366, 24418, 24419, 24430, 24435, 24440, 24448, 24457, 24461, 24462, 24467, 24528, 24554, 24561, 24830, 24918, 25297, 25670, 27402    
Bug Blocks: 8179, 18967, 30546, 32760, 32928, 27719, 29464, 29570, 30342    
Attachments: Bug 20212: Extend datatables for API interaction
Bug 20212: Add more embeddable objects to orders
[WIP] Bug 20212: Use the API to render orders
Bug 20212: Add more embeddable objects to orders
[WIP] Bug 20212: Use the API to render orders
Bug 20212: Add more embeddable objects to orders
[WIP] Bug 20212: Use the API to render orders
[WIP] Bug 20212: Use the API to render orders
Bug 20212: Display correct number of items
Bug 20212: Use the API to render orders
[WIP] Bug 20212: Use the API to render orders
[WIP] Bug 20212: Use the API to render orders
Bug 20212: Add more embeddable objects to orders
Bug 20212: Use the API to render orders
[WIP] Bug 20212: Use the API to render orders
Bug 20212: Use the API to render orders
Bug 20212: Add missing entried on schema files
Bug 20212: Add more embeddable objects to orders
Bug 20212: Allow filtering orders on ISBN
Bug 20212: Use API to render orders datatable
Bug 20212: Add missing entries on schema files
Bug 20212: Add more embeddable objects to orders
Bug 20212: Allow filtering orders on ISBN
Bug 20212: Use API to render orders datatable
Bug 20212: Add missing entries on schema files
Bug 20212: Add more embeddable objects to orders
Bug 20212: Allow filtering orders on biblioitems fields
Bug 20212: Use API to render orders datatable
Bug 20212: Use API to render orders datatable
Bug 20212: Clean parcel.pl
Bug 20212: Use API to render orders datatable
Bug 20212: Add missing entries on schema files
Bug 20212: Add more embeddable objects to orders
Bug 20212: Allow filtering orders on biblioitems fields
Bug 20212: Clean parcel.pl
Bug 20212: Use API to render orders datatable
Bug 20212: Add missing entries on schema files
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Use API to render orders datatable
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Use API to render orders datatable
Bug 20212: Add missing relation in Biblio
Bug 20212: Remove warning if no basket group
Plack error when trying to receive w/ library data
Bug 20212: Remove warning if no basket group
Bug 20212: Use API to render orders datatable
Bug 20212: Use API to render orders datatable
Screenshot of table after patch
Screenshot of table before patch
Bug 20212: Use API to render orders datatable
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: Use API to render orders datatable
Bug 20212: Use API to render orders datatable
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: (QA follow-up) Remember current filter
Bug 20212: Controller changes regression tests
Bug 20212: (QA follow-up) Restore table filter sticky
Bug 20212: (QA follow-up) Resurrect column filtering
Bug 20212: Make all biblioitems.* fields searchable
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: (QA follow-up) Remember current filter
Bug 20212: Controller changes regression tests
Bug 20212: (QA follow-up) Restore table filter sticky
Bug 20212: (QA follow-up) Resurrect column filtering
Bug 20212: Make all biblioitems.* fields searchable
Bug 20212: (QA follow-up) Restore missing comma
Bug 20212: (QA follow-up) Spelling typo fix
Bug 20212: (QA follow-up) Fix escaping
Bug 20212: Remove clone
Bug 20212: Fix test after 25670 follow-up
Bug 20212: Use functions to escape variables
Bug 20212: (follow-up) Avoid pagination on full resultset test
Bug 20212: Search on fund's name instead of id
Bug 20212: Remove fixedHeader
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Re-add EAN for UNIMARC
Bug 20212: Fix query for the global filter
Bug 20212: Remove fix_query and assume ugliness
Bug 20212: Search on fund's name instead of id
Bug 20212: Remove fixedHeader
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Re-add EAN for UNIMARC
Bug 20212: Fix query for the global filter
Bug 20212: Remove fix_query and assume ugliness
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: (QA follow-up) Remember current filter
Bug 20212: Controller changes regression tests
Bug 20212: (QA follow-up) Restore table filter sticky
Bug 20212: (QA follow-up) Resurrect column filtering
Bug 20212: Make all biblioitems.* fields searchable
Bug 20212: (QA follow-up) Restore missing comma
Bug 20212: (QA follow-up) Spelling typo fix
Bug 20212: (QA follow-up) Fix escaping
Bug 20212: Fix test after 25670 follow-up
Bug 20212: Use functions to escape variables
Bug 20212: (follow-up) Avoid pagination on full resultset test
Bug 20212: Search on fund's name instead of id
Bug 20212: Remove fixedHeader
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Re-add EAN for UNIMARC
Bug 20212: Fix query for the global filter
Bug 20212: Remove fix_query and assume ugliness
Bug 20212: Add filter for publisher
Bug 20212: Filter on publisher and notes
Screenshot of table after again
Bug 20212: Restore price formatting for ecost and replacement_price
Bug 20212: Add more embeddable objects to orders
Bug 20212: Clean parcel.pl
Bug 20212: Add missing relation in Biblio
Bug 20212: Use API to render orders datatable
Bug 20212: (QA follow-up) Remember current filter
Bug 20212: Controller changes regression tests
Bug 20212: (QA follow-up) Restore table filter sticky
Bug 20212: (QA follow-up) Resurrect column filtering
Bug 20212: Make all biblioitems.* fields searchable
Bug 20212: (QA follow-up) Restore missing comma
Bug 20212: (QA follow-up) Spelling typo fix
Bug 20212: (QA follow-up) Fix escaping
Bug 20212: Fix test after 25670 follow-up
Bug 20212: Use functions to escape variables
Bug 20212: (follow-up) Avoid pagination on full resultset test
Bug 20212: Search on fund's name instead of id
Bug 20212: Remove fixedHeader
Bug 20212: Use the DT column filtering provided by the wrapper
Bug 20212: Re-add EAN for UNIMARC
Bug 20212: Fix query for the global filter
Bug 20212: Remove fix_query and assume ugliness
Bug 20212: Filter on publisher and notes
Bug 20212: Restore price formatting for ecost and replacement_price
Bug 20212: Add missing html filter

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 ;-)
Comment 149 Tomás Cohen Arazi 2021-01-26 14:12:03 UTC
Created attachment 115817 [details] [review]
Bug 20212: Search on fund's name instead of id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 150 Tomás Cohen Arazi 2021-01-26 14:12:11 UTC
Created attachment 115818 [details] [review]
Bug 20212: Remove fixedHeader

It's an extension that needs to be added.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 151 Tomás Cohen Arazi 2021-01-26 14:12:18 UTC
Created attachment 115819 [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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 152 Tomás Cohen Arazi 2021-01-26 14:12:25 UTC
Created attachment 115820 [details] [review]
Bug 20212: Re-add EAN for UNIMARC

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 153 Tomás Cohen Arazi 2021-01-26 14:12:32 UTC
Created attachment 115821 [details] [review]
Bug 20212: Fix query for the global filter

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 154 Tomás Cohen Arazi 2021-01-26 14:12:39 UTC
Created attachment 115822 [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.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 155 Martin Renvoize 2021-01-26 17:25:39 UTC
This has moved a fair bit since the first QA run.. the code updates look good to me.. but as we're back to NSO it might be good to get a library to test this again.. 

Andrew perhaps, do you have time to go through the test plan one last time given all the extra work that has now taken place and double-check it all still performs as expected?
Comment 156 Tomás Cohen Arazi 2021-01-26 18:41:51 UTC
(In reply to Martin Renvoize from comment #155)
> This has moved a fair bit since the first QA run.. the code updates look
> good to me.. but as we're back to NSO it might be good to get a library to
> test this again.. 

Yeah, I totally agree. Some remarks about 'what to expect' from the latest changes:

- The fact that all 'new' orders were displayed, was solved properly on bug 25670 with regression tests and all. It should solved here, that's something to test, specifically.
- Jonathan provided a less hand-crafted way to implement the column filtering feature on bug 27402. It will serve any other API-driven DataTable, so YAY! Jonathan made this table use that instead of the original implementation.

Besides that, you shouldn't expect any noticeable behavior change compared to previous testing sessions.
Comment 157 Jonathan Druart 2021-01-27 08:32:16 UTC
An additional change to note: the filters on the left have been removed.
The column filters provide the same functionality.
Comment 158 Andrew Fuerste-Henry 2021-01-27 15:51:03 UTC
Created attachment 115886 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 159 Andrew Fuerste-Henry 2021-01-27 15:51:08 UTC
Created attachment 115887 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 160 Andrew Fuerste-Henry 2021-01-27 15:51:12 UTC
Created attachment 115888 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 161 Andrew Fuerste-Henry 2021-01-27 15:51:17 UTC
Created attachment 115889 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 162 Andrew Fuerste-Henry 2021-01-27 15:51:22 UTC
Created attachment 115890 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 163 Andrew Fuerste-Henry 2021-01-27 15:51:26 UTC
Created attachment 115891 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 164 Andrew Fuerste-Henry 2021-01-27 15:51:31 UTC
Created attachment 115892 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 165 Andrew Fuerste-Henry 2021-01-27 15:51:35 UTC
Created attachment 115893 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 166 Andrew Fuerste-Henry 2021-01-27 15:51:40 UTC
Created attachment 115894 [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>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 167 Andrew Fuerste-Henry 2021-01-27 15:51:44 UTC
Created attachment 115895 [details] [review]
Bug 20212: (QA follow-up) Restore missing comma

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 168 Andrew Fuerste-Henry 2021-01-27 15:51:48 UTC
Created attachment 115896 [details] [review]
Bug 20212: (QA follow-up) Spelling typo fix

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 169 Andrew Fuerste-Henry 2021-01-27 15:51:53 UTC
Created attachment 115897 [details] [review]
Bug 20212: (QA follow-up) Fix escaping

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 170 Andrew Fuerste-Henry 2021-01-27 15:51:57 UTC
Created attachment 115898 [details] [review]
Bug 20212: Fix test after 25670 follow-up

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 171 Andrew Fuerste-Henry 2021-01-27 15:52:02 UTC
Created attachment 115899 [details] [review]
Bug 20212: Use functions to escape variables

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 172 Andrew Fuerste-Henry 2021-01-27 15:52:06 UTC
Created attachment 115900 [details] [review]
Bug 20212: (follow-up) Avoid pagination on full resultset test

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 173 Andrew Fuerste-Henry 2021-01-27 15:52:11 UTC
Created attachment 115901 [details] [review]
Bug 20212: Search on fund's name instead of id

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 174 Andrew Fuerste-Henry 2021-01-27 15:52:15 UTC
Created attachment 115902 [details] [review]
Bug 20212: Remove fixedHeader

It's an extension that needs to be added.

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 175 Andrew Fuerste-Henry 2021-01-27 15:52:20 UTC
Created attachment 115903 [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

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 176 Andrew Fuerste-Henry 2021-01-27 15:52:24 UTC
Created attachment 115904 [details] [review]
Bug 20212: Re-add EAN for UNIMARC

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 177 Andrew Fuerste-Henry 2021-01-27 15:52:29 UTC
Created attachment 115905 [details] [review]
Bug 20212: Fix query for the global filter

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 178 Andrew Fuerste-Henry 2021-01-27 15:52:33 UTC
Created attachment 115906 [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.

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 179 Andrew Fuerste-Henry 2021-01-27 15:54:01 UTC
I've signed off, but have one thing to fix. The Summary column doesn't search publisher information, internal notes, or vendor notes. Otherwise, it looks great!
Comment 180 Jonathan Druart 2021-01-28 08:08:40 UTC
Created attachment 115920 [details] [review]
Bug 20212: Add filter for publisher
Comment 181 Jonathan Druart 2021-01-28 08:14:02 UTC
Created attachment 115921 [details] [review]
Bug 20212: Filter on publisher and notes
Comment 182 Katrin Fischer 2021-01-28 10:04:00 UTC
I was wondering about the removed search options on the left: Are there any issues to be expected performance wise? As I understand if you can't limit by search, you will always filter on the full data by using the datatable options?
Comment 183 Tomás Cohen Arazi 2021-01-28 10:53:51 UTC
(In reply to Katrin Fischer from comment #182)
> I was wondering about the removed search options on the left: Are there any
> issues to be expected performance wise? As I understand if you can't limit
> by search, you will always filter on the full data by using the datatable
> options?

Searches are very fast (at least compared to the original implementation).
The key, to streamline the receiving process, will be bug 8179, which will let the users perform several queries and accumulate to orders to receive, so they are all received in one step.
Comment 184 Martin Renvoize 2021-01-28 15:05:22 UTC
Created attachment 115940 [details]
Screenshot of table after again
Comment 185 Jonathan Druart 2021-01-28 15:07:41 UTC
Created attachment 115941 [details] [review]
Bug 20212: Restore price formatting for ecost and replacement_price
Comment 186 Martin Renvoize 2021-01-28 15:13:13 UTC
Some minor issues present I'm afraid:

1) We appear to have lost money formatting on some columns (i.e "8.00" vs "8") - QA Fail
2) Search filter texts are reversed ('Search replacement price' vs 'Replacement price search') - I prefer the old way as you see the relevant information first as opposed to 'Search ...' with the relevant data getting cut off on narrow columns.
3) Columns appear to be wrapping more often.. not sure why but it means the table displays less rows in the viewport. 

The first is a QA failure, but the second two points are more a matter of style and could be fixed later in my opinion.
Comment 187 Martin Renvoize 2021-01-28 15:13:29 UTC
OK, First one is now fixed already :)
Comment 188 Jonathan Druart 2021-01-28 15:18:56 UTC
(In reply to Martin Renvoize from comment #186)
> 2) Search filter texts are reversed ('Search replacement price' vs
> 'Replacement price search') - I prefer the old way as you see the relevant
> information first as opposed to 'Search ...' with the relevant data getting
> cut off on narrow columns.

Fixed on bug 27402 (Reverse search filter texts)
Comment 189 Martin Renvoize 2021-01-28 15:32:16 UTC
Created attachment 115943 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 190 Martin Renvoize 2021-01-28 15:32:21 UTC
Created attachment 115944 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 191 Martin Renvoize 2021-01-28 15:32:26 UTC
Created attachment 115945 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 192 Martin Renvoize 2021-01-28 15:32:32 UTC
Created attachment 115946 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 193 Martin Renvoize 2021-01-28 15:32:37 UTC
Created attachment 115947 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 194 Martin Renvoize 2021-01-28 15:32:42 UTC
Created attachment 115948 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 195 Martin Renvoize 2021-01-28 15:32:47 UTC
Created attachment 115949 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 196 Martin Renvoize 2021-01-28 15:32:52 UTC
Created attachment 115950 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 197 Martin Renvoize 2021-01-28 15:32:57 UTC
Created attachment 115951 [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>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 198 Martin Renvoize 2021-01-28 15:33:02 UTC
Created attachment 115952 [details] [review]
Bug 20212: (QA follow-up) Restore missing comma

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 199 Martin Renvoize 2021-01-28 15:33:07 UTC
Created attachment 115953 [details] [review]
Bug 20212: (QA follow-up) Spelling typo fix

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 200 Martin Renvoize 2021-01-28 15:33:12 UTC
Created attachment 115954 [details] [review]
Bug 20212: (QA follow-up) Fix escaping

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 201 Martin Renvoize 2021-01-28 15:33:18 UTC
Created attachment 115955 [details] [review]
Bug 20212: Fix test after 25670 follow-up

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 202 Martin Renvoize 2021-01-28 15:33:28 UTC
Created attachment 115956 [details] [review]
Bug 20212: Use functions to escape variables

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 203 Martin Renvoize 2021-01-28 15:33:33 UTC
Created attachment 115957 [details] [review]
Bug 20212: (follow-up) Avoid pagination on full resultset test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 204 Martin Renvoize 2021-01-28 15:33:38 UTC
Created attachment 115958 [details] [review]
Bug 20212: Search on fund's name instead of id

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 205 Martin Renvoize 2021-01-28 15:33:43 UTC
Created attachment 115959 [details] [review]
Bug 20212: Remove fixedHeader

It's an extension that needs to be added.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 206 Martin Renvoize 2021-01-28 15:33:48 UTC
Created attachment 115960 [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

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 207 Martin Renvoize 2021-01-28 15:33:53 UTC
Created attachment 115961 [details] [review]
Bug 20212: Re-add EAN for UNIMARC

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 208 Martin Renvoize 2021-01-28 15:33:59 UTC
Created attachment 115962 [details] [review]
Bug 20212: Fix query for the global filter

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 209 Martin Renvoize 2021-01-28 15:34:04 UTC
Created attachment 115963 [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.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 210 Martin Renvoize 2021-01-28 15:34:09 UTC
Created attachment 115964 [details] [review]
Bug 20212: Filter on publisher and notes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 211 Martin Renvoize 2021-01-28 15:34:14 UTC
Created attachment 115965 [details] [review]
Bug 20212: Restore price formatting for ecost and replacement_price

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 212 Martin Renvoize 2021-01-28 15:39:51 UTC
I'm happy to say this all works well and I think we should go with it.

I'd love to see a little love given to the styling as followup bugs as I think this make the table a bit 'Chunky' with fields changing width and wrapping more often.. and the filters bar being larger etc.

I also can see someone asking for the left bar filters back... but again don't think that should hold this up.

Final comment, the QA script is upset about a missing filter (but it's a variable that's built within the template so I don't think it's a big issue and Jonathan has promised to fix on push).

Soo.....


Passing QA
Comment 213 Jonathan Druart 2021-01-28 16:34:26 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 214 Tomás Cohen Arazi 2021-01-28 17:05:07 UTC
Thank you all, this was an important piece of teamwork.
Comment 215 Nicolas Legrand 2021-01-28 20:36:38 UTC
Thank you everyone! We binge watched this one like the trendiest series with plot twists and cliffhangers. We were hooked like on a soap opera, with signed off, passed QA and failed QA in places of affairs and divorces. Great suspense, great results, we're thrilled! Thank you all a lot for this incredible work!
Comment 216 Tomás Cohen Arazi 2021-01-28 20:42:06 UTC
(In reply to Nicolas Legrand from comment #215)
> Thank you everyone! We binge watched this one like the trendiest series with
> plot twists and cliffhangers. We were hooked like on a soap opera, with
> signed off, passed QA and failed QA in places of affairs and divorces. Great
> suspense, great results, we're thrilled! Thank you all a lot for this
> incredible work!

Acquisitions, REST API, rewriting Koha. It's a good mix for a soap opera haha. The cool thing is it didn't end up being just a stand-up :-D.
Comment 217 Jonathan Druart 2021-02-01 09:36:38 UTC
Created attachment 116139 [details] [review]
Bug 20212: Add missing html filter
Comment 218 Jonathan Druart 2021-02-01 09:40:09 UTC
Oops, looks like I forgot to amend the changes I made locally to add the filter!

Follow-up pushed to master.
Comment 219 Fridolin Somers 2021-02-01 13:45:48 UTC
Enhancement not pushed to 20.11.x
Comment 220 Katrin Fischer 2023-06-24 21:24:33 UTC
*** Bug 29383 has been marked as a duplicate of this bug. ***
Comment 221 Katrin Fischer 2023-07-28 21:15:07 UTC
*** Bug 13608 has been marked as a duplicate of this bug. ***