*** Bug 10959 has been marked as a duplicate of this bug. ***

Is this work in progress or are other solutions to this acceptable.

(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 :)

And please help make bug 18731 into master ASAP!

Time to finish this as all dependencies are in master \o/

Created attachment 100475 [details] [review]
Bug 20212: Add missing entried on schema files

Created attachment 100476 [details] [review]
Bug 20212: Add more embeddable objects to orders

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.

Created attachment 100478 [details] [review]
Bug 20212: Use API to render orders datatable

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!

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

Think I'm missing where this requires bug 24455?

(In reply to Martin Renvoize from comment #42)
> Think I'm missing where this requires bug 24455?

It doesn't, removed the dependency.

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.

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.

Created attachment 109868 [details] [review]
Bug 20212: Clean parcel.pl

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

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.

Created attachment 110229 [details]
Plack error when trying to receive w/ library data

Thanks Andrew, the last version of the patch should cover it.

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?

Created attachment 111568 [details]
Screenshot of table after patch

Created attachment 111569 [details]
Screenshot of table before patch

(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.

Hmm, I think you're right, it highlights bad data in my test set..  That's probably a good thing.

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.

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!

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Thanks, Andrew. It's been a very nice testing session, and the things you found were really helpful to make this solid!

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.

(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.

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>

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.

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>

There still appears to be some regression :( 

- The filters on the datatable are no longer kept 
- and the column filters have vanished altogether

(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?

(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.

Created attachment 112987 [details] [review]
Bug 20212: (QA follow-up) Restore table filter sticky

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

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 :(

> 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.

(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!

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.

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>

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>

(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?

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 114616 [details] [review]
Bug 20212: (QA follow-up) Restore missing comma

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

Created attachment 114617 [details] [review]
Bug 20212: (QA follow-up) Spelling typo fix

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

Works as expected. No regressions found and QA script is happy.

Passing QA

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.

(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.

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.

(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.

(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.

Created attachment 114942 [details] [review]
Bug 20212: (QA follow-up) Fix escaping

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

(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.

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)

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.

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.

Created attachment 114953 [details] [review]
Bug 20212: Remove clone

We don't need it here.

Created attachment 114954 [details] [review]
Bug 20212: Fix test after 25670 follow-up

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

Created attachment 114956 [details] [review]
Bug 20212: Use functions to escape variables

Created attachment 114959 [details] [review]
Bug 20212: (follow-up) Avoid pagination on full resultset test

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

Created attachment 114963 [details] [review]
Bug 20212: Search on fund's name instead of id

Orders from all the vendors are displayed.

I am working on this, I am afraid that a full retest will be needed.

Created attachment 115050 [details] [review]
Bug 20212: Remove fixedHeader

It's an extension that needs to be added.

(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.

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

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).

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

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

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

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.

(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.

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)?

(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)?

Created attachment 115107 [details] [review]
Bug 20212: Re-add EAN for UNIMARC

Created attachment 115108 [details] [review]
Bug 20212: Fix query for the global filter

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.

Tomas, if you agree with this last patch please reset the status to NSO.

(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 ;-)

Created attachment 115817 [details] [review]
Bug 20212: Search on fund's name instead of id

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

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>

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>

Created attachment 115820 [details] [review]
Bug 20212: Re-add EAN for UNIMARC

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

Created attachment 115821 [details] [review]
Bug 20212: Fix query for the global filter

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

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>

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?

(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.

An additional change to note: the filters on the left have been removed.
The column filters provide the same functionality.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 115899 [details] [review]
Bug 20212: Use functions to escape variables

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

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>

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>

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>

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>

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>

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>

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>

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!

Created attachment 115920 [details] [review]
Bug 20212: Add filter for publisher

Created attachment 115921 [details] [review]
Bug 20212: Filter on publisher and notes

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?

(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.

Created attachment 115940 [details]
Screenshot of table after again

Created attachment 115941 [details] [review]
Bug 20212: Restore price formatting for ecost and replacement_price

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.

OK, First one is now fixed already :)

(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)

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 115964 [details] [review]
Bug 20212: Filter on publisher and notes

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

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>

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

Pushed to master for 21.05, thanks to everybody involved!

Thank you all, this was an important piece of teamwork.

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!

(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.

Created attachment 116139 [details] [review]
Bug 20212: Add missing html filter

Oops, looks like I forgot to amend the changes I made locally to add the filter!

Follow-up pushed to master.

Enhancement not pushed to 20.11.x

*** Bug 29383 has been marked as a duplicate of this bug. ***

*** Bug 13608 has been marked as a duplicate of this bug. ***