Bug 23529

Summary: Interlibrary loan javascript is broken
Product: Koha Reporter: Andrew Isherwood <bugzilla>
Component: ILLAssignee: 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
This was caused by an overzealous fix in Bug 23229. A fix was appended as a follow-up patch on 23229 but Katrin advised that it should be a new bug, hence this one.
Comment 1 Andrew Isherwood 2019-09-02 08:31:07 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.
Comment 2 Andrew Isherwood 2019-09-02 09:36:29 UTC
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 3 Owen Leonard 2019-09-03 12:28:12 UTC
I get an error when testing this:

SyntaxError: missing ) after argument list ill-list-table_19.0600022.js:502:8
Comment 4 Andrew Isherwood 2019-09-03 12:46:15 UTC
Created attachment 92545 [details] [review]
Bug 23529: (follow-up) Fix syntax error

As mentioned in comment #3
Comment 5 Andrew Isherwood 2019-09-03 12:48:19 UTC
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!
Comment 6 Magnus Enger 2019-09-30 12:16:39 UTC
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.
Comment 7 Andrew Isherwood 2019-10-09 10:38:34 UTC
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?
Comment 8 Magnus Enger 2019-10-22 07:48:29 UTC
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     }
Comment 9 Andrew Isherwood 2019-10-24 11:27:36 UTC
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!
Comment 10 Nick Clemens 2019-10-25 11:32:41 UTC
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>
Comment 11 Nick Clemens 2019-10-25 11:32:44 UTC
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>
Comment 12 Nick Clemens 2019-10-25 11:32:47 UTC
Created attachment 94746 [details] [review]
Bug 23529: (QA follow-up) Fix closing bracket, add label

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2019-10-25 11:33:41 UTC
Heh, changed:

if (window.location.search.length == 0) {}

To:

if (window.location.search.length == 0) {
Comment 14 Martin Renvoize 2019-10-25 12:21:13 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Andrew Isherwood 2019-11-06 15:56:52 UTC
Is there any chance of this being backported to 18.11.x? Without it, ILL Javascript is completely broken in 18.11.x.
Comment 16 Katrin Fischer 2019-11-06 16:19:44 UTC
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.
Comment 17 Andrew Isherwood 2019-11-06 16:36:26 UTC
Actually, hold off on that last request... It may not actually be the case. Just trying to determine what the heck is going on.
Comment 18 Andrew Isherwood 2019-11-07 11:56:33 UTC
OK, ignore me. Nothing needs doing here. Sorry for the noise.
Comment 19 Fridolin Somers 2019-11-15 10:16:32 UTC
Pushed to 19.05.x for 19.05.05