Repository navigation
fix(settings): let a removed managing tag be added again - #1757
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDsVLaH7SqGi1bFk8nAtP1
There was a problem hiding this comment.
🟢 Approval recommended
The core regression is correctly fixed and covered, with only a non-blocking audit-field assertion gap.
1 open finding
What changed in this PR
Fixes managing-tag reassignment by reviving soft-deleted records instead of violating the unique index.
Changes:
- Reconciles tags using deduplicated IDs and existing rows.
- Records the updating user on removal and revival.
- Adds regression coverage for repeated removal and reassignment.
| File | Description |
|---|---|
TimeSettingService.cs |
Revives removed managing-tag rows and prevents duplicate inserts. |
SettingsServiceTests.cs |
Tests repeated tag removal, revival, and duplicate request IDs. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
…g 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDsVLaH7SqGi1bFk8nAtP1
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation addresses the duplicate-key failure and includes comprehensive regression coverage.
0 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Saving a leader's managing tags ("Administration af tags") in the worker dialog fails when one of the chosen tags was assigned to that leader before and later removed.
PUT /api/time-planning-pn/settings/assigned-sitereturns 500:The unique index also covers soft-deleted rows, but
UpdateAssignedSiteonly loaded live rows and inserted a new row for every tag it did not see. The dialog has no error path for this call, so it keeps spinning.Fix
TimeSettingService.UpdateAssignedSiteloads every managing-tag row for the site:WorkflowState = created)The removing/reviving user is now written to
UpdatedByUserId.Tests
SettingsServiceTests.UpdateAssignedSite_ManagingTags_RemovedTagCanBeAddedAgain: removes a tag, adds it back (with a duplicate id in the request), removes it and adds it back again; asserts the active set, that the old row is reused, and that no second row is created. On the old code the first save throws the duplicate-key error.SettingsServiceTestsis already in both CI shard filters.Not verified locally: the test run (CI only).
dotnet buildpasses.Follow-up (separate PR, backend-configuration plugin): the worker dialog should stop the spinner and show an error when the settings save fails.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VDsVLaH7SqGi1bFk8nAtP1