Grace period (et al) for membership cleaner#479
Open
andreasfrisch wants to merge 1 commit into
Open
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
andreasfrisch
commented
Jun 16, 2026
| "FirstSeenMissingAt" timestamp NOT NULL, | ||
| "LastCheckedAt" timestamp NOT NULL, | ||
| "CreatedAt" timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "UpdatedAt" timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, |
Contributor
Author
There was a problem hiding this comment.
CreatedAt and UpdatedAt are technically not necessary, but I like them to keep things aligned
andreasfrisch
commented
Jun 16, 2026
| ( | ||
| "Id" uuid NOT NULL, | ||
| "UserId" varchar(255) NOT NULL, | ||
| "Status" varchar(50) NOT NULL, -- NotFound, Deactivated |
Contributor
Author
There was a problem hiding this comment.
This may be overkill, but I like the additional data for why we remove people
andreasfrisch
commented
Jun 16, 2026
| private readonly ILogger<DeactivatedMemberCleanerApplicationService> _logger; | ||
|
|
||
| private readonly StringBuilder _sb = new(); | ||
| private const int GracePeriodDays = 7; |
Contributor
Author
There was a problem hiding this comment.
7 seems to be our magic grace period number 🤷
andreasfrisch
commented
Jun 16, 2026
|
|
||
| namespace SelfService.Domain.Events; | ||
|
|
||
| public class MemberMarkedAsMissing : IDomainEvent |
Contributor
Author
There was a problem hiding this comment.
Introduced for later usage.
We should be fully event based for this, I believe
andreasfrisch
commented
Jun 16, 2026
|
|
||
| namespace SelfService.Domain.Events; | ||
|
|
||
| public class MemberRemovedDueToMissingStatus : IDomainEvent |
Contributor
Author
There was a problem hiding this comment.
Introduced for later usage.
We should be fully event based for this, I believe
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the membership-cleaner workflow to avoid premature deletions due to transient Azure AD issues by tracking “missing” members and only deleting after a 7-day grace period, while also adding RBAC cleanup on deletion.
Changes:
- Add persistence + repository for tracking missing members across cleaner runs (grace period state).
- Update
DeactivatedMemberCleanerApplicationServiceto (1) mark members missing, (2) delete only after grace period, and (3) clean RBAC grants/group memberships when deleting. - Add SQLite-backed tests covering first-detection vs grace-period-expired deletion behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SelfService/Infrastructure/Persistence/SelfServiceDbContext.cs | Adds DbSet for missing-member tracking. |
| src/SelfService/Infrastructure/Persistence/MissingMemberRepository.cs | New repository for CRUD around missing-member tracking records. |
| src/SelfService/Domain/Models/MissingMemberRecord.cs | New domain model for missing-member state + grace-period checks. |
| src/SelfService/Domain/Models/IMissingMemberRepository.cs | New repository interface. |
| src/SelfService/Domain/Events/MemberRemovedDueToMissingStatus.cs | New domain event type (added in this PR). |
| src/SelfService/Domain/Events/MemberMarkedAsMissing.cs | New domain event type (added in this PR). |
| src/SelfService/Configuration/Domain.cs | Registers IMissingMemberRepository in DI. |
| src/SelfService/Application/DeactivatedMemberCleanerApplicationService.cs | Implements grace-period logic, missing-member bookkeeping, and RBAC cleanup on delete. |
| src/SelfService.Tests/Infrastructure/Persistence/TestMemberCleaner.cs | Adds tests for grace-period behavior. |
| src/SelfService.Tests/Builders/DeactivatedMemberCleanerApplicationServiceBuilder.cs | Updates builder for new cleaner dependencies; mocks RBAC repos. |
| db/migrations/20260616100000_add-missing-member-record-table.sql | Adds SQL migration for MissingMemberRecords. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ( | ||
| "Id" uuid NOT NULL, | ||
| "UserId" varchar(255) NOT NULL, | ||
| "Status" varchar(50) NOT NULL, -- NotFound, Deactivated |
Comment on lines
+166
to
+168
| // Keep heartbeat fresh only while still inside grace period. | ||
| existingRecord.UpdateLastChecked(); | ||
| await _missingMemberRepository.Update(existingRecord); |
Comment on lines
+229
to
+231
| var permissionGrants = await _rbacPermissionGrantRepository.GetAllWithPredicate(pg => | ||
| pg.AssignedEntityId == userId | ||
| ); |
Comment on lines
+261
to
+262
| var groupMembers = await _rbacGroupMemberRepository.GetAllWithPredicate(gm => gm.UserId == userId); | ||
|
|
Comment on lines
+11
to
+13
| public Guid Id { get; set; } | ||
| public string UserId { get; set; } | ||
| public MissingMemberStatus Status { get; set; } |
Comment on lines
+186
to
+188
| // Clean up RBAC permissions | ||
| await CleanupRbacPermissions(member.Id.ToString()); | ||
|
|
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.
💡 Context:
🌟 Changes: