-
Notifications
You must be signed in to change notification settings - Fork 1
Add schedule timezone column #344
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema upgrade files are also required. Please consider some useful default for migration.
Please take a look at #324, where such files are already being created.
|
The default an be inferred from the first |
|
@nilmerg not sure if I understand this right, but the Edit: Or do you mean to select the timezone value from the first |
5da9a2d to
7830d9e
Compare
Exactly. |
51a9f85 to
1cc5e6d
Compare
The value must not be null, so we have to set a default when upgrading the schema. For that we look for the first `timeperiod_entry` and use it's `timezone` column. If no entry exists, the fallback is **'UTC'**.
1cc5e6d to
d974095
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is fine and can be merged any time the corresponding Notifications Web PR is ready. Though it probably shouldn't merged earlier as the added column is required without a default, hence without the corresponding change in Notifications Web, older version can't insert new rows anymore.
Thus, I've converted the PR to a draft for the moment (so there's a hint that it maybe shouldn't be merged immediately despite this approval) with a corresponding note in the PR description.
Add a column to store the timezone the schedule is created for.
Draft until the following PR is merged: