Bug 22440

Summary: Improve ILL page performance by moving to server side filtering
Product: Koha Reporter: Andrew Isherwood <bugzilla>
Component: ILLAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P4 CC: aspencatteam, jonathan.druart, josef.moravec, katrin.fischer, kyle, lucas, magnus, martin.renvoize, nick, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://wiki.koha-community.org/wiki/Illrequests_endpoint_RFC
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30277
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28641
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22714
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22000
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31705
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
Bug Depends on: 26635, 29032    
Bug Blocks: 30719, 33544, 33545, 33874, 34058, 34283, 36565, 19774, 21983, 33685, 33786, 33834, 33873, 34130, 34223, 34226    
Attachments: Bug 22440: Add Koha::Illrequests->filter_by_visible
Bug 22440: Add missing foreign key
Bug 22440: Add standard accessors for later usage
Bug 22440: Add GET /ill_requests
Bug 22440: New ILL requests table
Bug 22440: ILL API changes
Bug 22440: Tests
Bug 22440: Move backend statuses api endpoint
Bug 22440: Schema Update
Bug 22440: (follow-up) Nothing is forever
Bug 22440: (follow-up) Fix existing tests

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 2023-04-16 14:18:59 UTC
*** Bug 30277 has been marked as a duplicate of this bug. ***
Comment 3 Martin Renvoize 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 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 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 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 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 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 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 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 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 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 2023-05-05 12:07:53 UTC
*** Bug 22714 has been marked as a duplicate of this bug. ***
Comment 14 Tomás Cohen Arazi 2023-05-05 12:16:49 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 15 Tomás Cohen Arazi 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 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 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 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 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 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. ***