Feat/set recipientname as messsage recipient #696
Conversation
…in initializeCorrespondencesHandler.cs
…oes not work with notification templates or with custom templates that use $recipientName$
…ecipientName$ is now the message recipient and not the notification recipient.
…tOverride limitations for email and number
📝 WalkthroughWalkthroughThis pull request updates the correspondence notification system by modifying test expectations, revising error codes, and altering recipient override validations. The tests now expect successful (HTTP 200) responses with valid data. Additionally, asynchronous handling and personalization of notification content have been introduced. A new migration updates the database schema and notification template configurations. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (4)
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: 1
🧹 Nitpick comments (1)
Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceNotificationTests.cs (1)
666-861: Add test coverage for recipient name content verification.While the current tests comprehensively cover the recipient lookup scenarios, consider adding test cases to verify that the correct recipient name is actually used in the notification content. This would directly validate the main objective of this PR.
Example test case to add:
+[Fact] +public async Task Correspondence_CustomRecipient_UsesCorrectRecipientNameInNotification() +{ + // Arrange + var recipient = $"{UrnConstants.OrganizationNumberAttribute}:991825827"; + var expectedRecipientName = "Test Recipient"; + var customRecipients = new List<CustomNotificationRecipientExt>() + { + new CustomNotificationRecipientExt() + { + RecipientToOverride = recipient, + Recipients = new List<NotificationRecipientExt>() + { + new NotificationRecipientExt() + { + EmailAddress = "[email protected]", + Name = expectedRecipientName + } + } + } + }; + + var payload = new CorrespondenceBuilder() + .CreateCorrespondence() + .WithRecipients([recipient]) + .WithNotificationTemplate(NotificationTemplateExt.CustomMessage) + .WithNotificationChannel(NotificationChannelExt.Email) + .WithEmailContent() + .WithCustomNotificationRecipients(customRecipients) + .Build(); + + // Capture the actual notification content + NotificationOrderRequest? capturedRequest = null; + var mockNotificationService = new Mock<IAltinnNotificationService>(); + mockNotificationService + .Setup(x => x.CreateNotification(It.IsAny<NotificationOrderRequest>(), It.IsAny<CancellationToken>())) + .Callback<NotificationOrderRequest, CancellationToken>((req, _) => capturedRequest = req) + .ReturnsAsync(new NotificationOrderRequestResponse { OrderId = Guid.NewGuid() }); + + var testFactory = new UnitWebApplicationFactory(services => + { + services.AddSingleton(mockNotificationService.Object); + }); + var senderClient = testFactory.CreateSenderClient(); + + // Act + var response = await senderClient.PostAsJsonAsync("correspondence/api/v1/correspondence", payload, _responseSerializerOptions); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.NotNull(capturedRequest); + Assert.Contains(expectedRecipientName, capturedRequest.NotificationContent); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceNotificationTests.cs(3 hunks)src/Altinn.Correspondence.Application/Errors.cs(1 hunks)src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs(0 hunks)src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs(4 hunks)src/Altinn.Correspondence.Persistence/Migrations/20240927122054_add_Notification_templates.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/20250129072844_update_notification_templates.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
src/Altinn.Correspondence.Persistence/Migrations/20250129072844_update_notification_templates.cs (1)
13-40: LGTM! The notification templates have been updated consistently across all languages.The changes correctly implement the new format using
$recipientName$placeholder and maintain consistency in punctuation and phrasing across all languages.Run the following script to verify the notification templates:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify the notification templates in the database. # Test: Check if the notification templates have been updated correctly. # Expect: All templates should use the $recipientName$ placeholder. psql -d correspondence -c "SELECT \"Language\", \"EmailBody\", \"ReminderEmailBody\", \"SmsBody\", \"ReminderSmsBody\" FROM correspondence.\"NotificationTemplates\" WHERE \"Template\" = 1;"Length of output: 272
Reminder: Manual Verification Required for Database Notification Templates
The shell script output indicates that the
psqlcommand was not found in the current environment, so we couldn’t verify the database content automatically. Please manually run the query in an environment where PostgreSQL’s CLI is available to ensure that all notification templates correctly use the$recipientName$placeholder.src/Altinn.Correspondence.Persistence/Migrations/20240927122054_add_Notification_templates.cs (1)
36-115: LGTM! The notification templates table is created with correct structure and initial data.The changes correctly:
- Create the table with all necessary columns
- Insert templates for different languages (no, en, nn) and recipient types
- Use consistent placeholders (
$recipientName$,$sendersName$) across all templatessrc/Altinn.Correspondence.Application/Errors.cs (1)
71-71: LGTM! Error declarations have been updated to reflect the new validation requirements.The changes correctly:
- Remove unnecessary error declarations for recipient override validations
- Reuse error code 3015 for
MissingRecipientsForRecipientOverridesrc/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (1)
218-218: LGTM! The method has been correctly made asynchronous.The change correctly awaits the asynchronous
CreateNotificationscall.Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceNotificationTests.cs (2)
597-628: LGTM! Test correctly validates custom recipient scenarios.The test method now properly validates that custom recipients with valid phone numbers or email addresses are accepted, which aligns with the PR objectives.
630-664: LGTM! Test properly validates recipient name tag in notifications.The test method correctly validates:
- Custom recipients with valid contact information
- Usage of
$recipientName$tag in notification bodies
...inn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#550
File: src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceNotificationMapper.cs:36-37
Timestamp: 2024-12-10T10:51:48.001Z
Learning: Null checking of `CustomNotificationRecipients` is performed within the `MapExternalRecipientsToRequest` method in the `NotificationMapper` class, so additional checks in the `InitializeCorrespondenceNotificationMapper` class are unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (1)
270-352: LGTM! The changes toCreateNotificationsmethod look good.The method has been correctly updated to support asynchronous operations and properly propagates the cancellation token. The logic for handling both organization and person recipients, as well as custom notification recipients, is well-implemented.
...inn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs
Outdated
Show resolved
Hide resolved
|
I`,m a bit unsure if we should change the keyword name, as it is the same one used by Altinn Notification to get the name of the notification recipient. How do we avoid confusion? |
…t the $recipientName$ is now the message recipient and not the notification recipient." This reverts commit 92e1785.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.Correspondence.Persistence/Migrations/20250214083302_ChangedNotificationTemplate.cs (2)
13-46: Ensure safe usage of the placeholder values and consider parameterized migrations.
The raw SQL statements are fine for targeted updates. However, if$recipientName$or$sendersName$will be replaced by user input at runtime, be cautious about potential injection risks. Normally for migrations, this is low-risk. Using parameters instead of inline string concatenation can further safeguard against unexpected characters or future expansions.
49-53: Consider implementing a corresponding rollback.
The Down method is empty, so these updates cannot be reverted if needed. Implementing a partial rollback (e.g., resetting templates to default values) is often a good practice, especially in production systems requiring robust rollbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Correspondence.Persistence/Migrations/20250214083302_ChangedNotificationTemplate.Designer.cs(1 hunks)src/Altinn.Correspondence.Persistence/Migrations/20250214083302_ChangedNotificationTemplate.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.Correspondence.Persistence/Migrations/20250214083302_ChangedNotificationTemplate.Designer.cs (1)
1-522: No manual changes needed.
Because this file is auto-generated by EF Core migration tooling, modifications should generally be avoided unless there's a strong requirement. The file structure and model definitions appear consistent with typical Entity Framework conventions.
…entName$ to $correspondenceRecpientName$ to avoid using the same keyword that is used to replace notification recipient name on notifications
Description
The recipientName in the notification templates should be the name of the message recipient and not the notification recipient. This PR adds logic that replaces the recipientName in the notification content to the message recipient.
The limitation that RecipientOverride does not work with email or phone number when using notification template or a custom template that uses recipientName is now removed. This limitation existed because the recipientName could not be looked up for an email or a phone number of which the notification recipient could be. The recipientName is now the message recipient, which is always either an organization or person so no limitation is needed.
This PR also changes the notification template texts to reflect the change that recipientName is now the message recipient and not the notification recipient.
User documentation update PR: Altinn/altinn-studio-docs#2008
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Chores