Feature/dan 145 prefill dataset#1626
Conversation
📝 WalkthroughWalkthroughThis pull request introduces Dan integration into Altinn.App.Core by adding a new configuration class, HTTP client implementation, and service registration. It removes the Maskinporten package dependency, integrates Dan data prefilling into PrefillSI, and updates tests and public API definitions to reflect the new functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
87396d7 to
66787d2
Compare
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
6098397 to
7d37da5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs (1)
53-59: Fix null literal causing build failure.The pipeline fails with
CS8625: Cannot convert null literal to non-nullable reference type. TheIDanClientparameter is non-nullable, so passingnulldirectly is not allowed.Use a mock instead:
+ var danClientMock = new Mock<IDanClient>(); + var prefillToTest = new PrefillSI( loggerMock.Object, appResourcesMock.Object, authenticationContextMock.Object, - sp, - null + sp, + danClientMock.Object );
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Implementation/PrefillSI.cs`:
- Line 226: The code sets subject = !string.IsNullOrWhiteSpace(party.SSN) ?
party.SSN : party.OrgNumber and may pass a null/empty subject into GetDataset;
before calling GetDataset, validate that subject is not null or whitespace
(check both party.SSN and party.OrgNumber), and if it is empty either skip the
Dan API call (and log a warning/error with context using the same logger) or
throw a clear exception; update the code around the subject declaration and the
GetDataset call (reference subject, party.SSN, party.OrgNumber, and GetDataset)
so no null/empty value is sent to the Dan API.
In `@src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs`:
- Around line 29-30: The constructor sets
_httpClient.DefaultRequestHeaders.Add("Accept", "application/json") but
GetDataset calls _httpClient.DefaultRequestHeaders.Clear(), which removes that
header; fix by either (A) remove the Clear() call inside GetDataset and rely on
the default Accept header, or (B) stop mutating DefaultRequestHeaders in the
DanClient constructor and instead set the Accept header on each outgoing request
via a per-request HttpRequestMessage.Headers.Accept (or request.Headers.Add)
inside GetDataset; update the code around the DanClient constructor and
GetDataset accordingly, referencing _httpClient, DefaultRequestHeaders, the
constructor, and GetDataset.
- Around line 42-43: The code is modifying _httpClient.DefaultRequestHeaders
(clearing and adding the subscription key) which is not thread-safe for
IHttpClientFactory clients; instead, create an HttpRequestMessage inside the
DanClient request method, set the per-request header on
request.Headers.Add("Ocp-Apim-Subscription-Key",
_settings.Value.SubscriptionKey), remove the DefaultRequestHeaders.Clear/Add
calls, and send the request via _httpClient.SendAsync(request) (or Send) so each
request uses its own headers without mutating the shared HttpClient.
- Line 68: The code in DanClient.cs uses a blocking call
"result.Content.ReadAsStringAsync().Result"; change this to an asynchronous
await by replacing it with "await result.Content.ReadAsStringAsync()" and make
the enclosing method in class DanClient async (update its return type to Task or
Task<T> as appropriate) and propagate the async change to callers; ensure proper
ConfigureAwait(false) if needed for library code and remove any remaining
.Result/.Wait() usages related to this response reading.
🧹 Nitpick comments (8)
src/Altinn.App.Core/Internal/Dan/IDanClient.cs (1)
8-15: AddCancellationTokenparameter and complete the<returns>documentation.The async method performs HTTP calls (per
DanClientimplementation) but lacks aCancellationTokenparameter, preventing callers from cancelling long-running requests. Also, the<returns>XML tag is empty.♻️ Suggested improvement
/// <summary> /// Method for getting a selected dataset from Dan Api /// </summary> /// <param name="dataset">Name of the dataset</param> /// <param name="subject">Usually ssn or OrgNumber</param> /// <param name="fields">fields to fetch from endpoint</param> - /// <returns></returns> - public Task<Dictionary<string, string>> GetDataset(string dataset, string subject, string fields); + /// <param name="cancellationToken">Cancellation token</param> + /// <returns>A dictionary containing the requested field values from the dataset</returns> + public Task<Dictionary<string, string>> GetDataset(string dataset, string subject, string fields, CancellationToken cancellationToken = default);Note: The
DanClientimplementation would also need updating to pass the token toPostAsync.test/Altinn.App.Api.Tests/Program.cs (1)
70-70: IncompleteDanSettingsconfiguration may cause issues ifIDanClientis resolved.The
DanSettingsclass has two required properties (BaseUrlandSubscriptionKey), but onlyBaseUrlis configured here. If any test resolvesIDanClient, this will result in a null or emptySubscriptionKeybeing used in API calls. Consider also setting theSubscriptionKeyto a placeholder value:-builder.Services.Configure<DanSettings>(settings => settings.BaseUrl = "http://localhost:7071/v1/"); +builder.Services.Configure<DanSettings>(settings => +{ + settings.BaseUrl = "http://localhost:7071/v1/"; + settings.SubscriptionKey = "test-subscription-key"; +});src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (3)
85-96:ConvertListToDictionarydoesn't need to be async.This method performs no asynchronous operations. Wrapping synchronous work in
Task.FromResultadds unnecessary overhead.-private static Task<Dictionary<string, string>> ConvertListToDictionary(string jsonString) +private static Dictionary<string, string> ConvertListToDictionary(string jsonString) { var list = JsonConvert.DeserializeObject<List<Dictionary<string, string>>>(jsonString); if (list != null) - return Task.FromResult( - list.SelectMany(d => d) - .GroupBy(kvp => kvp.Key) - .ToDictionary(g => g.Key, g => string.Join(",", g.Select(x => x.Value))) - ); + return list.SelectMany(d => d) + .GroupBy(kvp => kvp.Key) + .ToDictionary(g => g.Key, g => string.Join(",", g.Select(x => x.Value))); - return Task.FromResult(new Dictionary<string, string>()); + return new Dictionary<string, string>(); }Then update the call site (line 73):
-dictionary = await ConvertListToDictionary(resultJson); +dictionary = ConvertListToDictionary(resultJson);
2-8: Consider consolidating to a single JSON library.The file uses both
System.Text.Json(lines 2, 8) andNewtonsoft.Json(line 6). Mixing serialization libraries increases complexity and potential for inconsistent behavior.Consider using one library consistently—preferably
System.Text.Jsonas it's the modern .NET standard and avoids the additional dependency.
65-82: Silent failure on non-success responses lacks observability.When
result.IsSuccessStatusCodeis false, an empty dictionary is returned without logging the error. This makes debugging Dan API issues difficult in production.Consider adding logging for failures:
if (result.IsSuccessStatusCode) { // ... existing code ... } +else +{ + // Log the failure with status code and response body for debugging +} return new Dictionary<string, string>();src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
100-100: DanSettings registration lacks conditional check for missing configuration.
DanSettingshas required properties (BaseUrlandSubscriptionKey) but is registered unconditionally. UnlikeNetsPaymentSettingsat lines 311-317 which usesconfigurationSection.Exists(), apps without Dan configuration will fail at startup whenDanClientattempts to instantiate and access the uninitialized required properties.Consider either making the Dan client registration conditional (matching the Nets pattern) or making the
DanSettingsproperties nullable with appropriate null-handling inDanClient.src/Altinn.App.Core/Implementation/PrefillSI.cs (2)
223-254: Await in loop and missing error handling for external API call.Two concerns with this block:
Await in loop: Per coding guidelines, avoid awaiting async operations in a loop. If dataset order doesn't affect prefill results (no overwrites between datasets), consider batching:
No exception handling: Unlike other prefill sources (ER, DSF, UserProfile) which work with in-memory objects,
GetDatasetmakes an external HTTP call that can fail. A single failing dataset will abort prefill for all subsequent datasets.♻️ Suggested improvement with error handling
foreach (var dataset in datasetList) { var datasetName = dataset.SelectToken("name")?.ToString(); var subject = !string.IsNullOrWhiteSpace(party.SSN) ? party.SSN : party.OrgNumber; var fields = dataset.SelectToken("mappings"); if (fields != null) { var danPrefill = fields .SelectMany(obj => obj.Children<JProperty>()) .ToDictionary(prop => prop.Name, prop => prop.Value.ToString()); if (datasetName != null) { + try + { var danDataset = await _danClient.GetDataset(datasetName, subject, fields.ToString()); if (danDataset.Count > 0) { JObject danJsonObject = JObject.FromObject(danDataset); _logger.LogInformation($"Started prefill from {_danKey}"); LoopThroughDictionaryAndAssignValuesToDataModel( SwapKeyValuesForPrefill(danPrefill), danJsonObject, dataModel ); } else { string errorMessage = $"Could not prefill from {_danKey}, data is not defined."; _logger.LogError(errorMessage); } + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to fetch dataset {DatasetName} from Dan API", datasetName); + // Consider whether to continue with other datasets or rethrow + } } } }
249-249: Minor: Typo in error message."Could not prefill" has a double space. This matches the existing pattern in the file (lines 152, 181, 210), but all instances could be fixed together if desired.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Directory.Packages.propssrc/Altinn.App.Core/Altinn.App.Core.csprojsrc/Altinn.App.Core/Configuration/DanSettings.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cssrc/Altinn.App.Core/Internal/Dan/IDanClient.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
💤 Files with no reviewable changes (1)
- src/Altinn.App.Core/Altinn.App.Core.csproj
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Configuration/DanSettings.cssrc/Altinn.App.Core/Internal/Dan/IDanClient.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
🧠 Learnings (15)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
Directory.Packages.propssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Internal/Dan/IDanClient.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Internal/Dan/IDanClient.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Internal/Dan/IDanClient.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Register services in DI container properly following existing patterns
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (4)
src/Altinn.App.Core/Internal/Dan/IDanClient.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (2)
Task(40-83)Task(85-96)src/Altinn.App.Core/Implementation/PrefillSI.cs (2)
Task(71-257)Dictionary(377-380)src/Altinn.App.Core/Internal/Prefill/IPrefill.cs (1)
Task(23-28)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (2)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)src/Altinn.App.Core/Internal/Dan/IDanClient.cs (1)
Task(15-15)
test/Altinn.App.Api.Tests/Program.cs (1)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)
🪛 GitHub Actions: Build and Test on windows, macos and ubuntu
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
[error] 58-58: CS8625: Cannot convert null literal to non-nullable reference type.
🪛 GitHub Check: Run dotnet build and test (macos-latest)
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
[failure] 58-58:
Cannot convert null literal to non-nullable reference type.
[failure] 58-58:
Cannot convert null literal to non-nullable reference type.
🪛 GitHub Check: Run dotnet build and test (ubuntu-latest)
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
[failure] 58-58:
Cannot convert null literal to non-nullable reference type.
[failure] 58-58:
Cannot convert null literal to non-nullable reference type.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: Build and Test
🔇 Additional comments (8)
Directory.Packages.props (1)
23-23: No issues found. Version 1.0.330 is the latest available version of JmesPath.Net on NuGet and no security vulnerabilities are reported for this package.src/Altinn.App.Core/Internal/Dan/IDanClient.cs (1)
6-6: IDanClient should remainpublic.While the coding guideline states "use internal accessibility on types by default," the established codebase pattern shows that interfaces in the
Internalnamespace are consistently keptpublic(45+ public interfaces vs 6 internal). IDanClient correctly follows this pattern, similar toIDataClient,IAuthorizationClient, and other client interfaces registered in the DI container. The learnings referenceIDataClientas "internal for DI," yetIDataClientitself remainspublicin the codebase. Only specialized internal implementations (like task handlers or token resolvers) useinternalaccessibility.Likely an incorrect or invalid review comment.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
54-61: Snapshot update only: verify API-change intent; don’t hand-edit this file.This is a PublicApiTests snapshot; changes here should come from intentional public API updates + snapshot regeneration. Please double-check that the new/changed public surface (e.g.,
Altinn.App.Core.Configuration.DanSettings,Altinn.App.Core.Internal.Dan.IDanClient,Altinn.App.Core.Infrastructure.Clients.Dan.DanClient, and theAltinn.App.Core.Implementation.PrefillSIconstructor signature) is intended and covered in release notes/changelog if applicable. Based on learnings, don’t edit*.verified.txtdirectly—regenerate via the test workflow instead.Also applies to: 2373-2373, 2404-2411, 3005-3011
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
108-108: LGTM - HttpClient registration follows established patterns.The
AddHttpClient<IDanClient, DanClient>()registration is consistent with other platform clients in this file (e.g.,IDataClient,IInstanceClient).src/Altinn.App.Core/Configuration/DanSettings.cs (1)
6-17: DanSettings accessibility is correct as-is.All configuration classes in the codebase (
GeneralSettings,CacheSettings,PlatformSettings, etc.) are public to support the .NET Options pattern used for dependency injection. DanSettings follows this established convention consistently.Likely an incorrect or invalid review comment.
src/Altinn.App.Core/Implementation/PrefillSI.cs (3)
24-29: LGTM!The new
_danClientfield and_danKeyconstant follow the established patterns for other clients and configuration keys in this class.
42-57: LGTM!Constructor injection of
IDanClientfollows the preferred DI pattern. Direct injection is cleaner than the service locator pattern used forIRegisterClient.
377-380: LGTM!The helper method is correct. Note that duplicate values in the source dictionary would throw on swap (duplicate keys), but this appropriately surfaces configuration errors.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
3350199 to
1e923a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Implementation/PrefillSI.cs`:
- Around line 244-246: The code silently skips when dataset.SelectToken("name")
yields null (datasetName == null); update the PrefillSI logic around datasetName
(the block calling _danClient.GetDataset(datasetName, subject,
fields.ToString())) to log an error or warning when datasetName is null before
skipping, including contextual info (e.g., which dataset token or prefill id) so
missing name cases are visible in logs; ensure you use the same logger used
elsewhere in PrefillSI to match existing logging conventions.
In `@src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs`:
- Around line 49-55: GetQuery can return an empty collection so calling First()
on fieldsToFill will throw; add an early guard after var fieldsToFill =
GetQuery(fields) to handle empty results (e.g., if (!fieldsToFill.Any()) return
<appropriate empty result>), and only build baseQuery and iterate when
fieldsToFill has at least one element. Update the code around fieldsToFill,
baseQuery and the foreach so it never calls First() on an empty sequence.
- Around line 6-7: The file mixes Newtonsoft.Json and System.Text.Json: the top
alias "JsonException = Newtonsoft.Json.JsonException" conflicts with IsJsonArray
which uses System.Text.Json.JsonDocument.Parse and throws
System.Text.Json.JsonException; update IsJsonArray to catch
System.Text.Json.JsonException (or remove the Newtonsoft alias and explicitly
qualify exceptions) so the catch handles the correct exception type; inspect
other methods in DanClient that parse JSON and standardize on one library or
explicitly qualify exception types to avoid ambiguity.
♻️ Duplicate comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (1)
57-58: Consider adding a more detailed comment explaining the jmespath syntax.The
[].{...}||{...}syntax is non-obvious. A brief explanation of the jmespath semantics would help maintainability.
🧹 Nitpick comments (3)
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (2)
85-96: Method returnsTaskbut performs no async work.This method wraps synchronous operations in
Task.FromResult. Per coding guidelines, avoid unnecessary allocations.♻️ Proposed refactor to simplify
- private static Task<Dictionary<string, string>> ConvertListToDictionary(string jsonString) + private static Dictionary<string, string> ConvertListToDictionary(string jsonString) { var list = JsonConvert.DeserializeObject<List<Dictionary<string, string>>>(jsonString); if (list != null) - return Task.FromResult( - list.SelectMany(d => d) + return list.SelectMany(d => d) .GroupBy(kvp => kvp.Key) - .ToDictionary(g => g.Key, g => string.Join(",", g.Select(x => x.Value))) - ); + .ToDictionary(g => g.Key, g => string.Join(",", g.Select(x => x.Value))); - return Task.FromResult(new Dictionary<string, string>()); + return new Dictionary<string, string>(); }Then update the call site at line 73:
- dictionary = await ConvertListToDictionary(resultJson); + dictionary = ConvertListToDictionary(resultJson);
111-117: Consider adding a brief XML doc comment explaining the expected JSON structure.The method extracts the first key from each dictionary in a list, which implies a specific JSON format. A brief comment documenting the expected input structure would improve maintainability.
src/Altinn.App.Core/Implementation/PrefillSI.cs (1)
223-264: Awaiting async operations in a loop.Per coding guidelines: "Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)."
However, given that:
- The number of datasets is likely small (configuration-driven)
- Each dataset call may depend on the previous result in some scenarios
- Sequential processing provides simpler error handling
This is acceptable but consider batching if performance becomes a concern with many datasets.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs
🧠 Learnings (11)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (3)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)src/Altinn.App.Core/Implementation/PrefillSI.cs (2)
Task(71-267)Dictionary(387-390)src/Altinn.App.Core/Internal/Dan/IDanClient.cs (1)
Task(15-15)
🔇 Additional comments (4)
src/Altinn.App.Core/Infrastructure/Clients/Dan/DanClient.cs (1)
25-34: LGTM!The constructor correctly creates a dedicated
HttpClientinstance and configures headers once. This addresses the previous thread-safety concerns about modifyingDefaultRequestHeadersper-request.src/Altinn.App.Core/Implementation/PrefillSI.cs (3)
24-29: LGTM!The new field and constant follow the existing patterns established for
_erKey,_dsfKey, and_userProfileKey.
42-57: LGTM!The constructor properly accepts and stores the
IDanClientdependency following the established DI pattern. Based on learnings, new features should follow established patterns with DI registration.
228-235: Good addition of subject validation.This correctly addresses the previous concern about passing null/empty subjects to the Dan API.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/Altinn.App.Api.Tests/Program.cs`:
- Line 71: The test host configures DanSettings via
builder.Services.Configure<DanSettings> but only sets BaseUrl, leaving the
required SubscriptionKey unset; update the Configure call for DanSettings (the
builder.Services.Configure<DanSettings> invocation) to also assign a valid
SubscriptionKey (for tests use a dummy/test key or read from test configuration)
so the required property is populated when the settings are resolved.
🧹 Nitpick comments (1)
test/Altinn.App.Api.Tests/Mocks/DanClientMock.cs (1)
5-10: Addsealedmodifier and consider returning a default value instead of throwing.Per coding guidelines, classes should be
sealedunless inheritance is expected. Additionally, theNotImplementedExceptionwill cause any test exercising Dan prefill paths to fail unexpectedly. Consider returning an empty dictionary for a safe default.♻️ Suggested improvement
-public class DanClientMock : IDanClient +public sealed class DanClientMock : IDanClient { public Task<Dictionary<string, string>> GetDataset(string dataset, string subject, string fields) { - throw new NotImplementedException(); + return Task.FromResult(new Dictionary<string, string>()); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/Altinn.App.Api.Tests/Mocks/DanClientMock.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
test/Altinn.App.Api.Tests/Mocks/DanClientMock.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Mocks/DanClientMock.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cs
🧠 Learnings (15)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Api.Tests/Mocks/DanClientMock.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
test/Altinn.App.Api.Tests/Mocks/DanClientMock.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Api.Tests/Program.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Api.Tests/Program.cs
🧬 Code graph analysis (2)
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs (1)
src/Altinn.App.Core/Implementation/PrefillSI.cs (2)
PrefillSI(18-391)PrefillSI(42-57)
test/Altinn.App.Api.Tests/Program.cs (2)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)test/Altinn.App.Api.Tests/Mocks/DanClientMock.cs (1)
DanClientMock(5-11)
🔇 Additional comments (6)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (4)
54-59: *Snapshot update (DanSettings) — no review comments on .verified.txt files.
This file is a public-API verification snapshot; I’m intentionally not commenting on the API surface here.
2371-2371: *Snapshot update (PrefillSI ctor signature) — no review comments on .verified.txt files.
This file is a public-API verification snapshot; I’m intentionally not commenting on the signature change here.
2402-2409: *Snapshot update (DanClient) — no review comments on .verified.txt files.
This file is a public-API verification snapshot; I’m intentionally not commenting on the type/constructor/method surface here.
3003-3009: *Snapshot update (IDanClient) — no review comments on .verified.txt files.
This file is a public-API verification snapshot; I’m intentionally not commenting on the interface surface here.test/Altinn.App.Api.Tests/Program.cs (1)
14-14: LGTM!The using directive and DI registration for
IDanClientare correctly added, following the established mock service registration pattern in this test configuration file.Also applies to: 120-120
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs (1)
4-4: LGTM!The changes correctly integrate the
IDanClientdependency using Moq, following the coding guidelines and matching the updatedPrefillSIconstructor signature. The mock doesn't need specific setup since this test only validatesPrefillDataModelusing theexternalPrefilldictionary.Also applies to: 51-51, 59-60
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Implementation/PrefillSI.cs`:
- Line 259: Fix the typo with the double space in the constructed error
message(s): update the string assigned to errorMessage in PrefillSI (look for
the assignment using $"Could not prefill from {_danKey}, data is not defined.")
to use a single space ("Could not prefill...") and make the same correction for
the other occurrence noted (the similar errorMessage assignment around the other
occurrence). Ensure both instances use the exact same corrected wording.
🧹 Nitpick comments (2)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
199-204: Configuration section naming inconsistency.The configuration section is named
"DanClientSettings"but binds toDanSettingsclass. Other patterns in this file use matching names (e.g.,NetsPaymentSettingssection binds toNetsPaymentSettingsclass). Consider renaming the section to"DanSettings"for consistency, or rename the class toDanClientSettings.Also,
DanSettingshasrequiredproperties (BaseUrl,SubscriptionKey). If the section exists but is incomplete, the app will fail at runtime. Consider adding validation:♻️ Suggested improvement
var danSettings = configuration.GetSection("DanClientSettings"); if (danSettings.Exists()) { services.Configure<DanSettings>(danSettings); services.AddHttpClient<IDanClient, DanClient>(); + services.AddOptions<DanSettings>() + .ValidateDataAnnotations() + .ValidateOnStart(); }src/Altinn.App.Core/Implementation/PrefillSI.cs (1)
223-270: Consider parallelizing dataset fetches.Per coding guidelines, awaiting async operations in a loop is discouraged. If datasets are independent (which they appear to be), consider parallelizing:
♻️ Parallel approach sketch
var tasks = datasetList .Select(async dataset => { var datasetName = dataset.SelectToken("name")?.ToString(); // ... validation ... return (datasetName, await _danClient.GetDataset(...)); }); var results = await Task.WhenAll(tasks); foreach (var (name, data) in results) { /* apply prefill */ }However, sequential processing may be intentional if datasets could have ordering dependencies or if the number of datasets is typically small. This is a nice-to-have optimization.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Program.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
🧠 Learnings (10)
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)
🔇 Additional comments (3)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
27-27: LGTM!The new using statements for Dan namespaces are correctly placed and follow the existing alphabetical ordering pattern.
Also applies to: 39-39
src/Altinn.App.Core/Implementation/PrefillSI.cs (2)
24-24: LGTM!The new
_danClientfield and_danKeyconstant follow the established patterns in this class (matching_erKey,_dsfKey,_userProfileKey).Also applies to: 29-29
246-261: Guard against null_danClientif dependency becomes optional.If
IDanClientis made optional (per previous comment), add a null check before using it:🐛 Proposed fix
+ if (_danClient == null) + { + _logger.LogWarning("Dan prefill configured but IDanClient is not registered. Skipping Dan prefill."); + return; + } + if (datasetName != null) { var danDataset = await _danClient.GetDataset(datasetName, subject, fields.ToString());Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs`:
- Around line 199-204: Move the AddHttpClient registration for IDanClient so it
only runs when the DanClientSettings section exists: check
configuration.GetSection("DanClientSettings") (the existing danSettings
variable), and if danSettings.Exists() call
services.Configure<DanSettings>(danSettings) and then
services.AddHttpClient<IDanClient, DanClient>(); This ensures DanSettings is
configured before DanClient is constructed and prevents nulls in the DanClient
constructor.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Api.Tests/Program.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Program.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Register services in DI container properly following existing patterns
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)
🔇 Additional comments (1)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
27-27: LGTM!The new using directives for Dan-related namespaces are correctly added.
Also applies to: 39-39
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Implementation/PrefillSI.cs`:
- Around line 244-256: The code dereferences the nullable field _danClient when
calling _danClient.GetDataset inside the DAN prefill block, which can cause a
NullReferenceException; add a null check for _danClient before calling
GetDataset (or return/skip the DAN prefill section if _danClient is null), so
only call _danClient.GetDataset and proceed to create danJsonObject, log
_danKey, and call SwapKeyValuesForPrefill and
LoopThroughDictionaryAndAssignValuesToDataModel when _danClient is non-null and
danDataset has items; ensure the check guards the entire sequence that uses
_danClient and related variables like danPrefill and danJsonObject.
♻️ Duplicate comments (1)
src/Altinn.App.Core/Implementation/PrefillSI.cs (1)
259-259: Typo: double space in error message.The error message contains
"Could not prefill"(double space). This was flagged in a previous review and should be corrected.✏️ Suggested fix
- string errorMessage = $"Could not prefill from {_danKey}, data is not defined."; + string errorMessage = $"Could not prefill from {_danKey}, data is not defined.";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.App.Core/Implementation/PrefillSI.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
🧠 Learnings (11)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cs
🔇 Additional comments (4)
src/Altinn.App.Core/Implementation/PrefillSI.cs (4)
24-29: LGTM!The nullable field declaration and constant follow the existing patterns in the file. Making
_danClientnullable properly supports the optional dependency injection pattern.
42-57: LGTM!The optional
danClientparameter correctly addresses the DI resolution concern. Apps without Dan configuration will receivenulland function normally.
215-242: LGTM!The subject validation at lines 228-235 correctly addresses the null subject concern. The configuration parsing and field extraction logic follows the established patterns from ER/DSF prefill sections.
263-267: LGTM!The else block correctly logs when
datasetNameis null, addressing the silent skip concern. The error message at line 265 is properly formatted.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
5dbbf81 to
1f60b5c
Compare
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
6282298 to
f8359ac
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| foreach (var jsonKey in fieldsToFill.Skip(1)) | ||
| { | ||
| //if there is more than one field to fetch, add it to the query | ||
| baseQuery += $",{jsonKey} : {jsonKey}"; |
Check notice
Code scanning / CodeQL
String concatenation in loop Note
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
5b76d9e to
b9c2879
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs`:
- Line 110: The call to PrefillDataModel is asynchronous and must be awaited;
change the test method that calls prefillToTest.PrefillDataModel(partyId,
modelName, dataModel) to be async (e.g., return Task) and prepend await to the
call so the test waits for completion before running Verify on the mock; ensure
the test method signature and any test framework attributes support async Task.
♻️ Duplicate comments (1)
src/Altinn.App.Core/Implementation/PrefillSI.cs (1)
259-259: Typo: double space in error message.The error message contains
"Could not prefill"(double space). This should be corrected for consistency.✏️ Suggested fix
- string errorMessage = $"Could not prefill from {_danKey}, data is not defined."; + string errorMessage = $"Could not prefill from {_danKey}, data is not defined.";
🧹 Nitpick comments (2)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
103-104: Consider using a mock forIDanClientin test fixtures.The fixture registers
DanClientas a concrete implementation. SinceDanClientmakes HTTP calls to an external API, tests using this fixture may fail or behave unexpectedly if the Dan API is unavailable or unconfigured.Consider registering a mock instead, similar to other mocked services in this fixture:
public Mock<IDanClient> DanClientMock { get; } = new(MockBehavior.Strict); // ... _services.TryAddSingleton(DanClientMock.Object);Alternatively, if the concrete implementation is intentional for certain integration scenarios, ensure
DanSettingsis properly configured or that tests handle the absence gracefully.test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs (1)
119-122: Consider improving test JSON readability.The inline JSON string is hard to read. Consider using a raw string literal or a separate test data file for better maintainability.
♻️ Suggested improvement using raw string literal
private string GetJsonConfig() { - return "{\n \"$schema\" : \"https://altinncdn.no/schemas/json/prefill/prefill.schema.v1.json\",\n \"allowOverwrite\" : true,\n \"DAN\" : {\n \"datasets\" : [ {\n \"name\" : \"UnitBasicInformation\",\n \"mappings\" : [ {\n \"BusinessAddressCity\" : \"Email\"\n }, {\n \"SectorCode\" : \"OrganizationNumber\"\n } ]\n } ]\n }\n}"; + return """ + { + "$schema": "https://altinncdn.no/schemas/json/prefill/prefill.schema.v1.json", + "allowOverwrite": true, + "DAN": { + "datasets": [{ + "name": "UnitBasicInformation", + "mappings": [{ + "BusinessAddressCity": "Email" + }, { + "SectorCode": "OrganizationNumber" + }] + }] + } + } + """; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Directory.Packages.propssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Api.Tests/Program.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
✅ Files skipped from review due to trivial changes (1)
- Directory.Packages.props
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Program.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
🧠 Learnings (16)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Register services in DI container properly following existing patterns
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-10-22T08:35:24.567Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1532
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:2451-2451
Timestamp: 2025-10-22T08:35:24.567Z
Learning: In Altinn/app-lib-dotnet, IDataClient is generally not implemented by customers; it’s considered internal for DI. As of 2025-10-22, GetBinaryDataStream had not been released yet, so its parameter reordering (adding optional TimeSpan? timeout before CancellationToken) is acceptable without a compatibility shim; recommend documenting the new parameter and using named arguments in samples.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cssrc/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
src/Altinn.App.Core/Implementation/PrefillSI.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
🧬 Code graph analysis (2)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
src/Altinn.App.Core/Configuration/DanSettings.cs (1)
DanSettings(6-17)
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs (1)
src/Altinn.App.Core/Implementation/PrefillSI.cs (4)
PrefillSI(18-396)PrefillSI(42-57)Task(71-272)PrefillDataModel(60-68)
🪛 GitHub Check: Run dotnet build and test (ubuntu-latest)
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs
[failure] 110-110:
Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.
[failure] 110-110:
Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Build and Test
🔇 Additional comments (8)
src/Altinn.App.Core/Implementation/PrefillSI.cs (3)
6-6: LGTM - Dan client integration setup.The imports, nullable field declaration, and key constant are correctly added to support optional Dan prefill functionality.
Also applies to: 24-24, 29-29
42-57: LGTM - Optional Dan client injection.The constructor correctly accepts
IDanClient?as an optional parameter with defaultnull, preventing DI failures when Dan is not configured. This follows the established pattern for optional services.
215-271: Dan prefill integration looks good overall.The logic correctly:
- Guards against null
_danClientat line 218- Validates subject (SSN/OrgNumber) before API call at lines 228-235
- Logs errors when dataset name is missing at lines 263-267
- Handles empty dataset responses appropriately
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (1)
9-9: LGTM - Dan client namespace imports.The using directives are correctly added to support Dan client registration.
Also applies to: 14-14
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (2)
27-27: LGTM - Dan client namespace imports.The using directives are correctly added for the Dan client infrastructure and interface.
Also applies to: 39-39
199-204: LGTM - Conditional Dan client registration.The registration correctly follows the established pattern (similar to Nets at lines 315-321):
- Check if configuration section exists
- Register HTTP client and configure settings only when configuration is present
This prevents DI failures when apps don't have Dan configured.
test/Altinn.App.Core.Tests/Implementation/PrefillSITest.cs (2)
4-4: LGTM - Added using directives for Dan integration testing.The imports for
IDanClientandPartytypes are correctly added.Also applies to: 6-6
52-52: LGTM - Existing test updated for new constructor signature.The mock for
IDanClientis correctly added and passed to thePrefillSIconstructor.Also applies to: 60-61
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
2 times, most recently
from
cbed0ec to
0cbadae
Compare
# Conflicts: # Directory.Packages.props
DI for settings if they exist Made DanClient optional moved danclient in DI
pernillehofgaard
force-pushed
the
feature/DAN-145-prefill-dataset
branch
from
ab5e15c to
39d11d5
Compare
3 participants
Allow prefill to use data from Dan
Description
Added a Http client for Dan.
Prefill can now use datasets from Dan api.
Added settings for Dan api.
Related Issue(s)
Make proof of concept for generic DAN component in altinn studio data-altinn-no/core#145
Verification
Documentation
Added documentation and examples for prefill from DAN altinn-studio-docs#2631
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.