Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scheduling for negative timezones #1069

Merged

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Jan 15, 2025

And hopefully all other timezones. The date for a new scheduled event should now always be correct, no matter what hour/minute is selected.

Aims at fixing #1062.

How to test this

Can be tested as usual. Try scheduling events with many different dates, hours, minutes, durations and check if it works as intended. Switch timezones if possible.

@Arnei Arnei added the type:bug Something isn't working label Jan 15, 2025
@Arnei Arnei requested a review from gregorydlogan January 15, 2025 14:39
Copy link
Contributor

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 ghcr.io/opencast/opencast-admin-interface:pr-1069

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org ghcr.io/opencast/opencast-admin-interface:pr-1069

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

And hopefully all other timezones. The date for a
scheduled event should now always be correct,
no matter what hour/minute is selected.

Aims at fixing opencast#1062.
@Arnei Arnei force-pushed the fix-scheduling-for-negative-timezones branch from 1b4c565 to 72159e1 Compare January 15, 2025 14:42
Copy link
Contributor

This pull request is deployed at test.admin-interface.opencast.org/1069/2025-01-15_14-42-29/ .
It might take a few minutes for it to become available.

@gregorydlogan
Copy link
Member

Ok, super nitpicky here...

Single Event Scheduling:

  • Scheduling an event strictly in the future works
  • Scheduling an event that would currently in progress gives me 'The event has already ended'. We may not have a string for "Can't schedule things currently in progress", so this may have to be enough for now.
  • Scheduling an event strictly in the past gives me the correct error

Multiple Event Scheduling

  • Using the multiple event scheduling, I can events in the past :D
  • The modal does not prevent me from scheduling something for Monday and Tuesday, but restricting the date range to 25-01-16 -> 25-01-17. Upon submitting the scheduling request I get an immediate error saying the event could not be scheduled. Assuming at least one of the days and dates match, the even creation succeeds for those matching combination(s).
  • With an initial start time of 11:00, and end of 11:55, if I change the end time to 10 I get a duration of 23:55 without an indication if it's changing the end date
    • image
    • Changing the end time to 02 gets me a duration of 15 hrs, so I'm guessing it's incrementing the day, but that's not made clear like single event scheduling
    • Changing the start time such that it would cause (or undo) a wrap over a day similarly works correctly duration wise, but does not cause the end time to display the date next to it.
    • TBH, not sure scheduling multiples and going over the end of the day makes any sense whatsoever, but that's a different PR.

Have not tested agents in mismatched timezones (eg: user TZ != core TZ != agent TZ). Does not block this PR imo.

Summary: Fixes the issue, found a bunch of nits. Do you want to fix them here, or should I file follow-on issues?

@gregorydlogan gregorydlogan self-assigned this Jan 15, 2025
@Arnei
Copy link
Member Author

Arnei commented Jan 16, 2025

Being able to schedule events in the past at the very least sounds bad and sounds like it could have been caused by this PR, so I (or someone else) should take a look at that. Thanks for the review!

@Arnei
Copy link
Member Author

Arnei commented Jan 16, 2025

Did some quick testing, and it seems like the nitpicks you described already existed before this PR? In which case I would leave them for a separate PR, because this PR is not claiming to fix them and so this PR can get merged faster.

@gregorydlogan
Copy link
Member

I can file the relevant nitpicks as issues so they don't get lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants