LegacyController: Add UpdateCorrespondenceStatusHandler for Confirm and Archive #447
Conversation
CelineTrammi
added
kind/feature
ignore-for-release
|
Warning Rate limit exceeded@CelineTrammi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
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: 7
🧹 Outside diff range and nitpick comments (8)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs (1)
8-9: Add XML documentation and consider sealing the class.The class should include XML documentation to describe its purpose and responsibilities. Additionally, consider marking it as
sealedsince it's not designed for inheritance.+/// <summary> +/// Helper class for validating correspondence status updates and publishing related events. +/// </summary> -public class UpdateCorrespondenceStatusHelper +public sealed class UpdateCorrespondenceStatusHelpersrc/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (2)
82-82: Reconsider event publishing responsibilityThe helper class is being passed the
_eventBusinstance, which suggests it's taking on too much responsibility. Consider:
- Keep event publishing in the handler class
- Or create a dedicated event service for correspondence status changes
- The helper should focus on validation and status-related logic
- await updateStatusHelper.PublishEvent(_eventBus, correspondence, request.Status, cancellationToken); + await PublishStatusChangeEvent(correspondence, request.Status, cancellationToken); + private async Task PublishStatusChangeEvent( + CorrespondenceEntity correspondence, + CorrespondenceStatus status, + CancellationToken cancellationToken) + { + // Event publishing logic here + await _eventBus.PublishAsync(...); + }
Line range hint
39-85: Overall implementation looks solid with good security practicesThe implementation demonstrates good practices:
- Proper authorization checks before status updates
- Comprehensive error handling and validation
- Efficient use of background jobs for Dialogporten reporting
- Clear separation of concerns (despite the suggested improvements)
Consider breaking down the
Processmethod into smaller methods for better readability, but otherwise the implementation aligns well with the PR objectives.src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (1)
152-178: Consider refactoring to reduce code duplication.The
Archivemethod is very similar to theConfirmmethod. Consider extracting the common logic into a private helper method to improve maintainability:+ private async Task<ActionResult> UpdateCorrespondenceStatus( + Guid correspondenceId, + CorrespondenceStatus status, + LegacyUpdateCorrespondenceStatusHandler handler, + CancellationToken cancellationToken) + { + _logger.LogInformation("Updating Correspondence status to {status} for {correspondenceId}", + status, correspondenceId.ToString()); + + var commandResult = await handler.Process(new UpdateCorrespondenceStatusRequest + { + CorrespondenceId = correspondenceId, + Status = status + }, cancellationToken); + + return commandResult.Match( + data => Ok(data), + Problem + ); + } public async Task<ActionResult> Confirm( Guid correspondenceId, [FromServices] LegacyUpdateCorrespondenceStatusHandler handler, CancellationToken cancellationToken) - { - _logger.LogInformation("Marking Correspondence as confirmed for {correspondenceId}", correspondenceId.ToString()); - - var commandResult = await handler.Process(new UpdateCorrespondenceStatusRequest - { - CorrespondenceId = correspondenceId, - Status = CorrespondenceStatus.Confirmed - }, cancellationToken); - - return commandResult.Match( - data => Ok(data), - Problem - ); - } + => await UpdateCorrespondenceStatus(correspondenceId, CorrespondenceStatus.Confirmed, handler, cancellationToken); public async Task<ActionResult> Archive( Guid correspondenceId, [FromServices] LegacyUpdateCorrespondenceStatusHandler handler, CancellationToken cancellationToken) - { - _logger.LogInformation("Archiving Correspondence with id: {correspondenceId}", correspondenceId.ToString()); - - var commandResult = await handler.Process(new UpdateCorrespondenceStatusRequest - { - CorrespondenceId = correspondenceId, - Status = CorrespondenceStatus.Archived - }, cancellationToken); - - return commandResult.Match( - data => Ok(data), - Problem - ); - } + => await UpdateCorrespondenceStatus(correspondenceId, CorrespondenceStatus.Archived, handler, cancellationToken);Also, add XML documentation for the parameters similar to the
Confirmmethod.src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (4)
53-53: Address the TODO: Implement logic for mark as read and mark as unread.There's a TODO comment indicating that the logic for marking correspondence as read or unread is not implemented.
Would you like assistance in implementing the logic for marking correspondence as read or unread? I can provide a proposed implementation or open a GitHub issue to track this task.
54-59: Consider dependency injection for UpdateCorrespondenceStatusHelper.Currently,
UpdateCorrespondenceStatusHelperis instantiated directly within the method. This can make unit testing more challenging and reduces flexibility.Consider injecting
UpdateCorrespondenceStatusHelpervia the constructor:- var updateStatusHelper = new UpdateCorrespondenceStatusHelper(); + private readonly UpdateCorrespondenceStatusHelper _updateStatusHelper; public LegacyUpdateCorrespondenceStatusHandler(..., UpdateCorrespondenceStatusHelper updateStatusHelper) { ... + _updateStatusHelper = updateStatusHelper; } public async Task<OneOf<Guid, Error>> Process(... ) { - var updateError = updateStatusHelper.ValidateUpdateRequest(request, correspondence); + var updateError = _updateStatusHelper.ValidateUpdateRequest(request, correspondence); }This approach enhances testability and adheres to the Dependency Inversion Principle.
61-67: SetStatusTextappropriately or consider removing it.The
StatusTextfield is set torequest.Status.ToString(). IfStatusTextis redundant or not used elsewhere, consider removing it to simplify the model.If retaining
StatusText, ensure it provides meaningful information, possibly using a user-friendly status description.
69-69: Handle exceptions when publishing events to the event bus.The call to
updateStatusHelper.PublishEventis awaited but not wrapped in a try-catch block. If the event bus fails, it might impact the overall operation.Consider handling potential exceptions to ensure that failures in the event bus do not prevent the method from completing successfully.
try { await updateStatusHelper.PublishEvent(_eventBus, correspondence, request.Status, cancellationToken); } catch (Exception ex) { // Log the exception and decide if you need to handle it or propagate it. }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs(2 hunks)src/Altinn.Correspondence.Application/DependencyInjection.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(2 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/DependencyInjection.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/DependencyInjection.cs:0-0
Timestamp: 2024-10-29T13:40:14.651Z
Learning: In Altinn.Correspondence.Application, handlers with similar names—such as UploadAttachmentHandler, GetAttachmentDetailsHandler, GetAttachmentOverviewHandler, and PurgeAttachmentHandler—may have multiple distinct implementations and are intentionally registered separately in DependencyInjection.cs.
🔇 Additional comments (5)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs (1)
1-37: Verify integration with dependent components.
Let's verify that all event types and error codes used in this class are properly defined.
✅ Verification successful
Integration with dependent components verified successfully
All event types and error codes used in the class are properly defined:
- Event types
CorrespondenceReceiverConfirmedandCorrespondenceReceiverReadare defined insrc/Altinn.Correspondence.Core/Services/Enums/AltinnEventType.cs - Error codes
ReadBeforeFetched,ConfirmBeforeFetched, andArchiveBeforeConfirmedare defined insrc/Altinn.Correspondence.Application/Errors.cswith appropriate error messages and status codes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all event types and error codes used in this class exist
# Test: Search for event type definitions
echo "Verifying event types..."
rg -A 1 "CorrespondenceReceiverConfirmed|CorrespondenceReceiverRead" --type cs
# Test: Search for error code definitions
echo "Verifying error codes..."
rg -A 1 "ReadBeforeFetched|ConfirmBeforeFetched|ArchiveBeforeConfirmed" --type cs
Length of output: 3316
src/Altinn.Correspondence.Application/DependencyInjection.cs (1)
62-62: LGTM! Registration follows established patterns.
The registration of LegacyUpdateCorrespondenceStatusHandler is correctly placed in the Legacy section and uses the appropriate scoped lifetime consistent with other handlers.
Let's verify that all dependencies of the handler are properly registered:
✅ Verification successful
Dependencies are properly registered
Based on the verification results, all required dependencies for LegacyUpdateCorrespondenceStatusHandler are correctly registered:
ICorrespondenceRepositoryandICorrespondenceStatusRepositoryare registered in Persistence layer's DependencyInjection.csIAltinnRegisterServiceis registered in Integrations layer's DependencyInjection.csIEventBusis registered in Integrations layer's DependencyInjection.cs (either asAltinnEventBusorConsoleLogEventBusbased on configuration)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all dependencies of LegacyUpdateCorrespondenceStatusHandler are registered
# Expected: All constructor parameters should have corresponding registrations
# Get the handler's constructor parameters
echo "Checking constructor dependencies..."
ast-grep --pattern 'class LegacyUpdateCorrespondenceStatusHandler
{
$$$
public LegacyUpdateCorrespondenceStatusHandler($$$) {
$$$
}
$$$
}'
# Check for dependency registrations in DependencyInjection.cs
echo "Checking service registrations..."
rg "services\.Add\w+<.*>" src/Altinn.Correspondence.Application/DependencyInjection.cs
Length of output: 2168
Script:
#!/bin/bash
# Let's find the constructor parameters of LegacyUpdateCorrespondenceStatusHandler
rg -A 5 "class LegacyUpdateCorrespondenceStatusHandler" --type cs
Length of output: 1092
Script:
#!/bin/bash
# Let's check the registration of the dependencies
echo "Checking repository registrations..."
rg "services\.Add\w+<I(Correspondence|CorrespondenceStatus)Repository" src/Altinn.Correspondence.Application/DependencyInjection.cs
echo "\nChecking AltinnRegisterService registration..."
rg "services\.Add\w+<IAltinnRegisterService" src/Altinn.Correspondence.Application/DependencyInjection.cs
echo "\nChecking EventBus registration..."
rg "services\.Add\w+<IEventBus" src/Altinn.Correspondence.Application/DependencyInjection.cs
Length of output: 607
Script:
#!/bin/bash
# Let's check the entire DependencyInjection.cs file to see all registrations
echo "Checking full DependencyInjection.cs content..."
cat src/Altinn.Correspondence.Application/DependencyInjection.cs
# Also check if these interfaces might be registered in Infrastructure layer
echo "\nChecking Infrastructure registrations..."
fd -e cs DependencyInjection.cs --exec cat {}
Length of output: 23594
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (2)
9-10: LGTM!
The new using directives are appropriately added and necessary for the new functionality.
125-150: 🛠️ Refactor suggestion
Add XML documentation for parameters and verify status transition rules.
The method implementation needs improvements in documentation and validation:
- Add XML documentation for the parameters:
/// <summary>
/// Mark Correspondence found by ID as confirmed
/// </summary>
/// <remarks>
/// Meant for Receivers
/// </remarks>
+ /// <param name="correspondenceId">The ID of the correspondence to confirm</param>
+ /// <param name="handler">The handler for updating correspondence status</param>
+ /// <param name="cancellationToken">Cancellation token</param>
/// <returns>StatusId</returns>- According to PR objectives, correspondence must be retrieved through either
LegacyGetCorrespondenceOvervieworLegacyGetCorrespondenceDetailsbefore updating status. This requirement should be validated.
Let's verify if the handler implements these validations:
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (1)
49-52: Review the availability check of the correspondence status for the recipient.
The condition !currentStatus.Status.IsAvailableForRecipient() might not be sufficient to determine if the correspondence should be accessible to the recipient. Ensure that IsAvailableForRecipient() method accurately reflects the business logic.
Run the following script to check where IsAvailableForRecipient() is defined and how it's implemented:
✅ Verification successful
Let me generate another script to get more context about the implementation and usage:
Let me get the complete enum definition to see all possible status values:
The availability check for correspondence status is correctly implemented
The IsAvailableForRecipient() method correctly implements the business logic by allowing access to correspondence in the following states: Published, Fetched, Read, Replied, Confirmed, Archived, and Reserved. This aligns with the natural flow of correspondence status transitions and excludes states where the correspondence shouldn't be accessible (Initialized, ReadyForPublish, PurgedByRecipient, PurgedByAltinn, Failed). The method is consistently used across the codebase for access control in various correspondence operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the definition and usages of IsAvailableForRecipient().
# Expected: Identify implementation details of IsAvailableForRecipient().
rg -A 5 'bool\s+IsAvailableForRecipient\('
Length of output: 886
Script:
#!/bin/bash
# Get the full implementation of IsAvailableForRecipient and its usages
rg -A 10 'bool\s+IsAvailableForRecipient\('
# Find all usages of this method across the codebase
rg 'IsAvailableForRecipient\('
# Find the enum definition to understand all possible status values
rg -A 15 'enum\s+CorrespondenceStatus'
Length of output: 6286
Script:
#!/bin/bash
# Get the complete CorrespondenceStatus enum definition
cat src/Altinn.Correspondence.Core/Models/Enums/CorrespondenceStatus.cs
Length of output: 1712
...nn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs
Show resolved
Hide resolved
...nn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs
Show resolved
Hide resolved
...n.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs
Outdated
Show resolved
Hide resolved
...espondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
Outdated
Show resolved
Hide resolved
...espondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
Show resolved
Hide resolved
...espondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
Show resolved
Hide resolved
...espondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (3)
1-30: LGTM! Consider adding XML documentation.The class structure and dependency injection are well-implemented. Consider adding XML documentation to describe the purpose of the handler and its dependencies.
Add documentation like this:
+/// <summary> +/// Handles legacy correspondence status updates for confirm and archive actions. +/// </summary> public class LegacyUpdateCorrespondenceStatusHandler : IHandler<UpdateCorrespondenceStatusRequest, Guid> {
61-61: Address the TODO comment for mark as read/unread functionality.The TODO comment indicates missing functionality. Please clarify if this functionality is planned for a future PR or should be implemented as part of this change.
Would you like me to help implement the mark as read/unread logic or create a GitHub issue to track this task?
68-75: Consider adding audit information to CorrespondenceStatusEntity.The status update should track who made the change for audit purposes.
Consider adding user information:
await _correspondenceStatusRepository.AddCorrespondenceStatus(new CorrespondenceStatusEntity { CorrespondenceId = correspondence.Id, Status = request.Status, StatusChanged = DateTime.UtcNow, StatusText = request.Status.ToString(), + LastChangedBy = _userClaimsHelper.GetUserId(), }, cancellationToken);src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (1)
Line range hint
1-150: Consider performance and maintainability improvementsSeveral improvements could enhance the code:
- Parallelize external service calls:
LookUpPartyByIdand notification detail fetching could be executed concurrently- Extract notification processing logic:
- Move the notification processing loop to a separate method for better maintainability
- Add comprehensive error handling:
- Handle potential failures in external service calls
Here's a suggested refactor for the notification processing:
+ private async Task<List<LegacyCorrespondenceStatus>> GetNotificationHistory( + IEnumerable<NotificationModel> notifications, + string senderPartyId, + int authenticationLevel, + CancellationToken cancellationToken) + { + var notificationHistory = new List<LegacyCorrespondenceStatus>(); + var tasks = notifications + .Where(n => !string.IsNullOrEmpty(n.NotificationOrderId.ToString())) + .Select(async notification => + { + var details = await _altinnNotificationService + .GetNotificationDetails(notification.NotificationOrderId.ToString(), cancellationToken); + if (details?.NotificationsStatusDetails is null) return null; + + var statuses = new List<LegacyCorrespondenceStatus>(); + if (details.NotificationsStatusDetails.Sms is not null) + { + statuses.Add(GetNotificationStatus( + details.NotificationsStatusDetails.Sms.SendStatus, + details.NotificationsStatusDetails.Sms.Recipient, + notification.IsReminder, + senderPartyId, + authenticationLevel)); + } + if (details.NotificationsStatusDetails.Email is not null) + { + statuses.Add(GetNotificationStatus( + details.NotificationsStatusDetails.Email.SendStatus, + details.NotificationsStatusDetails.Email.Recipient, + notification.IsReminder, + senderPartyId, + authenticationLevel)); + } + return statuses; + }); + + var results = await Task.WhenAll(tasks); + return results.Where(r => r != null).SelectMany(r => r).ToList(); + }Then update the
Processmethod to use this:- var notificationHistory = new List<LegacyCorrespondenceStatus>(); - foreach (var notification in correspondence.Notifications) - { - // ... existing notification processing ... - } + var notificationHistory = await GetNotificationHistory( + correspondence.Notifications, + senderParty.PartyId.ToString(), + authenticationLevel, + cancellationToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs
🔇 Additional comments (1)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (1)
49-50:
Security Concern: Verify the authentication level fallback strategy
The change from returning an error to defaulting to authentication level 2 when the authorization check fails raises security concerns:
- This could potentially bypass important access control checks
- Authorization failures are now silently converted to a default access level
- The commented-out code should be removed rather than left in place
Please verify if this change is intentional and document the security implications.
Let's verify the authentication requirements across the codebase:
...espondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
Show resolved
Hide resolved
Description
Add
LegacyUpdateCorrespondenceStatusHandlerfor Legacy endpoint. This only supports "confirm" and "archive" as of now. It follows the same logic as the one in CorrespondenceController, meaning that the correspondence needs to be "fetched" via theLegacyGetCorrespondenceOvervieworLegacyGetCorrespondenceDetailsrequest.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Refactor