[Bug 36217] New: Jobs page include last hour filter does not work
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Bug ID: 36217 Summary: Jobs page include last hour filter does not work Change sponsored?: --- Product: Koha Version: 23.05 Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: System Administration Assignee: koha-bugs@lists.koha-community.org Reporter: ephetteplace@cca.edu QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com The "include_last_hour" checkbox labelled "Only include jobs enqueued in the last hour" on the Background Jobs page /cgi-bin/koha/admin/background_jobs.pl does not seem to work, it filters out all jobs regardless of the time they were enqueued. To create: - stage a MARC file for import (or do something else that creates a job) - visit Administration > Jobs page Expected: The job your just created should be listed. The filters "include_last_hour" and "only_current" ("Current jobs only", I don't actually know what this means, jobs that have not completed?) are on by default. Even removing the current filter, the job does not appear until you remove the include last hour filter. No job ever appears if include last hour is checked. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |emmi.takkinen@koha-suomi.fi |ity.org | CC| |emmi.takkinen@koha-suomi.fi Status|NEW |Needs Signoff --- Comment #1 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 167526 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=167526&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org --- Comment #2 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #3 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #167526|0 |1 is obsolete| | Status|Failed QA |Needs Signoff CC| |johanna.raisa@koha-suomi.fi Assignee|emmi.takkinen@koha-suomi.fi |johanna.raisa@koha-suomi.fi --- Comment #5 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- Created attachment 169190 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169190&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #6 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #7 from David Cook <dcook@prosentient.com.au> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- P.S. I'm excited to get this into the main branch. It's a bug that has been bugging me for soooo long. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169190|0 |1 is obsolete| | --- Comment #9 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- Created attachment 169549 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169549&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #10 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169549|0 |1 is obsolete| | --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Created attachment 169635 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169635&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #12 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169635|0 |1 is obsolete| | --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 169694 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169694&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #15 from Jonathan Druart <jonathan.druart@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #18 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171018 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171018&action=edit Revert "Bug 36217: Fix background jobs page's include_last_hour filter" This reverts commit f5c839348091d129bb7100af2c9e64e486d0ea1d. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #19 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171019 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171019&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #20 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171020 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171020&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #21 from Jonathan Druart <jonathan.druart@gmail.com> --- Can we reconsider this? I think my patch fixes it the correct way (without redoing the whole datetime/timezone handling in Koha). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|23.05 |unspecified -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37831 --- Comment #22 from Jonathan Druart <jonathan.druart@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #23 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 171019 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171019 Bug 36217: Use the server's tz to limit the jobs by "enqueued in the last hour" Review of attachment 171019: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36217&attachment=171019) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #24 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #25 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #26 from Jonathan Druart <jonathan.druart@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #27 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #28 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #29 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #30 from Jonathan Druart <jonathan.druart@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #31 from Jonathan Druart <jonathan.druart@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #32 from Julian Maurice <julian.maurice@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #34 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Can we perhaps cypress test this? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #35 from Jonathan Druart <jonathan.druart@gmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #36 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #35)
Eric, Emmi, Phil, David, would you agree with that?
And Johanna :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #37 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171018|0 |1 is obsolete| | --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 171297 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171297&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171019|0 |1 is obsolete| | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 171298 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171298&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171020|0 |1 is obsolete| | --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 171299 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171299&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37888 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37888 [Bug 37888] Default filtering of background jobs could be improved -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #41 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #42 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Still looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37831 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37831 [Bug 37831] Timezone ignored when passing rfc3339 formatted date -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Keywords|additional_work_needed | Status|ASSIGNED |RESOLVED Blocks|37831, 37888 |37905 --- Comment #43 from Jonathan Druart <jonathan.druart@gmail.com> --- Alternative approach moved to bug 37905. Please help there! Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37831 [Bug 37831] [OMNIBUS] Timezone ignored when passing rfc3339 formatted date https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37888 [Bug 37888] Default filtering of background jobs could be improved https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37905 [Bug 37905] Correctly fix the "last hour" filter on the job list -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36217 --- Comment #44 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #43)
Alternative approach moved to bug 37905. Please help there!
Ok going to look over there -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org