Bug 24043 - ILL module can't show requests from more than one backend
Summary: ILL module can't show requests from more than one backend
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Andrew Isherwood
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-14 10:10 UTC by Magnus Enger
Modified: 2020-05-15 15:58 UTC (History)
8 users (show)

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


Attachments
Bug 24043: Fix retrieval of of status name (2.29 KB, patch)
2020-01-13 10:25 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24043: Fix retrieval of of status name (2.35 KB, patch)
2020-04-30 11:18 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24043: Fix retrieval of of status name (2.40 KB, patch)
2020-04-30 11:33 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 24043: (QA follow-up) Fix another TypeError when retriaving status name (1.48 KB, patch)
2020-04-30 11:33 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 24043: (QA follow-up) Fix another TypeError when retriaving status name (1.54 KB, patch)
2020-04-30 11:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2019-11-14 10:10:12 UTC
A customer has 6 requests created with the FreeForm backend and 10 with the Libris backend. The table of requests tries to display a mix of these, but shows this error in the dev tools console:

Uncaught TypeError: Cannot read property 'name' of undefined
    at createStatus (ill-requests.pl:714)
    at jquery.dataTables.min_18.1105000.js:18
    at Object.b.fnGetData (jquery.dataTables.min_18.1105000.js:12)
    at B (jquery.dataTables.min_18.1105000.js:16)
    at Ia (jquery.dataTables.min_18.1105000.js:14)
    at mb (jquery.dataTables.min_18.1105000.js:66)
    at T (jquery.dataTables.min_18.1105000.js:30)
    at ia (jquery.dataTables.min_18.1105000.js:48)
    at HTMLTableElement.<anonymous> (jquery.dataTables.min_18.1105000.js:95)
    at Function.each (jquery-2.2.3.min_18.1105000.js:2)

The code around ill-requests.pl line 714 looks like this:

        var createStatus = function(data, type, row, meta) {
            var origData = meta.settings.oInit.originalData;
            if (origData.length > 0) {
                var status_name = meta.settings.oInit.originalData[0].capabilities[
                    row.status
                ].name;
                switch( status_name ) {
                    case "New request":
                        return _("Ny förfrågan");
                    case "Requested":
                        return _("Begärd");

Now if I manipulate Koha/REST/V1/Illrequests.pm line 56 to look like this:

    # Get all requests
    my @requests = Koha::Illrequests->as_list;
    # @requests = @requests[6..9];

I can choose which requests to display in the table, by changing the numbers in the brackets. Displaying only the FreeForm requests works fine, and displaying only the Libris requests works fine. So it looks like the problem is to display a mix. 

I am not sure I completely understand the JS here, but I have a hunch that meta.settings.oInit.originalData[0].capabilities[ means we are trying to use the metadata and the capabilities of the first ([0]) request returned to display *all* the requests. And the backends have different statuses, so trying to look up a Libris status in the set of FreeForm statuses fails?
Comment 1 Magnus Enger 2019-11-14 10:17:23 UTC
Tested on 18.11.05.
Comment 2 Magnus Enger 2019-11-14 12:47:24 UTC
Played around a bit with the JS debugger, and it looks like my hunch was right. "meta" is a variable with this (very simplified) structure:

meta
  col
  row
  settings
    oInit
      originalData
        0
          capabilities
            NEW
            REQ
          status
        1
          capabilities
            NEW
            REQ
          status

So when we do this: 

     var status_name = meta.settings.oInit.originalData[0].capabilities[
         row.status
     ].name;

we are trying to use the "capabilities" (a hash with status codes as keys and status names as values) of the first request returned ([0]) to translate the status codes of *all* requests into their status names. But requests from different backends can have completely different statuses, so this fails when there are requests from more than one backend. 

It looks like the solution is to use meta.settings.oInit.originalData[ 
meta.row ].capabilities to look up the correct capabilities for the current request being handled. Patch coming in a sec or two!
Comment 3 Magnus Enger 2019-11-14 13:41:12 UTC
Looks like this has been fixed in master. In the example I looked at there was a bunch of FreeForm requests, then a bunch of Libris Requests, if you look at them in order of increasing illrequest_id. As long as the code was looping over the early FreeForm requests, meta.settings.oInit.originalData[0].capabilities contained only FreeForm statuses. When it came time to look at the Libris erquests, meta.settings.oInit.originalData[0].capabilities contained both FreeForm and Libris statuses, which ensures the status names can be displayed correctly.
Comment 4 Martin Renvoize 2020-01-09 17:35:04 UTC
Sounds like this works in master then.. did you work out what fixed it and does that need backporting Magnus.. or can this one be closed?
Comment 5 Magnus Enger 2020-01-10 08:37:39 UTC
(In reply to Martin Renvoize from comment #4)
> Sounds like this works in master then.. did you work out what fixed it and
> does that need backporting Magnus.. or can this one be closed?

I *think* it can be closed.
Comment 6 Andrew Isherwood 2020-01-13 08:48:56 UTC
I'm just taking a look at this now to ensure it's not a problem
Comment 7 Andrew Isherwood 2020-01-13 09:29:44 UTC
This is a problem. It's a bug and I can't believe I've never come across it before. It occurs when a request has a status that is unique to that request's backend (so *not* something like "NEW") *and* the request's backend is not the same as the backend used by the first request in the array. Looking at the code I don't think it can ever have worked since it's fundamentally flawed.

I'll create a fix for master. Thanks for highlighting it Magnus!
Comment 8 Andrew Isherwood 2020-01-13 10:25:09 UTC
Created attachment 97297 [details] [review]
Bug 24043: Fix retrieval of of status name

This patch modifies the way in which a request's status name is
retrieved. I think the previous way it was done (which can never have
worked properly) must have come from a time when we weren't embedding
the request's backend capabilities in each request. So now we can just use
the current row.

Test plan:

1. *Before applying the patch*:
2. Ensure you have at least two requests, from two different backends.
One of the backends you are using must have a possible status that
doesn't exist in the other. For example, the BLDSS backend has a status
of STAT, that doesn't not exist in any other backends.
3. Ensure that the request who's backend has the unique status is *not*
the first request in the returned list.
4. Load the "View ILL requests" page
5. Observe that the page JS fails with a "Cannot read property 'name' of
undefined" error
6. Apply the patch
7. Reload the page (maybe doing a hard reload to ensure the JS is not
cached)
8. TEST: Observe that the error no longer occurs and the requests all
have their statuses displayed correctly.
Comment 9 Andrew Isherwood 2020-01-13 10:26:39 UTC
Note: Despite this bug being originally reported on 18.11, it exists in master, this patch was created on master
Comment 10 Magnus Enger 2020-01-14 11:56:48 UTC
(In reply to Andrew Isherwood from comment #9)
> Note: Despite this bug being originally reported on 18.11, it exists in
> master, this patch was created on master

Hm, I can't seem to be able to reproduce this any longer, on master. Are you sure you have all the steps necessary to reproduce? It might be a problem with e.g. my data, from earlier testing, though.
Comment 11 Andrew Isherwood 2020-01-14 12:32:52 UTC
Yes, it's pretty tricky to reproduce, the required conditions are detailed in the test plan above. It would be interesting to see if you can replicate it by meeting the conditions listed
Comment 12 Martin Renvoize 2020-02-10 15:48:31 UTC
Any update on testing here Magnus?
Comment 13 Magnus Enger 2020-02-10 16:10:02 UTC
Trying to get round to it...
Comment 14 Victor Grousset/tuxayo 2020-04-26 22:03:37 UTC
For someone wanting to test this with absolutely zero knowledge in ILL. Which two backends could be the easiest to setup?

Note, I've found at least this doc:
https://koha-community.org/manual/20.05/en/html/ILL_requests.html
https://wiki.koha-community.org/wiki/ILL_backends

Anymore useful doc? Any "classic" traps?
Comment 15 Josef Moravec 2020-04-29 05:52:43 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> For someone wanting to test this with absolutely zero knowledge in ILL.
> Which two backends could be the easiest to setup?
> 
> Note, I've found at least this doc:
> https://koha-community.org/manual/20.05/en/html/ILL_requests.html
> https://wiki.koha-community.org/wiki/ILL_backends
> 
> Anymore useful doc? Any "classic" traps?

I would go with Freeform and BLDSS
Comment 16 Victor Grousset/tuxayo 2020-04-30 11:03:40 UTC
Can't reproduce the issue. Any clue about the difference in the trials?

== 1: FreeForm and BLDSS ==
- have only Freeform and BLDSS installed
- have a new request with each one
- BLDSS has the unique possible status "STAT" so it has to go first
- the BLDSS is the first one (created last)
- no JS error on the page /cgi-bin/koha/ill/ill-requests.pl

== 2: FreeForm and Koha ==
- have only FreeForm and Koha installed
- FreeForm has the unique possible status "EDITITEM" so it has to go first
- here is the list, because it was a bit messy after the attempts ^^"
  - https://picat.drycat.fr/cR5FyRM3.png
- no JS error
Comment 17 Victor Grousset/tuxayo 2020-04-30 11:18:24 UTC
Created attachment 104017 [details] [review]
Bug 24043: Fix retrieval of of status name

This patch modifies the way in which a request's status name is
retrieved. I think the previous way it was done (which can never have
worked properly) must have come from a time when we weren't embedding
the request's backend capabilities in each request. So now we can just use
the current row.

Test plan:

1. *Before applying the patch*:
2. Ensure you have at least two requests, from two different backends.
One of the backends you are using must have a possible status that
doesn't exist in the other. For example, the BLDSS backend has a status
of STAT, that doesn't not exist in any other backends.
3. Ensure that the request who's backend has the unique status is *not*
the first request in the returned list.
4. Load the "View ILL requests" page
5. Observe that the page JS fails with a "Cannot read property 'name' of
undefined" error
6. Apply the patch
7. Reload the page (maybe doing a hard reload to ensure the JS is not
cached)
8. TEST: Observe that the error no longer occurs and the requests all
have their statuses displayed correctly.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Victor Grousset/tuxayo 2020-04-30 11:25:30 UTC
It kinda worked by using force...:

update illrequests set status='EDITITEM' where illrequest_id = 6;
🥊🥊🥊💥

Which differs from the test plan.
One other difference. The JS error is:

TypeError: meta.settings.oInit.originalData[0].capabilities[row.status] is undefined

But in the end, it created a similar situation that was fixed by the patch.
Thanks Josef for the help reproducing it!

Are we optimistic and move on to QA?
Comment 19 Josef Moravec 2020-04-30 11:33:54 UTC
Created attachment 104020 [details] [review]
Bug 24043: Fix retrieval of of status name

This patch modifies the way in which a request's status name is
retrieved. I think the previous way it was done (which can never have
worked properly) must have come from a time when we weren't embedding
the request's backend capabilities in each request. So now we can just use
the current row.

Test plan:

1. *Before applying the patch*:
2. Ensure you have at least two requests, from two different backends.
One of the backends you are using must have a possible status that
doesn't exist in the other. For example, the BLDSS backend has a status
of STAT, that doesn't not exist in any other backends.
3. Ensure that the request who's backend has the unique status is *not*
the first request in the returned list.
4. Load the "View ILL requests" page
5. Observe that the page JS fails with a "Cannot read property 'name' of
undefined" error
6. Apply the patch
7. Reload the page (maybe doing a hard reload to ensure the JS is not
cached)
8. TEST: Observe that the error no longer occurs and the requests all
have their statuses displayed correctly.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Josef Moravec 2020-04-30 11:33:59 UTC
Created attachment 104021 [details] [review]
Bug 24043: (QA follow-up) Fix another TypeError when retriaving status name

Test plan:
1) Apply first patch, and have the same requests as in previous patch
test plan
2) Load ill requests list
2a) You can see another TypeError in console
2b) You can notice, the Placed on and Updated on columns have prepended
another columns with unformatted dates and without header label
3) Apply this patch
4) Reload page (be sure it is not loaded from cache - Ctrl+F5)
5) Errors from 2a and 2b are gone

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Victor Grousset/tuxayo 2020-04-30 11:36:05 UTC
Well actually, the table was visible thanks to the patch. But there is still another TypeError.

And there are columns with not formatted dates and without header label.

Josef troubleshooted it and ~~will~~ make a followup.

edit: already sent a followup by the time I wrote this :D
Comment 22 Victor Grousset/tuxayo 2020-04-30 11:37:51 UTC
Created attachment 104022 [details] [review]
Bug 24043: (QA follow-up) Fix another TypeError when retriaving status name

Test plan:
1) Apply first patch, and have the same requests as in previous patch
test plan
2) Load ill requests list
2a) You can see another TypeError in console
2b) You can notice, the Placed on and Updated on columns have prepended
another columns with unformatted dates and without header label
3) Apply this patch
4) Reload page (be sure it is not loaded from cache - Ctrl+F5)
5) Errors from 2a and 2b are gone

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 23 Martin Renvoize 2020-05-01 07:20:09 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 24 Joy Nelson 2020-05-08 22:50:03 UTC
backported to 19.11.x for 19.11.06
Comment 25 Lucas Gass 2020-05-15 15:58:32 UTC
backported to 19.05.x for 19.05.11