Summary: | Interlibrary loan javascript is broken | ||
---|---|---|---|
Product: | Koha | Reporter: | Andrew Isherwood <bugzilla> |
Component: | ILL | Assignee: | Bugs List <koha-bugs> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P1 - high | CC: | fridolin.somers, katrin.fischer, lucas, magnus, nick |
Version: | master | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
19.11.00,19.05.05
|
|
Bug Depends on: | 23229 | ||
Bug Blocks: | |||
Attachments: |
Bug 23529: Narrow scope of changed
Bug 23529: (follow-up) Fix syntax error Bug 23529: (follow-up) Fix syntax error Bug 23529: Narrow scope of changed Bug 23529: (follow-up) Fix syntax error Bug 23529: (QA follow-up) Fix closing bracket, add label |
Description
Andrew Isherwood
2019-09-02 08:23:53 UTC
Created attachment 92533 [details] [review] Bug 23529: Narrow scope of changed Whilst working on something else, it came to light that the previous fix in bug 23229 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. 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 I get an error when testing this: SyntaxError: missing ) after argument list ill-list-table_19.0600022.js:502:8 Created attachment 92545 [details] [review] Bug 23529: (follow-up) Fix syntax error As mentioned in comment #3 Sorry Owen, bad commit! What's super weird is that, although I could see the syntax error in my editor, my browser wasn't complaining about it! Hence me not catching if before I attached the patch. Anyway, it should be fixed now. Apologies for that! Two problems: 1. I can't for the life of me recreate the original problem, if the problem was that /api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners was getting called on the "New ILL request" pages for Dummy and FreeForm 2. After applying the patch I get a JS error in the console: "Uncaught SyntaxError: missing ) after argument list" and then it points to ill-list-table_19.0600029.js:501. Hi Magnus Thanks for looking at this. > I can't for the life of me recreate the original problem [...] The reason you can't recreate the problem is that it was fixed in Bug 23229, which is now in master. However, that fix was too far reaching and causes ILL JS to be broken. For example, without this bug applied, try clicking on "ILL request log" on the "Manage request" page, it doesn't work. This bug, once applied, fixes that problem. > After applying the patch I get a JS error in the console [...] I can't replicate that. I applied the patch and it all worked fine. Are you sure you hard refreshed your browser to pick up the new JS? The second patch does not quite fix the syntax problems, I think. As far as I can see, the "if" that starts like this: 366 if (window.location.search.length == 0) { 367 var ajax = $.ajax( 368 '/api/v1/illrequests?embed=metadata,patron,capabilities,library,status_alias,comments,requested_partners' 369 + filterParam 370 ).done(function() { has to be closed like this, or the parens and brackets don't add up: 497 return placedPassed && modifiedPassed; 498 499 }); 500 501 }); 502 } Created attachment 94657 [details] [review] Bug 23529: (follow-up) Fix syntax error As mentioned by Magnus in comment #8 Sorry Magnus, definitely fixed this time! Created attachment 94744 [details] [review] Bug 23529: Narrow scope of changed Whilst working on something else, it came to light that the previous fix in bug 23229 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. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 94745 [details] [review] Bug 23529: (follow-up) Fix syntax error As mentioned by Magnus in comment #8 Sorry Magnus, definitely fixed this time! Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 94746 [details] [review] Bug 23529: (QA follow-up) Fix closing bracket, add label Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Heh, changed: if (window.location.search.length == 0) {} To: if (window.location.search.length == 0) { Nice work! Pushed to master for 19.11.00 Is there any chance of this being backported to 18.11.x? Without it, ILL Javascript is completely broken in 18.11.x. It needs to go into 19.05 first - adding Fridolin und Lucas. As discussed at the IRC meeting today, you might want to email them. Actually, hold off on that last request... It may not actually be the case. Just trying to determine what the heck is going on. OK, ignore me. Nothing needs doing here. Sorry for the noise. Pushed to 19.05.x for 19.05.05 |