Bug 22440 - Improve ILL page performance by moving to server side filtering
Summary: Improve ILL page performance by moving to server side filtering
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P4 normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact:
URL: https://wiki.koha-community.org/wiki/...
Keywords:
: 22714 23557 30277 34246 (view as bug list)
Depends on: 26635 29032
Blocks: 30719 33544 33545 33874 34283 19774 21983 33685 33786 33834 33873 34058 34130 34223 34226 36565
  Show dependency treegraph
 
Reported: 2019-03-01 10:33 UTC by Andrew Isherwood
Modified: 2024-09-06 20:50 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06
Circulation function:


Attachments
Bug 22440: Add Koha::Illrequests->filter_by_visible (10.85 KB, patch)
2023-05-05 11:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: Add missing foreign key (2.34 KB, patch)
2023-05-05 11:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: Add standard accessors for later usage (4.31 KB, patch)
2023-05-05 11:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: Add GET /ill_requests (16.01 KB, patch)
2023-05-05 11:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: New ILL requests table (66.52 KB, patch)
2023-05-05 11:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: ILL API changes (10.00 KB, patch)
2023-05-05 11:04 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: Tests (24.35 KB, patch)
2023-05-05 11:05 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: Move backend statuses api endpoint (14.09 KB, patch)
2023-05-05 11:05 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: Schema Update (2.92 KB, patch)
2023-05-05 11:05 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22440: (follow-up) Nothing is forever (1.81 KB, patch)
2023-05-05 19:05 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 22440: (follow-up) Fix existing tests (4.77 KB, patch)
2023-05-05 19:26 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-03-01 10:33:09 UTC
The response object returned from the /api/vi/illrequest GET route isn't consistent with response objects supplied by other routes, such as the patrons route.

We have an RFC for this route here: https://wiki.koha-community.org/wiki/Illrequests_endpoint_RFC

Josef has made some initial recommendations on what the properties should be named to make them consistent with other routes. We could do with expanding this RFC to make it complete, including a spec on how object embedding should be done since, to date, this is the only route that does this.
Comment 1 Andrew Isherwood 2019-03-11 09:13:16 UTC
Further suggestions from Josef to bring this endpoint into line. We should follow the convention illustrated here: https://github.com/Koha-Community/Koha/blob/master/Koha/REST/V1/Library.pm#L46

This automagically looks at the parameters passed in the request and, if they match parameters described in the OpenAPI spec and, if they match, uses their values as filters. The _to_api & _to_model methods will allow us to carry out the property name consistency described in the bug description
Comment 2 Tomás Cohen Arazi (tcohen) 2023-04-16 14:18:59 UTC
*** Bug 30277 has been marked as a duplicate of this bug. ***
Comment 3 Martin Renvoize (ashimema) 2023-05-05 11:04:43 UTC
Created attachment 150715 [details] [review]
Bug 22440: Add Koha::Illrequests->filter_by_visible

This patch introduces a method for filtering out requests that match the
statuses specified on the *ILLHiddenRequestStatuses* system preference.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Illrequests.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize (ashimema) 2023-05-05 11:04:45 UTC
Created attachment 150716 [details] [review]
Bug 22440: Add missing foreign key

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize (ashimema) 2023-05-05 11:04:48 UTC
Created attachment 150717 [details] [review]
Bug 22440: Add standard accessors for later usage

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize (ashimema) 2023-05-05 11:04:51 UTC
Created attachment 150718 [details] [review]
Bug 22440: Add GET /ill_requests

Co-authored-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2023-05-05 11:04:55 UTC
Created attachment 150719 [details] [review]
Bug 22440: New ILL requests table

Use kohaTable requesting the REST api instead of the old KohaTable

Co-authored-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2023-05-05 11:04:57 UTC
Created attachment 150720 [details] [review]
Bug 22440: ILL API changes

* Add GET /illbackends/{id}/statuses
* Change API route to ill/requests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2023-05-05 11:05:01 UTC
Created attachment 150721 [details] [review]
Bug 22440: Tests

Updated API tests

Co-authored-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2023-05-05 11:05:04 UTC
Created attachment 150722 [details] [review]
Bug 22440: Move backend statuses api endpoint

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2023-05-05 11:05:07 UTC
Created attachment 150723 [details] [review]
Bug 22440: Schema Update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2023-05-05 11:07:47 UTC
This has had significant testing in house at PTFS-E and was worked on collaboratively by Tomas and Pedro (with a little interruption from myself a few times)

We all agreed to go PQA given the eyes's it's had on :)
Comment 13 Martin Renvoize (ashimema) 2023-05-05 12:07:53 UTC
*** Bug 22714 has been marked as a duplicate of this bug. ***
Comment 14 Tomás Cohen Arazi (tcohen) 2023-05-05 12:16:49 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 15 Tomás Cohen Arazi (tcohen) 2023-05-05 19:05:43 UTC
Created attachment 150788 [details] [review]
Bug 22440: (follow-up) Nothing is forever

This patch picks (yet) another class for testing purposes. The class
needs to not implement to_api() and to_api_mapping().
Comment 16 Tomás Cohen Arazi (tcohen) 2023-05-05 19:26:39 UTC
Created attachment 150789 [details] [review]
Bug 22440: (follow-up) Fix existing tests

As we are introducing a FK for illrequests.biblio_id, TestBuilder is
generating a linked biblio, and so tests expecting undef are failing.

This fixes it by explicitly setting biblio_id => undef on creating the
request.

The patch also cleans the area a bit, removing a redundant and fragile
test as well.
Comment 17 Martin Renvoize (ashimema) 2023-05-12 07:31:20 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 18 Lucas Gass (lukeg) 2023-05-15 22:40:44 UTC
Missing dependencies for 22.05.x, no backport.
Comment 19 Katrin Fischer 2023-05-31 19:51:31 UTC
Hm, I am not so happy about the FK change hidden/sneaked in with this bug. Especially since it's now without documentation/warning to users and can create data loss. 

If you don't store all information also in ILL request attributes it will be hard to determine what has been ordered for making acq decisions and creating reports.

Also: there already was as specific bug for this change, stuck in discussion for these reasons: bug 21983
Comment 20 Jonathan Druart 2023-06-01 12:26:38 UTC
From https://tree.taiga.io/project/koha-ils/task/117

And IRC:
Joubu> tcohen: see https://tree.taiga.io/project/koha-ils/task/117
Joubu> tcohen: installer/data/mysql/db_revs/221200026.pl
Joubu> it must be *unless* fk exists!
Joubu> there is also a typo in the description endopoint
Comment 21 Tomás Cohen Arazi (tcohen) 2023-06-01 14:59:46 UTC
Fixed in 23.05 and 23.11. Please backport the fix for 22.11.
Comment 22 Katrin Fischer 2023-06-01 15:10:02 UTC
(In reply to Katrin Fischer from comment #19)
> Hm, I am not so happy about the FK change hidden/sneaked in with this bug.
> Especially since it's now without documentation/warning to users and can
> create data loss. 
> 
> If you don't store all information also in ILL request attributes it will be
> hard to determine what has been ordered for making acq decisions and
> creating reports.
> 
> Also: there already was as specific bug for this change, stuck in discussion
> for these reasons: bug 21983

I'd have loved to at least get a comment/reaction. The fix mentioned seems to be to enforce the change I marked as critical.
Comment 23 Katrin Fischer 2023-06-01 15:15:13 UTC
(In reply to Katrin Fischer from comment #22)
> (In reply to Katrin Fischer from comment #19)
> > Hm, I am not so happy about the FK change hidden/sneaked in with this bug.
> > Especially since it's now without documentation/warning to users and can
> > create data loss. 
> > 
> > If you don't store all information also in ILL request attributes it will be
> > hard to determine what has been ordered for making acq decisions and
> > creating reports.
> > 
> > Also: there already was as specific bug for this change, stuck in discussion
> > for these reasons: bug 21983
> 
> I'd have loved to at least get a comment/reaction. The fix mentioned seems
> to be to enforce the change I marked as critical.

Also, if you really want to do this, because it needs to be done: what's with the old ill_requests? It caused bug 33873.
Comment 24 Tomás Cohen Arazi (tcohen) 2023-06-01 15:15:40 UTC
(In reply to Katrin Fischer from comment #23)
> (In reply to Katrin Fischer from comment #22)
> > (In reply to Katrin Fischer from comment #19)
> > > Hm, I am not so happy about the FK change hidden/sneaked in with this bug.
> > > Especially since it's now without documentation/warning to users and can
> > > create data loss. 
> > > 
> > > If you don't store all information also in ILL request attributes it will be
> > > hard to determine what has been ordered for making acq decisions and
> > > creating reports.
> > > 
> > > Also: there already was as specific bug for this change, stuck in discussion
> > > for these reasons: bug 21983
> > 
> > I'd have loved to at least get a comment/reaction. The fix mentioned seems
> > to be to enforce the change I marked as critical.
> 
> Also, if you really want to do this, because it needs to be done: what's
> with the old ill_requests? It caused bug 33873.

Should we do the same we did for orderlines?
Comment 25 Katrin Fischer 2023-06-01 15:22:39 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> (In reply to Katrin Fischer from comment #23)
> > (In reply to Katrin Fischer from comment #22)
> > > (In reply to Katrin Fischer from comment #19)
> > > > Hm, I am not so happy about the FK change hidden/sneaked in with this bug.
> > > > Especially since it's now without documentation/warning to users and can
> > > > create data loss. 
> > > > 
> > > > If you don't store all information also in ILL request attributes it will be
> > > > hard to determine what has been ordered for making acq decisions and
> > > > creating reports.
> > > > 
> > > > Also: there already was as specific bug for this change, stuck in discussion
> > > > for these reasons: bug 21983
> > > 
> > > I'd have loved to at least get a comment/reaction. The fix mentioned seems
> > > to be to enforce the change I marked as critical.
> > 
> > Also, if you really want to do this, because it needs to be done: what's
> > with the old ill_requests? It caused bug 33873.
> 
> Should we do the same we did for orderlines?

A database table has the option to use COALESCE, so that might be an option. We could also then shift the existing data.

I am not sure what the technical requirement for the FK was, so can't tell if removing it again might be a valid option.
Comment 26 Jonathan Druart 2023-07-07 12:40:24 UTC
This is not used, right?

+                        if ( col.datatype == 'date' ) {
+                            let rfc3339 = $date_to_rfc3339(value);
+                            if ( rfc3339 != 'Invalid Date' ) {
+                                built_value = rfc3339;
+                            }
+                        }
Comment 27 Katrin Fischer 2023-07-10 14:46:12 UTC
*** Bug 34246 has been marked as a duplicate of this bug. ***
Comment 28 Lisette Scheer 2024-09-06 20:50:35 UTC
*** Bug 23557 has been marked as a duplicate of this bug. ***