Enhancement/deactivate dialog confirm button #865
Conversation
…est that removes a guiAction
…d if the request status is confirmed
📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the correspondence status update functionality. A new method, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (4)
src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/PatchDialogRequestMapper.cs (2)
3-16: Consider using strongly-typed models instead of anonymous objects.The mapper creates JSON patch operations using anonymous objects. While this works for simple cases, using strongly-typed models would provide better type safety, IDE support, and maintainability.
+public class JsonPatchOperation +{ + public string op { get; set; } = string.Empty; + public string path { get; set; } = string.Empty; +} internal class PatchDialogRequestMapper { - internal static List<object> CreateRemoveGuiActionPatchRequest(int guiActionToRemoveIndex) + internal static List<JsonPatchOperation> CreateRemoveGuiActionPatchRequest(int guiActionToRemoveIndex) { - return new List<object> + return new List<JsonPatchOperation> { - new - { - op = "remove", - path = $"/guiActions/{guiActionToRemoveIndex}" - } + new JsonPatchOperation + { + op = "remove", + path = $"/guiActions/{guiActionToRemoveIndex}" + } }; } }
5-15: Add XML documentation to explain the method's purpose.Adding XML documentation would improve code maintainability by clearly explaining the purpose of the method and its parameters.
+/// <summary> +/// Creates a JSON Patch request to remove a GUI action from a dialog. +/// </summary> +/// <param name="guiActionToRemoveIndex">The index of the GUI action to remove.</param> +/// <returns>A list containing a JSON Patch operation to remove the specified GUI action.</returns> internal static List<object> CreateRemoveGuiActionPatchRequest(int guiActionToRemoveIndex) { return new List<object> { new { op = "remove", path = $"/guiActions/{guiActionToRemoveIndex}" } }; }src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
54-57: Consider adding specific logging for the dialog patch operation.While the code correctly calls the dialog service when the correspondence status is confirmed, adding specific logging before and after this operation would improve debugging capabilities.
if (request.Status == CorrespondenceStatus.Confirmed) { + logger.LogInformation("Attempting to patch correspondence dialog to confirmed for correspondence {CorrespondenceId}", correspondence.Id); await dialogportenService.PatchCorrespondenceDialogToConfirmed(correspondence.Id); + logger.LogInformation("Successfully patched correspondence dialog to confirmed for correspondence {CorrespondenceId}", correspondence.Id); }src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
41-178: Consider refactoring to reduce code duplication.Several methods in this class share similar patterns for retrieving correspondence by ID, finding dialog ID in external references, and error handling for API calls. Consider extracting these common patterns into helper methods to improve maintainability.
For example, you could create helper methods like:
private async Task<(CorrespondenceEntity correspondence, string dialogId)> GetCorrespondenceAndDialogId(Guid correspondenceId, CancellationToken cancellationToken) { var correspondence = await _correspondenceRepository.GetCorrespondenceById(correspondenceId, true, true, cancellationToken); if (correspondence is null) { logger.LogError("Correspondence with id {correspondenceId} not found", correspondenceId); throw new ArgumentException($"Correspondence with id {correspondenceId} not found", nameof(correspondenceId)); } var dialogId = correspondence.ExternalReferences.FirstOrDefault(reference => reference.ReferenceType == ReferenceType.DialogportenDialogId)?.ReferenceValue; if (dialogId is null) { throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); } return (correspondence, dialogId); } private async Task<T?> SendRequestAndHandleResponse<T>(Func<CancellationToken, Task<HttpResponseMessage>> requestFunc, CancellationToken cancellationToken, string errorContext = "") { var response = await requestFunc(cancellationToken); if (!response.IsSuccessStatusCode) { throw new Exception($"Response from Dialogporten {errorContext} was not successful: {response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); } if (typeof(T) == typeof(Unit)) { return default; } var result = await response.Content.ReadFromJsonAsync<T>(cancellationToken); if (result is null) { throw new Exception($"Failed to deserialize the response {errorContext}."); } return result; }This would help simplify your methods and make them more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(2 hunks)src/Altinn.Correspondence.Core/Services/IDialogportenService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs(3 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/PatchDialogRequestMapper.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2025-03-20T07:58:04.606Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
🧬 Code Definitions (3)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cs (2)
src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (6)
Task(7-7)Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (7)
Task(16-39)Task(41-69)Task(71-98)Task(100-126)Task(127-148)Task(150-160)Task(162-178)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs (1)
CorrespondenceStatus(399-413)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (6)
src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (6)
Task(7-7)Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cs (6)
Task(8-11)Task(13-16)Task(18-21)Task(23-26)Task(28-31)Task(33-36)src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
Task(34-141)src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (3)
Task(45-170)Task(172-195)Task(196-312)src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/CreateDialogRequestMapper.cs (2)
Content(35-82)CreateDialogRequest(10-33)src/Altinn.Correspondence.Integrations/Dialogporten/Models/CreateDialogRequest.cs (2)
Content(116-136)CreateDialogRequest(5-66)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenDevService.cs (1)
13-16: Implementation aligns with development service pattern.The implementation of
PatchCorrespondenceDialogToConfirmedfollows the established pattern for other methods in this development service class, using a minimal implementation withTask.CompletedTask. This is appropriate for a development environment where external services might not be available.src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (1)
8-8: Method signature consistent with interface pattern.The addition of the
PatchCorrespondenceDialogToConfirmedmethod to the interface follows the existing pattern of asynchronous methods returningTaskand uses appropriate naming conventions.src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (2)
17-17: Dependency injection correctly implemented.The addition of
IDialogportenServiceas a constructor parameter follows proper dependency injection patterns.
54-57:❓ Verification inconclusive
Verify error handling for dialog patching.
The transaction will handle retries for failures, but please verify that any exceptions from
PatchCorrespondenceDialogToConfirmedare properly caught and handled in the production implementation.
🏁 Script executed:
#!/bin/bash # Check the implementation of DialogportenService.PatchCorrespondenceDialogToConfirmed to verify exception handling rg -A 5 -B 3 "public async Task PatchCorrespondenceDialogToConfirmed" --type csharp --glob "src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs"Length of output: 1269
Review: Confirm Exception Handling in Dialog Patching
- In
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(lines 54–57), the call toPatchCorrespondenceDialogToConfirmedisn’t wrapped in a try-catch block.- The implementation in
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs(as seen in the snippet) doesn’t include explicit error handling—it relies on the surrounding transaction to manage retries.- Action: Please verify that in production, any exceptions thrown by
PatchCorrespondenceDialogToConfirmedpropagate in a way that the global transaction or error-handling infrastructure catches them. Confirm that this design correctly triggers retries and logs errors as needed. If not, consider adding explicit exception handling at this boundary.src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
7-7: LGTM: Relevant import for Dialog models.The added import is necessary for the new methods to work with
CreateDialogRequestobjects.
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (2)
41-69: 🛠️ Refactor suggestionFix resource management and add null check for GuiActions.
The implementation of
PatchCorrespondenceDialogToConfirmedlooks good overall, but has several issues:
- The
CancellationTokenSourceis not being properly disposed- There's no check for null
GuiActionsbefore calling FindIndex- There's a potential typo in the error message on line 54 (compare with line 140 which has "on on")
public async Task PatchCorrespondenceDialogToConfirmed(Guid correspondenceId) { - var cancellationTokenSource = new CancellationTokenSource(); - var cancellationToken = cancellationTokenSource.Token; + using var cancellationTokenSource = new CancellationTokenSource(); + var cancellationToken = cancellationTokenSource.Token; var correspondence = await _correspondenceRepository.GetCorrespondenceById(correspondenceId, true, true, cancellationToken); if (correspondence is null) { logger.LogError("Correspondence with id {correspondenceId} not found", correspondenceId); throw new ArgumentException($"Correspondence with id {correspondenceId} not found", nameof(correspondenceId)); } var dialogId = correspondence.ExternalReferences.FirstOrDefault(reference => reference.ReferenceType == ReferenceType.DialogportenDialogId)?.ReferenceValue; if (dialogId is null) { throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); } var dialog = await GetDialog(dialogId); + if (dialog.GuiActions == null) + { + logger.LogInformation("No GuiActions found on dialog {dialogId} for correspondence {correspondenceId}", dialogId, correspondenceId); + return; + } var guiActionIndexToDelete = dialog.GuiActions.FindIndex(dialog => dialog.Url == $"{generalSettings.Value.CorrespondenceBaseUrl.TrimEnd('/')}/correspondence/api/v1/correspondence/{correspondence.Id}/confirm"); if (guiActionIndexToDelete == -1) { logger.LogInformation("No confirm guiAction found on dialog {dialogId} for correspondence {correspondenceId} when attempting to remove confirm guiAction", dialogId, correspondenceId); return; } var patchDialogRequest = PatchDialogRequestMapper.CreateRemoveGuiActionPatchRequest(guiActionIndexToDelete); var response = await _httpClient.PatchAsJsonAsync($"dialogporten/api/v1/serviceowner/dialogs/{dialogId}", patchDialogRequest, cancellationToken); if (!response.IsSuccessStatusCode) { throw new Exception($"Response from Dialogporten was not successful: {response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); } }
162-178: 🛠️ Refactor suggestionAdd parameter validation and proper disposal of resources.
The method looks good overall but has the following issues:
- No validation for the
dialogIdparameter- CancellationTokenSource is not being properly disposed
- The error message could include the dialogId for easier debugging
public async Task<CreateDialogRequest> GetDialog(string dialogId) { - var cancellationTokenSource = new CancellationTokenSource(); - var cancellationToken = cancellationTokenSource.Token; + if (string.IsNullOrEmpty(dialogId)) + { + throw new ArgumentException("DialogId cannot be null or empty", nameof(dialogId)); + } + + using var cancellationTokenSource = new CancellationTokenSource(); + var cancellationToken = cancellationTokenSource.Token; var response = await _httpClient.GetAsync($"dialogporten/api/v1/serviceowner/dialogs/{dialogId}", cancellationToken); if (!response.IsSuccessStatusCode) { - throw new Exception($"Response from Dialogporten was not successful: {response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); + throw new Exception($"Response from Dialogporten for dialog {dialogId} was not successful: {response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); } var dialogRequest = await response.Content.ReadFromJsonAsync<CreateDialogRequest>(cancellationToken); if (dialogRequest is null) { - throw new Exception("Failed to deserialize the dialog request from the response."); + throw new Exception($"Failed to deserialize the dialog request for dialog {dialogId} from the response."); } return dialogRequest; }
🧹 Nitpick comments (3)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (3)
89-89: Fix duplicate typo in error messages.There's a duplicate "on on" typo in the error messages on lines 89 and 140.
// Line 89 - throw new ArgumentException($"No dialog found on on correspondence with id {correspondenceId}"); + throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); // Line 140 - throw new ArgumentException($"No dialog found on on correspondence with id {correspondenceId}"); + throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}");Also applies to: 140-140
41-69: Consider adding unit tests for the PatchCorrespondenceDialogToConfirmed method.This method has multiple conditions and edge cases that should be tested, including:
- What happens when correspondence doesn't exist
- What happens when dialog ID is missing
- What happens when GuiActions is null
- What happens when the confirm action is not found
- What happens when the API call succeeds or fails
41-69: Consider refactoring duplicate code across methods.The first part of many methods in this class follows the same pattern: create cancellation token, get correspondence by ID, check if null, get dialog ID, check if null. Consider extracting this common logic to a private helper method.
+ private async Task<(string dialogId, Guid correspondenceId)> GetDialogIdFromCorrespondence(Guid correspondenceId, CancellationToken cancellationToken) + { + var correspondence = await _correspondenceRepository.GetCorrespondenceById(correspondenceId, true, true, cancellationToken); + if (correspondence is null) + { + logger.LogError("Correspondence with id {correspondenceId} not found", correspondenceId); + throw new ArgumentException($"Correspondence with id {correspondenceId} not found", nameof(correspondenceId)); + } + + var dialogId = correspondence.ExternalReferences.FirstOrDefault(reference => reference.ReferenceType == ReferenceType.DialogportenDialogId)?.ReferenceValue; + if (dialogId is null) + { + throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); + } + + return (dialogId, correspondence.Id); + }Then use this method in all the places with similar code:
public async Task PatchCorrespondenceDialogToConfirmed(Guid correspondenceId) { using var cancellationTokenSource = new CancellationTokenSource(); var cancellationToken = cancellationTokenSource.Token; - var correspondence = await _correspondenceRepository.GetCorrespondenceById(correspondenceId, true, true, cancellationToken); - if (correspondence is null) - { - logger.LogError("Correspondence with id {correspondenceId} not found", correspondenceId); - throw new ArgumentException($"Correspondence with id {correspondenceId} not found", nameof(correspondenceId)); - } - var dialogId = correspondence.ExternalReferences.FirstOrDefault(reference => reference.ReferenceType == ReferenceType.DialogportenDialogId)?.ReferenceValue; - if (dialogId is null) - { - throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); - } + var (dialogId, id) = await GetDialogIdFromCorrespondence(correspondenceId, cancellationToken); var dialog = await GetDialog(dialogId); // ...remaining implementation... }Also applies to: 129-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2025-03-20T07:58:04.606Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
🧬 Code Definitions (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (4)
src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (6)
Task(7-7)Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)src/Altinn.Correspondence.Application/PublishCorrespondence/PublishCorrespondenceHandler.cs (1)
Task(34-141)src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/CreateDialogRequestMapper.cs (2)
Content(35-82)CreateDialogRequest(10-33)src/Altinn.Correspondence.Integrations/Dialogporten/Models/CreateDialogRequest.cs (2)
Content(116-136)CreateDialogRequest(5-66)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
7-7: Good import addition for using the CreateDialogRequest model.Adding the import for
Altinn.Correspondence.Integrations.Dialogporten.Modelsis appropriate since you're now using theCreateDialogRequestmodel in the newGetDialogmethod.
…actored the patchRequestMapper to a builder to improve its usability
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Altinn.Correspondence.Integrations/Dialogporten/Factories/PatchDialogRequestBuilder.cs (1)
1-36: Improve naming conventions and code structure.The implementation of
DialogPatchRequestBuilderlooks good functionally but has several issues related to naming conventions and organization:
- The file is in a "Factories" folder but the namespace doesn't reflect this
- The field name
_PatchDialogRequestdoesn't follow C# naming conventions for private fields (should be camelCase)- Missing XML documentation for public methods
- No validation for input indices
-namespace Altinn.Correspondence.Integrations.Dialogporten +namespace Altinn.Correspondence.Integrations.Dialogporten.Factories { internal class DialogPatchRequestBuilder { - private List<object> _PatchDialogRequest = new List<object>(); + private readonly List<object> _patchDialogRequest = new List<object>(); + /// <summary> + /// Builds and returns the patch request operations list. + /// </summary> + /// <returns>A list of patch operations.</returns> public List<object> Build() { - return _PatchDialogRequest; + return _patchDialogRequest; } + /// <summary> + /// Adds an operation to remove a GUI action at the specified index. + /// </summary> + /// <param name="guiActionToRemoveIndex">The zero-based index of the GUI action to remove.</param> + /// <returns>The current instance for method chaining.</returns> + /// <exception cref="ArgumentOutOfRangeException">Thrown when the index is negative.</exception> internal DialogPatchRequestBuilder WithRemoveGuiActionOperation(int guiActionToRemoveIndex) { + if (guiActionToRemoveIndex < 0) + { + throw new ArgumentOutOfRangeException(nameof(guiActionToRemoveIndex), "Index cannot be negative."); + } - _PatchDialogRequest.Add( + _patchDialogRequest.Add( new { op = "remove", path = $"/guiActions/{guiActionToRemoveIndex}" } ); return this; } + /// <summary> + /// Adds an operation to remove an API action at the specified index. + /// </summary> + /// <param name="apiActionToRemoveIndex">The zero-based index of the API action to remove.</param> + /// <returns>The current instance for method chaining.</returns> + /// <exception cref="ArgumentOutOfRangeException">Thrown when the index is negative.</exception> internal DialogPatchRequestBuilder WithRemoveApiActionOperation(int apiActionToRemoveIndex) { + if (apiActionToRemoveIndex < 0) + { + throw new ArgumentOutOfRangeException(nameof(apiActionToRemoveIndex), "Index cannot be negative."); + } - _PatchDialogRequest.Add( + _patchDialogRequest.Add( new { op = "remove", path = $"/apiActions/{apiActionToRemoveIndex}" } ); return this; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs(3 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/Factories/PatchDialogRequestBuilder.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2025-03-20T07:58:04.606Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
🧬 Code Definitions (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (3)
src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (6)
Task(7-7)Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)src/Altinn.Correspondence.Integrations/Dialogporten/Factories/PatchDialogRequestBuilder.cs (3)
DialogPatchRequestBuilder(3-35)DialogPatchRequestBuilder(12-22)DialogPatchRequestBuilder(24-34)src/Altinn.Correspondence.Integrations/Dialogporten/Models/CreateDialogRequest.cs (2)
Content(116-136)CreateDialogRequest(5-66)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
167-183: Add parameter validation and proper resource disposal.The GetDialog method implementation has several improvements that need to be made:
- No validation for the
dialogIdparameter- CancellationTokenSource is not being properly disposed
- The error message could include the dialogId for easier debugging
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs
Outdated
Show resolved
Hide resolved
…ic to UpdateCorrespondenceStatusHelper
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs (1)
93-105: LGTM! Nice addition of the PatchCorrespondenceDialog method.The new method correctly implements the functionality to update the dialog in Dialogporten when a correspondence is confirmed, following the established pattern in the codebase. It properly uses Hangfire to queue the job as a fire-and-forget operation.
A couple of minor suggestions:
- The
return;statement on line 104 is redundant and could be removed- Consider adding a code comment explaining the relationship between this method and
ReportActivityToDialogportensince both handle theConfirmedstatus but serve different purposes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.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/UpdateCorrespondenceStatusHandler.cs
🧰 Additional context used
🧠 Learnings (1)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs (2)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#387
File: src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs:72-75
Timestamp: 2025-03-20T07:58:04.607Z
Learning: The `IsConfirmationNeeded` property in the `CorrespondenceEntity` class is no longer a required property.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#483
File: src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs:75-75
Timestamp: 2025-03-20T07:58:04.607Z
Learning: In `LegacyUpdateCorrespondenceStatusHandler.cs`, the call to `_updateCorrespondenceStatusHelper.ReportActivityToDialogporten(...)` is intended to be a fire-and-forget event and does not need to be awaited.
🧬 Code Definitions (1)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHelper.cs (1)
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs (1)
CorrespondenceStatus(399-413)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
41-80: 🛠️ Refactor suggestionEnsure proper resource disposal in PatchCorrespondenceDialogToConfirmed method.
The implementation looks good overall, with proper null checks, error handling, and empty request validation. However, the CancellationTokenSource is not being properly disposed, which could lead to resource leaks.
public async Task PatchCorrespondenceDialogToConfirmed(Guid correspondenceId) { - var cancellationTokenSource = new CancellationTokenSource(); - var cancellationToken = cancellationTokenSource.Token; + using var cancellationTokenSource = new CancellationTokenSource(); + var cancellationToken = cancellationTokenSource.Token; var correspondence = await _correspondenceRepository.GetCorrespondenceById(correspondenceId, true, true, cancellationToken); // Rest of the method remains unchanged
🧹 Nitpick comments (4)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (4)
51-55: Add null check for ExternalReferences collection.The code assumes ExternalReferences is not null when calling FirstOrDefault. While this may be true in most cases, adding a null check would make the code more robust.
- var dialogId = correspondence.ExternalReferences.FirstOrDefault(reference => reference.ReferenceType == ReferenceType.DialogportenDialogId)?.ReferenceValue; + var dialogId = correspondence.ExternalReferences?.FirstOrDefault(reference => reference.ReferenceType == ReferenceType.DialogportenDialogId)?.ReferenceValue; if (dialogId is null) { throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); }
99-101: Fix typo in error message.There's a typo in the error message: "No dialog found on on correspondence" (duplicated word "on").
if (dialogId is null) { - throw new ArgumentException($"No dialog found on on correspondence with id {correspondenceId}"); + throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); }
151-152: Fix typo in error message.There's a typo in the error message: "No dialog found on on correspondence" (duplicated word "on").
if (dialogId is null) { - throw new ArgumentException($"No dialog found on on correspondence with id {correspondenceId}"); + throw new ArgumentException($"No dialog found on correspondence with id {correspondenceId}"); }
82-109: Apply consistent resource management across all methods.Multiple methods in this class are using CancellationTokenSource without proper disposal. This pattern should be fixed consistently across all methods in the class.
All methods that create a CancellationTokenSource should use the
usingkeyword to ensure proper disposal, for example:public async Task CreateInformationActivity(Guid correspondenceId, DialogportenActorType actorType, DialogportenTextType textType, params string[] tokens) { logger.LogInformation("CreateInformationActivity {actorType}: {textType} for correspondence {correspondenceId}", Enum.GetName(typeof(DialogportenActorType), actorType), Enum.GetName(typeof(DialogportenTextType), textType), correspondenceId ); - var cancellationTokenSource = new CancellationTokenSource(); - var cancellationToken = cancellationTokenSource.Token; + using var cancellationTokenSource = new CancellationTokenSource(); + var cancellationToken = cancellationTokenSource.Token; // Rest of the method remains unchangedApply similar changes to all methods in this class.
Also applies to: 117-137, 138-159, 161-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2025-03-20T07:58:04.606Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
🧬 Code Definitions (1)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (3)
src/Altinn.Correspondence.Core/Services/IDialogportenService.cs (6)
Task(7-7)Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)src/Altinn.Correspondence.Integrations/Dialogporten/Factories/PatchDialogRequestBuilder.cs (3)
DialogPatchRequestBuilder(3-35)DialogPatchRequestBuilder(12-22)DialogPatchRequestBuilder(24-34)src/Altinn.Correspondence.Integrations/Dialogporten/Models/CreateDialogRequest.cs (2)
Content(116-136)CreateDialogRequest(5-66)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/Altinn.Correspondence.Integrations/Dialogporten/DialogportenService.cs (2)
69-74: Looks good!The code correctly checks if the patch request is empty and returns early if there are no actions to remove, which avoids unnecessary API calls. This is a good optimization.
173-189: Add parameter validation and proper disposal of resources.The method looks good overall but has the following issues:
- No validation for the
dialogIdparameter- CancellationTokenSource is not being properly disposed
- The error message could include the dialogId for easier debugging
public async Task<CreateDialogRequest> GetDialog(string dialogId) { - var cancellationTokenSource = new CancellationTokenSource(); - var cancellationToken = cancellationTokenSource.Token; + if (string.IsNullOrEmpty(dialogId)) + { + throw new ArgumentException("DialogId cannot be null or empty", nameof(dialogId)); + } + + using var cancellationTokenSource = new CancellationTokenSource(); + var cancellationToken = cancellationTokenSource.Token; var response = await _httpClient.GetAsync($"dialogporten/api/v1/serviceowner/dialogs/{dialogId}", cancellationToken); if (!response.IsSuccessStatusCode) { - throw new Exception($"Response from Dialogporten was not successful: {response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); + throw new Exception($"Response from Dialogporten for dialog {dialogId} was not successful: {response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); } var dialogRequest = await response.Content.ReadFromJsonAsync<CreateDialogRequest>(cancellationToken); if (dialogRequest is null) { - throw new Exception("Failed to deserialize the dialog request from the response."); + throw new Exception($"Failed to deserialize the dialog request for dialog {dialogId} from the response."); } return dialogRequest; }
…cation on UpdateCorrespondenceStatusHandler confirm request
Description
When a message is confirmed, the confirm button in arbeidsflate should deactivate. This PR makes the confirm endpoint also send a patch request to dialogporten to remove the guiAction and apiAction for confirm if they exist. Sending a patch request to remove the guiAction for confirm in dialogporten, removes the confirm button in arbeidsflate.
The PR removes the guiAction instead of deactivating it because dialogPorten currently does not support explicitly setting a guiAction as deactivated.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit