Description
Eric Phetteplace
2024-03-01 17:50:13 UTC
Created attachment 167526 [details] [review] Bug 36217: Fix background jobs page's include_last_hour filter Test plan: 1) Create a background job 2) Go to the background jobs page 3) See your job listed 4) Unselect "include_last_hour" from the filter 5) Verify that the job is not listed 6) Apply the patch 7) Repeat steps 1-3 8) Verify that the job is now listed What timezone are the values for Queued, Started, and Ended in? The (sad) answer is that Queued is in server-time, and Started and Ended are in db-server-time, and your patch only works if server-time is UTC and the browser is at-or-after UTC. I see this bug on my production server, where both server-time and db-server-time are my own timezone, and your patch would work fine. But in ktd by default, both times are UTC and I'm UTC+7 or UTC+8, and so I don't see the bug, because "an hour ago or more recently" nicely covers the fact that jobs are enqueued 7 or 8 hours after the local time I actually enqueue them. If I put TZ=America/Los_Angeles in ktd's .env, then server time matches my time, and I can see the bug without the patch, and the patch fixes the bug, though it's a little odd that jobs are started and finished 7 or 8 hours after they were enqueued, and in the future while I'm looking at the job page (apparently getting db-server-time set in Docker is a bit harder than just setting TZ in ENV). If I put TZ=Pacific/Honolulu in ktd's .env, then jobs are enqueued three hours before I actually created them, and the patch does not work. This issue is a pet peeve of mine but I haven't had time to address it. Marking this as Failed QA based on Phil's comments. We'll want to send UTC time and process as UTC time on the backend. Actually, it's a bit funny how "Current jobs only" and "Only include jobs enqueued in the last hour" are handled completely differently. We don't really need to send a timestamp even, since we know it's always going to be just the last hour. It would be better to have a "only_last_hour" parameter sent by "koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt" and picked up in "Koha/REST/V1/BackgroundJobs.pm" There we can generate a timestamp that is (now - 1 hour) with a timezone that matches our database connection (see "sub dbh" in Koha/Database.pm for more details about the db connection). Created attachment 169190 [details] [review] Fix background jobs page's include_last_hour filter Test plan: 1) Create a background job 2) Go to the background jobs page 3) See your job listed 4) Unselect "include_last_hour" from the filter 5) Verify that the job is not listed 6) Apply the patch 7) Repeat steps 1-3 8) Verify that the job is now listed Sponsored-by: Koha-Suomi Oy (In reply to David Cook from comment #4) > Actually, it's a bit funny how "Current jobs only" and "Only include jobs > enqueued in the last hour" are handled completely differently. We don't > really need to send a timestamp even, since we know it's always going to be > just the last hour. > > It would be better to have a "only_last_hour" parameter sent by > "koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt" and > picked up in "Koha/REST/V1/BackgroundJobs.pm" > > There we can generate a timestamp that is (now - 1 hour) with a timezone > that matches our database connection (see "sub dbh" in Koha/Database.pm for > more details about the db connection). I added a patch which now does what David suggested here. A boolean is passed to the rest endpoint and the filtering is made with database timezone. Great job here, Johanna. Thanks so much for writing this patch! It is failing the QA tools at the moment for tidiness[1] With the test plan, there are some missing steps. After applying the patch, the tester needs to run "perl build-resources.PL" to update the swagger_bundle.json. At least in koha-testing-docker. That's a fairly new thing though. Not a big issue there, but worth noting. The test plan (and the unit test) should also include some steps for verifying that the "Current jobs only" filter works in combination with the "Only include jobs enqueued in the last hour". I manually tested it, and it works (yay!), but good to have an automated test for it too. That shouldn't take you long. So marking this as Failed QA, but I think you'll have it fixed up quickly. If you're struggling with any of the above, let me know any I'll give you a hand. Thanks! -- [1] testing 1 commit(s) (applied to 5c8baaf '61 Bug 37302: (follow-up) Update yarn') Processing files before patches |========================>| 5 / 5 (100.00%) Processing files after patches |========================>| 5 / 5 (100.00%) WARN Koha/BackgroundJobs.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 9, now: 14) WARN Koha/REST/V1/BackgroundJobs.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 7, now: 8) OK api/v1/swagger/paths/jobs.yaml OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt WARN t/db_dependent/Koha/BackgroundJobs.t WARN tidiness The file is less tidy than before (bad/messy lines before: 19, now: 21) Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - 6b17c7d888 Running tests (1) * Proving /kohadevbox/koha/t/db_dependent/Koha/BackgroundJobs.t OK! P.S. I'm excited to get this into the main branch. It's a bug that has been bugging me for soooo long. Created attachment 169549 [details] [review] Bug 36217: Fix background jobs page's include_last_hour filter Test plan: 1) Create a background job 2) Go to the background jobs page 3) See your job listed 4) Unselect "include_last_hour" from the filter 5) Verify that the job is not listed 6) Apply the patch 7) perl build-resources.PL 8) Repeat steps 1-3 9) Verify that the job is now listed 10) prove t/db_dependent/Koha/BackgroundJobs.t 11) Verify that the tests pass Sponsored-by: Koha-Suomi Oy (In reply to David Cook from comment #7) > Great job here, Johanna. Thanks so much for writing this patch! > > It is failing the QA tools at the moment for tidiness[1] > > With the test plan, there are some missing steps. After applying the patch, > the tester needs to run "perl build-resources.PL" to update the > swagger_bundle.json. At least in koha-testing-docker. That's a fairly new > thing though. Not a big issue there, but worth noting. > > The test plan (and the unit test) should also include some steps for > verifying that the "Current jobs only" filter works in combination with the > "Only include jobs enqueued in the last hour". I manually tested it, and it > works (yay!), but good to have an automated test for it too. That shouldn't > take you long. > > So marking this as Failed QA, but I think you'll have it fixed up quickly. > If you're struggling with any of the above, let me know any I'll give you a > hand. Thanks! > > -- > > [1] > testing 1 commit(s) (applied to 5c8baaf '61 Bug 37302: (follow-up) Update > yarn') > > Processing files before patches > |========================>| 5 / 5 (100.00%) > Processing files after patches > |========================>| 5 / 5 (100.00%) > > WARN Koha/BackgroundJobs.pm > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 9, now: 14) > > WARN Koha/REST/V1/BackgroundJobs.pm > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 7, now: 8) > > OK api/v1/swagger/paths/jobs.yaml > > OK koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt > > WARN t/db_dependent/Koha/BackgroundJobs.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 19, now: 21) > > > Processing additional checks > > * Commit title does not start with 'Bug XXXXX: ' - 6b17c7d888 > > Running tests (1) > * Proving /kohadevbox/koha/t/db_dependent/Koha/BackgroundJobs.t OK! Now the patch is updated. It should pass the QA tool, also fixed the test plan and I added more tests. Created attachment 169635 [details] [review] Bug 36217: Fix background jobs page's include_last_hour filter Test plan: 1) Create a background job 2) Go to the background jobs page 3) See your job listed 4) Unselect "include_last_hour" from the filter 5) Verify that the job is not listed 6) Apply the patch 7) perl build-resources.PL 8) Repeat steps 1-3 9) Verify that the job is now listed 10) prove t/db_dependent/Koha/BackgroundJobs.t 11) Verify that the tests pass Sponsored-by: Koha-Suomi Oy Signed-off-by: David Cook <dcook@prosentient.com.au> (In reply to Johanna Räisä from comment #10) > Now the patch is updated. It should pass the QA tool, also fixed the test > plan and I added more tests. Beautiful! Thanks for that! If someone else signs this off, I'd be happy to upgrade this to Passed QA, but just signing off for now. Created attachment 169694 [details] [review] Bug 36217: Fix background jobs page's include_last_hour filter Test plan: 1) Create a background job 2) Go to the background jobs page 3) See your job listed 4) Unselect "include_last_hour" from the filter 5) Verify that the job is not listed 6) Apply the patch 7) perl build-resources.PL 8) Repeat steps 1-3 9) Verify that the job is now listed 10) prove t/db_dependent/Koha/BackgroundJobs.t 11) Verify that the tests pass Sponsored-by: Koha-Suomi Oy Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed for 24.11! Well done everyone, thank you! I don't have time at the moment to review this, but it feels like adding a new 'only_last_hour' flag server-side is wrong. The previous way was more flexible. Why didn't it work? I have not found the reason in the comments. (In reply to Jonathan Druart from comment #15) > I don't have time at the moment to review this, but it feels like adding a > new 'only_last_hour' flag server-side is wrong. It's much better for the status quo. If we did want to make the time adjustable (so not just the last hour) then it would be wrong/bad. But for now I think it makes a lot of sense, as it is easy to understand/maintain and reduces complexity. > The previous way was more flexible. Why didn't it work? I have not found the > reason in the comments. If I recall correctly, it's because the client/browser sent the string in UTC, and the API server processed it according to the server local time zone or the DB's local time zone. So on koha-testing-docker, it would work because it uses UTC for the local time zone. But for most servers in the real world it wouldn't work because they used their own local time. Of course, an alternative fix would've been to just treat the incoming timestamp as UTC and then convert it, but datetime math can be complicated. That would've been a more flexible solution, but also a more complicated one. (In reply to David Cook from comment #16) > Of course, an alternative fix would've been to just treat the incoming > timestamp as UTC and then convert it, but datetime math can be complicated. > That would've been a more flexible solution, but also a more complicated one. Of course, now that I look at it again, Jonathan might have a point. We solved this 1 problem, but the real problem is that the API server handles all incoming dates according to the DB's local time zone (if only using DBIx::Class and no additional handling). I've said for years that storing timestamps in a local timezone in Koha was madness/problematic, but we've mostly just ignored that warning. Of course, the problem is so systemic that I don't know how feasible it is to actually fix it in Koha. Yet, expecting every API consumer to know the timezone of the DB server is also not great... There might be some DBIx::Class magic we can do to automatically detect any date field and automatically convert timezones, but I hate magic. It ends up being messy and difficult to maintain. I think that's why we went with this solution for this bug. The alternative was just too overwhelming. Created attachment 171018 [details] [review] Revert "Bug 36217: Fix background jobs page's include_last_hour filter" This reverts commit f5c839348091d129bb7100af2c9e64e486d0ea1d. Created attachment 171019 [details] [review] Bug 36217: Use the server's tz to limit the jobs by "enqueued in the last hour" This patch suggests to revert the previous attempt to fix this problem. The date was using the client-side's tz and so the "last hour" was not matching the one from the server. I have noticed a couple of things when working on this: * We cannot pass a RFC3339 formatted date to the server (will open a separate bug report) * toISOString() and the handling of timezone in dayjs is ambiguous or buggy https://github.com/iamkun/dayjs/issues/2720 https://github.com/iamkun/dayjs/issues/2611 Created attachment 171020 [details] [review] Bug 36217: Fix parameter name It's named enqueued_date on the API side, not enqueued_on (which is the DB column's name) No behaviour change expected here. Can we reconsider this? I think my patch fixes it the correct way (without redoing the whole datetime/timezone handling in Koha). (In reply to Jonathan Druart from comment #19) > I have noticed a couple of things when working on this: > * We cannot pass a RFC3339 formatted date to the server (will open a > separate bug report) See bug 37831. Comment on attachment 171019 [details] [review] Bug 36217: Use the server's tz to limit the jobs by "enqueued in the last hour" Review of attachment 171019 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt @@ +264,4 @@ > let now = new Date(); > if ( $("#include_last_hour").is(":checked") ) { > now.setHours(now.getHours() - 1); > + let now_with_server_tz = $datetime(now.toISOString(), { dateformat: 'iso' }); Can you speak to this more? Why do you call it "now_with_server_tz"? The server TZ isn't necessarily in UTC. That's the bug that is trying to be fixed. If I understand this patch correctly, this doesn't fix the problem. (In reply to Jonathan Druart from comment #19) > This patch suggests to revert the previous attempt to fix this problem. > The date was using the client-side's tz and so the "last hour" was not > matching the one from the server. The problem isn't the client side TZ. The problem is the server side TZ. In ktd, the server side TZ is UTC, so the unpatched version works as expected. However, in practice, many servers are set to TZs like Australia/Sydney or America/Montreal. When that happens, the feature works very badly, because the wrong time is used. (In reply to Jonathan Druart from comment #21) > Can we reconsider this? I think my patch fixes it the correct way (without > redoing the whole datetime/timezone handling in Koha). Happy to support alternatives of course though. Please test the patch, it addresses the issues correctly (I think!) Did you look at js-date-format.inc? $datetime is dealing with the server's timezone. To test it: Set timezone to America/Argentina/Buenos_Aires, which is UTC-3 (in $KOHA_CONF) Client is CEST (UTC+2) I am doing my test now, it is 4:20 in Buenos Aires 7:20 UTC 9:20 CEST In the console: let now = new Date(); now Date Thu Sep 05 2024 09:20:38 GMT+0200 (Central European Summer Time) now.toISOString() "2024-09-05T07:20:38.077Z" $datetime(now.toISOString(), { dateformat: 'iso' }); "2024-09-05T04:20:38.3838-03:00" Which is what we want to send to the server. (In reply to David Cook from comment #24) > (In reply to Jonathan Druart from comment #19) > > This patch suggests to revert the previous attempt to fix this problem. > > The date was using the client-side's tz and so the "last hour" was not > > matching the one from the server. > > The problem isn't the client side TZ. The problem is the server side TZ. > > In ktd, the server side TZ is UTC, so the unpatched version works as > expected. > > However, in practice, many servers are set to TZs like Australia/Sydney or > America/Montreal. > > When that happens, the feature works very badly, because the wrong time is > used. I have been working on date handling in Koha for a while, no need to explain more the problem, I totally understood it. Hence the patch and my suggestion to revert what has been done :D (In reply to Jonathan Druart from comment #26) > Please test the patch, it addresses the issues correctly (I think!) > > Did you look at js-date-format.inc? > > $datetime is dealing with the server's timezone. > > To test it: > Set timezone to America/Argentina/Buenos_Aires, which is UTC-3 (in > $KOHA_CONF) > Client is CEST (UTC+2) > > I am doing my test now, it is > 4:20 in Buenos Aires > 7:20 UTC > 9:20 CEST > > In the console: > let now = new Date(); > now > Date Thu Sep 05 2024 09:20:38 GMT+0200 (Central European Summer Time) > > now.toISOString() > "2024-09-05T07:20:38.077Z" > > $datetime(now.toISOString(), { dateformat: 'iso' }); > "2024-09-05T04:20:38.3838-03:00" I added UTC-3 to the timezone and tested this. now = Thu Sep 05 2024 13:16:55 GMT+0300 (Eastern European Summer Time) now_with_server_tz = 2024-09-05T13:16:55.5555+03:00 I'm not sure if there is some other setting I should add, but I got the same result after adding UTC-3 to $KOHA_CONF (In reply to Johanna Räisä from comment #27) > (In reply to Jonathan Druart from comment #26) > > Please test the patch, it addresses the issues correctly (I think!) > > > > Did you look at js-date-format.inc? > > > > $datetime is dealing with the server's timezone. > > > > To test it: > > Set timezone to America/Argentina/Buenos_Aires, which is UTC-3 (in > > $KOHA_CONF) > > Client is CEST (UTC+2) > > > > I am doing my test now, it is > > 4:20 in Buenos Aires > > 7:20 UTC > > 9:20 CEST > > > > In the console: > > let now = new Date(); > > now > > Date Thu Sep 05 2024 09:20:38 GMT+0200 (Central European Summer Time) > > > > now.toISOString() > > "2024-09-05T07:20:38.077Z" > > > > $datetime(now.toISOString(), { dateformat: 'iso' }); > > "2024-09-05T04:20:38.3838-03:00" > > > I added UTC-3 to the timezone and tested this. > > now = Thu Sep 05 2024 13:16:55 GMT+0300 (Eastern European Summer Time) > now_with_server_tz = 2024-09-05T13:16:55.5555+03:00 > > I'm not sure if there is some other setting I should add, but I got the same > result after adding UTC-3 to $KOHA_CONF Had to add time zones to mysql to get this work. (In reply to Jonathan Druart from comment #26) > Did you look at js-date-format.inc? Not closely enough it seems. Looking at the full file now... and I don't love it. It's a hack, but I can regrettably see the usefulness of the hack considering all the other hacks and other problems it is piled on top of... Ideally, we should be working with UTC timestamps. Until we do... we're just going to keep using hacks and it's going to harm us in the long-term. > I have been working on date handling in Koha for a while, no need to explain > more the problem, I totally understood it. Hence the patch and my suggestion > to revert what has been done :D I think that I misunderstood what you were saying. My apologies for re-explaining the problem. That said, I'm not sure why you want to fix a fixed problem, and I don't think this is the right way of doing it. But... if it's how you want to spend your time, that's up to you. (In reply to David Cook from comment #29) > That said, I'm not sure why you want to fix a fixed problem, and I don't > think this is the right way of doing it. > > But... if it's how you want to spend your time, that's up to you. The time is spent already, the patch is attached and fixed the problem raised by this bug report. My point is that it's not fixed correctly, and we need to keep the flexibility the implementation brought. Also we don't want to see new attributes added to the REST API when we want to search on dates, we simply need to use this method. Introducing a pattern that is limited (only search on "last hour") and bring a lot of new code to maintain is not something we should promote IMO. Especially when it can be replaced with 2 lines of JS code. Please explain a bit more why you don't like it. Yes we need to rework the whole datetime handling in Koha, but it's a totally separate discussion (that I will be happy to have, and to work on it if people wants it). I also prefer to spend a bit more time now than a lot more later. What will we do when one will want to add a "show more jobs from the last day" checkboxes? A couple of JS lines vs a new flag at the REST API level. My thoughts on this: I don't like the two available filters and the fact that they are enabled by default. 99% of the time I see an empty table because the jobs I just launched are already finished by the time I go to this page or because the job I want to check was queued more than an hour ago. Why don't we display the last X queued jobs by default ? I don't want to see unfinished jobs or "queued less than an hour ago" jobs. I want to see the more recent jobs. An option to filter out finished jobs is useful but in my case it would be less annoying if it was disabled by default. However, the 1 hour filter is arbitrary (why 1 hour ?) and not so useful in my opinion, as long as we can sort by queued date/time Looking again here.. sorry David, I know we spoke about this before but I do actually agree with Jonathan here. I'd like to see the API maintain consistency.. adding in extra params when there's already a 'standard' for searching against datetime fields adds confusion and inconsistency in my opinion. At the time we discussed it however, I wasn't able to look at the code.. I'm glad to see Jonathan's gotten to the bottom of the query problem. Looking in detail now. As for Julians comment.. I think that's a great idea.. time here is a bit arbitrary.. I like the idea of X most recent jobs and then optional filters on status as an alternative. Can we perhaps cypress test this? The checkbox is there since: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30462#c19 It seems that the goal was to limit the number of jobs for perf purpose. Since we now have the table using the REST API maybe Julian is correct and we could simply remove the checkbox. Eric, Emmi, Phil, David, would you agree with that? (In reply to Jonathan Druart from comment #35) > Eric, Emmi, Phil, David, would you agree with that? And Johanna :) (In reply to Jonathan Druart from comment #36) > (In reply to Jonathan Druart from comment #35) > > Eric, Emmi, Phil, David, would you agree with that? > > And Johanna :) I agree with Julian. Filters are not really userful, since the table usually is empty. I was thinking that maybe showing the jobs for current day would be more user friendly and add a datepicker to change the date. Created attachment 171297 [details] [review] Revert "Bug 36217: Fix background jobs page's include_last_hour filter" This reverts commit f5c839348091d129bb7100af2c9e64e486d0ea1d. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 171298 [details] [review] Bug 36217: Use the server's tz to limit the jobs by "enqueued in the last hour" This patch suggests to revert the previous attempt to fix this problem. The date was using the client-side's tz and so the "last hour" was not matching the one from the server. I have noticed a couple of things when working on this: * We cannot pass a RFC3339 formatted date to the server (will open a separate bug report) * toISOString() and the handling of timezone in dayjs is ambiguous or buggy https://github.com/iamkun/dayjs/issues/2720 https://github.com/iamkun/dayjs/issues/2611 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 171299 [details] [review] Bug 36217: Fix parameter name It's named enqueued_date on the API side, not enqueued_on (which is the DB column's name) No behaviour change expected here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Passed QA on the follow-ups that revert the original and fix the datetime in js correctly. Opened a new bug 37888 to work on Julians suggestion. Still looking here Alternative approach moved to bug 37905. Please help there! (In reply to Jonathan Druart from comment #43) > Alternative approach moved to bug 37905. Please help there! Ok going to look over there |