Ct/legacy/history changes #515
Conversation
CelineTrammi
added
kind/bug
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the handling of correspondence history within the application. Key modifications include updating the response data types in 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 (6)
Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyRetrievalTests.cs (3)
84-86: Strengthen test assertions for better coverage.While the test correctly verifies the presence of "Fetched" status, it could be more robust with additional assertions:
Consider adding these assertions:
var historyData = await historyResponse.Content.ReadFromJsonAsync<List<LegacyGetCorrespondenceHistoryResponse>>(); Assert.NotNull(historyData); +Assert.Equal(2, historyData.Count); // Assuming Published and Fetched are the only expected statuses +Assert.Equal(CorrespondenceStatus.Published.ToString(), historyData[0].Status); // First status should be Published +Assert.Equal(CorrespondenceStatus.Fetched.ToString(), historyData[1].Status); // Second status should be Fetched Assert.Contains(historyData, status => status.Status.Contains(CorrespondenceStatus.Fetched.ToString()));
133-139: Enhance status verification for comprehensive testing.While the test verifies the presence of all expected statuses, it could be more thorough in validating the complete status history:
Consider enhancing the assertions:
var content = await response.Content.ReadFromJsonAsync<List<LegacyGetCorrespondenceHistoryResponse>>(_serializerOptions); Assert.NotNull(content); +Assert.Equal(4, content.Count); // Verify exact number of statuses + +// Verify chronological order and complete status objects +Assert.Collection(content, + item => { + Assert.Contains(CorrespondenceStatus.Published.ToString(), item.Status); + Assert.NotNull(item.LastUpdate); + }, + item => { + Assert.Contains(CorrespondenceStatus.Fetched.ToString(), item.Status); + Assert.True(item.LastUpdate > content[0].LastUpdate); + }, + item => { + Assert.Contains(CorrespondenceStatus.Confirmed.ToString(), item.Status); + Assert.True(item.LastUpdate > content[1].LastUpdate); + }, + item => { + Assert.Contains(CorrespondenceStatus.Archived.ToString(), item.Status); + Assert.True(item.LastUpdate > content[2].LastUpdate); + } +); -Assert.Contains(content, status => status.User.PartyId == _digdirPartyId); -Assert.Contains(content, status => status.Status.Contains(CorrespondenceStatus.Published.ToString())); -Assert.Contains(content, status => status.Status.Contains(CorrespondenceStatus.Fetched.ToString())); -Assert.Contains(content, status => status.Status.Contains(CorrespondenceStatus.Confirmed.ToString())); -Assert.Contains(content, status => status.Status.Contains(CorrespondenceStatus.Archived.ToString())); + +// Verify user information is consistent +Assert.All(content, item => Assert.Equal(_digdirPartyId, item.User.PartyId));
Line range hint
1-165: Consider architectural improvements for better test maintainability.The test class could benefit from the following improvements:
- Add data-driven tests using
[Theory]and[InlineData]for different status combinations:[Theory] [InlineData(new[] { CorrespondenceStatus.Published })] [InlineData(new[] { CorrespondenceStatus.Published, CorrespondenceStatus.Fetched })] [InlineData(new[] { CorrespondenceStatus.Published, CorrespondenceStatus.Fetched, CorrespondenceStatus.Confirmed })] public async Task LegacyGetCorrespondenceHistory_WithDifferentStatuses_ReturnsExpectedHistory(CorrespondenceStatus[] expectedStatuses)
- Add helper methods to reduce assertion duplication:
private static void AssertStatusHistory( List<LegacyGetCorrespondenceHistoryResponse> history, params CorrespondenceStatus[] expectedStatuses) { Assert.Equal(expectedStatuses.Length, history.Count); for (int i = 0; i < expectedStatuses.Length; i++) { Assert.Contains(expectedStatuses[i].ToString(), history[i].Status); if (i > 0) { Assert.True(history[i].LastUpdate > history[i-1].LastUpdate); } } }src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (3)
107-108: Correct typo in method nameGetPartyIdForNotficationThe method name
GetPartyIdForNotficationis misspelled. It should beGetPartyIdForNotification. Please update the method name and all references to it.Apply this diff to correct the method name and references:
-private async Task<int?> GetPartyIdForNotfication(Recipient recipient, CancellationToken cancellationToken) +private async Task<int?> GetPartyIdForNotification(Recipient recipient, CancellationToken cancellationToken) - int? partyId = await GetPartyIdForNotfication(recipient, cancellationToken); + int? partyId = await GetPartyIdForNotification(recipient, cancellationToken);Also applies to: 121-134
Line range hint
31-35: Ensure appropriate error messages are returnedThe error
Errors.CouldNotFindOrgNomay not accurately represent the situation whererecipientPartyorsenderPartyis null or missing SSN and OrgNumber. Consider using a more descriptive error message or creating a new error to reflect this specific condition.Also applies to: 41-45
87-134: Ensure unit tests cover new methods and changesConsider adding unit tests for the new private methods
GetCorrespondenceStatusandGetPartyIdForNotificationto ensure their correctness and to prevent future regressions.Do you need assistance in writing unit tests for these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyRetrievalTests.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs(4 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 (3)
Line range hint 5-11: Verify impact of response structure change
The removal of the History collection property indicates a significant architectural change where the response now represents a single history entry instead of a collection. This change requires careful consideration of backward compatibility and consumer updates.
Let's verify the usage of this response type across the codebase:
13-16: Verify removal of AuthenticationLevel property
The AuthenticationLevel property has been removed. Since this was previously noted as under development, please confirm:
- Authentication is handled elsewhere
- No security implications from this removal
Let's check for any remaining authentication level references:
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information to verify the authentication concerns:
Authentication is properly handled through multiple mechanisms
The removal of the AuthenticationLevel property from LegacyGetCorrespondenceHistoryResponse is safe because:
-
Authentication is handled through a robust multi-scheme system:
- The API uses
[Authorize]attributes with policy-based authorization - Authentication is managed through
CascadeAuthenticationHandlersupporting multiple schemes - Token-based authentication is implemented via
AltinnTokenEventsHelperandTokenHelper - Minimum authentication levels are still enforced through XACML policies in
IdPortenXacmlMapper
- The API uses
-
The system maintains security through:
- Proper authorization checks in
AltinnAuthorizationService - Access management via
AltinnAccessManagementService - Unauthorized access returns appropriate 401 responses (see
Errors.cs) - Authentication level validation is handled at the platform level through token validation
- Proper authorization checks in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AuthenticationLevel usage
rg -A 5 "AuthenticationLevel.*LegacyUser"
# Look for any alternative authentication implementations
rg -A 10 "authentication|authorize" --type cs
Length of output: 43981
14-14: Verify handling of nullable PartyId
The change to make PartyId nullable is significant. Based on previous learnings, this property was under development. Please ensure that:
- All consumers properly handle null values
- Validation logic is updated accordingly
- Documentation reflects when null values are expected
Let's check the validation and usage patterns:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Nullable PartyId is properly handled throughout the codebase
The change to make PartyId nullable is safe as the codebase already has comprehensive null handling:
UserClaimsHelper.GetPartyId()returns nullable int and has proper null checks- All handlers (GetCorrespondence, UpdateStatus, etc.) validate PartyId with pattern matching:
if (_userClaimsHelper.GetPartyId() is not int partyId) { return Errors.InvalidPartyId; }
- Notification status handling explicitly supports nullable PartyId through
GetPartyIdForNotfication - Tests verify invalid PartyId scenarios with dedicated test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PartyId validation and usage
rg -A 5 "PartyId" --type cs
# Look for any null checks on PartyId
ast-grep --pattern 'if ($x?.PartyId == null) { $$$ }'
Length of output: 43834
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (3)
82-84:
Correct the list initialization syntax
The initialization of joinedList uses invalid syntax. In C#, to create a list from an IEnumerable, you should use the ToList() method.
Apply this diff to fix the list initialization:
-List<LegacyGetCorrespondenceHistoryResponse> joinedList = [.. correspondenceHistory.Concat(notificationHistory).OrderByDescending(s => s.StatusChanged)];
-
-return joinedList;
+var joinedList = correspondenceHistory
+ .Concat(notificationHistory)
+ .OrderByDescending(s => s.StatusChanged)
+ .ToList();
+
+return joinedList;Likely invalid or redundant comment.
11-22:
Fix the class declaration and field initializations
The current class declaration uses an invalid syntax for C#. In C#, constructors cannot be declared inline with the class definition in this manner. Additionally, private fields should be initialized within the constructor body, not at the field declaration.
Apply this diff to correct the class declaration and field initializations:
-public class LegacyGetCorrespondenceHistoryHandler(
- ICorrespondenceRepository correspondenceRepository,
- IAltinnNotificationService altinnNotificationService,
- IAltinnRegisterService altinnRegisterService,
- IAltinnAuthorizationService altinnAuthorizationService,
- UserClaimsHelper userClaimsHelper) : IHandler<Guid, List<LegacyGetCorrespondenceHistoryResponse>>
-{
- private readonly ICorrespondenceRepository _correspondenceRepository = correspondenceRepository;
- private readonly IAltinnNotificationService _altinnNotificationService = altinnNotificationService;
- private readonly IAltinnRegisterService _altinnRegisterService = altinnRegisterService;
- private readonly IAltinnAuthorizationService _altinnAuthorizationService = altinnAuthorizationService;
- private readonly UserClaimsHelper _userClaimsHelper = userClaimsHelper;
+public class LegacyGetCorrespondenceHistoryHandler : IHandler<Guid, List<LegacyGetCorrespondenceHistoryResponse>>
+{
+ private readonly ICorrespondenceRepository _correspondenceRepository;
+ private readonly IAltinnNotificationService _altinnNotificationService;
+ private readonly IAltinnRegisterService _altinnRegisterService;
+ private readonly IAltinnAuthorizationService _altinnAuthorizationService;
+ private readonly UserClaimsHelper _userClaimsHelper;
+
+ public LegacyGetCorrespondenceHistoryHandler(
+ ICorrespondenceRepository correspondenceRepository,
+ IAltinnNotificationService altinnNotificationService,
+ IAltinnRegisterService altinnRegisterService,
+ IAltinnAuthorizationService altinnAuthorizationService,
+ UserClaimsHelper userClaimsHelper)
+ {
+ _correspondenceRepository = correspondenceRepository;
+ _altinnNotificationService = altinnNotificationService;
+ _altinnRegisterService = altinnRegisterService;
+ _altinnAuthorizationService = altinnAuthorizationService;
+ _userClaimsHelper = userClaimsHelper;
+ }Likely invalid or redundant comment.
123-132:
Verify the correctness of LookUpPartyById method usage
The LookUpPartyById method is being called with recipient.NationalIdentityNumber and recipient.OrganizationNumber, which may not be appropriate. Typically, LookUpPartyById expects a PartyId (an integer). Ensure that you're using the correct methods to look up parties by national identity number or organization number.
Run the following script to find possible methods for party lookup:
✅ Verification successful
The LookUpPartyById usage is correct
The implementation in AltinnRegisterService.LookUpPartyById shows that the method is specifically designed to handle both national identity numbers and organization numbers. It includes proper validation for:
- 11-digit person numbers (SSN)
- 9-digit organization numbers
- Organization numbers with 4-digit prefix (format: nnnn:nnnnnnnnn)
The method internally creates a PartyLookup object that correctly routes the identification to either OrgNo or Ssn field before making the API call. Therefore, the usage in LegacyGetCorrespondenceHistoryHandler.cs is appropriate and safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find methods in 'IAltinnRegisterService' for looking up parties by national identity number or organization number.
# Search for methods in 'IAltinnRegisterService' interface related to party lookup.
rg --type cs --no-heading -A 3 -B 2 'interface IAltinnRegisterService' .
# Find methods containing 'LookUpPartyBySSN' or 'LookUpPartyByOrgNumber' in the codebase.
rg --type cs --no-heading 'LookUpPartyBySSN|LookUpPartyByOrgNumber' .
Length of output: 831
Script:
#!/bin/bash
# Let's check the implementation of LookUpPartyById and related methods
# First, find all implementations of LookUpPartyById
ast-grep --pattern 'LookUpPartyById($$$)'
# Also check the actual implementation of IAltinnRegisterService
cat ./src/Altinn.Correspondence.Core/Services/IAltinnRegisterService.cs
# Find any usages of LookUpPartyId method
rg --type cs 'LookUpPartyId\(' -A 2 -B 2
Length of output: 3551
Script:
#!/bin/bash
# Let's check the implementation of LookUpPartyById in AltinnRegisterService
cat ./src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterService.cs
# Also check the dev service implementation for additional context
cat ./src/Altinn.Correspondence.Integrations/Altinn/Register/AltinnRegisterDevService.cs
Length of output: 6337
...Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs
Show resolved
Hide resolved
Description
Remove
needsConfirmationandHistoryfields from response.MinimumAuthLevel is removed from the User object.
CorrespondeneStatus
Publishednow uses the partyId fromcorrespondence.Senderinstead ofcorrespondence.RecipientPartyId for the NotificationStatuses are retrieved from either the OrganizationNumber or NationalIdentityNumber if it exists, otherwise null.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores