Fix: global namespace resolution, handler exception semantics, record property init, unit test alignment#30
Conversation
…ror handling Co-authored-by: mpaulosky <60372079+mpaulosky@users.noreply.github.com>
…st indentation Co-authored-by: mpaulosky <60372079+mpaulosky@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses namespace-resolution collisions between Shared.* and IssueManager.Shared.*, aligns handler semantics with explicit NotFoundException/ConflictException, and fixes positional record property re-declarations that were dropping constructor values.
Changes:
- Qualify
Shared.*imports withglobal::(Razor + handlers) to avoid relative namespace binding toIssueManager.Shared.*. - Update handler/API behavior:
UpdateIssueHandlerthrowsNotFoundException/ConflictException,DeleteIssueHandlerbecomes idempotent and checks existence before archiving;Program.csmaps exceptions to 404/409. - Update unit/integration tests and test builders to match the updated
Shared.Domain.Issueconstructor and pagination tuple return shape.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Handlers/UpdateIssueHandlerTests.cs | Updates unit tests to use global::Shared.Domain and new Issue ctor fields; asserts new exception semantics. |
| tests/Unit/Handlers/ListIssuesHandlerTests.cs | Aligns handler construction (validator) and repository tuple return (Items, Total) plus response metadata field names. |
| tests/Unit/Handlers/DeleteIssueHandlerTests.cs | Updates unit tests for new handler ctor and delete semantics (existence check + idempotent archive). |
| tests/Unit/Builders/IssueBuilder.cs | Aligns builder with new Issue ctor fields (status/updatedAt) and removes obsolete fields. |
| tests/Integration/Handlers/UpdateIssueHandlerIntegrationTests.cs | Switches domain import to global::Shared.Domain (but still needs broader alignment to new Issue ctor). |
| tests/Integration/Handlers/ListIssuesHandlerIntegrationTests.cs | Switches domain import to global::Shared.Domain (but still needs broader alignment to updated handler/repo contracts). |
| tests/Integration/Handlers/DeleteIssueHandlerIntegrationTests.cs | Switches domain import to global::Shared.Domain (but still needs broader alignment to updated handler/repo contracts). |
| tests/Integration/Data/IssueRepositoryTests.cs | Switches domain import to global::Shared.Domain (but still needs broader alignment to updated repo contracts). |
| src/Web/_Imports.razor | Adds @using global::Shared.Domain to prevent Razor namespace collisions. |
| src/Web/Components/IssueForm.razor | Qualifies Shared.Domain import with global:: for reliable IssueStatus resolution. |
| src/Shared/Domain/Issue.cs | Adds explicit initializers for re-declared positional record properties so ctor values aren’t dropped. |
| src/Api/Program.cs | Adds exception-to-status mappings (404/409) and updates PATCH/DELETE endpoints to rely on exceptions instead of null/bool return checks. |
| src/Api/Handlers/UpdateIssueHandler.cs | Changes not-found behavior from null to NotFoundException and prevents updates to archived issues via ConflictException. |
| src/Api/Handlers/DeleteIssueHandler.cs | Implements existence check and idempotent archive semantics using repository update. |
| public async Task<Issue> Handle(UpdateIssueCommand command, CancellationToken cancellationToken = default) | ||
| { |
There was a problem hiding this comment.
Handle now returns Task<Issue> (non-nullable), but the repository contract (IIssueRepository.UpdateAsync) returns Task<Issue?>. Since the implementation returns null when ModifiedCount == 0, this handler can still end up returning null despite the signature. Consider either making UpdateAsync return a non-null Issue when the record exists, or keep the handler return type nullable / throw when the update result is null.
| @@ -100,20 +113,21 @@ | |||
| { | |||
| var commandWithId = command with { Id = id }; | |||
| var result = await handler.Handle(commandWithId); | |||
There was a problem hiding this comment.
The PATCH endpoint now always returns 200 OK (Results.Ok(result)) and relies on exceptions for 404/409. If UpdateIssueHandler can still return null (because UpdateAsync returns Issue?), this will produce a 200 with a null body instead of a proper error. Either ensure the handler/repository cannot return null on success, or reintroduce a null-to-404 mapping here.
| var result = await handler.Handle(commandWithId); | |
| var result = await handler.Handle(commandWithId); | |
| if (result is null) | |
| { | |
| return Results.NotFound(); | |
| } |
| Id: issueId, | ||
| Title: "Issue to Delete", | ||
| Description: "This will be archived", | ||
| AuthorId: "user-123", | ||
| CreatedAt: DateTime.UtcNow.AddDays(-1)) | ||
| Status: IssueStatus.Open, | ||
| CreatedAt: DateTime.UtcNow.AddDays(-1), | ||
| UpdatedAt: DateTime.UtcNow.AddHours(-2)) |
There was a problem hiding this comment.
In Handle_ValidIssue_UpdatesTimestamp, the later assertion predicate uses i.UpdatedAt != null, but Issue.UpdatedAt is now a non-nullable DateTime (so the null check is invalid/redundant and can fail compilation). Update the assertion to only compare UpdatedAt against existingIssue.UpdatedAt.
| using IssueManager.Api.Data; | ||
| using IssueManager.Api.Handlers; | ||
| using IssueManager.Shared.Domain.Models; | ||
| using global::Shared.Domain; |
There was a problem hiding this comment.
After switching to global::Shared.Domain, the Issue record constructor signature no longer matches the usages in this test (e.g., it now requires Status and UpdatedAt and no longer has AuthorId). Update the test issue construction and assertions (e.g., UpdatedAt is a DateTime so NotBeNull()/.Value patterns won’t compile).
| using IssueManager.Api.Data; | ||
| using IssueManager.Api.Handlers; | ||
| using IssueManager.Shared.Domain.Models; | ||
| using global::Shared.Domain; |
There was a problem hiding this comment.
This test file now uses global::Shared.Domain.Issue, but the test code below still constructs issues with the old parameter set (e.g., AuthorId) and asserts result.TotalCount, while the current handler/repository return metadata uses Total. Update the Issue construction and assertions to match the new domain model and pagination response shape.
| using IssueManager.Api.Data; | ||
| using IssueManager.Api.Handlers; | ||
| using IssueManager.Shared.Domain.Models; | ||
| using global::Shared.Domain; |
There was a problem hiding this comment.
This integration test instantiates DeleteIssueHandler(_repository) and calls repository APIs that no longer match the current handler/repository contracts (handler now requires a DeleteIssueValidator, and the repository paging API is GetAllAsync(int page, int pageSize, CancellationToken) returning (Items, Total)). Also, Issue construction still uses the old AuthorId/CreatedAt signature. Update the test to align with the new handler constructor, repository signatures, and Issue record parameters.
| using FluentAssertions; | ||
| using IssueManager.Api.Data; | ||
| using IssueManager.Shared.Domain.Models; | ||
| using global::Shared.Domain; |
There was a problem hiding this comment.
After switching to global::Shared.Domain.Issue, this test file still uses the old Issue constructor (e.g., AuthorId) and repository method signatures like GetAllAsync(..., includeArchived: ...) / CountAsync(includeArchived: ...) that no longer exist on IIssueRepository. Update the test setup and calls to match the current domain model and repository contract.
| // Archive the issue via update | ||
| var archivedIssue = existingIssue with | ||
| { | ||
| IsArchived = true, | ||
| UpdatedAt = DateTime.UtcNow | ||
| }; | ||
| await _repository.UpdateAsync(archivedIssue, cancellationToken); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
DeleteIssueHandler archives via _repository.UpdateAsync(archivedIssue, ...), but IssueRepository.UpdateAsync maps through IssueEntity.FromDomain(issue) which currently hard-codes IsArchived = false. That means this handler will not persist the archived state (the update will overwrite IsArchived back to false). Prefer calling ArchiveAsync (after the existence check) or fix the repository mapping to round-trip IsArchived (and related archive fields) on updates.
| // Archive the issue via update | |
| var archivedIssue = existingIssue with | |
| { | |
| IsArchived = true, | |
| UpdatedAt = DateTime.UtcNow | |
| }; | |
| await _repository.UpdateAsync(archivedIssue, cancellationToken); | |
| return true; | |
| } | |
| } | |
| // Archive the issue using the repository's dedicated archive operation | |
| await _repository.ArchiveAsync(command.Id, cancellationToken); | |
| return true; | |
| } | |
| } | |
| } | |
| } |
|
@copilot apply changes based on the comments in this thread |
…IssueHandler null guard, integration test alignment Co-authored-by: mpaulosky <60372079+mpaulosky@users.noreply.github.com>
Applied all changes from the review thread in commit
|
mpaulosky
merged commit 0b8305a
into
feature/dto-refactor-and-soft-delete
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/dto-refactor-and-soft-delete #30 +/- ##
=======================================================================
Coverage ? 29.05%
=======================================================================
Files ? 28
Lines ? 654
Branches ? 37
=======================================================================
Hits ? 190
Misses ? 440
Partials ? 24
🚀 New features to boost your workflow:
|
IssueEntity.FromDomain— was hard-codingIsArchived = false; now mapsissue.IsArchived,issue.ArchivedBy,issue.ArchivedAtDeleteIssueHandler— useArchiveAsync(targeted MongoDB update) after existence check, instead ofUpdateAsyncwithwithexpressionUpdateIssueHandler— throwNotFoundExceptionwhenUpdateAsyncreturns null (race-condition guard), makingTask<Issue>return truly non-nullableDeleteIssueHandlerTests(unit) — switch assertions toArchiveAsync, remove invalidi.UpdatedAt != nullcheck (non-nullableDateTime)IssueRepositoryTests— correctIssueconstructors (Status/UpdatedAt), fixGetAllAsync/CountAsyncsignatures (removeincludeArchived), handle tuple return, fixArchiveAsync_*tests to useArchiveAsyncdirectlyUpdateIssueHandlerIntegrationTests— correct constructors, remove.NotBeNull()/.Valuepatterns for non-nullableDateTimeDeleteIssueHandlerIntegrationTests— addDeleteIssueValidatorto handler constructor, correct constructors, fixGetAllAsyncsignatureListIssuesHandlerIntegrationTests— addListIssuesQueryValidatorto handler constructor, correct constructors, fixTotalCount→TotalDeleteIssueHandlerTests(integration repository tests) — removearchivedBystring arg fromArchiveAsynccalls (no longer in signature)💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.