Bug 23529 - Interlibrary loan javascript is broken
Summary: Interlibrary loan javascript is broken
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on: 23229
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-02 08:23 UTC by Andrew Isherwood
Modified: 2019-10-22 07:48 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23529: Narrow scope of changed (14.70 KB, patch)
2019-09-02 08:31 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23529: (follow-up) Fix syntax error (1.07 KB, patch)
2019-09-03 12:46 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-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     }