Verify that a correspondence has been published before notification #702
Conversation
…and if not retry until it is.
📝 WalkthroughWalkthroughThis pull request updates the correspondence notification system. In the application layer, it injects a background job client into the CheckNotificationHandler, adds a new EnsureNotificationHandler for processing notifications, and enhances notification creation by serializing order requests and adding a condition endpoint. In the core, an Changes
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 (
|
Ceredron
added
kind/bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/Altinn.Correspondence.Application/EnsureNotification/EnsureNotificationHandler.cs (2)
34-37: Use a more suitable exception type for a missingOrderRequest.Throwing
ArgumentExceptionmight be misleading in this scenario, since the missing order request is more of a state issue rather than an invalid argument. Consider throwingInvalidOperationExceptionor a domain-specific exception instead.-throw new ArgumentException("Order request must be set in order to retry"); +throw new InvalidOperationException("Order request must be set in order to retry");
46-46: Throw a more specific exception.Rather than throwing a plain
Exception, consider using a domain-specific or built-in typed exception (e.g.,InvalidOperationException) for better clarity and error handling.-throw new Exception("Correspondence is not published"); +throw new InvalidOperationException("Correspondence is not published");src/Altinn.Correspondence.Core/Repositories/ICorrespondenceNotificationRepository.cs (1)
9-10: Refine naming for clarity.While the functionality is correct, consider clarifying how "primary" notifications are determined in the documentation. Also, renaming
WipeOrdertoClearOrderRequestcan help convey intent more explicitly.-Task WipeOrder(Guid notificationId, CancellationToken cancellationToken); +Task ClearOrderRequest(Guid notificationId, CancellationToken cancellationToken);src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs (3)
20-26: Document and optimize GetPrimaryNotification method.The method could benefit from XML documentation and might perform better with SingleOrDefaultAsync for the expected single result case.
Apply this diff:
+ /// <summary> + /// Gets the primary (non-reminder) notification for a correspondence. + /// </summary> + /// <param name="correspondenceId">The correspondence ID.</param> + /// <param name="cancellationToken">Cancellation token.</param> + /// <returns>The primary notification or null if not found.</returns> public async Task<CorrespondenceNotificationEntity?> GetPrimaryNotification(Guid correspondenceId, CancellationToken cancellationToken) { return await _context.CorrespondenceNotifications .Where(n => n.CorrespondenceId == correspondenceId && !n.IsReminder) .OrderByDescending(n => n.RequestedSendTime) - .FirstOrDefaultAsync(cancellationToken); + .SingleOrDefaultAsync(cancellationToken); }
28-37: Document exceptions in WipeOrder method.The method should document the ArgumentException it throws.
Apply this diff:
+ /// <summary> + /// Clears the order request for a notification. + /// </summary> + /// <param name="notificationId">The notification ID.</param> + /// <param name="cancellationToken">Cancellation token.</param> + /// <exception cref="ArgumentException">Thrown when notification is not found.</exception> public async Task WipeOrder(Guid notificationId, CancellationToken cancellationToken)
30-30: Use FindAsync for primary key lookup.When querying by primary key, FindAsync is more efficient than FirstOrDefaultAsync.
Apply this diff:
- var notification = await _context.CorrespondenceNotifications.FirstOrDefaultAsync(notification => notification.Id == notificationId, cancellationToken); + var notification = await _context.CorrespondenceNotifications.FindAsync([notificationId], cancellationToken);src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (2)
242-242: Add error handling for JSON serialization.While the serialization is correctly implemented, consider adding error handling to gracefully handle any serialization failures.
Apply this diff to add error handling:
- OrderRequest = JsonSerializer.Serialize(notification) + OrderRequest = JsonSerializer.Serialize(notification, new JsonSerializerOptions { WriteIndented = false }) // Compact JSON + ?? throw new InvalidOperationException($"Failed to serialize notification for correspondence {correspondence.Id}")
320-320: Enforce HTTPS for condition endpoint.While the endpoint construction and localhost check are good, consider enforcing HTTPS for security.
Apply this diff to enforce HTTPS:
private Uri? CreateConditionEndpoint(string correspondenceId) { - var conditionEndpoint = new Uri($"{_generalSettings.CorrespondenceBaseUrl.TrimEnd('/')}/correspondence/api/v1/correspondence/{correspondenceId}/notification/check"); + var baseUrl = _generalSettings.CorrespondenceBaseUrl.TrimEnd('/'); + if (!baseUrl.StartsWith("https://", StringComparison.OrdinalIgnoreCase)) + { + baseUrl = baseUrl.Replace("http://", "https://"); + } + var conditionEndpoint = new Uri($"{baseUrl}/correspondence/api/v1/correspondence/{correspondenceId}/notification/check"); if (conditionEndpoint.Host == "localhost") { return null; } return conditionEndpoint; }Also applies to: 383-391
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Altinn.Correspondence.Application/CheckNotification/CheckNotificationHandler.cs(2 hunks)src/Altinn.Correspondence.Application/EnsureNotification/EnsureNotificationHandler.cs(1 hunks)src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs(3 hunks)src/Altinn.Correspondence.Core/Models/Entities/CorrespondenceNotificationEntity.cs(2 hunks)src/Altinn.Correspondence.Core/Repositories/ICorrespondenceNotificationRepository.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/20250213201903_NotificationOrderRequest.Designer.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/20250213201903_NotificationOrderRequest.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs(1 hunks)src/Altinn.Correspondence.Persistence/Repositories/CorrespondenceNotificationRepository.cs(2 hunks)
🔇 Additional comments (5)
src/Altinn.Correspondence.Application/EnsureNotification/EnsureNotificationHandler.cs (2)
16-22: Good use of typed error responses withOneOf.Returning typed errors for not-found scenarios helps keep the error-handling workflow consistent and explicit.
38-40: Add error handling for JSON deserialization.Deserialization can fail if the JSON is malformed. Consider adding error handling (e.g., try/catch) to either rethrow with a more descriptive message or handle the failure gracefully.
-var orderRequest = JsonSerializer.Deserialize<NotificationOrderRequest>(primaryNotification.OrderRequest); -orderRequest.RequestedSendTime = DateTime.Now; +NotificationOrderRequest orderRequest; +try +{ + orderRequest = JsonSerializer.Deserialize<NotificationOrderRequest>(primaryNotification.OrderRequest) + ?? throw new InvalidDataException("Deserialization returned null"); +} +catch (JsonException ex) +{ + throw new InvalidDataException("Failed to parse notification order request", ex); +} +orderRequest.RequestedSendTime = DateTime.Now;src/Altinn.Correspondence.Persistence/Migrations/20250213201903_NotificationOrderRequest.Designer.cs (1)
1-526: Auto-generated EF Core migration looks good.No concerns with this designer file as it correctly reflects the schema updates for the
OrderRequestcolumn.src/Altinn.Correspondence.Persistence/Migrations/ApplicationDbContextModelSnapshot.cs (1)
261-263: LGTM!The
OrderRequestproperty is correctly defined as a required text column, which is appropriate for storing JSON data.src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (1)
19-19: LGTM!The System.Text.Json import is correctly added for JSON serialization.
| migrationBuilder.AddColumn<string>( | ||
| name: "OrderRequest", | ||
| schema: "correspondence", | ||
| table: "CorrespondenceNotifications", | ||
| type: "text", | ||
| nullable: false, | ||
| defaultValue: ""); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making OrderRequest column nullable.
The column is currently non-nullable with an empty string default. Since this appears to store request data (likely JSON), an empty string might not be a meaningful default value and could cause issues with deserialization.
Apply this diff to make the column nullable:
migrationBuilder.AddColumn<string>(
name: "OrderRequest",
schema: "correspondence",
table: "CorrespondenceNotifications",
type: "text",
- nullable: false,
- defaultValue: "");
+ nullable: true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| migrationBuilder.AddColumn<string>( | |
| name: "OrderRequest", | |
| schema: "correspondence", | |
| table: "CorrespondenceNotifications", | |
| type: "text", | |
| nullable: false, | |
| defaultValue: ""); | |
| } | |
| migrationBuilder.AddColumn<string>( | |
| name: "OrderRequest", | |
| schema: "correspondence", | |
| table: "CorrespondenceNotifications", | |
| type: "text", | |
| nullable: true); | |
| } |
| @@ -31,5 +31,7 @@ public class CorrespondenceNotificationEntity | |||
| public string? NotificationAddress { get; set; } | |||
|
|
|||
| public int? Altinn2NotificationId { get; set; } | |||
|
|
|||
| public string OrderRequest { get; set; } | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and make OrderRequest nullable.
The property lacks XML documentation explaining its purpose and expected format. Additionally, it should be nullable to match the suggested migration change.
Apply this diff:
- public string OrderRequest { get; set; }
+ /// <summary>
+ /// Serialized request data for the notification order.
+ /// </summary>
+ public string? OrderRequest { get; set; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public string OrderRequest { get; set; } | |
| /// <summary> | |
| /// Serialized request data for the notification order. | |
| /// </summary> | |
| public string? OrderRequest { get; set; } |
| @@ -25,6 +27,11 @@ public async Task<OneOf<CheckNotificationResponse, Error>> Process(Guid correspo | |||
| { | |||
| response.SendNotification = false; | |||
| } | |||
| if (!correspondence.StatusHasBeen(CorrespondenceStatus.Published)) | |||
| { | |||
| backgroundJobClient.Schedule<EnsureNotificationHandler>(handler => handler.Process(correspondenceId, null, cancellationToken), DateTimeOffset.Now.AddHours(1)); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Configure retry delay and avoid passing cancellation token to background job.
The retry delay is hard-coded and the cancellation token passed to the background job might be cancelled before the job runs.
Consider these improvements:
- Move the delay to configuration
- Remove the cancellation token from the background job
- Add maximum retry attempts
Apply this diff:
- backgroundJobClient.Schedule<EnsureNotificationHandler>(handler => handler.Process(correspondenceId, null, cancellationToken), DateTimeOffset.Now.AddHours(1));
+ backgroundJobClient.Schedule<EnsureNotificationHandler>(
+ handler => handler.Process(correspondenceId, null, CancellationToken.None),
+ DateTimeOffset.Now.Add(_configuration.RetryDelay));Add to configuration:
public class NotificationConfiguration
{
public TimeSpan RetryDelay { get; set; } = TimeSpan.FromHours(1);
public int MaxRetryAttempts { get; set; } = 24; // Retry for 24 hours
}
Description
Verify that a correspondence has been published before notification, and retry if not.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Chores