Add ASD search in KRR by phone number - #1005
Open
SandGrainOne wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds phone-number contact preference retrieval, dashboard contact-point mapping, dashboard authorization attributes, and automated API, repository, and Bruno test coverage. ChangesPhone contact lookup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Altinn.Profile.Core/Integrations/IPersonService.cs`:
- Around line 41-42: Align GetContactPreferencesByPhoneNumberAsync’s null-input
behavior across the contract and implementation: either revise the
ArgumentNullException documentation to match the repository’s empty-list
behavior, or update PersonRepository to throw ArgumentNullException for null
phoneNumber while preserving the interface contract; apply the chosen behavior
consistently in both affected files.
In `@src/Altinn.Profile.Core/User.ContactPoints/IUserContactPointsService.cs`:
- Around line 6-8: Restore the opening XML <summary> tag immediately before the
existing interface description for IUserContactPointsService, ensuring it
correctly pairs with the closing </summary> and documents the public interface.
In `@test/Bruno/Profile/Dashboard/User` Contact Information By PhoneNumber.bru:
- Around line 67-120: Update the documentation in the phone contact information
request to describe KRR-only searches and a list response that always uses 200
OK. Remove the SI-user claims and example, change the 400 description to invalid
PhoneNumber header values, and remove the unrelated 404 status entry.
- Around line 53-58: Remove the conditional verification-timestamp assertions
from both contact tests: test/Bruno/Profile/Dashboard/User Contact Information
By PhoneNumber.bru lines 53-58 and test/Bruno/Profile/Dashboard/User Contact
Information By Email.bru lines 53-58. Keep the contact value checks unchanged
and allow emailAddress or phoneNumber to have independently nullable
corresponding timestamps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0097feab-a029-42ed-940c-f6c26f30e235
📒 Files selected for processing (9)
src/Altinn.Profile.Core/Integrations/IPersonService.cssrc/Altinn.Profile.Core/User.ContactPoints/IUserContactPointsService.cssrc/Altinn.Profile.Core/User.ContactPoints/UserContactPointService.cssrc/Altinn.Profile.Integrations/Repositories/PersonRepository.cssrc/Altinn.Profile/Controllers/DashboardController.cstest/Altinn.Profile.Tests/IntegrationTests/API/Controllers/DashboardUserPrivateContactInformationControllerTests.cstest/Altinn.Profile.Tests/Profile.Integrations/Person/PersonRepositoryTests.cstest/Bruno/Profile/Dashboard/User Contact Information By Email.brutest/Bruno/Profile/Dashboard/User Contact Information By PhoneNumber.bru
Comment on lines
+53
to
+58
| if (contact.emailAddress !== null) { | ||
| expect(contact.emailLastUpdatedOrVerified).to.be.a('string'); | ||
| } | ||
| if (contact.phoneNumber !== null) { | ||
| expect(contact.phoneNumberLastUpdatedOrVerified).to.be.a('string'); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow nullable verification timestamps independently of contact values. The response model and controller mapping permit a populated email/phone value with a null corresponding timestamp, so these assertions can reject valid responses.
test/Bruno/Profile/Dashboard/User Contact Information By PhoneNumber.bru#L53-L58: remove the assertions requiring timestamps when the contact value is non-null.test/Bruno/Profile/Dashboard/User Contact Information By Email.bru#L53-L58: remove the same requirement to keep both request tests aligned with the nullable response contract.
📍 Affects 2 files
test/Bruno/Profile/Dashboard/User Contact Information By PhoneNumber.bru#L53-L58(this comment)test/Bruno/Profile/Dashboard/User Contact Information By Email.bru#L53-L58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Bruno/Profile/Dashboard/User` Contact Information By PhoneNumber.bru
around lines 53 - 58, Remove the conditional verification-timestamp assertions
from both contact tests: test/Bruno/Profile/Dashboard/User Contact Information
By PhoneNumber.bru lines 53-58 and test/Bruno/Profile/Dashboard/User Contact
Information By Email.bru lines 53-58. Keep the contact value checks unchanged
and allow emailAddress or phoneNumber to have independently nullable
corresponding timestamps.
Comment on lines
+67
to
+120
| # Get Contact Information by Phone number | ||
|
|
||
| Retrieves personal contact information that the user has registered in the national registry (KRR) and from SI users private contact information | ||
|
|
||
| ## Authentication | ||
|
|
||
| Requires Dashboard Maskinporten scope: `altinn:profile.support.admin` | ||
|
|
||
| ## Parameters | ||
|
|
||
| - `PhoneNumber` (header): Phone number to search for | ||
|
|
||
| ## Response Examples | ||
|
|
||
| ```json | ||
| { | ||
| "nationalIdentityNumber": "01010112345", | ||
| "isReserved": true, | ||
| "emailAddress": null, | ||
| "emailLastUpdatedOrVerified": "2025-01-10T11:30:00Z", | ||
| "phoneNumber": "+4798765432", | ||
| "phoneNumberLastUpdatedOrVerified": "2025-01-15T14:20:00Z", | ||
| "username": null | ||
| } | ||
| ``` | ||
|
|
||
| ```json | ||
| { | ||
| "nationalIdentityNumber": null, | ||
| "isReserved": false, | ||
| "emailAddress": "jane.smith@example.com", | ||
| "emailLastUpdatedOrVerified": "2025-01-10T11:30:00Z", | ||
| "phoneNumber": null, | ||
| "phoneNumberLastUpdatedOrVerified": null, | ||
| "username": "epost:jane.smith@example.com" | ||
| } | ||
| ``` | ||
|
|
||
| ## Features | ||
|
|
||
| - Returns the contact information that the given user (identified by NIN) has registered in the national common contact register (KRR) | ||
| - `phoneNumberLastUpdatedOrVerified` reflects the last time that the user updated or verified their phone number (whichever operation was performed most recently). | ||
| - equivalent for email | ||
| - holds `null` if the user has not yet registered a value for that address type | ||
| - may have a value for cases where the user has added and then removed the address | ||
| - `emailLastUpdatedOrVerified` may have a value even though `emailAddress` is null, if the user has intentionally removed a previous email address from their contact information settings | ||
| - Does not apply for `phoneNumber`, as that field cannot be removed once set (only updated to a different valid number) | ||
|
|
||
| ## Status Codes | ||
|
|
||
| - **200 OK**: Successfully retrieved user contact information | ||
| - **400 Bad Request**: Invalid request parameters (invalid Email header value) | ||
| - **403 Forbidden**: Missing required scope | ||
| - **404 Not Found**: User with the provided National Identity Number does not exist |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the phone endpoint documentation.
This endpoint searches KRR only, not SI users, and valid requests always return 200 OK with a list. Remove the SI example/claims and the unrelated 404 status; describe an invalid PhoneNumber header for 400.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Bruno/Profile/Dashboard/User` Contact Information By PhoneNumber.bru
around lines 67 - 120, Update the documentation in the phone contact information
request to describe KRR-only searches and a list response that always uses 200
OK. Remove the SI-user claims and example, change the 400 description to invalid
PhoneNumber header values, and remove the unrelated 404 status entry.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adding an endpoint to the Dashboard spesific API. The new endpoint will enable phone number based search in KRR.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit