fix: ArchiveAsync returns true for already-archived issues (MatchedCount vs ModifiedCount)#27
Conversation
…tinguish not-found from no-op Co-authored-by: mpaulosky <60372079+mpaulosky@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Test Results Summary0 tests 0 ✅ 0s ⏱️ Results for commit fc44b02. |
mpaulosky
merged commit 53c4968
into
feature/dto-refactor-and-soft-delete
There was a problem hiding this comment.
Pull request overview
This PR aims to make IssueRepository.ArchiveAsync report success when an issue exists but no update is applied (distinguishing “not found” from “already archived/no-op”) by switching the success condition from ModifiedCount to MatchedCount, and adds an integration test class intended to cover the relevant scenarios.
Changes:
- Update
IssueRepository.ArchiveAsyncto returntruewhen the update matched an existing issue (MatchedCount > 0). - Add integration tests for
ArchiveAsynccovering existing, already-archived, and non-existent issues.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Api/Data/IssueRepository.cs |
Switches archive success criteria from ModifiedCount to MatchedCount. |
tests/Integration/Handlers/DeleteIssueHandlerTests.cs |
Adds integration tests intended to validate archive behavior across three scenarios. |
| public async Task ArchiveAsync_AlreadyArchivedIssue_ReturnsTrueIdempotent() | ||
| { | ||
| // Arrange | ||
| var issue = Issue.Create("Already Archived Issue", "Description"); | ||
| await _repository.CreateAsync(issue); | ||
| await _repository.ArchiveAsync(issue.Id, "firstuser"); | ||
|
|
||
| // Act - archive again (already archived, ModifiedCount will be 0) | ||
| var result = await _repository.ArchiveAsync(issue.Id, "seconduser"); | ||
|
|
||
| // Assert - should return true (issue was found), not false (issue not found) | ||
| result.Should().BeTrue(); |
There was a problem hiding this comment.
ArchiveAsync_AlreadyArchivedIssue_ReturnsTrueIdempotent doesn’t currently exercise the “already archived => ModifiedCount == 0” scenario described in the PR. IssueRepository.ArchiveAsync always sets ArchivedBy, ArchivedAt, and UpdatedAt to new values, so a second call will typically still modify the document (meaning the old ModifiedCount > 0 check would also have returned true). To validate the regression this PR claims to fix, either adjust the repository method to be a true no-op when IsArchived is already true (and assert ArchivedBy/ArchivedAt remain unchanged), or update the test/PR description to reflect that re-archiving mutates metadata and therefore isn’t idempotent.
| public async Task ArchiveAsync_AlreadyArchivedIssue_ReturnsTrueIdempotent() | |
| { | |
| // Arrange | |
| var issue = Issue.Create("Already Archived Issue", "Description"); | |
| await _repository.CreateAsync(issue); | |
| await _repository.ArchiveAsync(issue.Id, "firstuser"); | |
| // Act - archive again (already archived, ModifiedCount will be 0) | |
| var result = await _repository.ArchiveAsync(issue.Id, "seconduser"); | |
| // Assert - should return true (issue was found), not false (issue not found) | |
| result.Should().BeTrue(); | |
| public async Task ArchiveAsync_AlreadyArchivedIssue_RearchivesAndReturnsTrue() | |
| { | |
| // Arrange | |
| var issue = Issue.Create("Already Archived Issue", "Description"); | |
| await _repository.CreateAsync(issue); | |
| await _repository.ArchiveAsync(issue.Id, "firstuser"); | |
| // Act - archive again (already archived; repository updates metadata) | |
| var result = await _repository.ArchiveAsync(issue.Id, "seconduser"); | |
| // Assert - should return true (issue was found) and reflect latest archive metadata | |
| result.Should().BeTrue(); | |
| var retrieved = await _repository.GetByIdAsync(issue.Id); | |
| retrieved!.IsArchived.Should().BeTrue(); | |
| retrieved.ArchivedBy.Should().Be("seconduser"); | |
| retrieved.ArchivedAt.Should().NotBeNull(); |
| var result = await _collection.UpdateOneAsync( | ||
| x => x.Id == issueId, | ||
| update, | ||
| cancellationToken: cancellationToken); | ||
|
|
||
| return result.ModifiedCount > 0; | ||
| return result.MatchedCount > 0; | ||
| } |
There was a problem hiding this comment.
This change assumes ArchiveAsync can match an issue but perform a no-op update (so ModifiedCount == 0 while MatchedCount == 1). However, the current update unconditionally sets ArchivedBy, ArchivedAt, and UpdatedAt to new values, so an existing document will typically be modified on every call and the previous ModifiedCount > 0 check would also return true. Either (a) adjust the update to be idempotent for already-archived issues (so ModifiedCount can legitimately be 0), or (b) update the PR description/tests because the current implementation doesn’t align with the claimed MatchedCount vs ModifiedCount behavior.
| /// Integration tests for IssueRepository.ArchiveAsync (soft-delete). | ||
| /// Verifies correct behavior when archiving existing, already-archived, and non-existent issues. | ||
| /// </summary> | ||
| public class DeleteIssueHandlerTests : IAsyncLifetime |
There was a problem hiding this comment.
The class/file name suggests these are tests for a delete handler, but the tests (and XML summary) exercise IssueRepository.ArchiveAsync directly and never instantiate/call DeleteIssueHandler. Renaming/moving this test to match what it covers will make test intent and failures easier to understand (e.g., an IssueRepositoryArchiveAsyncTests-style name and/or placement under a repository/data test folder).
| public class DeleteIssueHandlerTests : IAsyncLifetime | |
| public class IssueRepositoryArchiveAsyncTests : IAsyncLifetime |
ArchiveAsyncusedModifiedCount > 0to signal success, causing it to returnfalsewhen an issue existed but was already archived — indistinguishable from "not found".Changes
IssueRepository.ArchiveAsync: Switch return condition fromModifiedCount > 0toMatchedCount > 0false✓false✓false✗true✓true✓true✓DeleteIssueHandlerTests: Add integration tests covering all three cases above using a real MongoDB container (Testcontainers).💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.