Ct/legacy/history/separate user and notification #516
Conversation
CelineTrammi
added
kind/bug
📝 WalkthroughWalkthroughThe pull request introduces significant changes 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 (
|
![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
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs (1)
Line range hint
1-24: Document the legacy data structure transitionThe separation of user and notification data is well-structured. Consider:
- Adding XML documentation to clarify the purpose and lifecycle of these legacy classes
- Creating a migration guide for teams consuming this API
- Planning for eventual removal of the "Legacy" prefix when the transition is complete
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (1)
50-57: Optimize asynchronous calls within the loop for better performanceCurrently,
GetCorrespondenceStatusis awaited inside aforeachloop, processing each status sequentially. To improve performance, consider processing the tasks concurrently usingTask.WhenAll. This can reduce the total processing time when handling multiple correspondence statuses.Here is a suggested refactor:
var correspondenceHistory = new List<LegacyGetCorrespondenceHistoryResponse>(); - foreach (var correspondenceStatus in correspondence.Statuses) - { - if (correspondenceStatus.Status.IsAvailableForRecipient()) - { - correspondenceHistory.Add(await GetCorrespondenceStatus(correspondenceStatus, recipientParty, senderParty, cancellationToken)); - } - } + var tasks = correspondence.Statuses + .Where(status => status.Status.IsAvailableForRecipient()) + .Select(status => GetCorrespondenceStatus(status, recipientParty, senderParty, cancellationToken)); + correspondenceHistory.AddRange(await Task.WhenAll(tasks));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs(1 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-11-12T05:02:53.065Z
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 (6)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs (2)
11-11: LGTM! Good separation of notification data
The addition of the nullable Notification property effectively separates notification-specific data from the main response object, improving the overall structure.
14-18: Verify data migration for user identification fields
The restructuring of user fields looks good, but please ensure:
- Data migration handles the transition from the old
Recipientproperty to the new structure - All consuming code is updated to handle the nullable fields
✅ Verification successful
Let me gather more specific information about the usage of these properties.
Based on the search results, I can now generate the final response:
Property changes are properly handled in the codebase
The changes to the user fields are properly integrated:
-
The
LegacyGetCorrespondenceHistoryHandlercorrectly maps the new nullable properties:Nameis populated fromparty?.NameNationalIdentityNumberis populated fromparty?.SSNorrecipient.NationalIdentityNumber- All properties are properly handled as nullable
-
The old
Recipientproperty references in the codebase are part of different contexts (notifications, dialogporten integration) and not related to this response model. -
No data migration is needed as this is just a response model restructuring, with data being mapped from existing sources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the old Recipient property to ensure complete migration
rg -l "Recipient" --type cs
# Search for direct access to the new properties to verify proper null handling
ast-grep --pattern 'NationalIdentityNumber' --lang cs
ast-grep --pattern 'Name' --lang cs
Length of output: 9037
Script:
#!/bin/bash
# Search for direct references to the new properties with context
rg -A 3 -B 3 "NationalIdentityNumber\s*[{=}]" --type cs
rg -A 3 -B 3 "Name\s*[{=}]" --type cs
# Look for the migration file that might handle this change
fd ".*Migration.*\.cs$" -x grep -l "LegacyGetCorrespondenceHistoryResponse"
# Check for any mapping or transformation of these properties
rg -A 3 -B 3 "LegacyGetCorrespondenceHistoryResponse" --type cs
Length of output: 74518
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (4)
96-98: LGTM: Party ID selection logic is clear and appropriate
The logic for selecting the partyId based on the status is well implemented and correctly determines whether to use the sender or recipient party.
101-108: LGTM: User information mapping is correctly handled
The mapping of party information to LegacyUser accommodates potential null values using the null-conditional operator, ensuring that the properties are assigned safely.
115-127: LGTM: Constructing 'LegacyGetCorrespondenceHistoryResponse' with notification details
The initialization of the response object with notification details, including the new LegacyNotification object, is appropriately structured and enhances clarity.
130-139: LGTM: Party lookup and 'User' assignment in 'GetNotificationStatus' method
The logic for looking up the party based on recipient information and assigning it to response.User is correct and accounts for cases where the party may not be found.
...orrespondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs
Show resolved
Hide resolved
CelineTrammi
deleted the
ct/legacy/history/separate-user-and-notification
branch
Description
Separate fields related user and notification into their own objects.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
LegacyNotificationclass to provide detailed recipient information, including email and mobile number.Improvements
Bug Fixes