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

editoast: use DateTime with timezone in work schedule API #9656

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

emersion
Copy link
Member

@emersion emersion commented Nov 8, 2024

The timezone was stored in the database, but was stripped at the HTTP API boundary.

This is a breaking change: the timezone is required when submitting a work schedule, and is always returned when projecting a work schedule.

Closes: #9652

@emersion emersion requested a review from a team as a code owner November 8, 2024 10:16
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sincerely like that change ❤️

editoast/src/models/work_schedules.rs Outdated Show resolved Hide resolved
@emersion emersion force-pushed the emr/work-schedule-tz branch from f01986e to fd55d57 Compare November 8, 2024 10:50
@emersion emersion requested a review from a team as a code owner November 8, 2024 10:50
@emersion emersion requested a review from bloussou November 8, 2024 10:50
@emersion emersion force-pushed the emr/work-schedule-tz branch from fd55d57 to cf5690b Compare November 8, 2024 10:57
@emersion
Copy link
Member Author

emersion commented Nov 8, 2024

If this is deemed too intrusive for a bugfix, we could fix only the GET side of the API as a first step, and take more time to merge the POST side (and adapt the various users).

@emersion emersion force-pushed the emr/work-schedule-tz branch 2 times, most recently from 7063a02 to 506a107 Compare November 8, 2024 11:23
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@bloussou bloussou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check with @Morgane-SJK no problem we are already using TZ everywhere in elt code

The timezone was stored in the database, but was stripped at the
HTTP API boundary.

This is a breaking change: the timezone is required when submitting
a work schedule, and is always returned when projecting a work
schedule.

Signed-off-by: Simon Ser <contact@emersion.fr>
@emersion emersion force-pushed the emr/work-schedule-tz branch from 506a107 to f65d82c Compare November 15, 2024 14:00
@emersion emersion enabled auto-merge November 15, 2024 14:00
@emersion emersion added this pull request to the merge queue Nov 15, 2024
Merged via the queue into dev with commit a4ab1ee Nov 15, 2024
27 checks passed
@emersion emersion deleted the emr/work-schedule-tz branch November 15, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Work schedule projection does not contain timezone
4 participants