Bug 23229 - "Get all requests" API call fired when loading any ILL page
Summary: "Get all requests" API call fired when loading any ILL page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Andrew Isherwood
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 23529
  Show dependency treegraph
 
Reported: 2019-06-28 09:20 UTC by Andrew Isherwood
Modified: 2020-11-30 21:44 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03,18.11.09


Attachments
Bug 23229: Only load ILL table JS when needed (1.56 KB, patch)
2019-06-28 09:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23229: Only load ILL table JS when needed (1.63 KB, patch)
2019-06-28 15:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23229: (QA follow-up) Fix indentation (1.13 KB, patch)
2019-06-28 15:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23229: Only load ILL table JS when needed (1.67 KB, patch)
2019-07-25 17:00 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23229: (QA follow-up) Fix indentation (1.17 KB, patch)
2019-07-25 17:00 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 23229: (follow-up) Narrow scope of changes (14.83 KB, patch)
2019-08-29 13:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23229: Only load ILL table JS when needed (1.75 KB, patch)
2019-08-29 13:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23229: (QA follow-up) Fix indentation (1.17 KB, patch)
2019-08-29 13:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23229: (follow-up) Narrow scope of changes (14.83 KB, patch)
2019-08-29 13:36 UTC, Andrew Isherwood
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-06-28 09:20:09 UTC
This is a substantial bug that, as far as I can determine, has always existed.

The JS that retrieves all ILL requests for the main ILL table (ill-list-table.js) runs on every ILL page that utilises ill-requests.tt (pretty much every ILL page). This is clearly wrong, it should only run when the ILL list table is being displayed.

This has significant performance implications since the cost in constructing and delivering the response to this request can be substantial.
Comment 1 Andrew Isherwood 2019-06-28 09:28:06 UTC
Created attachment 91062 [details] [review]
Bug 23229: Only load ILL table JS when needed

This patch prevents the JS associated with the ILL table from loading
unless we're actually displaying the table. This has the knock on effect
of not causing the "get all ILL requests" API request to fire when
loading any ILL page.

Test plan:

- Go to "ILL requests" page
- TEST: Observe that the table loads all ILL requests
- Go to the "New ILL request" page for any installed backend
- TEST: Check the browser dev tools "Network" panel, observe that the
/api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners
request is not made.
Comment 2 Kyle M Hall 2019-06-28 15:04:45 UTC
Created attachment 91096 [details] [review]
Bug 23229: Only load ILL table JS when needed

This patch prevents the JS associated with the ILL table from loading
unless we're actually displaying the table. This has the knock on effect
of not causing the "get all ILL requests" API request to fire when
loading any ILL page.

Test plan:

- Go to "ILL requests" page
- TEST: Observe that the table loads all ILL requests
- Go to the "New ILL request" page for any installed backend
- TEST: Check the browser dev tools "Network" panel, observe that the
/api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners
request is not made.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 Kyle M Hall 2019-06-28 15:05:45 UTC
Created attachment 91097 [details] [review]
Bug 23229: (QA follow-up) Fix indentation

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Josef Moravec 2019-07-25 17:00:24 UTC
Created attachment 91769 [details] [review]
Bug 23229: Only load ILL table JS when needed

This patch prevents the JS associated with the ILL table from loading
unless we're actually displaying the table. This has the knock on effect
of not causing the "get all ILL requests" API request to fire when
loading any ILL page.

Test plan:

- Go to "ILL requests" page
- TEST: Observe that the table loads all ILL requests
- Go to the "New ILL request" page for any installed backend
- TEST: Check the browser dev tools "Network" panel, observe that the
/api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners
request is not made.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Josef Moravec 2019-07-25 17:00:29 UTC
Created attachment 91770 [details] [review]
Bug 23229: (QA follow-up) Fix indentation

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Martin Renvoize 2019-07-26 10:19:26 UTC
Nice work!

Pushed to master for 19.11.00
Comment 7 Fridolin Somers 2019-08-09 12:16:38 UTC
Pushed to 19.05.x for 19.05.03
Comment 8 Lucas Gass 2019-08-28 15:09:45 UTC
backported to 18.11.x for 18.11.09
Comment 9 Andrew Isherwood 2019-08-29 13:35:56 UTC
Created attachment 92491 [details] [review]
Bug 23229: (follow-up) Narrow scope of changes

Sorry guys!

Whilst working on something else, it came to light that the previous fix
in this bug was waaaaay too heavy handed and basically kills all ILL
related JS :( This is obviously a big problem, this patch fixes that.

The main problem is that all the ILL related JS is in a single file,
which isn't ideal as bits of it are used on different pages. Ideally,
we'd refactor it out into more manageable chunks which can be included
as required. Until such a time, this patch will do, it just prevents the
"get all requests" AJAX request unless you're on the ILL list page.
Comment 10 Andrew Isherwood 2019-08-29 13:36:41 UTC
Created attachment 92492 [details] [review]
Bug 23229: Only load ILL table JS when needed

This patch prevents the JS associated with the ILL table from loading
unless we're actually displaying the table. This has the knock on effect
of not causing the "get all ILL requests" API request to fire when
loading any ILL page.

Test plan:

- Go to "ILL requests" page
- TEST: Observe that the table loads all ILL requests
- Go to the "New ILL request" page for any installed backend
- TEST: Check the browser dev tools "Network" panel, observe that the
/api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners
request is not made.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Andrew Isherwood 2019-08-29 13:36:45 UTC
Created attachment 92493 [details] [review]
Bug 23229: (QA follow-up) Fix indentation

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Andrew Isherwood 2019-08-29 13:36:51 UTC
Created attachment 92494 [details] [review]
Bug 23229: (follow-up) Narrow scope of changes

Sorry guys!

Whilst working on something else, it came to light that the previous fix
in this bug was waaaaay too heavy handed and basically kills all ILL
related JS :( This is obviously a big problem, this patch fixes that.

The main problem is that all the ILL related JS is in a single file,
which isn't ideal as bits of it are used on different pages. Ideally,
we'd refactor it out into more manageable chunks which can be included
as required. Until such a time, this patch will do, it just prevents the
"get all requests" AJAX request unless you're on the ILL list page.
Comment 13 Andrew Isherwood 2019-08-29 13:38:40 UTC
New test plan:

- Go to "ILL requests" page
- TEST: Observe that the table loads all ILL requests
- Go to the "New ILL request" page for any installed backend
- TEST: Check the browser dev tools "Network" panel, observe that the
/api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners
request is not made.
- Go to "Manage request" page for a request
- TEST: Check that the "ILL request log" and "Display supplier metadata" buttons work
Comment 14 Andrew Isherwood 2019-08-29 13:39:09 UTC
Anyone got any idea how I can set this back to "Signed off" so the last patch can be tested?
Comment 15 Katrin Fischer 2019-08-29 20:33:06 UTC
The code was just released - so you need to file a new bug - this is no longer a follow-up patch, it's now a bug fix and should be treated with high priority.
Comment 16 Katrin Fischer 2019-08-29 20:35:35 UTC
(In reply to Katrin Fischer from comment #15)
> The code was just released - so you need to file a new bug - this is no
> longer a follow-up patch, it's now a bug fix and should be treated with high
> priority.

Ok, it was a bug fix before. What I am trying ot explain: once code is in a released version, you should not reuse the bug - it will be confusing, especially in release notes. Better to have a new distinctive bug that describes what is fixed.
Comment 17 Andrew Isherwood 2019-09-02 08:33:07 UTC
Thanks Katrin, Bug 23529 now addresses this.