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?
Tested on 18.11.05.
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!
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.
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?
(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.
I'm just taking a look at this now to ensure it's not a problem
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!
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.
Note: Despite this bug being originally reported on 18.11, it exists in master, this patch was created on master
(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.
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
Any update on testing here Magnus?
Trying to get round to it...
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?
(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
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
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>
It kinda worked by using force...: update illrequests set status='EDITITEM' where illrequest_id = 6; [U+1F94A][U+1F94A][U+1F94A][U+1F4A5] 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?
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>
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>
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
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>
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.06
backported to 19.05.x for 19.05.11