Bug 20996 - Fix API response time on ILL request endpoint
Summary: Fix API response time on ILL request endpoint
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Andrew Isherwood
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on: 20548
Blocks: 21079 21835
  Show dependency treegraph
 
Reported: 2018-06-26 10:48 UTC by Andrew Isherwood
Modified: 2018-11-16 15:33 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This patch makes the <branch> configuration section compulsory in the <interlibrary_loans> section of your Koha configuration file. The <branch> section allows you define per-branch Interlibrary loan options for each branch. In it's most basic form, the branch section can be: <branch><code>code_for_your_branch</code></branch>


Attachments
Bug 20996: Remove prefix use of borrower category (13.10 KB, patch)
2018-07-04 14:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: Fix unit tests (7.94 KB, patch)
2018-07-04 14:34 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: Further enhance performance (3.90 KB, patch)
2018-07-05 14:20 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Script for creating large numbers of ILL requests, for performance testing (1.06 KB, application/x-perl)
2018-07-19 10:25 UTC, Andrew Isherwood
Details
Bug 20996: (follow-up) Add <branch> to conf tpl (2.05 KB, patch)
2018-09-25 14:26 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix column name expectation (1.42 KB, patch)
2018-09-28 12:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: Remove prefix use of borrower category (13.21 KB, patch)
2018-09-28 13:21 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: Fix unit tests (8.04 KB, patch)
2018-09-28 13:21 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: Further enhance performance (4.01 KB, patch)
2018-09-28 13:21 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: (follow-up) Add <branch> to conf tpl (2.16 KB, patch)
2018-09-28 13:21 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix column name expectation (1.87 KB, patch)
2018-09-28 13:21 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: Remove prefix use of borrower category (13.15 KB, patch)
2018-09-28 13:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: Fix unit tests (7.99 KB, patch)
2018-09-28 13:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: Further enhance performance (3.95 KB, patch)
2018-09-28 13:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: (follow-up) Add <branch> to conf tpl (2.11 KB, patch)
2018-09-28 13:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix column name expectation (2.22 KB, patch)
2018-09-28 13:30 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 20996: Remove prefix use of borrower category (13.29 KB, patch)
2018-10-31 12:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: Fix unit tests (7.98 KB, patch)
2018-10-31 12:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: Further enhance performance (3.95 KB, patch)
2018-10-31 12:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (follow-up) Add <branch> to conf tpl (2.09 KB, patch)
2018-10-31 12:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix column name expectation (2.21 KB, patch)
2018-10-31 12:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (QA follow-up) Fix pod (622 bytes, patch)
2018-10-31 12:28 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: Remove prefix use of borrower category (12.01 KB, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: Fix unit tests (7.99 KB, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: Further enhance performance (3.95 KB, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: (follow-up) Add <branch> to conf tpl (2.11 KB, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix column name expectation (2.22 KB, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix merge problems (2.33 KB, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: (QA follow-up) Fix pod (641 bytes, patch)
2018-10-31 15:27 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20996: Remove prefix use of borrower category (12.02 KB, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: Fix unit tests (8.03 KB, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: Further enhance performance (4.00 KB, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (follow-up) Add <branch> to conf tpl (2.15 KB, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix column name expectation (2.27 KB, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix merge problems (2.38 KB, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (QA follow-up) Fix pod (689 bytes, patch)
2018-10-31 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20966: Add column configuration to table of orders in a basket (15.63 KB, patch)
2018-11-02 15:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20966: (follow-up) Add template variable filter; Fix table export (2.40 KB, patch)
2018-11-02 15:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix test (2.75 KB, patch)
2018-11-05 08:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20996: (follow-up) Fix test (2.81 KB, patch)
2018-11-05 14:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20996: (RM follow-up) Fix the api tests (3.40 KB, patch)
2018-11-06 16:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20996: (RM follow-up) Fix the api tests (3.12 KB, patch)
2018-11-06 16:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20996: (RM follow-up) Fix the api tests (3.15 KB, patch)
2018-11-06 17:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20996: Remove warning 'Un-mocked method' (1.00 KB, patch)
2018-11-06 17:26 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-06-26 10:48:36 UTC
A bug has crept into the /api/v1/illrequests API endpoint causing a GET on this route to take an unreasonably long time to respond, even with a few queries. This never used to be the case so either we're fetching something that we didn't before and that's slowing things down, or there's an actual bug.

For example, a query on my dev server with 6 requests takes 2.76 seconds to respond. This is clearly an unreasonably long time and will almost certainly increase as more requests are added.

We need to investigate the source of the slowdown and address it.
Comment 1 Benjamin Rokseth 2018-06-26 11:33:29 UTC
Hi,
db design at least partly to blame. At least a quick fix would be to create an index on illrequestattributes

CREATE INDEX `illrequestattributes_type_value` ON illrequestattributes(type,value(32));
Comment 2 Andrew Isherwood 2018-07-02 15:17:56 UTC
Hi Benjamin

I'm not aware of anywhere where we query that table on type or value, it's only ever the illrequest_id we query via the join. So I'm not sure of the value of adding these indexes.

I might be missing something though, so please put me straight if so!
Comment 3 Andrew Isherwood 2018-07-04 14:34:27 UTC
Created attachment 76687 [details] [review]
Bug 20996: Remove prefix use of borrower category

This patch removes the potential use of borrower category as a ILL
request ID prefix. It makes no sense. We provide the ability for a site
to define a request prefix based on branch, there is no use case for
using the borrower category. Add to this that the borrower for every
request was being retrieved in order to get the category, it's a huge
performance hit also.

We also now require the <branch> block in the <interlibrary_loans> block
and complain if it's not present. The request prefix should be defined
in this block.

This patch also improves the performance of the API request that returns all
requests, optionally including additional data.

It also deprecates the overloaded TO_JSON method and moves the request
augmentation code into the API route's controller. It may be that we
want to shift it out of there at some point, but it is fine where it is
for now.
Comment 4 Andrew Isherwood 2018-07-04 14:34:39 UTC
Created attachment 76688 [details] [review]
Bug 20996: Fix unit tests

- Remove unit tests relating to TO_JSON
- Add tests for new explicit embedding
- Modify tests relating to now deprecated brw_cat
Comment 5 Andrew Isherwood 2018-07-05 14:20:49 UTC
Created attachment 76705 [details] [review]
Bug 20996: Further enhance performance

We were immediately unblessing all request objects after retrieval then,
at a later stage, re-instantiating each one so we could call methods on
that. This was a very bad idea. We now preserve the objects for as long
as we can.
Comment 6 Andrew Isherwood 2018-07-19 10:25:27 UTC
Created attachment 77100 [details]
Script for creating large numbers of ILL requests, for performance testing
Comment 7 Andrew Isherwood 2018-07-19 10:29:13 UTC
Since this is a performance enhancement, it's very hard to test without a substantial number of ILL requests in the database. I have therefore attached the script I created to populate large number of requests (add_ill.pl).

To test:
- Before applying patches, add a large number of requests, maybe 5000, using attached add_ill.pl
- Make a request to: http://your-koha/api/v1/illrequests?embed=metadata,patron,capabilities,library
- Time how long the response takes
- Apply patches
- Make another request to the same URL
- Time the response
=> TEST: The response time should be substantially reduced
Comment 8 Magnus Enger 2018-09-25 07:56:50 UTC
If <branch> is now mandatory inside <interlibrary_loans> in koha-conf.xml it should probably be added to these template files? 

- etc/koha-conf.xml
- debian/templates/koha-conf-site.xml.in

It looks like the increase in speed is really significant, so I'll try to sign off as soon as I can.
Comment 9 Andrew Isherwood 2018-09-25 08:12:53 UTC
Excellent point, thanks Magnus. I'll create a patch to add these
Comment 10 Magnus Enger 2018-09-25 08:39:03 UTC
Some observations from a DB with 359 illrequests and 34,485 illrequestattributes. Measurements of the standard API call to /api/v1/illrequests?embed=metadata,patron,capabilities,library:

Without the patch: 

Size of data: 1MB
Time: ~12 seconds

With the patch:

Size of data: 1.8MB
Time: ~8 seconds

The decrease in time is much appreciated! I'm not sure where the increase in weight comes from. Could it be avoided and save us even more time?
Comment 11 Andrew Isherwood 2018-09-25 08:53:03 UTC
Interesting, I'd not noticed the weight increase. I'll investigate!
Comment 12 Magnus Enger 2018-09-25 08:56:27 UTC
Also, with the patch applied, when I try to view /cgi-bin/koha/ill/ill-requests.pl I get two JS popup warnings: 

DataTables warning: table id=ill-requests - Requested unknown parameter 'metadata_Author' for row 0, column 0. For more information about this error, please see http://datatables.net/tn/4

DataTables warning: table id=ill-requests - Requested unknown parameter 'metadata_Author' for row 358, column 0. For more information about this error, please see http://datatables.net/tn/4

Author and Title is empty in the table.

Not sure if this is a data problem, but it did not happen before I applied the patches.
Comment 13 Andrew Isherwood 2018-09-25 09:03:43 UTC
It's not related to this bug, it's due to an item not having an author, I've seen it before. I'll create a bug if I've not already created one. Thanks for mentioning it!
Comment 14 Andrew Isherwood 2018-09-25 14:26:13 UTC
Created attachment 79379 [details] [review]
Bug 20996: (follow-up) Add <branch> to conf tpl

Since we now require the <branch> block, we should add it to the config
templates
Comment 15 Andrew Isherwood 2018-09-26 10:56:38 UTC
Hi Magnus. I can't account for the weight increase, I've not been able to replicate it. You've not also got bug 21079 applied have you? That runs a database update that creates additional illrequestmetadata values for request from the FreeForm backend. That's the only thing I can thing that might have increased the response size.
Comment 16 Magnus Enger 2018-09-26 13:06:53 UTC
No bug 21079 applied, no. 

At least I think I found the difference in the JSON delivered, thanks to the "Response preview" in the Chromium developer tools. 

Without the patches applied, each request described in the JSON data has a data element "metadata" which contains just the properties that are set up by "sub metadata" in Base.pm.

With the patches applied, this same data element contains *all* the request attributes, but not the data from "sub metadata" (this results in Title and Author not being populated in the table, as well as the errors/warnings I mentioned earlier).

I have not looked into what is causing this difference.
Comment 17 Andrew Isherwood 2018-09-26 13:10:31 UTC
Oooh, good investigating, I completely failed to find any difference in responses. I clearly didn't have sufficiently well populated requests! What you've found sounds promising, I'll do some digging and see what I can find. Thanks!
Comment 18 Andrew Isherwood 2018-09-28 11:43:08 UTC
I've dug into this some more. The additional weight appears to be due to exactly what you observed. This is due to us now returning all metadata associated with a request rather than just a subset defined by the relevant backend. For example, prior to these patches, the metadata was populated via (for example, with the BLDSS backend):

https://github.com/PTFS-Europe/koha-ill-bldss/blob/master/Base.pm#L244-L293

With this bug, it's populated by:

https://github.com/PTFS-Europe/koha/blob/bug_20996/Koha/REST/V1/Illrequests.pm#L126-L136

which gets *all* metadata regardless of the backend.

Whether this is a good idea or not is up for debate I guess. Any calls on a request object's 'metadata' method will still use the backend's metadata method to return the curated metadata from the backend. Dunno what the best thing to do is really..

Anyway, putting the response weight aside, this all now appears to be working as designed, so I'm going to change the status back to "Needs Signoff"
Comment 19 Magnus Enger 2018-09-28 12:20:40 UTC
Not quite that simple, I'm afraid... When I am on the master branch everything looks fine. Title and Author are displayed for the requests that have them. 

When I apply the patches from this bug title and author are not displayed, and this warning pops up, both on initial rendering of the "View ILL requests" page and whenever I use the Next and Previous links in the datatable to see more requests. 

DataTables warning: table id=ill-requests - Requested unknown parameter 'metadata_Author' for row 367, column 0. For more information about this error, please see http://datatables.net/tn/4

Could this be because we are only getting the full requestattributes, and not the data from "sub metadata"? 

I am testing with my in-progress backend for Libris, so I guess this might be an issue with that backend. Especially if things work as they should for you.
Comment 20 Andrew Isherwood 2018-09-28 12:29:07 UTC
No, it's not backend dependent. I looked into it yesterday, prior to these patches "title" and "author" metadata were returned by the API as "metadata_Title" & "metadata_Author", which is what the JS expects, hence no error. With the patches, they're returned as "metadata_title" & "metadata_author". I didn't get to the bottom of why, but now I write it out, I think I might know why. I suspect the fix is going to be to change what the JS expects, since the API is just returning illrequestattribute names as they appear in the database, which is ultimately correct. I'm going to fix this now, I'm not going to rest until it's done damn it! :)
Comment 21 Andrew Isherwood 2018-09-28 12:43:25 UTC
OK, I know what it is. In the FreeForm backend's metadata method (and, I presume, your backend's metadata method), there is this line:

https://github.com/PTFS-Europe/koha-ill-freeform/blob/master/Base.pm#L124

Which, as you can see, title cases the name of the property (Author etc), which breaks the JS when we use the alternative method of getting our metadata, which doesn't title case it. I've no idea why the properties are title cased, but it seems to have been the case since day 1.

I'm going to fix it by changing what the JS expects from "metadata_Author" to "metadata_author", same for title, and leave it at that. This whole issue is made irrelevant by bug 21063 which changes how the datatable JS works anyway and has no expectation of column names from the API.

I'll attach a patch momentarily.
Comment 22 Andrew Isherwood 2018-09-28 12:47:56 UTC
Created attachment 79576 [details] [review]
Bug 20996: (follow-up) Fix column name expectation

This bug changes how request metadata is obtained. Previously the
appropriate backend's metadata function would return it. For at least
the FreeForm backend, the metadata property names we title cased. The
datatable JS was expecting this and breaks with the new metadata getting
which is performed in the API controller, which just returns the
property names as they appear in the DB.

This patch changes the expectation of the JS to match what's coming back
from the API
Comment 23 Magnus Enger 2018-09-28 13:21:22 UTC Comment hidden (obsolete)
Comment 24 Magnus Enger 2018-09-28 13:21:27 UTC Comment hidden (obsolete)
Comment 25 Magnus Enger 2018-09-28 13:21:35 UTC Comment hidden (obsolete)
Comment 26 Magnus Enger 2018-09-28 13:21:42 UTC Comment hidden (obsolete)
Comment 27 Magnus Enger 2018-09-28 13:21:49 UTC Comment hidden (obsolete)
Comment 28 Magnus Enger 2018-09-28 13:29:36 UTC
Created attachment 79588 [details] [review]
Bug 20996: Remove prefix use of borrower category

This patch removes the potential use of borrower category as a ILL
request ID prefix. It makes no sense. We provide the ability for a site
to define a request prefix based on branch, there is no use case for
using the borrower category. Add to this that the borrower for every
request was being retrieved in order to get the category, it's a huge
performance hit also.

We also now require the <branch> block in the <interlibrary_loans> block
and complain if it's not present. The request prefix should be defined
in this block.

This patch also improves the performance of the API request that returns all
requests, optionally including additional data.

It also deprecates the overloaded TO_JSON method and moves the request
augmentation code into the API route's controller. It may be that we
want to shift it out of there at some point, but it is fine where it is
for now.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 29 Magnus Enger 2018-09-28 13:29:41 UTC
Created attachment 79589 [details] [review]
Bug 20996: Fix unit tests

- Remove unit tests relating to TO_JSON
- Add tests for new explicit embedding
- Modify tests relating to now deprecated brw_cat

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 30 Magnus Enger 2018-09-28 13:29:46 UTC
Created attachment 79590 [details] [review]
Bug 20996: Further enhance performance

We were immediately unblessing all request objects after retrieval then,
at a later stage, re-instantiating each one so we could call methods on
that. This was a very bad idea. We now preserve the objects for as long
as we can.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 31 Magnus Enger 2018-09-28 13:29:54 UTC
Created attachment 79591 [details] [review]
Bug 20996: (follow-up) Add <branch> to conf tpl

Since we now require the <branch> block, we should add it to the config
templates

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 32 Magnus Enger 2018-09-28 13:30:00 UTC
Created attachment 79592 [details] [review]
Bug 20996: (follow-up) Fix column name expectation

This bug changes how request metadata is obtained. Previously the
appropriate backend's metadata function would return it. For at least
the FreeForm backend, the metadata property names we title cased. The
datatable JS was expecting this and breaks with the new metadata getting
which is performed in the API controller, which just returns the
property names as they appear in the DB.

This patch changes the expectation of the JS to match what's coming back
from the API

Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.
Comment 33 Josef Moravec 2018-10-31 12:27:07 UTC
Created attachment 81715 [details] [review]
Bug 20996: Remove prefix use of borrower category

This patch removes the potential use of borrower category as a ILL
request ID prefix. It makes no sense. We provide the ability for a site
to define a request prefix based on branch, there is no use case for
using the borrower category. Add to this that the borrower for every
request was being retrieved in order to get the category, it's a huge
performance hit also.

We also now require the <branch> block in the <interlibrary_loans> block
and complain if it's not present. The request prefix should be defined
in this block.

This patch also improves the performance of the API request that returns all
requests, optionally including additional data.

It also deprecates the overloaded TO_JSON method and moves the request
augmentation code into the API route's controller. It may be that we
want to shift it out of there at some point, but it is fine where it is
for now.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 34 Josef Moravec 2018-10-31 12:27:30 UTC
Created attachment 81716 [details] [review]
Bug 20996: Fix unit tests

- Remove unit tests relating to TO_JSON
- Add tests for new explicit embedding
- Modify tests relating to now deprecated brw_cat

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 35 Josef Moravec 2018-10-31 12:27:41 UTC
Created attachment 81717 [details] [review]
Bug 20996: Further enhance performance

We were immediately unblessing all request objects after retrieval then,
at a later stage, re-instantiating each one so we could call methods on
that. This was a very bad idea. We now preserve the objects for as long
as we can.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 36 Josef Moravec 2018-10-31 12:27:50 UTC
Created attachment 81718 [details] [review]
Bug 20996: (follow-up) Add <branch> to conf tpl

Since we now require the <branch> block, we should add it to the config
templates

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 37 Josef Moravec 2018-10-31 12:27:59 UTC
Created attachment 81719 [details] [review]
Bug 20996: (follow-up) Fix column name expectation

This bug changes how request metadata is obtained. Previously the
appropriate backend's metadata function would return it. For at least
the FreeForm backend, the metadata property names we title cased. The
datatable JS was expecting this and breaks with the new metadata getting
which is performed in the API controller, which just returns the
property names as they appear in the DB.

This patch changes the expectation of the JS to match what's coming back
from the API

Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.
Comment 38 Josef Moravec 2018-10-31 12:28:56 UTC
Created attachment 81720 [details] [review]
Bug 20996: (QA follow-up) Fix pod
Comment 39 Josef Moravec 2018-10-31 12:29:51 UTC
Rebased on master and add small follow-up
Comment 40 Josef Moravec 2018-10-31 12:30:59 UTC
Andrew is going to add comments to embed as it was done in bug 18591
Comment 41 Andrew Isherwood 2018-10-31 14:35:35 UTC
Comment on attachment 81715 [details] [review]
Bug 20996: Remove prefix use of borrower category

One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment 42 Andrew Isherwood 2018-10-31 14:35:51 UTC
Comment on attachment 81716 [details] [review]
Bug 20996: Fix unit tests

One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment 43 Andrew Isherwood 2018-10-31 14:35:56 UTC
Comment on attachment 81717 [details] [review]
Bug 20996: Further enhance performance

One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment 44 Andrew Isherwood 2018-10-31 14:36:00 UTC
Comment on attachment 81718 [details] [review]
Bug 20996: (follow-up) Add <branch> to conf tpl

One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment 45 Andrew Isherwood 2018-10-31 14:36:05 UTC
Comment on attachment 81719 [details] [review]
Bug 20996: (follow-up) Fix column name expectation

One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment 46 Andrew Isherwood 2018-10-31 14:36:11 UTC
Comment on attachment 81720 [details] [review]
Bug 20996: (QA follow-up) Fix pod

One of these patches in the rebased set is failing with "error: sha1 information is lacking or useless (Koha/Illrequest.pm)", so I'm obsoleting them all, un-obsoleting their predecessors, then going to rebase and re-attach patches (including josef's POD fix)
Comment 47 Andrew Isherwood 2018-10-31 15:27:28 UTC
Created attachment 81747 [details] [review]
Bug 20996: Remove prefix use of borrower category

This patch removes the potential use of borrower category as a ILL
request ID prefix. It makes no sense. We provide the ability for a site
to define a request prefix based on branch, there is no use case for
using the borrower category. Add to this that the borrower for every
request was being retrieved in order to get the category, it's a huge
performance hit also.

We also now require the <branch> block in the <interlibrary_loans> block
and complain if it's not present. The request prefix should be defined
in this block.

This patch also improves the performance of the API request that returns all
requests, optionally including additional data.

It also deprecates the overloaded TO_JSON method and moves the request
augmentation code into the API route's controller. It may be that we
want to shift it out of there at some point, but it is fine where it is
for now.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 48 Andrew Isherwood 2018-10-31 15:27:33 UTC
Created attachment 81748 [details] [review]
Bug 20996: Fix unit tests

- Remove unit tests relating to TO_JSON
- Add tests for new explicit embedding
- Modify tests relating to now deprecated brw_cat

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 49 Andrew Isherwood 2018-10-31 15:27:36 UTC
Created attachment 81749 [details] [review]
Bug 20996: Further enhance performance

We were immediately unblessing all request objects after retrieval then,
at a later stage, re-instantiating each one so we could call methods on
that. This was a very bad idea. We now preserve the objects for as long
as we can.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 50 Andrew Isherwood 2018-10-31 15:27:40 UTC
Created attachment 81750 [details] [review]
Bug 20996: (follow-up) Add <branch> to conf tpl

Since we now require the <branch> block, we should add it to the config
templates

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 51 Andrew Isherwood 2018-10-31 15:27:44 UTC
Created attachment 81751 [details] [review]
Bug 20996: (follow-up) Fix column name expectation

This bug changes how request metadata is obtained. Previously the
appropriate backend's metadata function would return it. For at least
the FreeForm backend, the metadata property names we title cased. The
datatable JS was expecting this and breaks with the new metadata getting
which is performed in the API controller, which just returns the
property names as they appear in the DB.

This patch changes the expectation of the JS to match what's coming back
from the API

Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.
Comment 52 Andrew Isherwood 2018-10-31 15:27:48 UTC
Created attachment 81752 [details] [review]
Bug 20996: (follow-up) Fix merge problems

Merge conflicts resolved
Comment 53 Andrew Isherwood 2018-10-31 15:27:52 UTC
Created attachment 81753 [details] [review]
Bug 20996: (QA follow-up) Fix pod
Comment 54 Andrew Isherwood 2018-10-31 15:29:54 UTC
Rebased, fixed merge problems (including the "comments" embed in the API response) and added Josef's POD fix.

Ready for QA again
Comment 55 Josef Moravec 2018-10-31 16:46:09 UTC
Created attachment 81755 [details] [review]
Bug 20996: Remove prefix use of borrower category

This patch removes the potential use of borrower category as a ILL
request ID prefix. It makes no sense. We provide the ability for a site
to define a request prefix based on branch, there is no use case for
using the borrower category. Add to this that the borrower for every
request was being retrieved in order to get the category, it's a huge
performance hit also.

We also now require the <branch> block in the <interlibrary_loans> block
and complain if it's not present. The request prefix should be defined
in this block.

This patch also improves the performance of the API request that returns all
requests, optionally including additional data.

It also deprecates the overloaded TO_JSON method and moves the request
augmentation code into the API route's controller. It may be that we
want to shift it out of there at some point, but it is fine where it is
for now.

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 56 Josef Moravec 2018-10-31 16:46:14 UTC
Created attachment 81756 [details] [review]
Bug 20996: Fix unit tests

- Remove unit tests relating to TO_JSON
- Add tests for new explicit embedding
- Modify tests relating to now deprecated brw_cat

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 57 Josef Moravec 2018-10-31 16:46:19 UTC
Created attachment 81757 [details] [review]
Bug 20996: Further enhance performance

We were immediately unblessing all request objects after retrieval then,
at a later stage, re-instantiating each one so we could call methods on
that. This was a very bad idea. We now preserve the objects for as long
as we can.

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 58 Josef Moravec 2018-10-31 16:46:24 UTC
Created attachment 81758 [details] [review]
Bug 20996: (follow-up) Add <branch> to conf tpl

Since we now require the <branch> block, we should add it to the config
templates

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 59 Josef Moravec 2018-10-31 16:46:28 UTC
Created attachment 81759 [details] [review]
Bug 20996: (follow-up) Fix column name expectation

This bug changes how request metadata is obtained. Previously the
appropriate backend's metadata function would return it. For at least
the FreeForm backend, the metadata property names we title cased. The
datatable JS was expecting this and breaks with the new metadata getting
which is performed in the API controller, which just returns the
property names as they appear in the DB.

This patch changes the expectation of the JS to match what's coming back
from the API

Signed-off-by: Magnus Enger <magnus@enger.priv.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Tested by repeatedly loading the API-call
http://intranet/api/v1/illrequests?embed=metadata,patron,capabilities,library
in Chromium and watching the reported time to load the URL. There
is a siginificant decrease in the time with the patches compared to
the time it takes without the patches. I had some concerns along the
way, but they have all been fixed.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 60 Josef Moravec 2018-10-31 16:46:33 UTC
Created attachment 81760 [details] [review]
Bug 20996: (follow-up) Fix merge problems

Merge conflicts resolved

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 61 Josef Moravec 2018-10-31 16:46:37 UTC
Created attachment 81761 [details] [review]
Bug 20996: (QA follow-up) Fix pod

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 62 Nick Clemens 2018-11-02 10:43:05 UTC
Awesome work all!

Pushed to master for 18.11
Comment 63 Jonathan Druart 2018-11-02 12:47:21 UTC
t/db_dependent/Illrequest/Config.t is failing.

New code added to Koha/Illrequest/Config.pm but no tests?
Comment 64 Martin Renvoize 2018-11-02 15:21:44 UTC
Created attachment 81908 [details] [review]
Bug 20966: Add column configuration to table of orders in a basket

This patch adds column configuration to the table of orders in a basket.
It replaces the "Show all details" checkbox which previously was used to
show the hidden "tax included" columns. Now those columns are hidden by
default in the columns configuration file.

To test, apply the patch and view a basket with multiple orders. Test
all DataTables functionality (paging, sorting, filtering) including
showing and hiding columns.

Test with both an open and a closed basket.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2018-11-02 15:21:48 UTC
Created attachment 81909 [details] [review]
Bug 20966: (follow-up) Add template variable filter; Fix table export

This patch makes to corrections:

1. The "$raw" filter has been added to the column configuration
2. The columns configuration now specifies which columns are exported in
   Copy, Excel, etc. operations.

To test, apply the patch and test the various table export buttons on a
both a closed and open basket. Confirm that the 'Modify' and 'Cancel'
columns are not exported.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 66 Josef Moravec 2018-11-05 08:55:52 UTC
Created attachment 81928 [details] [review]
Bug 20996: (follow-up) Fix test

- Fixes the t/db_dependent/Illrequest/Config.t
- Adds tests for Koha::Illrequest::Config->has_branch

0) Do not apply the path
1) Run test t/db_dependent/Illrequest/Config.t
2) It fails - it is because the Koha/Illrequest/Config.pm prefixes
handling changed
3) Apply the patch
4) Run test again, it should pass now
5) Read through the test code and confirm it makes sense
Comment 67 Josef Moravec 2018-11-05 08:57:02 UTC
(In reply to Jonathan Druart from comment #63)
> t/db_dependent/Illrequest/Config.t is failing.
> 
> New code added to Koha/Illrequest/Config.pm but no tests?

I added a follow-up patch with test changes
Comment 68 Tomás Cohen Arazi 2018-11-05 14:29:55 UTC
Created attachment 81967 [details] [review]
Bug 20996: (follow-up) Fix test

- Fixes the t/db_dependent/Illrequest/Config.t
- Adds tests for Koha::Illrequest::Config->has_branch

0) Do not apply the path
1) Run test t/db_dependent/Illrequest/Config.t
2) It fails - it is because the Koha/Illrequest/Config.pm prefixes
handling changed
3) Apply the patch
4) Run test again, it should pass now
5) Read through the test code and confirm it makes sense

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Nick Clemens 2018-11-05 14:57:28 UTC
Awesome work all!

Pushed to master for 18.11(In reply to Tomás Cohen Arazi from comment #68)
> Created attachment 81967 [details] [review] [review]
> Bug 20996: (follow-up) Fix test
> 
> - Fixes the t/db_dependent/Illrequest/Config.t
> - Adds tests for Koha::Illrequest::Config->has_branch
> 
> 0) Do not apply the path
> 1) Run test t/db_dependent/Illrequest/Config.t
> 2) It fails - it is because the Koha/Illrequest/Config.pm prefixes
> handling changed
> 3) Apply the patch
> 4) Run test again, it should pass now
> 5) Read through the test code and confirm it makes sense
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Followup pushed to master for 18.11
Comment 70 Martin Renvoize 2018-11-06 16:37:23 UTC
Created attachment 81994 [details] [review]
Bug 20996: (RM follow-up) Fix the api tests
Comment 71 Martin Renvoize 2018-11-06 16:40:08 UTC
Added a follow-up for the RM that should fix the last remaining failing test.
Comment 72 Martin Renvoize 2018-11-06 16:43:03 UTC
Created attachment 81996 [details] [review]
Bug 20996: (RM follow-up) Fix the api tests

https://bugs.koha-community.org/show_bug.cgi?id=21766
Comment 73 Jonathan Druart 2018-11-06 17:20:19 UTC
I get a warning with the last patch:
t/db_dependent/api/v1/illrequests.t .. Un-mocked method 'status_graph()' called at /home/vagrant/kohaclone/Koha/Illrequest.pm line 439.
Comment 74 Jonathan Druart 2018-11-06 17:26:00 UTC
Created attachment 81997 [details] [review]
Bug 20996: (RM follow-up) Fix the api tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 75 Jonathan Druart 2018-11-06 17:26:11 UTC
Created attachment 81998 [details] [review]
Bug 20996: Remove warning 'Un-mocked method'

t/db_dependent/api/v1/illrequests.t .. Un-mocked method 'status_graph()'
called at /home/vagrant/kohaclone/Koha/Illrequest.pm line 439.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 76 Nick Clemens 2018-11-06 17:56:02 UTC
Last two patches pushed to master for 18.11
Comment 77 Martin Renvoize 2018-11-06 18:36:55 UTC
Odd.. I didn't get that warning at all when I tested the patch.
Comment 78 Martin Renvoize 2018-11-16 12:06:15 UTC
Enhancement, will not be backported to 18.05.x series.