From 57459177cd3008aec0b7034b88fd580451c86083 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Thu, 8 Oct 2026 08:33:11 +0200 Subject: [PATCH 1/2] fix(settings): let a removed managing tag be added again The unique index on AssignedSiteManagingTags (AssignedSiteId, TagId) also covers soft-deleted rows, but UpdateAssignedSite only loaded live rows. Giving a leader back a tag that had been removed inserted a second row, the insert failed with a duplicate key and the settings save returned 500, which left the worker dialog spinning. Load every row for the site: delete unwanted live rows, revive wanted removed rows, and insert only tags that have never had a row. Duplicate ids in the request are ignored. The removing and reviving user is now recorded. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VDsVLaH7SqGi1bFk8nAtP1 --- .../SettingsServiceTests.cs | 65 +++++++++++++++++++ .../TimeSettingService.cs | 47 ++++++++------ 2 files changed, 91 insertions(+), 21 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs index 359b1559..b57b9144 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs @@ -12,6 +12,7 @@ using NUnit.Framework; using Microting.EformAngularFrontendBase.Infrastructure.Data; using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; +using Microting.eFormApi.BasePn.Infrastructure.Models.API; using TimePlanning.Pn.Infrastructure.Models.Settings; using TimePlanning.Pn.Services.TimePlanningLocalizationService; using TimePlanning.Pn.Services.TimePlanningSettingService; @@ -200,6 +201,70 @@ public async Task UpdateAssignedSite_UpdatesGpsEnabled_Successfully() Assert.That(updatedSite.SnapshotEnabled, Is.False); } + // A managing tag that was removed must be addable again: the unique index on + // (AssignedSiteId, TagId) also covers soft-deleted rows, so inserting a second + // row for the tag failed with a duplicate key and the settings save returned 500. + [Test] + public async Task UpdateAssignedSite_ManagingTags_RemovedTagCanBeAddedAgain() + { + var assignedSite = new AssignedSiteEntity + { + SiteId = 61, + IsManager = true, + UseGoogleSheetAsDefault = true, + CreatedByUserId = 1, + UpdatedByUserId = 1 + }; + await assignedSite.Create(TimePlanningPnDbContext); + + var removedTag = new AssignedSiteManagingTag + { + AssignedSiteId = assignedSite.Id, TagId = 3, CreatedByUserId = 1, UpdatedByUserId = 1 + }; + await removedTag.Create(TimePlanningPnDbContext); + await removedTag.Delete(TimePlanningPnDbContext); + await new AssignedSiteManagingTag + { + AssignedSiteId = assignedSite.Id, TagId = 5, CreatedByUserId = 1, UpdatedByUserId = 1 + }.Create(TimePlanningPnDbContext); + + Task Save(params int[] tagIds) => + _settingsService.UpdateAssignedSite(new AssignedSiteModel + { + Id = assignedSite.Id, + SiteId = 61, + IsManager = true, + UseGoogleSheetAsDefault = true, + ManagingTagIds = tagIds.ToList() + }); + + Task> Rows() => + TimePlanningPnDbContext.AssignedSiteManagingTags.AsNoTracking() + .Where(x => x.AssignedSiteId == assignedSite.Id) + .ToListAsync(); + + static int[] ActiveTagIds(List rows) => + rows.Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Select(x => x.TagId).OrderBy(x => x).ToArray(); + + // Give back the removed tag 3, keep 5, add the new tag 10 (twice in the request). + Assert.That((await Save(3, 5, 10, 10)).Success, Is.True); + var rows = await Rows(); + Assert.That(ActiveTagIds(rows), Is.EqualTo(new[] { 3, 5, 10 })); + Assert.That(rows.Single(x => x.TagId == 3).Id, Is.EqualTo(removedTag.Id), "tag 3 must reuse its old row"); + + // Remove 3 and 10 again (soft delete keeps the rows), then give 3 back once more. + Assert.That((await Save(5)).Success, Is.True); + rows = await Rows(); + Assert.That(ActiveTagIds(rows), Is.EqualTo(new[] { 5 })); + Assert.That(rows.Count, Is.EqualTo(3)); + + Assert.That((await Save(3)).Success, Is.True); + rows = await Rows(); + Assert.That(ActiveTagIds(rows), Is.EqualTo(new[] { 3 })); + Assert.That(rows.Single(x => x.TagId == 3).Id, Is.EqualTo(removedTag.Id)); + } + // #1740: every site runs on 1-minute intervals and the settings checkbox is // gone, so UpdateAssignedSite must ignore whatever a (stale) client sends for // UseOneMinuteIntervals — in both directions — and never touch the recorded diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs index 5276042c..1171ff48 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs @@ -1195,38 +1195,43 @@ await pushNotificationService.SendToSiteAsync( dbAssignedSite.SiteId); } - // Update managing tags - var existingManagingTags = await dbContext.AssignedSiteManagingTags + // Update managing tags. Removed rows are loaded too: the unique index on + // (AssignedSiteId, TagId) also covers soft-deleted rows, so a tag that is + // given back must revive its old row instead of inserting a new one. + var managingTagRows = await dbContext.AssignedSiteManagingTags .Where(x => x.AssignedSiteId == dbAssignedSite.Id) - .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) .ToListAsync(); - var managingTagIds = site.ManagingTagIds ?? new List(); + var managingTagIds = site.ManagingTagIds?.ToHashSet() ?? new HashSet(); - // Remove tags that are no longer in the list - foreach (var existingTag in existingManagingTags) + foreach (var row in managingTagRows) { - if (!managingTagIds.Contains(existingTag.TagId)) + var wanted = managingTagIds.Contains(row.TagId); + var removed = row.WorkflowState == Constants.WorkflowStates.Removed; + if (!wanted && !removed) + { + row.UpdatedByUserId = userService.UserId; + await row.Delete(dbContext); + } + else if (wanted && removed) { - await existingTag.Delete(dbContext); + row.WorkflowState = Constants.WorkflowStates.Created; + row.UpdatedByUserId = userService.UserId; + await row.Update(dbContext); } } - // Add new tags - var existingTagIds = existingManagingTags.Select(x => x.TagId).ToList(); - foreach (var tagId in managingTagIds) + var knownTagIds = managingTagRows.Select(x => x.TagId).ToHashSet(); + foreach (var tagId in managingTagIds.Except(knownTagIds)) { - if (!existingTagIds.Contains(tagId)) + var newManagingTag = new AssignedSiteManagingTag { - var newManagingTag = new AssignedSiteManagingTag - { - AssignedSiteId = dbAssignedSite.Id, - TagId = tagId, - CreatedByUserId = userService.UserId, - UpdatedByUserId = userService.UserId - }; - await newManagingTag.Create(dbContext); - } + AssignedSiteId = dbAssignedSite.Id, + TagId = tagId, + CreatedByUserId = userService.UserId, + UpdatedByUserId = userService.UserId + }; + await newManagingTag.Create(dbContext); } if (dbAssignedSite.UseGoogleSheetAsDefault) From 88622144d33782460c158b615d0acc4dd81cb316 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Thu, 8 Oct 2026 08:45:07 +0200 Subject: [PATCH 2/2] test(settings): assert who is recorded on revived and removed managing tags Seed the rows with another user so the test fails if UpdateAssignedSite stops stamping the acting user on the revive or delete path (Copilot review). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VDsVLaH7SqGi1bFk8nAtP1 --- .../TimePlanning.Pn.Test/SettingsServiceTests.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs index b57b9144..8f4dd13d 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/SettingsServiceTests.cs @@ -217,15 +217,17 @@ public async Task UpdateAssignedSite_ManagingTags_RemovedTagCanBeAddedAgain() }; await assignedSite.Create(TimePlanningPnDbContext); + // Seeded by another user (2), so the saves below must stamp the acting user (1). + const int otherUserId = 2; var removedTag = new AssignedSiteManagingTag { - AssignedSiteId = assignedSite.Id, TagId = 3, CreatedByUserId = 1, UpdatedByUserId = 1 + AssignedSiteId = assignedSite.Id, TagId = 3, CreatedByUserId = otherUserId, UpdatedByUserId = otherUserId }; await removedTag.Create(TimePlanningPnDbContext); await removedTag.Delete(TimePlanningPnDbContext); await new AssignedSiteManagingTag { - AssignedSiteId = assignedSite.Id, TagId = 5, CreatedByUserId = 1, UpdatedByUserId = 1 + AssignedSiteId = assignedSite.Id, TagId = 5, CreatedByUserId = otherUserId, UpdatedByUserId = otherUserId }.Create(TimePlanningPnDbContext); Task Save(params int[] tagIds) => @@ -252,6 +254,8 @@ static int[] ActiveTagIds(List rows) => var rows = await Rows(); Assert.That(ActiveTagIds(rows), Is.EqualTo(new[] { 3, 5, 10 })); Assert.That(rows.Single(x => x.TagId == 3).Id, Is.EqualTo(removedTag.Id), "tag 3 must reuse its old row"); + Assert.That(rows.Single(x => x.TagId == 3).UpdatedByUserId, Is.EqualTo(1), "reviving must record the acting user"); + Assert.That(rows.Single(x => x.TagId == 5).UpdatedByUserId, Is.EqualTo(otherUserId), "an untouched row keeps its updater"); // Remove 3 and 10 again (soft delete keeps the rows), then give 3 back once more. Assert.That((await Save(5)).Success, Is.True); @@ -263,6 +267,7 @@ static int[] ActiveTagIds(List rows) => rows = await Rows(); Assert.That(ActiveTagIds(rows), Is.EqualTo(new[] { 3 })); Assert.That(rows.Single(x => x.TagId == 3).Id, Is.EqualTo(removedTag.Id)); + Assert.That(rows.Single(x => x.TagId == 5).UpdatedByUserId, Is.EqualTo(1), "removing must record the acting user"); } // #1740: every site runs on 1-minute intervals and the settings checkbox is