Changes to legacy response models #435
Conversation
|
Warning Rate limit exceeded@Andreass2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the correspondence mapping and response structures within the Altinn Correspondence API. Key modifications include the removal of 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 (
|
Andreass2
added
kind/feature-request
Andreass2
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesResponse.cs (1)
28-28: Document the purpose and usage of the new Confirmed property.The new Confirmed property has been added but lacks documentation explaining its purpose, when it gets set, and its significance in the correspondence lifecycle.
Add XML documentation to clarify the property's usage:
+ /// <summary> + /// Gets or sets the timestamp when the correspondence was confirmed by the recipient. + /// Null indicates an unconfirmed correspondence. + /// </summary> public DateTimeOffset? Confirmed { get; set; }src/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs (1)
75-79: Enhance XML documentation for InstanceOwnerPartyId.The current documentation is minimal. Consider adding more context about what this ID represents and its significance.
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (1)
101-105: Consider using a more specific error message.The error message
CouldNotFindOrgNomight not accurately describe the failure to find the resource owner party. Consider creating a new error likeResourceOwnerPartyNotFoundfor better error tracking and debugging.src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs (2)
118-130: Improve consistency in null handling.Some properties use the null-conditional operator (?.) while others don't. Consider applying consistent null handling across all properties that access potentially null objects.
- MessageTitle = correspondence.Content.MessageTitle, + MessageTitle = correspondence.Content?.MessageTitle, Status = correspondence.GetLatestStatusWithoutPurged().Status, CorrespondenceId = correspondence.Id, MinimumAuthenticationLevel = 0, Published = correspondence.Published, - PurgedStatus = purgedStatus?.Status, - Purged = purgedStatus?.StatusChanged, + PurgedStatus = purgedStatus?.Status ?? CorrespondenceStatus.None, + Purged = purgedStatus?.StatusChanged ?? DateTimeOffset.MinValue, DueDateTime = correspondence.DueDateTime, - Archived = correspondence.Statuses?.FirstOrDefault(s => s.Status == CorrespondenceStatus.Archived)?.StatusChanged, - Confirmed = correspondence.Statuses?.FirstOrDefault(s => s.Status == CorrespondenceStatus.Confirmed)?.StatusChanged, + Archived = correspondence.Statuses?.FirstOrDefault(s => s.Status == CorrespondenceStatus.Archived)?.StatusChanged ?? DateTimeOffset.MinValue, + Confirmed = correspondence.Statuses?.FirstOrDefault(s => s.Status == CorrespondenceStatus.Confirmed)?.StatusChanged ?? DateTimeOffset.MinValue,
Line range hint
147-153: Security: Implement pending authorization checks.The TODO comments indicate missing security features:
- Resource access verification for parties
- Multi-request authorization with minimum authentication level checks
These are critical security features that should be implemented before this code goes to production.
Would you like me to help create GitHub issues to track these security implementations?
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs (2)
41-47: Ensure consistency in initializing collection propertiesFor consistency, consider initializing all collection properties either by marking them as
requiredor initializing them to empty lists. Currently,Attachmentsis marked asrequiredwithout an initializer, whileReplyOptions,Notifications, andExternalReferencesare initialized to empty lists.
49-49: Consider nullability and initialization ofResourceId
ResourceIdis declared without an initial value or therequiredmodifier. IfResourceIdis mandatory and should not be null, consider marking it asrequiredor initializing it tostring.Emptyfor consistency with other string properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs(2 hunks)src/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesResponse.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.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 (9)
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesResponse.cs (1)
25-25: Property rename from DueDate to DueDateTime is appropriate but requires careful migration.
The rename provides better clarity by explicitly indicating the time component. However, as this is a breaking change, we should verify its impact on API consumers.
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about the DueDateTime usage across the codebase.
Property rename from DueDate to DueDateTime is consistently implemented across the codebase
The rename from DueDate to DueDateTime has been properly implemented throughout the codebase:
- All references use the new "DueDateTime" naming
- Migration scripts handle the database schema updates
- Error messages and handlers consistently use the new terminology
- Mappers and models across different layers (API, Core, Application) use the updated property name
- Test cases have been updated to reflect the new naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to "DueDate" that might need updating
rg -i "duedate" --type cs
Length of output: 12282
src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs (3)
24-28: Consider adding null checks for the new properties
The new property mappings might need null value handling. Consider:
- Adding null checks before mapping attachments
- Validating that required properties (Language, MessageTitle, etc.) are not null
#!/bin/bash
# Check if these properties are marked as required in the model
ast-grep --pattern 'class LegacyCorrespondenceOverviewExt {
$$$
required $_
$$$
}'
# Check if CorrespondenceAttachmentMapper handles null
rg -A 5 "MapListToExternal" --type csharp44-44: LGTM on InstanceOwnerPartyId, but verify duedate terminology
While this file correctly uses DueDateTime, the PR objective mentions changing "duedate" to "duedatetime". Let's verify this change is consistent across the codebase.
#!/bin/bash
# Search for any remaining instances of "duedate"
rg -i "duedate\b" --type csharp
# Search for "duedatetime" usage
rg -i "duedatetime\b" --type csharp23-23: Verify the impact of nullifying the Content property
Setting Content to null instead of removing the property entirely might impact API consumers. Consider either:
- Removing the property completely if it's no longer needed
- Adding a code comment explaining why it's intentionally set to null
✅ Verification successful
Setting Content to null is intentional as part of API version migration
The codebase shows that Content property handling differs between legacy and new mapper implementations:
LegacyCorrespondenceOverviewMapperexplicitly setsContent = null- New mappers (
CorrespondenceOverviewMapper,CorrespondenceDetailsMapper) useCorrespondenceContentMapper.MapToExternal()
This indicates an intentional change in the API structure where the legacy endpoint returns null for Content while newer endpoints map it properly. This is a common pattern during API versioning to maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Content property usage in the codebase
rg -A 3 "Content\s*{.*get;" --type csharp
# Search for direct Content property access
rg "\.Content\b" --type csharp
Length of output: 19726
Script:
#!/bin/bash
# Search for Content property usage in models and mappers
rg -A 3 "Content\s*{.*get;" --type csharp src/Altinn.Correspondence.API/Models/
rg -A 3 "Content\s*{.*get;" --type csharp src/Altinn.Correspondence.Core/Models/
# Search for Content property mapping in mapper classes
rg -A 5 -B 5 "Content\s*=" --type csharp src/Altinn.Correspondence.API/Mappers/
Length of output: 9644
src/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs (2)
42-47: LGTM! Well-structured Language property with proper validation.
The property is well-documented and includes appropriate validation using the ISO6391 attribute to ensure valid language codes.
Line range hint 15-19: LGTM! Property name aligns with PR objective.
The DueDateTime property name correctly implements the PR objective of changing from "duedate" to "duedatetime".
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (2)
120-120: LGTM!
Good use of String.IsNullOrWhiteSpace for the fallback logic to resource owner's name.
140-140: Verify PartyId usage.
Based on previous learnings from PR #413, the PartyId property is still under development and its validation hasn't been finalized. Please ensure this aligns with the ongoing development work.
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs (1)
15-41:
Ensure compatibility when using the required keyword from C# 11
The required keyword is a feature introduced in C# 11. Please confirm that the project's language version is set to C# 11 or higher to avoid compilation issues.
Run the following script to verify the project's language version setting:
✅ Verification successful
The required keyword is compatible with the codebase
The project is targeting .NET 8.0 (<TargetFramework>net8.0</TargetFramework>), which by default uses C# 12. This means the required keyword (introduced in C# 11) is fully supported and there are no compatibility concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the project is targeting C# 11 or higher.
# Expected result: The LangVersion should be set to 11.0 or higher in all .csproj files.
# Command: Search all .csproj files for LangVersion settings.
fd --extension csproj | xargs grep -E "<LangVersion>([0-9]+(\.[0-9]+)?|latest)</LangVersion>"
Length of output: 95
Script:
#!/bin/bash
# Let's try a different approach to check the project settings
# First, let's see all .csproj files content to analyze language settings
fd -e csproj -x cat {}
Length of output: 8950
src/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs
Outdated
Show resolved
Hide resolved
...rrespondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs
Show resolved
Hide resolved
...rrespondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs
Outdated
Show resolved
Hide resolved
...respondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs
Show resolved
Hide resolved
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Language,MessageTitle,MessageSummary, andMessageBody.ConfirmedandDueDateTimeproperties for better correspondence status tracking.InstanceOwnerPartyId,Attachments, and various metadata fields.Bug Fixes