Tester CustomNotificationRecipients #663
Conversation
📝 WalkthroughWalkthroughThis pull request modifies the notification mapping process by commenting out the assignment for the Changes
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
🧹 Nitpick comments (3)
src/Altinn.Correspondence.API/Models/CustomNotificationRecipientExt.cs (2)
18-18: Remove Norwegian comments.The Norwegian comments "ok, den består av denne" and "og av enda en liste" should be removed or translated to English to maintain consistency and improve code readability.
- //ok, den består av denne [JsonPropertyName("recipientToOverride")] public required string RecipientToOverride { get; set; } /// <summary> /// List of custom recipients to override the default recipients /// </summary> - //og av enda en liste [JsonPropertyName("notificationRecipient")] public required List<NotificationRecipientExt> Recipients { get; set; }Also applies to: 27-27
16-17: Remove unnecessary empty lines.Remove the extra empty lines between XML comments and property declarations to improve code readability and maintain consistent spacing.
Also applies to: 25-26
src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceNotificationMapper.cs (1)
37-37: Remove Norwegian comment.The Norwegian comment "CustomNotificationRecipients blir ikke populert fra API siden" should be removed or translated to English to maintain consistency and improve code readability.
- //CustomNotificationRecipients blir ikke populert fra API siden CustomNotificationRecipients = NotificationMapper.MapExternalRecipientsToRequest(correspondenceNotificationExt.CustomNotificationRecipients)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceNotificationMapper.cs(1 hunks)src/Altinn.Correspondence.API/Models/CustomNotificationRecipientExt.cs(1 hunks)src/Altinn.Correspondence.API/Models/NotificationDetailsExt.cs(1 hunks)src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Altinn.Correspondence.API/Models/NotificationDetailsExt.cs
- src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
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.
src/Altinn.Correspondence.API/Mappers/InitializeCorrespondenceNotificationMapper.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.API/Mappers/InitializeCorrespondenceNotificationMapper.cs (1)
38-38: LGTM! Null checking is handled correctly.Based on the retrieved learnings, null checking of
CustomNotificationRecipientsis performed within theMapExternalRecipientsToRequestmethod in theNotificationMapperclass, so this implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs (2)
174-180: Consider adding null check forRecipientToOverride.The code assumes
RecipientToOverrideis not null. Consider adding a null check to prevent potential null reference exceptions.Apply this diff to add the null check:
- foreach (var recipientToOverride in customRecipients) - { - if (!recipients.Any(recipient => recipient.WithoutPrefix() == recipientToOverride.RecipientToOverride.WithoutPrefix())) - { - return NotificationErrors.CouldNotFindRecipientToOverride(recipientToOverride.RecipientToOverride); - } - } + foreach (var recipientToOverride in customRecipients) + { + if (recipientToOverride.RecipientToOverride == null) + { + return NotificationErrors.InvalidRecipientOverride; + } + if (!recipients.Any(recipient => recipient.WithoutPrefix() == recipientToOverride.RecipientToOverride.WithoutPrefix())) + { + return NotificationErrors.CouldNotFindRecipientToOverride(recipientToOverride.RecipientToOverride); + } + }🧰 Tools
🪛 GitHub Check: Test application
[failure] 174-174:
The name 'customRecipients' does not exist in the current context
[failure] 174-174:
The name 'customRecipients' does not exist in the current context
182-197: Consider adding null check forRecipientscollection.The code assumes
Recipientscollection is not null. Consider adding a null check to prevent potential null reference exceptions.Apply this diff to add the null check:
foreach (var recipientOverride in customRecipients) { + if (recipientOverride.Recipients == null) + { + return NotificationErrors.InvalidRecipientOverride; + } foreach (var recipient in recipientOverride.Recipients) {🧰 Tools
🪛 GitHub Check: Test application
[failure] 182-182:
The name 'customRecipients' does not exist in the current context
[failure] 182-182:
The name 'customRecipients' does not exist in the current context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs (2)
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.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#550
File: src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs:195-213
Timestamp: 2024-12-10T10:51:58.965Z
Learning: In `src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs`, within the `ValidateNotificationChannelForRecipientOverrides` method, there are four options for `notificationChannel`: `Email`, `EmailPreferred`, `Sms`, and `SmsPreferred`. `Email` and `Sms` explicitly state that the other won't be used. Therefore, using `!=` with `||` in the conditions is intentional and correct to validate the notification channels appropriately.
🪛 GitHub Check: Test application
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs
[failure] 160-160:
The name 'rOrEmail' does not exist in the current context
[failure] 160-160:
The name 'customRecipients' does not exist in the current context
[failure] 160-160:
The name 'rOrEmail' does not exist in the current context
[failure] 160-160:
The name 'customRecipients' does not exist in the current context
🪛 GitHub Actions: Test application
src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs
[error] 160-160: The name 'rOrEmail' does not exist in the current context.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
| //denne brukes | ||
| //CustomNotificationRecipients er tom | ||
| rOrEmail = customRecipients.Where(recipient => recipient.Recipients.Any(r => r.EmailAddress != null || r.MobileNumber != null)).ToList(); |
There was a problem hiding this comment.
Fix compilation errors in recipient validation.
The code has critical compilation errors that need to be addressed:
- The variable
customRecipientsis not declared or initialized. - The variable name
rOrEmailis incorrect. - The line appears to be truncated.
Apply this diff to fix the compilation errors:
- //denne brukes
- //CustomNotificationRecipients er tom
-rOrEmail = customRecipients.Where(recipient => recipient.Recipients.Any(r => r.EmailAddress != null || r.MobileNumber != null)).ToList();
+ var customRecipients = notification.CustomNotificationRecipients ?? [];
+ var recipientWithNumberOrEmail = customRecipients.Where(recipient =>
+ recipient.Recipients.Any(r => r.EmailAddress != null || r.MobileNumber != null))
+ .ToList();📝 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.
| //denne brukes | |
| //CustomNotificationRecipients er tom | |
| rOrEmail = customRecipients.Where(recipient => recipient.Recipients.Any(r => r.EmailAddress != null || r.MobileNumber != null)).ToList(); | |
| var customRecipients = notification.CustomNotificationRecipients ?? []; | |
| var recipientWithNumberOrEmail = customRecipients.Where(recipient => | |
| recipient.Recipients.Any(r => r.EmailAddress != null || r.MobileNumber != null)) | |
| .ToList(); |
🧰 Tools
🪛 GitHub Check: Test application
[failure] 160-160:
The name 'rOrEmail' does not exist in the current context
[failure] 160-160:
The name 'customRecipients' does not exist in the current context
[failure] 160-160:
The name 'rOrEmail' does not exist in the current context
[failure] 160-160:
The name 'customRecipients' does not exist in the current context
🪛 GitHub Actions: Test application
[error] 160-160: The name 'rOrEmail' does not exist in the current context.
There was a problem hiding this comment.
Can I chat whit you without creating a bug on purpose?
There was a problem hiding this comment.
@mSunberg Absolutely! You can chat with me through PR comments without creating bugs. I can help you with various tasks like:
- Code review and analysis
- Generating tests
- Explaining code behavior
- Providing implementation suggestions
- Answering questions about the codebase
- Creating diagrams
- Gathering codebase statistics
Just let me know what you'd like to discuss or what kind of assistance you need!
There was a problem hiding this comment.
Do I have to refer to you in the comment in order for you to answer?
There was a problem hiding this comment.
@mSunberg No, you don't need to refer to me specifically in your comments. I automatically monitor and respond to all comments made in pull requests where I'm enabled. Feel free to write your comments naturally, and I'll be here to help!
There was a problem hiding this comment.
@mSunberg The form field path should be:
correspondence.notification.customNotificationRecipients[0].recipientToOverride
The issue is that you're using recipients in the path, but the model shows that the property is named customNotificationRecipients. Try using this updated path and let me know if you still have any issues.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CustomNotificationRecipients usage in tests and examples
rg -l "CustomNotificationRecipients" --type cs
# Search for test files containing "notification" and "recipient"
rg -l "notification.*recipient" --type cs test/
Length of output: 742
Script:
#!/bin/bash
# Check test files for CustomNotificationRecipients usage
ast-grep --pattern 'class CorrespondenceNotificationTests {
$$$
$test_method($$$) {
$$$
CustomNotificationRecipients
$$$
}
$$$
}'
# Get the model structure
cat src/Altinn.Correspondence.API/Models/InitializeCorrespondenceNotificationExt.cs
Length of output: 3902
Script:
#!/bin/bash
# Get the correspondence model structure
cat src/Altinn.Correspondence.API/Models/InitializeCorrespondenceExt.cs
# Search for any example usages in tests
rg -A 5 "CustomNotificationRecipients" Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceNotificationTests.cs
Length of output: 4714
Script:
#!/bin/bash
# Get the builder class content
cat Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs
# Also check the request model
rg -l "InitializeCorrespondencesRequest" --type cs
Length of output: 10383
✏️ Learnings added
Learnt from: mSunberg
PR: Altinn/altinn-correspondence#663
File: src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs:158-160
Timestamp: 2025-02-03T09:12:16.185Z
Learning: In the Altinn Correspondence API, the form field path for accessing CustomNotificationRecipients should use `customNotificationRecipients` (not `recipients`) in the path, following the model structure: `correspondence.notification.customNotificationRecipients[index].recipientToOverride`
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
thanks, what about:
correspondence.notification.customNotificationRecipients[0].emailAddress
There was a problem hiding this comment.
@mSunberg The correct form field path should be:
correspondence.notification.customNotificationRecipients[0].recipients[0].emailAddress
This is because:
customNotificationRecipientscontains a list ofCustomNotificationRecipientExt- Each
CustomNotificationRecipientExthas arecipientsproperty which is a list ofNotificationRecipientExt emailAddressis a property ofNotificationRecipientExt
🐰 thoughtfully taking notes about the model structure
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Get the model structure for CustomNotificationRecipientExt
cat src/Altinn.Correspondence.API/Models/CustomNotificationRecipientExt.cs
# Get the model structure for NotificationRecipientExt
cat src/Altinn.Correspondence.API/Models/NotificationRecipientExt.cs
# Search for any example usages in tests
rg -A 5 "EmailAddress.*Recipients" --type cs test/
Length of output: 2247
✏️ Learnings added
Learnt from: mSunberg
PR: Altinn/altinn-correspondence#663
File: src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs:158-160
Timestamp: 2025-02-03T09:16:22.006Z
Learning: In the Altinn Correspondence API, the form field path for accessing email address in CustomNotificationRecipients should follow the nested structure: `correspondence.notification.customNotificationRecipients[index].recipients[index].emailAddress` as EmailAddress is a property of NotificationRecipientExt which is contained in the Recipients list of CustomNotificationRecipientExt.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Recipient overrides with email or mobile number are not allowed when using a custom notification template because of name lookup
There was a problem hiding this comment.
@mSunberg Thank you for the clarification. I understand now - when using a custom notification template, recipient overrides cannot include email or mobile number details due to the name lookup requirements.
🐰 thoughtfully taking notes about the validation rules
✏️ Learnings added
Learnt from: mSunberg
PR: Altinn/altinn-correspondence#663
File: src/Altinn.Correspondence.Application/Helpers/InitializeCorrespondenceHelper.cs:158-160
Timestamp: 2025-02-03T09:20:02.031Z
Learning: In the Altinn Correspondence API, recipient overrides containing email addresses or mobile numbers are not allowed when using NotificationTemplate.CustomMessage due to name lookup requirements. This is validated in InitializeCorrespondenceHelper.ValidateRecipientOverrides().
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Description
Just a PR for testing functionality around CustomNotificationRecipients
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation