As per comments on bug 36586, there are situations where self-checkouts restart every second. The cause - or at least one identified cause - appears to be invalid data in the SelfCheckInTimeout and SelfCheckTimeout system preferences. The Javascript isn't properly validating the incoming data and thus the bug appears.
Created attachment 183910 [details] [review] Bug 40336: Validate timeout syspref for self-checks This patch validates the numbers passed to the self-check timeout code, so that it must be a valid number >= 10 seconds. If it fails this validation, it defaults to 120 seconds. Test plan: 0. Apply the patch 1. Enable the SelfCheckInModule and WebBasedSelfCheck syspref 2. Try out these modules using varying values in the SelfCheckInTimeout and SelfCheckTimeout system preferences resepectives. For instance: - A blank space (ie " ") - A negative number (e.g. -7) - An alphabetic string (e.g. aa) - A positive number less than 10 (e.g. 5) - A positive number greater than 10 (e.g. 60) 3. Check the "idle_timeout" value set in the timer objects using the following in the browser console: console.log(window.sci_login_timer); console.log(window.sco_login_timer); 4. Note that all cases except positive numbers greater than or equal to 10 get reset to 120
Tested this on SCO and all values expect positive 10 or greater indeed reset to 120. But I'm still seeing constant restarting even with these values. So patch does work as intended but doesn't solve the whole problem. I'll try to set up a SCI on my environment and test that too just to be sure.
Tested now with SCI, same results.
It would be good to have systempreferences.type supports "integer" and have such problems caught before (during the input).
Sorry David, we've decided to revert Bug 36586 for now.
(In reply to Emmi Takkinen from comment #2) > Tested this on SCO and all values expect positive 10 or greater indeed reset > to 120. But I'm still seeing constant restarting even with these values. So > patch does work as intended but doesn't solve the whole problem. I'll try to > set up a SCI on my environment and test that too just to be sure. Do you have an Internet accessible Koha that can reproduce this problem? I could take a look there. Otherwise, I think I've troubleshooted this as far as I can.
(In reply to Jonathan Druart from comment #4) > It would be good to have systempreferences.type supports "integer" and have > such problems caught before (during the input). Agreed. This would be a good use case for adding syspref validation methods.
(In reply to Lucas Gass (lukeg) from comment #5) > Sorry David, we've decided to revert Bug 36586 for now. No worries. Did you try out the patch or was it a strategic decision? Based off Emmi's experience, it looks like there could be something else going on there in any case. Locally, we are suspending CSRF for self-check for older versions without bug 40108. I guess we'll just keep doing that until some other fix is done. We recently had an experience where a library's PC catalogue was resubmitting the opac-search.pl every second which was interesting. Library staff suspect a glitch with the kiosk software (rather than Koha) but who knows.
(In reply to David Cook from comment #8) > Locally, we are suspending CSRF for self-check for older versions without > bug 40108. I guess we'll just keep doing that until some other fix is done. Sorry I meant bug 36586
(In reply to David Cook from comment #6) > (In reply to Emmi Takkinen from comment #2) > > Tested this on SCO and all values expect positive 10 or greater indeed reset > > to 120. But I'm still seeing constant restarting even with these values. So > > patch does work as intended but doesn't solve the whole problem. I'll try to > > set up a SCI on my environment and test that too just to be sure. > > Do you have an Internet accessible Koha that can reproduce this problem? I > could take a look there. > > Otherwise, I think I've troubleshooted this as far as I can. All our Koha instances have restricted access. But I can try to take a look at this and see if what I can find.
David, you were on the right track with this. I ran code from bug 36586 through Copilot and it pointed out that variable const idle_timeout has to be converted as number also in function login_timeout found from sci-main.tt and sco-main.tt: const idleTimeout = Number("[% Koha.Preference('SelfCheckTimeout') | html %]"); This stopped the constant refreshing. Unfortunately with this bugs patch in place converting no longer helps no matter how many Number() methods I add :D
(In reply to Emmi Takkinen from comment #11) > David, you were on the right track with this. I ran code from bug 36586 > through Copilot and it pointed out that variable const idle_timeout has to > be converted as number also in function login_timeout found from sci-main.tt > and sco-main.tt: const idleTimeout = Number("[% > Koha.Preference('SelfCheckTimeout') | html %]"); This stopped the constant > refreshing. Unfortunately with this bugs patch in place converting no longer > helps no matter how many Number() methods I add :D Sorry I'm very confused about what you're trying to say here. It sounds like you're saying that the patch works, but that it also does not work?
I suppose it doesn't really matter anyway since bug 36586 has been reverted.