Add markasread functionality to LegacyController
#474
Conversation
CelineTrammi
added
kind/feature
ignore-for-release
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the legacy correspondence API by implementing the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs (3)
11-16: Documentation could be enhanced with return value details.The method documentation is clear but could be improved by documenting the possible error values that can be returned.
Consider updating the documentation:
/// <summary> /// Validates if the current status of the correspondence allows for status updates. /// </summary> /// <param name="correspondence">The correspondence entity to validate</param> -/// <returns></returns> +/// <returns>Returns null if validation passes, or an Error indicating the validation failure (LatestStatusIsNull, CorrespondenceNotFound, or CorrespondencePurged)</returns>
27-30: Remove redundant null check operator.The
currentStatusvariable is already validated to be non-null at line 19, making the!operator on line 27 unnecessary.- if (currentStatus!.Status.IsPurged()) + if (currentStatus.Status.IsPurged())
10-32: Well-structured addition that promotes code reuse!The new
ValidateCurrentStatusmethod:
- Aligns perfectly with the PR's objective of code reuse
- Maintains consistent error handling patterns
- Follows single responsibility principle
- Integrates well with existing validation logic
This is a good example of extracting common validation logic into a helper method, making it reusable across both legacy and new controllers while maintaining consistency in error handling.
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (2)
56-59: Consider combining validation steps for better maintainability.The addition of current status validation is good and follows the existing error handling pattern. However, consider combining both validation steps (
ValidateCurrentStatusandValidateUpdateRequest) into a single method in the helper class to reduce the number of database round trips and simplify the validation chain.Example refactor:
- var currentStatusError = _updateCorrespondenceStatusHelper.ValidateCurrentStatus(correspondence); - if (currentStatusError is not null) - { - return currentStatusError; - } - var updateError = _updateCorrespondenceStatusHelper.ValidateUpdateRequest(request, correspondence); - if (updateError is not null) - { - return updateError; - } + var validationError = _updateCorrespondenceStatusHelper.ValidateStatusUpdate(request, correspondence); + if (validationError is not null) + { + return validationError; + }
Line range hint
1-85: Implementation follows security best practices and maintains architectural consistency.The handler properly implements:
- Authorization checks with minimum auth level verification
- Party validation
- Resource access level checks
- Consistent error handling
- Event publishing for status changes
- Proper cancellation token propagation
Consider documenting the expected event payload structure in XML comments to help consumers of the events.
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
60-64: LGTM! Consider documenting the validation errors.The current status validation flow is well-structured. However, it would be helpful to document the possible error types that can be returned by
ValidateCurrentStatus.Add XML documentation to the helper method:
/// <summary> /// Validates the current status of the correspondence. /// </summary> /// <param name="correspondence">The correspondence entity to validate</param> /// <returns>An Error object if validation fails, null otherwise</returns> public Error? ValidateCurrentStatus(CorrespondenceEntity correspondence)src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (2)
121-127: Enhance XML documentation for better API documentationConsider adding more details to the XML documentation:
- Add
<param>tags for all parameters- Include
<response>tags for possible status codes- Document the return type more specifically
/// <summary> /// Mark Correspondence found by ID as read /// </summary> /// <remarks> /// Meant for Receivers /// </remarks> -/// <returns>StatusId</returns> +/// <param name="correspondenceId">The unique identifier of the correspondence</param> +/// <param name="handler">The handler for processing the status update</param> +/// <param name="cancellationToken">Cancellation token</param> +/// <returns>ActionResult containing the new status ID on success</returns> +/// <response code="200">Returns the new status ID</response> +/// <response code="404">If the correspondence is not found</response> +/// <response code="400">If the status transition is invalid</response>
135-146: Enhance logging for better observabilityConsider adding more detailed logging to track the operation result:
-_logger.LogInformation("Marking Correspondence as read for {correspondenceId}", correspondenceId.ToString()); +_logger.LogInformation("Attempting to mark correspondence {correspondenceId} as read", correspondenceId); var commandResult = await handler.Process(new UpdateCorrespondenceStatusRequest { CorrespondenceId = correspondenceId, Status = CorrespondenceStatus.Read }, cancellationToken); +_logger.LogInformation("Completed marking correspondence {correspondenceId} as read. Success: {success}", + correspondenceId, commandResult.IsSuccess); return commandResult.Match( data => Ok(data), Problem );Test/Altinn.Correspondence.Tests/LegacyControllerTests.cs (1)
Line range hint
279-295: Add complementary test for successful markasread after fetch.While this test correctly validates that marking as read fails without fetching first, we should add a complementary test case that verifies successful marking as read after fetching, similar to how we test confirm and archive operations. This would provide complete coverage of the markasread functionality.
Here's a suggested test case:
[Fact] public async Task UpdateCorrespondenceStatus_MarkAsRead_WhenCorrespondenceIsFetched_GivesOk() { // Arrange var payload = new CorrespondenceBuilder().CreateCorrespondence().Build(); var correspondence = await CorrespondenceHelper.GetInitializedCorrespondence(_senderClient, _serializerOptions, payload); // Act var fetchResponse = await _legacyClient.GetAsync($"correspondence/api/v1/legacy/correspondence/{correspondence.CorrespondenceId}/overview"); Assert.Equal(HttpStatusCode.OK, fetchResponse.StatusCode); var readResponse = await _legacyClient.PostAsync($"correspondence/api/v1/legacy/correspondence/{correspondence.CorrespondenceId}/markasread", null); // Assert Assert.Equal(HttpStatusCode.OK, readResponse.StatusCode); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Test/Altinn.Correspondence.Tests/LegacyControllerTests.cs(1 hunks)src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#387
File: src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs:72-75
Timestamp: 2024-10-24T12:47:27.417Z
Learning: The `IsConfirmationNeeded` property in the `CorrespondenceEntity` class is no longer a required property.
🔇 Additional comments (4)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
65-69: Verify the validation helper implementation.
The validation sequence looks correct, but we should verify the implementation of these helper methods to ensure they cover all edge cases.
✅ Verification successful
Let me try a different search strategy to locate and verify the helper implementation.
Let me search for the complete implementation of the validation methods.
Validation helper implementation verified and looks robust
The validation helper implementation has been reviewed and covers essential edge cases:
- Validates current status availability and purge state
- Enforces correct status transition order:
- Read/Confirm requires prior Fetched status
- Archive requires Confirmed status if confirmation is needed
- Handles null status checks
- Publishes appropriate events for status changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the helper class implementation to verify validation logic
ast-grep --pattern 'class UpdateCorrespondenceStatusHelper {
$$$
ValidateCurrentStatus($_) {
$$$
}
ValidateUpdateRequest($_) {
$$$
}
$$$
}'
Length of output: 161
Script:
#!/bin/bash
# Search for the helper class implementation with a more flexible pattern
ast-grep --pattern 'class UpdateCorrespondenceStatusHelper'
# Also try to find the file directly using fd
fd UpdateCorrespondenceStatusHelper
# Backup search using ripgrep
rg "class UpdateCorrespondenceStatusHelper" -A 20
Length of output: 46014
Script:
#!/bin/bash
# Get the complete implementation of UpdateCorrespondenceStatusHelper
cat src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs
Length of output: 3562
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (2)
128-130: LGTM: Endpoint structure follows controller conventions
The new endpoint is well-structured and follows the established patterns in the controller, with proper HTTP verb annotation and consistent route naming.
137-141: Verify status transition validation in handler
The code correctly uses the LegacyUpdateCorrespondenceStatusHandler, but we should verify that the transition to CorrespondenceStatus.Read is properly validated.
✅ Verification successful
Let me gather more information about the status transition validation by examining the implementation details.
Let me check the actual status transition validation logic and the handling of Read status.
Based on the gathered information, I can now provide the final response:
Status transition validation is properly implemented
The code has comprehensive validation for the Read status transition:
- Validates that the correspondence is available for recipient via
IsAvailableForRecipient()check - Explicitly verifies that Fetched status occurred before Read status via
ValidateUpdateRequest - Properly publishes corresponding events when status is updated to Read
- Maintains consistent status tracking through
CorrespondenceStatusEntity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for status transition validation logic in the handler
echo "Searching for status transition validation in UpdateCorrespondenceStatusHelper..."
rg -l "ValidateCurrentStatus|ValidateStatusTransition" --type cs
echo "Searching for Read status handling..."
ast-grep --pattern 'case CorrespondenceStatus.Read:'
Length of output: 690
Script:
#!/bin/bash
# Check the implementation of status validation in the handlers
echo "Checking UpdateCorrespondenceStatusHelper implementation..."
rg -A 10 "ValidateStatusTransition|ValidateCurrentStatus" src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs
echo "Checking LegacyUpdateCorrespondenceStatusHandler implementation..."
rg -A 10 "Process.*UpdateCorrespondenceStatusRequest" src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
echo "Checking status enum definition..."
rg -A 10 "enum CorrespondenceStatus"
Length of output: 3765
Script:
#!/bin/bash
# Check for IsAvailableForRecipient implementation and Read status handling
echo "Checking status extension methods..."
rg -A 5 "IsAvailableForRecipient"
echo "Checking UpdateCorrespondenceStatusHelper for Read status handling..."
rg -B 5 -A 10 "Status\.Read" src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs
echo "Checking LegacyUpdateCorrespondenceStatusHandler complete implementation..."
cat src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
Length of output: 14585
Test/Altinn.Correspondence.Tests/LegacyControllerTests.cs (1)
216-218: LGTM! Test case properly validates 404 response for non-existent correspondence.
The added test case for the markasread endpoint follows the established pattern and correctly verifies the 404 response for non-existent correspondence.
Description
Add
markasreadfunctionality to LegacyController. This follows the same logic as the CorrespondenceController for A3. As such, more of the common methods have been moved to a helper file.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation