Your answer was pretty close. There were a few problems:
You were assuming that today in a given time zone was the same date as today in UTC. Depending on time zone, these could be different days. For example, 1 AM UTC on 2019-10-18, is 8:00 PM in US Central Time on 2019-10-17.
If you design around "has it happened yet today", you'll potentially skip over legitimate occurrences. Instead, it's much easier to just think about "what is the next future occurrence".
You weren't doing anything to handle invalid or ambiguous local times, such as occur with the start or end of DST and with changes in standard time. This is important for recurring events.
So on to the code:
// Get the current UTC time just once at the start
var utcNow = DateTimeOffset.UtcNow;
foreach (var schedule in schedules)
{
// schedule notification only if not already scheduled in the future
if (schedule.LastScheduledDateTime == null || schedule.LastScheduledDateTime.Value < utcNow)
{
// Get the time zone for this schedule
var tz = TimeZoneInfo.FindSystemTimeZoneById(schedule.User.TimeZone);
// Decide the next time to run within the given zone's local time
var nextDateTime = nowInZone.TimeOfDay <= schedule.PreferredTime
? nowInZone.Date.Add(schedule.PreferredTime)
: nowInZone.Date.AddDays(1).Add(schedule.PreferredTime);
// Get the point in time for the next scheduled future occurrence
var nextOccurrence = nextDateTime.ToDateTimeOffset(tz);
// Do the scheduling
BackgroundJob.Schedule<INotificationService>(x => x.Notify(schedule.CompanyUserID), nextOccurrence);
// Update the schedule
schedule.LastScheduledDateTime = nextOccurrence;
}
}
I think you'll find that your code and data are much clearer if you make your LastScheduledDateTime
a DateTimeOffset?
instead of a DateTime?
. The above code assumes that. If you don't want to, then you can change that last line to:
schedule.LastScheduledDateTime = nextOccurrence.UtcDateTime;
Also note the use of ToDateTimeOffset
, which is an extension method. Place it in a static class somewhere. Its purpose is to create a DateTimeOffset
from a DateTime
taking a specific time zone into account. It applies typical scheduling concerns when dealing with ambiguous and invalid local times. (I last posted about it in this other Stack Overflow answer if you want to read more.) Here is the implementation:
public static DateTimeOffset ToDateTimeOffset(this DateTime dt, TimeZoneInfo tz)
{
if (dt.Kind != DateTimeKind.Unspecified)
{
// Handle UTC or Local kinds (regular and hidden 4th kind)
DateTimeOffset dto = new DateTimeOffset(dt.ToUniversalTime(), TimeSpan.Zero);
return TimeZoneInfo.ConvertTime(dto, tz);
}
if (tz.IsAmbiguousTime(dt))
{
// Prefer the daylight offset, because it comes first sequentially (1:30 ET becomes 1:30 EDT)
TimeSpan[] offsets = tz.GetAmbiguousTimeOffsets(dt);
TimeSpan offset = offsets[0] > offsets[1] ? offsets[0] : offsets[1];
return new DateTimeOffset(dt, offset);
}
if (tz.IsInvalidTime(dt))
{
// Advance by the gap, and return with the daylight offset (2:30 ET becomes 3:30 EDT)
TimeSpan[] offsets = { tz.GetUtcOffset(dt.AddDays(-1)), tz.GetUtcOffset(dt.AddDays(1)) };
TimeSpan gap = offsets[1] - offsets[0];
return new DateTimeOffset(dt.Add(gap), offsets[1]);
}
// Simple case
return new DateTimeOffset(dt, tz.GetUtcOffset(dt));
}
(In your case, the kind is always unspecified, so you could remove that first check if you want to, but I prefer to keep it fully functional in case of other usage.)
Incidentally, you don't need the if (!schedules.HasAny()) { return; }
check. Entity Framework already tests for changes during SaveChangesAsync
, and does nothing if there aren't any.