-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Recipient to user in GetCorrespondenceHistory #415
Add Recipient to user in GetCorrespondenceHistory #415
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs (1)
19-23
: Consider comprehensive validation strategy for LegacyUser.
Given that PartyId and AuthenticationLevel are still under development and validation requirements are not yet determined, it would be beneficial to plan a comprehensive validation strategy for all properties in the LegacyUser class, including this new Recipient property.
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (1)
Line range hint 93-106
: Consider documenting the recipient information structure
Since this change introduces recipient information that will be displayed in the A2 GUI solution, it would be valuable to document the expected structure and format of the recipient information. This will help future maintainers understand the contract between the backend and frontend.
Consider:
- Adding XML documentation comments to describe the recipient property
- Documenting any validation requirements or formatting expectations
- Including examples of how this information will be displayed in the GUI
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (3 hunks)
- src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs (2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs:17-21
Timestamp: 2024-10-29T13:10:17.164Z
Learning: The `PartyId` and `AuthenticationLevel` properties in the `LegacyUser` class (in `src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs`) are still under development, and the correct validation has not been determined yet.
🔇 Additional comments (4)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs (2)
1-1
: LGTM!
The using directive is correctly added to support the new Recipient type.
22-22
: Add XML documentation for the new property.
Since this property is used in the A2 GUI, adding XML documentation would help API consumers understand its purpose and usage.
+ /// <summary>
+ /// Gets or sets the recipient information for correspondence history.
+ /// Used to display contact information in the A2 GUI solution.
+ /// </summary>
public Recipient Recipient { get; set; }
Consider null safety and validation.
Since this property is used for display purposes, consider:
- Initializing the property to avoid potential null reference exceptions
- Adding null checks in consuming code
Let's verify the property usage:
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (2)
69-73
: LGTM! Recipient parameter correctly added to notification status handling
The changes consistently enhance both SMS and Email notification blocks by including recipient information, which aligns well with the PR objective of displaying contact information in the A2 GUI solution.
Also applies to: 77-81
Line range hint 93-104
: LGTM! Recipient parameter properly integrated
The GetNotificationStatus method cleanly incorporates the new recipient information while maintaining its existing functionality.
Description
Adds contactinformation about the user in the GET call for displayment in the A2 GUI solution.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
LegacyUser
class to store recipient details.Bug Fixes
Documentation