fix(lock): use UTC for the freeze, and stop three fail-open paths and two silent skips

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DayLockHelperTests.cs

@@ -169,16 +169,27 @@ public void IsLocked_IgnoresTimeOfDay()
             Is.True, "the boundary day is locked for its whole length");
     }
 
+    /// <summary>
+    /// UtcNow, not Now, on BOTH sides, because CanReconcile reads UtcNow. The
+    /// shipped container has no TZ set so the two agree there; on a dev machine
+    /// they diverge, and then DateTime.Now here breaks in either direction:
+    ///  - UTC+N, just after local midnight (Now.Date is a day AHEAD of
+    ///    UtcNow.Date): the "yesterday is reconcilable" assertion fails, because
+    ///    Now.Date.AddDays(-1) IS UtcNow.Date, i.e. still today in UTC.
+    ///  - UTC-N, late in the local evening (Now.Date is a day BEHIND): the
+    ///    "today is not reconcilable" assertion fails instead, because Now.Date
+    ///    is already yesterday in UTC and so genuinely reconcilable.
+    /// </summary>
     [Test]
     public void CanReconcile_TodayAndFuture_False_Past_True()
     {
         Assert.Multiple(() =>
         {
-            Assert.That(DayLockHelper.CanReconcile(DateTime.Now.Date), Is.False,
+            Assert.That(DayLockHelper.CanReconcile(DateTime.UtcNow.Date), Is.False,
                 "today must stay open so time can still be registered");
-            Assert.That(DayLockHelper.CanReconcile(DateTime.Now.Date.AddDays(1)), Is.False);
-            Assert.That(DayLockHelper.CanReconcile(DateTime.Now.Date.AddDays(-1)), Is.True);
-            Assert.That(DayLockHelper.CanReconcile(DateTime.Now.Date.AddHours(23)), Is.False,
+            Assert.That(DayLockHelper.CanReconcile(DateTime.UtcNow.Date.AddDays(1)), Is.False);
+            Assert.That(DayLockHelper.CanReconcile(DateTime.UtcNow.Date.AddDays(-1)), Is.True);
+            Assert.That(DayLockHelper.CanReconcile(DateTime.UtcNow.Date.AddHours(23)), Is.False,
                 "a time-of-day on today is still today");
         });
     }

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs

@@ -384,10 +384,28 @@ public async Task Reconcile_APastDay_SetsFlagAndTimestamp()
         });
     }
 
+    /// <summary>
+    /// UtcNow, deliberately: this date is fed to CanReconcile, which compares
+    /// against UtcNow.Date, and it sits exactly ON that boundary -- so a local
+    /// clock an offset away from UTC flips the outcome.
+    ///
+    /// The fixture's other DateTime.Now dates stay as they are, including the
+    /// many that DO reach CanReconcile via _service.Reconcile. Every date that
+    /// reaches it is -3 or older, or +3 forward, and the largest real UTC offset
+    /// is 14 hours, so no offset can move one across the boundary. The file's -1
+    /// dates never reach CanReconcile: they are open days and window bounds,
+    /// tested by IsLocked against a boundary the fixture stored on the same
+    /// clock. Only a date sitting exactly ON the boundary can be flipped, and
+    /// this test holds the only one.
+    ///
+    /// Reconcile_AFutureDay_IsRejected below moved too, even though +3 days is
+    /// safe at any offset, so the pair of tests naming this one rule reads on
+    /// one clock rather than two.
+    /// </summary>
     [Test]
     public async Task Reconcile_Today_IsRejected()
     {
-        var row = await SeedPlain(901, DateTime.Now.Date);
+        var row = await SeedPlain(901, DateTime.UtcNow.Date);
 
         var result = await _service.Reconcile(row.Id);
 
@@ -399,7 +417,7 @@ public async Task Reconcile_Today_IsRejected()
     [Test]
     public async Task Reconcile_AFutureDay_IsRejected()
     {
-        var row = await SeedPlain(902, DateTime.Now.Date.AddDays(3));
+        var row = await SeedPlain(902, DateTime.UtcNow.Date.AddDays(3));
 
         var result = await _service.Reconcile(row.Id);
 
@@ -876,6 +894,38 @@ public async Task UpdateWorkingHour_Kiosk_AnOlderReconciledDay_ReturnsDayIsRecon
         Assert.That(result.Message, Is.EqualTo("DayIsReconciled"));
     }
 
+    /// <summary>
+    /// The lock's unknown case must REFUSE, not permit. The kiosk overload takes
+    /// an int? site id with no null guard of its own, so before the fix a null
+    /// sailed straight past the guard (RefuseIfNotWritableAsync's null answer
+    /// means "writable, proceed") and then died on a `sdkSiteId!.Value`
+    /// dereference further down. Now it answers with a message and writes
+    /// nothing.
+    ///
+    /// Note what the null case is NOT: with no site id the lock is never
+    /// evaluated at all, so relaxing the refusal would not "let a locked day
+    /// through" -- it would crash on that dereference instead, which is how the
+    /// bug presented. The seeded boundary is here to make the scenario realistic
+    /// (a real kiosk posting into a frozen period), not because the assertion
+    /// depends on the day being locked.
+    /// </summary>
+    [Test]
+    public async Task UpdateWorkingHour_Kiosk_WithNoSiteId_IsRefused_AndWritesNothing()
+    {
+        var token = await SeedKioskDeviceAsync();
+        await SeedReconciledBoundaryAsync(934, DateTime.Now.Date.AddDays(-3));
+        var lockedDate = DateTime.Now.Date.AddDays(-4);
+
+        var result = await BuildWorkingHoursService().UpdateWorkingHour(
+            null, new TimePlanningWorkingHoursUpdateModel { Date = lockedDate }, token);
+
+        Assert.That(result.Success, Is.False);
+        Assert.That(result.Message, Is.EqualTo("SiteNotFound"));
+        Assert.That(await TimePlanningPnDbContext!.PlanRegistrations
+            .AnyAsync(x => x.Date == lockedDate), Is.False,
+            "an unresolvable site must not create a row on a locked day");
+    }
+
     [Test]
     public async Task UpdateWorkingHour_Personal_TheBoundaryDay_ReturnsDayIsReconciled()
     {

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursImportRemovedRowTests.cs

@@ -48,6 +48,11 @@ public async Task SetUpTest()
 
         var localizationService = Substitute.For<ITimePlanningLocalizationService>();
         localizationService.GetString(Arg.Any<string>()).Returns(x => x[0]?.ToString());
+        // The format overload echoes its arguments, so a test can assert what a
+        // message NAMES (here: how many locked days the import left alone), not
+        // just which key it used.
+        localizationService.GetString(Arg.Any<string>(), Arg.Any<object[]>())
+            .Returns(x => x[0] + "|" + string.Join("|", (object[])x[1]));
 
         _coreService = Substitute.For<IEFormCoreService>();
         var core = await GetCore();
@@ -114,6 +119,8 @@ public async Task Import_SkipsRemovedRow_AndDoesNotCrashOnRemovedActivePair()
 
         // Post-fix: no crash, and the ACTIVE row is the import target.
         Assert.That(result.Success, Is.True, result.Message);
+        Assert.That(result.Message, Is.EqualTo("Imported"),
+            "an import that skipped nothing says only that it imported");
 
         var reloadedActive = await TimePlanningPnDbContext.PlanRegistrations
             .AsNoTracking().FirstAsync(x => x.Id == activeId);
@@ -142,6 +149,11 @@ public async Task Import_SkipsRemovedRow_AndDoesNotCrashOnRemovedActivePair()
     /// day (it drops dates before now minus one day, which by time of day also
     /// drops yesterday), so no boundary that satisfies I2 is reachable. The
     /// lock itself does not check I2; this pins the skip as defense in depth.
+    ///
+    /// It also pins that the skip is REPORTED. A silent skip is what makes
+    /// someone re-import a corrected timesheet over a reconciled month forever:
+    /// they are told "Imported", they see the old numbers, and nothing in the
+    /// result says which days did not move.
     /// </summary>
     [Test]
     public async Task Import_SkipsALockedDay_AndImportsTheRest()
@@ -178,6 +190,10 @@ public async Task Import_SkipsALockedDay_AndImportsTheRest()
         var result = await _service.Import(FormFile(xlsx));
 
         Assert.That(result.Success, Is.True, result.Message);
+        // The mock echoes "key|arg", so this asserts both the message and the
+        // count it names -- one locked day, not "some".
+        Assert.That(result.Message, Is.EqualTo("Imported ImportLockedDaysSkipped|1"),
+            "the result must say how many days the lock left unchanged");
 
         var lockedAfter = await TimePlanningPnDbContext.PlanRegistrations
             .AsNoTracking().FirstAsync(x => x.Id == boundary.Id);
@@ -192,6 +208,50 @@ public async Task Import_SkipsALockedDay_AndImportsTheRest()
         Assert.That(openRow.PlanText, Is.EqualTo("IMPORTED-OPEN"), "the open day is still imported");
     }
 
+    /// <summary>
+    /// A sheet whose name DOES match a worker, but whose worker has no
+    /// MicrotingUid, is skipped whole AND reported. Skipping it is the lock
+    /// requirement -- an unreadable boundary makes IsLocked false for every row,
+    /// so importing it would import the sheet with no lock check at all. But a
+    /// silent skip here would be its own bug: the name matched, so the user
+    /// believes that worker's sheet went in, and a plain success would leave
+    /// them re-importing a file that never lands.
+    ///
+    /// Distinct from a sheet matching NO worker, which stays deliberately
+    /// silent -- such a tab may not be about a worker at all.
+    /// </summary>
+    [Test]
+    public async Task Import_ASheetWhoseWorkerHasNoMicrotingUid_IsSkippedAndReported()
+    {
+        const string siteName = "ImportNoUidSite";
+        var importDate = DateTime.Now.AddDays(5).Date;
+
+        var core = await _coreService.GetCore();
+        var sdkDbContext = core.DbContextHelper.GetDbContext();
+        // Name matches the worksheet; MicrotingUid deliberately absent. Cleared
+        // AFTER Create and then re-read, so the arrange cannot quietly test the
+        // wrong thing if Create ever starts back-filling a uid of its own.
+        var site = new SdkSite { Name = siteName, MicrotingUid = null };
+        await site.Create(sdkDbContext);
+        site.MicrotingUid = null;
+        await sdkDbContext.SaveChangesAsync();
+        Assert.That((await sdkDbContext.Sites.AsNoTracking().FirstAsync(x => x.Id == site.Id))
+            .MicrotingUid, Is.Null, "arrange: the site must really have no MicrotingUid");
+
+        var xlsx = BuildWorkbook(siteName,
+            (importDate.ToString("dd.MM.yyyy"), "8", "SHOULD-NOT-LAND"));
+
+        var result = await _service.Import(FormFile(xlsx));
+
+        Assert.That(result.Success, Is.True, result.Message);
+        Assert.That(result.Message, Is.EqualTo("Imported ImportUnresolvableSheetsSkipped|1"),
+            "the result must name the sheet it could not import");
+
+        Assert.That(await TimePlanningPnDbContext.PlanRegistrations
+                .AnyAsync(x => x.Date == importDate), Is.False,
+            "a sheet that cannot be lock-checked must not be imported at all");
+    }
+
     private static IFormFile FormFile(byte[] xlsx)
     {
         var file = Substitute.For<IFormFile>();

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/DayLockHelper.cs

@@ -131,10 +131,29 @@ public static async Task<string> LockedMessageKeyAsync(
     /// registered. This is also what makes the forward flex cascades unable to
     /// reach a locked day -- see the design doc before relaxing it.
     ///
-    /// DateTime.Now, not UtcNow: PlanRegistration.Date is a local midnight, and
-    /// the existing mobile guard compares the same way.
+    /// DateTime.UtcNow, not Now. PlanRegistration.Date is a calendar-day LABEL
+    /// with the time zeroed, not an instant in any timezone, so there is no
+    /// local midnight for a local clock to be "consistent" with. Writers do not
+    /// even agree on how they build that label -- most zero a constructed date,
+    /// but TimePlanningFlexService derives one from DateTime.Now -- which is a
+    /// reason to pin THIS comparison to one clock, not to follow theirs.
+    ///
+    /// Direction, stated as what it is rather than as a law: at a POSITIVE
+    /// offset (CET/CEST, where this product runs) UtcNow is the conservative
+    /// choice -- late in the local day it briefly declines to freeze a day that
+    /// is still "today" in UTC, and for a rule whose whole purpose is "this day
+    /// can no longer be written", refusing too much beats allowing too much. At
+    /// a NEGATIVE offset the same expression is the PERMISSIVE one: it would
+    /// accept the local today and break I2 from the worker's point of view.
+    /// That case does not arise here, and the answer if it ever does is an
+    /// explicit business timezone, not a switch back to the server's clock.
+    ///
+    /// This repo's Dockerfile sets no TZ and installs no tzdata, and the
+    /// mcr.microsoft.com/dotnet/aspnet base image it runs on defaults to UTC, so
+    /// the container this plugin ships in compares the same way before and after
+    /// this change; it only makes dev machines behave like the container.
     /// </summary>
-    public static bool CanReconcile(DateTime date) => date.Date < DateTime.Now.Date;
+    public static bool CanReconcile(DateTime date) => date.Date < DateTime.UtcNow.Date;
 
     /// <summary>
     /// What counts as a boundary row, in one place: Reconciled and not

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/GoogleSheetHelper.cs

@@ -355,8 +355,14 @@ public static async Task PullEverythingFromGoogleSheet(Core core, TimePlanningPn
 
                     // Decided before the row is even loaded, so a locked row is
                     // never tracked and no later save in this loop can flush it.
-                    if (site.MicrotingUid is { } lockSiteUid
-                        && DayLockHelper.IsLocked(lockedThroughBySite, lockSiteUid, dateValue))
+                    //
+                    // `!` deliberately: sitesByKey filters MicrotingUid != null
+                    // (the Where above), and `workers` is built only from
+                    // sitesByKey, so a null cannot reach this line. Written as a
+                    // null-tolerant check it would not be defensive -- it would
+                    // silently skip the lock check for a value that cannot
+                    // occur, which is the one outcome this rule must never have.
+                    if (DayLockHelper.IsLocked(lockedThroughBySite, (int)site.MicrotingUid!, dateValue))
                     {
                         lockedDaysSkipped++;
                         continue;

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanRegistrationHelper.cs

@@ -379,7 +379,7 @@
     /// in, whose zero would silently discard the accumulated balance.
     /// A null cursor means nothing is known, so behave exactly as before.
     /// </summary>
     public static PlanRegistration? ResolveChainAnchor(
         IEnumerable<PlanRegistration> candidates, AssignedSite site, DateTime windowStart)
     {
         var eligible = candidates.Where(x => x.Date < windowStart);
@@ -402,7 +402,7 @@
         DateTime midnightOfDateFrom,
         DateTime midnightOfDateTo,
         IPluginDbOptions<TimePlanningBaseSettings> options,
         string? messageLanguage = null
         )
     {
         var tainted = false;
@@ -867,13 +867,17 @@
                 // stays tracked and the next save fails on it anyway.
                 catch (Exception e) when (e is not DayLockedException)
                 {
-                    logger.LogError(
-                        $"Could not parse PlanText for planning with id: {planRegistration.Id} the PlanText was: {planRegistration.PlanText}");
-                    SentrySdk.CaptureMessage(
-                        $"Could not parse PlanText for planning with id: {planRegistration.Id} the PlanText was: {planRegistration.PlanText}");
-                    //SentrySdk.CaptureException(e);
-                    logger.LogError(e.Message);
-                    logger.LogTrace(e.StackTrace);
+                    // The try above ends in planRegistration.Update calls, so
+                    // this catches DbUpdateException, DbUpdateConcurrencyException,
+                    // MySqlException and friends -- not just PlanText parsing.
+                    // It used to guess "Could not parse PlanText" twice (log +
+                    // Sentry message) while the actual type and stack went to
+                    // Trace, which most hosts do not emit. Report the exception
+                    // ONCE, on each channel, and let it name itself.
+                    SentrySdk.CaptureException(e);
+                    logger.LogError(e,
+                        "UpdatePlanRegistrationsInPeriod failed for PlanRegistration {PlanRegistrationId} (PlanText: {PlanText})",
+                        planRegistration.Id, planRegistration.PlanText);
                 }
             }
 
@@ -1557,10 +1561,26 @@
                     await planRegistration.Update(dbContext).ConfigureAwait(false);
                 }
             }
-            catch (Exception)
+            // Same shape as the catch in UpdatePlanRegistrationsInPeriod, and
+            // fixed the same way. The try above ends in a planRegistration.Update,
+            // so this catches DbUpdateException, DbUpdateConcurrencyException,
+            // MySqlException and friends, which the old "Could not parse
+            // PlanText" message mislabelled while losing the type and stack.
+            // Capture the exception itself, and only that.
+            //
+            // The DayLockedException filter matches the other site: a lock
+            // refusal is routine, not an incident, and must not be reported.
+            // It is a control-flow change, but not an outcome change -- the
+            // rejected entry stays tracked either way, so the sole caller's own
+            // Update (TimePlanningWorkingHoursService.UpdatePlanning) rethrows
+            // it whether or not this catch swallowed it.
+            //
+            // This method takes no ILogger, so Sentry carries the detail. (The
+            // sibling UpdatePlanRegistrationsInPeriod in this class does take
+            // one and logs there.)
+            catch (Exception e) when (e is not DayLockedException)
             {
-                SentrySdk.CaptureMessage(
-                    $"Could not parse PlanText for planning with id: {planRegistration.Id} the PlanText was: {planRegistration.PlanText}");
+                SentrySdk.CaptureException(e);
             }
         // }
         return planRegistration;

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.da.resx

@@ -225,4 +225,16 @@
     <data name="SuccessfullyUnlockedDay" xml:space="preserve">
         <value>Dagen er låst op</value>
     </data>
+    <data name="SiteNotFound" xml:space="preserve">
+        <value>Medarbejder ikke fundet.</value>
+    </data>
+    <data name="Imported" xml:space="preserve">
+        <value>Importeret.</value>
+    </data>
+    <data name="ImportLockedDaysSkipped" xml:space="preserve">
+        <value>{0} låst(e) dag(e) blev ikke ændret.</value>
+    </data>
+    <data name="ImportUnresolvableSheetsSkipped" xml:space="preserve">
+        <value>Ark ikke importeret, fordi medarbejderen ikke er fuldt oprettet: {0}.</value>
+    </data>
 </root>

eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.resx

@@ -225,4 +225,16 @@
     <data name="SuccessfullyUnlockedDay" xml:space="preserve">
         <value>Day unlocked</value>
     </data>
+    <data name="SiteNotFound" xml:space="preserve">
+        <value>Worker not found.</value>
+    </data>
+    <data name="Imported" xml:space="preserve">
+        <value>Imported.</value>
+    </data>
+    <data name="ImportLockedDaysSkipped" xml:space="preserve">
+        <value>Locked days left unchanged: {0}.</value>
+    </data>
+    <data name="ImportUnresolvableSheetsSkipped" xml:space="preserve">
+        <value>Sheets not imported because the worker is not fully registered: {0}.</value>
+    </data>
 </root>