Skip to content

Conversation

@cameronhargreaves1-nhs
Copy link
Contributor

@cameronhargreaves1-nhs cameronhargreaves1-nhs commented Dec 15, 2025

Description

Because clinic and clinic slot start times are both datetime, it is currently possible to have a clinic slot that starts on a different day to the clinic, which means having a date on the clinic slot start time is redundant.

This PR starts the transition from clinic slot time being a datetime field to a TimeField, for a future PR to remove the datetime field altogether.

Jira link

https://nhsd-jira.digital.nhs.uk/browse/DTOSS-9752

Review notes

Review checklist

  • Check database queries are correctly scoped to current_provider

@cameronhargreaves1-nhs cameronhargreaves1-nhs force-pushed the DTOSS-9752-clinic-start-time-bug branch 3 times, most recently from 8ec1ed2 to 5c72140 Compare December 15, 2025 14:41
@cameronhargreaves1-nhs cameronhargreaves1-nhs changed the title [DTOSS-9752] Change ClinicSlot starts_at field from datetime to time [DTOSS-9752] add starts_at to ClinicSlot Dec 15, 2025
@cameronhargreaves1-nhs cameronhargreaves1-nhs marked this pull request as ready for review December 15, 2025 14:48
@cameronhargreaves1-nhs cameronhargreaves1-nhs requested a review from a team December 15, 2025 14:48
"clinics.Clinic", on_delete=models.PROTECT, related_name="clinic_slots"
)
starts_at = models.DateTimeField()
starts_at_time = models.TimeField()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make this nullable in this first migration so as we don't get the default value set. It probably doesn't matter as we don't have any existing data, well, we do but it's demo data and we're going to repopulate it anyway but good practice to get into I think. Make it nullable -> populate them all -> make it un-nullable in the subsequent PR when we get rid of starts_at.

ClinicSlots are created in the context of a Clinic which already contains the date, meaning the started_at datetime is redundant information, and may cause issues if the two are not lined up. This PR adds a started_at_time field which is a timefield to remove that issue. This is the first step to migrating to just time, the next PR will remove the started_at datetime field.
@cameronhargreaves1-nhs cameronhargreaves1-nhs force-pushed the DTOSS-9752-clinic-start-time-bug branch from 5c72140 to 0135881 Compare December 16, 2025 11:53
@gpeng
Copy link
Contributor

gpeng commented Dec 16, 2025

Just had a thought about this... If we only store Time is it going to make handling displaying the local time correctly a pain? We'd still need the date from the Clinic to localise the time from UTC wouldn't we? @MatMoore any thoughts? Maybe just adding validation would be a simpler approach after all.

@MatMoore
Copy link
Contributor

Just had a thought about this... If we only store Time is it going to make handling displaying the local time correctly a pain? We'd still need the date from the Clinic to localise the time from UTC wouldn't we? @MatMoore any thoughts? Maybe just adding validation would be a simpler approach after all.

Hmmm I think there are complications here for sure.

  • Only datetime objects have an associated timezone, and I don't think TimeField does any timezone conversions automatically. If we decide to store a UTC time in the TimeField, then yeah, we'd need to fetch the date from the Clinic record in order to present it properly (taking into account daylight savings). I don't think that's a good idea.
  • We could store the local time in the TimeField, but that means not using UTC consistently in the DB, and it will be more fiddly to convert to an unambiguous datetime later on.

So yeah I think you're probably right that the validation method makes more sense.

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.

3 participants