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.
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.
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>
Created attachment 91097 [details] [review] Bug 23229: (QA follow-up) Fix indentation Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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>
Nice work! Pushed to master for 19.11.00
Pushed to 19.05.x for 19.05.03
backported to 18.11.x for 18.11.09
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.
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>
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>
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.
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
Anyone got any idea how I can set this back to "Signed off" so the last patch can be tested?
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.
(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.
Thanks Katrin, Bug 23529 now addresses this.