Add web hook support for nets #1599
Conversation
📝 WalkthroughWalkthroughAdds Nets webhook payload and listener, integrates webhook callback registration into Nets payment flow, extends payment service and telemetry, centralizes Altinn task-type constants, makes InstanceClient internal and replaces token provider with IAuthenticationTokenResolver, and propagates per-call StorageAuthenticationMethod? and CancellationToken across instance/data APIs; updates tests and test fixtures. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
ivarne
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Altinn.App.Core/Internal/Data/DataService.cs (1)
118-139: Stream should be disposed after deserialization.The
dataStreamreturned fromGetBinaryDatais not disposed, which could lead to resource leaks.private async Task<T> GetDataForDataElement<T>( InstanceIdentifier instanceIdentifier, DataElement dataElement, StorageAuthenticationMethod? authenticationMethod = null ) { - Stream dataStream = await _dataClient.GetBinaryData( + await using Stream dataStream = await _dataClient.GetBinaryData( instanceIdentifier.InstanceOwnerPartyId, instanceIdentifier.InstanceGuid, new Guid(dataElement.Id), authenticationMethod ); if (dataStream == null) { throw new ArgumentNullException( $"Failed to retrieve binary dataStream from dataClient using dataElement.Id {dataElement.Id}." ); } return await JsonSerializer.DeserializeAsync<T>(dataStream, _jsonSerializerOptions) ?? throw new InvalidOperationException($"Unable to deserialize data from dataStream to type {nameof(T)}."); }src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
132-149: QueryInstances should propagate CancellationToken.The
GetInstancesmethod receives aCancellationTokenbut doesn't propagate it to the privateQueryInstanceshelper, meaning HTTP calls during pagination cannot be cancelled.- private async Task<QueryResponse<Instance>> QueryInstances(string token, string url) + private async Task<QueryResponse<Instance>> QueryInstances(string token, string url, CancellationToken ct = default) { using var activity = _telemetry?.StartQueryInstancesActivity(); - HttpResponseMessage response = await _client.GetAsync(token, url); + HttpResponseMessage response = await _client.GetAsync(token, url, cancellationToken: ct); if (response.StatusCode == HttpStatusCode.OK) { QueryResponse<Instance> queryResponse = await JsonSerializerPermissive.DeserializeAsync< QueryResponse<Instance> - >(response.Content); + >(response.Content, ct); return queryResponse; }Then update the calls in
GetInstances:- QueryResponse<Instance> queryResponse = await QueryInstances(token, apiUrl); + QueryResponse<Instance> queryResponse = await QueryInstances(token, apiUrl, ct); ... - queryResponse = await QueryInstances(token, queryResponse.Next); + queryResponse = await QueryInstances(token, queryResponse.Next, ct);
🧹 Nitpick comments (12)
src/Altinn.App.Core/Internal/Instances/IInstanceClient.cs (1)
65-70: Consider documenting new params consistently on all affected methods.
Some methods have<param>docs forauthenticationMethod/ct, others don’t—minor, but improves API clarity.Also applies to: 82-91, 99-107, 116-124, 133-144, 153-164, 173-183, 201-212, 229-237
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs (1)
37-43: Test mock should forwardct(and optionallyauthenticationMethod) to keep semantics consistent.
Right now the new parameters are accepted but largely ignored, including not passingctintoReadJsonFile/WriteJsonFile, and not forwarding inGetInstance(Instance ...).public async Task<Instance> GetInstance( Instance instance, StorageAuthenticationMethod? authenticationMethod = null, CancellationToken ct = default ) { string app = instance.AppId.Split("/")[1]; string org = instance.Org; int instanceOwnerId = int.Parse(instance.InstanceOwner.PartyId); Guid instanceGuid = Guid.Parse(instance.Id.Split("/")[1]); - return await GetInstance(app, org, instanceOwnerId, instanceGuid); + return await GetInstance(app, org, instanceOwnerId, instanceGuid, authenticationMethod, ct); }(And similarly: thread
ctintoReadJsonFile(...),WriteJsonFile(...),WaitAsync(...)call sites where feasible.)Also applies to: 69-82, 84-104, 106-110, 142-150, 226-233, 235-240, 264-270, 296-302, 333-339, 376-382, 419-425, 457-461
src/Altinn.App.Api/Controllers/InstancesController.cs (1)
163-169: (Optional) Consider usingHttpContext.RequestAbortedconsistently across controller calls.
This method now acceptscancellationToken, but other internal calls in this controller still rely onRequestAbortedor no token—worth standardizing when you touch those paths next.src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (1)
30-35: Add XML docs (and consider CancellationToken) for new interface method
This is a new internal contract surface; it should be self-describing like the others, and webhook handling is typically cancellation-aware.@@ - Task HandlePaymentCompletedWebhook( + /// <summary> + /// Handle a payment provider "completed" webhook and update internal payment state for the instance. + /// </summary> + Task HandlePaymentCompletedWebhook( Instance instance, ValidAltinnPaymentConfiguration paymentConfiguration, StorageAuthenticationMethod storageAuthenticationMethod );(If the implementation does I/O, consider adding an optional
CancellationToken cancellationToken = defaultas well, but I can’t confirm usage from the provided files.)test/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cs (1)
5-5: Consider a local helper to DRY repeatedGetInstancesetups
The signature expansion forced repeated boilerplate; a tiny helper in this test class would cut noise and reduce future churn.@@ public class SigningControllerTests { @@ + private ISetup<IInstanceClient, Task<Instance>> SetupGetInstanceReturns(Instance instance) => + _instanceClientMock.Setup(x => + x.GetInstance( + It.IsAny<string>(), + It.IsAny<string>(), + It.IsAny<int>(), + It.IsAny<Guid>(), + It.IsAny<StorageAuthenticationMethod?>(), + It.IsAny<CancellationToken>() + ) + ).ReturnsAsync(instance); @@ - _instanceClientMock - .Setup(x => - x.GetInstance( - It.IsAny<string>(), - It.IsAny<string>(), - It.IsAny<int>(), - It.IsAny<Guid>(), - It.IsAny<StorageAuthenticationMethod?>(), - It.IsAny<CancellationToken>() - ) - ) - .ReturnsAsync( - new Instance - { - ... - } - ); + SetupGetInstanceReturns( + new Instance + { + ... + } + );Also applies to: 75-85, 608-618, 732-742, 783-793, 865-875
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (1)
200-271: New webhook handler looks structurally sound.The implementation correctly mirrors the existing
CheckAndStorePaymentStatuslogic while using the providedstorageAuthenticationMethodfor data access operations.A few observations:
Log message (Line 206): The log message "Checking payment status for instance..." is identical to
CheckAndStorePaymentStatus. Consider making it more specific, e.g., "Handling payment completed webhook for instance...".Silent returns: When
paymentInformationis null (Lines 215-223) or status isSkipped(Lines 228-237), the method returns silently. The caller has no indication of what happened. This may be intentional for webhook idempotency, but consider if a return type (e.g.,Task<bool>or a result enum) would be useful for observability/logging at the caller level.- public async Task HandlePaymentCompletedWebhook( + public async Task HandlePaymentCompletedWebhook( Instance instance, ValidAltinnPaymentConfiguration paymentConfiguration, StorageAuthenticationMethod storageAuthenticationMethod ) { - _logger.LogInformation("Checking payment status for instance {InstanceId}.", instance.Id); + _logger.LogInformation("Handling payment completed webhook for instance {InstanceId}.", instance.Id);test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (1)
1-758: Missing test coverage forHandlePaymentCompletedWebhook.The new
HandlePaymentCompletedWebhookmethod inPaymentService(seesrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cs, Lines 200-271) is not covered by any tests in this file.Consider adding tests for:
- Successfully updating payment status via webhook
- Early return when
paymentInformationis null- Early return when status is
PaymentStatus.Skipped- Exception when
PaymentProcessoris not found- Exception when
PaymentDetailsis nullWould you like me to generate test cases for the
HandlePaymentCompletedWebhookmethod?src/Altinn.App.Api/Controllers/PaymentController.cs (3)
36-44: Consider using explicit constructor injection instead of IServiceProvider.Using
IServiceProviderdirectly (service locator pattern) obscures dependencies and makes testing harder. The existing endpoints in this controller already had explicit DI before this change.- public PaymentController(IServiceProvider serviceProvider) + public PaymentController( + IInstanceClient instanceClient, + IProcessReader processReader, + IPaymentService paymentService, + AppImplementationFactory appImplementationFactory, + ILogger<PaymentController> logger, + IOptions<NetsPaymentSettings> netsPaymentSettings) { - _instanceClient = serviceProvider.GetRequiredService<IInstanceClient>(); - _processReader = serviceProvider.GetRequiredService<IProcessReader>(); - _logger = serviceProvider.GetRequiredService<ILogger<PaymentController>>(); - _netsPaymentSettings = serviceProvider.GetRequiredService<IOptions<NetsPaymentSettings>>(); - _paymentService = serviceProvider.GetRequiredService<IPaymentService>(); - _appImplementationFactory = serviceProvider.GetRequiredService<AppImplementationFactory>(); + _instanceClient = instanceClient; + _processReader = processReader; + _paymentService = paymentService; + _appImplementationFactory = appImplementationFactory; + _logger = logger; + _netsPaymentSettings = netsPaymentSettings; }
133-140: Consider adding CancellationToken parameter.The endpoint makes async calls but doesn't propagate a
CancellationToken. This allows request cancellation to be honored.public async Task<IActionResult> PaymentWebhookListener( [FromRoute] string org, [FromRoute] string app, [FromRoute] int instanceOwnerPartyId, [FromRoute] Guid instanceGuid, [FromBody] NetsCompleteWebhookPayload webhookPayload, - [FromHeader(Name = "Authorization")] string authorizationHeader + [FromHeader(Name = "Authorization")] string authorizationHeader, + CancellationToken ct = default )Then propagate
cttoGetInstanceandHandlePaymentCompletedWebhookcalls.
160-164: Consider validating the webhook event name.The
EventNamefrom the webhook payload is not validated. Per Nets webhook configuration, this endpoint is intended forpayment.checkout.completedevents. Validating this provides defense in depth._logger.LogInformation( "Received valid Nets webhook callback for instance {InstanceGuid} for {Payment}", instanceGuid, webhookPayload.Data.PaymentId ); + + if (webhookPayload.EventName != "payment.checkout.completed") + { + _logger.LogWarning( + "Received unexpected Nets webhook event {EventName} for instance {InstanceGuid}. Ignoring.", + webhookPayload.EventName, + instanceGuid + ); + return Ok(); // Acknowledge but don't process + }src/Altinn.App.Core/Internal/Data/DataService.cs (1)
92-99: Use named parameter for consistency.
InsertJsonObjectusesauthenticationMethod: authenticationMethod(line 76) butUpdateBinaryDatauses positional. Consider using named parameter for clarity and consistency.return await _dataClient.UpdateBinaryData( instanceIdentifier, "application/json", dataTypeId + ".json", dataElementId, referenceStream, - authenticationMethod + authenticationMethod: authenticationMethod );src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
300-306: Inconsistent error handling: consider throwing instead of returning null.This method returns
nullon failure while other methods throwPlatformHttpException. The#nullable disablepragma masks a design issue. This is existing behavior, but consider aligning with the pattern used elsewhere.if (response.StatusCode == HttpStatusCode.OK) { Instance instance = await JsonSerializerPermissive.DeserializeAsync<Instance>(response.Content, ct); return instance; } _logger.LogError( $"Could not update read status for instance {instanceOwnerPartyId}/{instanceGuid}. Request failed with status code {response.StatusCode}" ); -#nullable disable - return null; -#nullable restore + throw await PlatformHttpException.CreateAsync(response); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/Altinn.App.Api/Controllers/InstancesController.cs(1 hunks)src/Altinn.App.Api/Controllers/PaymentController.cs(3 hunks)src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cs(1 hunks)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs(1 hunks)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs(2 hunks)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs(1 hunks)src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs(10 hunks)src/Altinn.App.Core/Internal/Data/DataService.cs(6 hunks)src/Altinn.App.Core/Internal/Data/IDataService.cs(2 hunks)src/Altinn.App.Core/Internal/Instances/IInstanceClient.cs(8 hunks)src/Altinn.App.Core/Internal/Process/ProcessEngine.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cs(2 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs(13 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs(9 hunks)test/Altinn.App.Api.Tests/Controllers/PdfControllerTests.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cs(6 hunks)test/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cs(5 hunks)test/Altinn.App.Api.Tests/Controllers/ValidateControllerValidateDataTests.cs(1 hunks)test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs(13 hunks)test/Altinn.App.Core.Tests/Features/Action/SigningUserActionTests.cs(1 hunks)test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs(7 hunks)test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs(18 hunks)test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs(14 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs(14 hunks)test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cs(3 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)
🧰 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/Features/Payment/Services/IPaymentService.cstest/Altinn.App.Api.Tests/Controllers/ValidateControllerValidateDataTests.cstest/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cstest/Altinn.App.Core.Tests/Features/Action/SigningUserActionTests.cstest/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cstest/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Controllers/PdfControllerTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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/Controllers/ValidateControllerValidateDataTests.cstest/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cstest/Altinn.App.Core.Tests/Features/Action/SigningUserActionTests.cstest/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cstest/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Controllers/PdfControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
🧠 Learnings (17)
📓 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: 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.
📚 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/Features/Payment/Services/IPaymentService.cstest/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cstest/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cssrc/Altinn.App.Api/Controllers/InstancesController.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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.Api.Tests/Controllers/ValidateControllerValidateDataTests.cstest/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cstest/Altinn.App.Core.Tests/Features/Action/SigningUserActionTests.cstest/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cstest/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Controllers/PdfControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.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.Api.Tests/Controllers/ValidateControllerValidateDataTests.cstest/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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:
test/Altinn.App.Api.Tests/Controllers/ValidateControllerValidateDataTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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 **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ValidateControllerValidateDataTests.cstest/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.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.Api.Tests/Controllers/ValidateControllerValidateDataTests.cstest/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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:
test/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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.Api.Tests/Controllers/SigningControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-10-30T14:57:20.739Z
Learnt from: danielskovli
Repo: Altinn/app-lib-dotnet PR: 1550
File: src/Altinn.App.Core/Internal/Process/ProcessEngine.cs:295-297
Timestamp: 2025-10-30T14:57:20.739Z
Learning: In ProcessEngine.ProcessNext() for service tasks, after handling a service task, processNextAction is intentionally set to `(serviceTaskResult as ServiceTaskFailedResult)?.ErrorHandling.Action`, which will be null for successful service tasks or the ErrorHandling.Action for failed tasks. This override of the original request.Action is the desired behavior, as service tasks control what action (if any) should be passed to HandleMoveToNext.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.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:
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.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/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 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/Data/IDataService.cssrc/Altinn.App.Core/Internal/Data/DataService.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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:
src/Altinn.App.Core/Internal/Data/IDataService.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cssrc/Altinn.App.Core/Internal/Instances/IInstanceClient.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:
test/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cstest/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
🧬 Code graph analysis (10)
src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (4)
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (2)
Instance(717-728)ValidAltinnPaymentConfiguration(750-757)test/Altinn.App.Core.Tests/Features/Action/PaymentUserActionTests.cs (1)
Instance(149-158)test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs (1)
Instance(266-277)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPaymentConfiguration.cs (1)
ValidAltinnPaymentConfiguration(24-38)
test/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cs (1)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)
test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs (3)
src/Altinn.App.Core/Features/Signing/Services/SigningDelegationService.cs (1)
Guid(146-156)src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Api.Tests/Controllers/SigningControllerTests.cs (2)
src/Altinn.App.Core/Features/Maskinporten/Constants/JwtClaimTypes.cs (1)
Altinn(17-30)src/Altinn.App.Core/Features/Signing/Services/SigningDelegationService.cs (1)
Guid(146-156)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (1)
src/Altinn.App.Core/Features/AuthenticationMethod.cs (1)
AuthenticationMethod(44-44)
test/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs (1)
test/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cs (1)
Mock(49-85)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (1)
src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsNotifications.cs (2)
NetsNotifications(6-12)NetsWebHook(14-36)
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (5)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (3)
Task(45-144)Task(147-156)Task(159-202)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
Task(15-19)Task(24-28)Task(30-34)Task(39-39)Task(44-44)src/Altinn.App.Core/Features/Payment/Models/PaymentInformation.cs (1)
PaymentInformation(6-27)src/Altinn.App.Core/Features/Payment/Exceptions/PaymentException.cs (2)
PaymentException(8-16)PaymentException(14-15)src/Altinn.App.Core/Features/Payment/Models/PaymentDetails.cs (1)
PaymentDetails(6-52)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
test/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cs (1)
Mock(49-85)
test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs (1)
src/Altinn.App.Core/Internal/Instances/IInstanceClient.cs (1)
Task(16-23)
🪛 GitHub Actions: Verify dotnet format
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
[error] 99-99: CSharpier formatting issue detected. Code around line 99 is not formatted according to CSharpier. Run 'dotnet csharpier --write' to fix formatting.
🪛 Gitleaks (8.30.0)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs
[high] 506-506: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
⏰ 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). (5)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (21)
test/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs (1)
123-133: Good update to extendedUpdateProcessAndEventssignature (auth + ct).
Mocks now match the new interface surface cleanly.Also applies to: 142-153, 188-198, 207-218, 259-269, 278-289, 330-340, 349-360, 401-411, 420-431, 492-502, 512-523
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
182-186: Good: propagatectwhen re-fetching instance in the loop.test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs (1)
176-185: Good test updates forIInstanceClient.GetInstance(..., auth, ct)usage.Also applies to: 238-247, 344-353, 409-418, 474-483, 539-548, 571-590
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (1)
57-63: Good alignment with new token resolution flow (IAuthenticationTokenResolver).Also applies to: 88-94, 128-134, 170-176, 292-298, 324-330, 469-475
test/Altinn.App.Core.Tests/Features/Action/SigningUserActionTests.cs (1)
110-121: Good: mock setup updated to the newGetInstance(instance, auth, ct)signature.src/Altinn.App.Api/Controllers/InstancesController.cs (1)
140-146: Good: propagate requestCancellationTokeninto storage calls.Also applies to: 163-169, 176-181
src/Altinn.App.Core/Internal/Instances/IInstanceClient.cs (1)
16-77: XML documentation is complete and consistent across all modified methods; no additional release notes required.All modified methods in
IInstanceClientinclude proper<param>documentation for the new optional parameters (authenticationMethodandct). SinceIInstanceClientis internal (used for DI only), per established patterns in this codebase, the XML documentation is the appropriate level of documentation. The interface changes follow the same approach validated in prior PRs (#1474, #1532) where optional parameters on public/internal members are acceptable when properly documented.test/Altinn.App.Api.Tests/Controllers/ValidateControllerTests.cs (1)
64-75: Mock signature updates look correct; optional DRY as follow-up
All updated setups match the expandedGetInstancesignature. If this pattern repeats across many tests, a local helper (or fixture helper) would reduce churn.Also applies to: 102-113, 156-167, 197-208, 266-276
test/Altinn.App.Core.Tests/Internal/Process/ServiceTasks/Legacy/EformidlingServiceTaskLegacyTests.cs (1)
3-3: Test updates align with expandedIInstanceClient.GetInstanceoverload
Setup/Verify now match the new signature and remain strict.Also applies to: 34-38, 47-50
test/Altinn.App.Api.Tests/Controllers/PdfControllerTests.cs (1)
52-62: GetInstance mock updated correctly for new signature
[such changes are mechanical; this one looks consistent with the new API surface.]test/Altinn.App.Api.Tests/Controllers/FileScanControllerTests.cs (1)
72-82: LGTM - Mock setup correctly updated for new interface signature.The mock setup properly accommodates the expanded
GetInstancesignature withStorageAuthenticationMethod?andCancellationTokenparameters usingIt.IsAny<>matchers.test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
158-168: LGTM - Mock setup correctly updated for expanded GetInstance signature.The mock setup properly includes
StorageAuthenticationMethod?andCancellationTokenparameters withIt.IsAny<>matchers, maintaining test coverage while accommodating the interface changes.test/Altinn.App.Api.Tests/Controllers/ValidateControllerValidateDataTests.cs (1)
242-253: LGTM - Mock setup correctly updated for new interface signature.The mock setup appropriately accommodates the expanded
GetInstancesignature while preserving the specific parameter matching forapp,org,instanceOwnerId, andtestScenario.InstanceId.test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (1)
68-86: LGTM - Mock setups correctly updated for new IDataService signatures.The mock configurations for
GetByType<PaymentInformation>andInsertJsonObjectproperly include theStorageAuthenticationMethodparameter, maintaining alignment with the updated interface.src/Altinn.App.Core/Internal/Data/IDataService.cs (1)
19-25: Interface expansion looks good.The addition of optional
StorageAuthenticationMethod?parameters maintains backward compatibility while enabling per-call authentication context. This aligns with the broader pattern across storage operations in this PR.Based on learnings, optional parameters are acceptable in this repo since consumers upgrade via NuGet and rebuild.
src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cs (2)
8-39: Model structure looks good.The webhook payload model is well-designed with
sealedclasses,requiredproperties, and proper JSON serialization attributes.
44-52: Payload data class is appropriately minimal.The TODO comment for adding properties in the future is reasonable. The current implementation captures the essential
PaymentIdneeded for webhook processing.src/Altinn.App.Core/Internal/Data/DataService.cs (1)
24-42: Good defensive null handling in GetByType.The early return with
(Guid.Empty, default)when no data element is found is a good improvement over potentially throwing exceptions.src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (2)
25-32: Class visibility and authentication refactoring looks good.Making
InstanceClientinternal and usingIAuthenticationTokenResolverwith a default authentication method is a clean approach that enables per-call authentication overrides.
61-87: Good implementation of per-call authentication and cancellation support.The pattern of accepting optional
authenticationMethodandCancellationTokenparameters, with proper propagation to HTTP operations and deserialization, is well implemented.test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
3184-3197: Skip: snapshot file update (public API verification output).
This*.verified.txtis a generated verification snapshot; I’m not providing review suggestions on it.
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
Show resolved
Hide resolved
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs
Show resolved
Hide resolved
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs
Show resolved
Hide resolved
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
Show resolved
Hide resolved
test/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs
Dismissed
Show dismissed
Hide dismissed
ivarne
force-pushed
the
fix/nets-callback
branch
from
ab3b4eb to
999d9d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Altinn.App.Core/Internal/Data/DataService.cs (1)
123-139: Potential resource leak:dataStreamis not disposed.The
Streamreturned byGetBinaryDatashould be disposed after deserialization to avoid resource leaks. Consider wrapping in ausingstatement or callingawait using.🔎 Proposed fix
private async Task<T> GetDataForDataElement<T>( InstanceIdentifier instanceIdentifier, DataElement dataElement, StorageAuthenticationMethod? authenticationMethod = null ) { - Stream dataStream = await _dataClient.GetBinaryData( + await using Stream dataStream = await _dataClient.GetBinaryData( instanceIdentifier.InstanceOwnerPartyId, instanceIdentifier.InstanceGuid, new Guid(dataElement.Id), authenticationMethod ); if (dataStream == null) { throw new ArgumentNullException( $"Failed to retrieve binary dataStream from dataClient using dataElement.Id {dataElement.Id}." ); } return await JsonSerializer.DeserializeAsync<T>(dataStream, _jsonSerializerOptions) ?? throw new InvalidOperationException($"Unable to deserialize data from dataStream to type {nameof(T)}."); }src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
132-149:CancellationTokennot propagated toQueryInstances.The private
QueryInstancesmethod is called fromGetInstanceswhich accepts aCancellationToken, butctis not passed through. This means pagination queries cannot be cancelled.🔎 Proposed fix
- private async Task<QueryResponse<Instance>> QueryInstances(string token, string url) + private async Task<QueryResponse<Instance>> QueryInstances(string token, string url, CancellationToken ct = default) { using var activity = _telemetry?.StartQueryInstancesActivity(); - HttpResponseMessage response = await _client.GetAsync(token, url); + HttpResponseMessage response = await _client.GetAsync(token, url, cancellationToken: ct); if (response.StatusCode == HttpStatusCode.OK) { QueryResponse<Instance> queryResponse = await JsonSerializerPermissive.DeserializeAsync< QueryResponse<Instance> - >(response.Content); + >(response.Content, ct); return queryResponse; } else { _logger.LogError("Unable to query instances from Platform Storage"); throw await PlatformHttpException.CreateAsync(response); } }And update the call sites in
GetInstances:- QueryResponse<Instance> queryResponse = await QueryInstances(token, apiUrl); + QueryResponse<Instance> queryResponse = await QueryInstances(token, apiUrl, ct); ... - queryResponse = await QueryInstances(token, queryResponse.Next); + queryResponse = await QueryInstances(token, queryResponse.Next, ct);
♻️ Duplicate comments (5)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
427-433: Fix mock signatures to use nullableStorageAuthenticationMethod?parameter.Multiple mock setups for
IInstanceClient.GetInstance(Instance, ...)use non-nullableIt.IsAny<StorageAuthenticationMethod>(), but the interface defines the parameter as nullableStorageAuthenticationMethod?. This signature mismatch could prevent the mock from matching actual calls. Update all four occurrences (lines 430, 804, 1028, 1259) to useIt.IsAny<StorageAuthenticationMethod?>(), matching the correct pattern shown at line 579.🔎 Proposed fix
Apply this change at lines 430, 804, 1028, and 1259:
- It.IsAny<StorageAuthenticationMethod>(), + It.IsAny<StorageAuthenticationMethod?>(),Also applies to: 801-807, 1025-1031, 1256-1262
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (1)
502-508: Hardcoded JWT literal still triggers secret scanning.As noted in previous review comments, the hardcoded JWT-like string triggers gitleaks failures. Consider replacing with a helper function or non-JWT placeholder as previously suggested.
test/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs (1)
550-560: Fix Moq argument matching for list instances.As noted in previous review comments, using
new()inSetup/VerifyforList<InstanceEvent>relies on reference equality and will not match a different empty list instance created by the SUT. Replace withIt.Is<List<InstanceEvent>>(e => e.Count == 0)orIt.IsAny<List<InstanceEvent>>()as previously suggested.Also applies to: 569-579
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (1)
95-107: Critical: Guard null webhook key, use baseUrl variable, and fix CSharpier formatting.This is a repeat of the previous review. Three issues must be fixed:
- Security: Setting
Authorizationto a null/emptyWebhookCallbackKeycreates an unauthenticated webhook that can be spoofed by attackers.- Correctness: Line 103 uses
_generalSettings.FormattedExternalAppBaseUrl(_app)instead of thebaseUrlvariable computed at line 51, potentially breaking webhook URLs.- Formatting: Remove parentheses from
new NetsNotifications()andnew NetsWebHook()per CSharpier standards.🔎 Required fix
- Notifications = new NetsNotifications() - { - WebHooks = - [ - new NetsWebHook() + Notifications = + string.IsNullOrWhiteSpace(_settings.WebhookCallbackKey) + ? null + : new NetsNotifications { - Authorization = _settings.WebhookCallbackKey, - Url = - $"{_generalSettings.FormattedExternalAppBaseUrl(_app)}instances/{instance.Id}/payment/nets-webhook-listener", - EventName = "payment.checkout.completed", - }, - ], - }, + WebHooks = + [ + new NetsWebHook + { + Authorization = _settings.WebhookCallbackKey, + Url = $"{baseUrl}instances/{instance.Id}/payment/nets-webhook-listener", + EventName = "payment.checkout.completed", + }, + ], + },test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs (1)
120-122: Use nullableStorageAuthenticationMethod?consistently in verifications.Lines 120, 186, 251, 318, and 384 use
It.IsAny<StorageAuthenticationMethod>()(non-nullable) while other verifications in this file useIt.IsAny<StorageAuthenticationMethod?>()(nullable). Since the method signature likely accepts the nullable form, all verifications should match for consistency and correctness.🔎 Recommended fix
- It.IsAny<StorageAuthenticationMethod>(), + It.IsAny<StorageAuthenticationMethod?>(), It.IsAny<CancellationToken>()Also applies to lines 186, 251, 318, 384.
🧹 Nitpick comments (10)
test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs (3)
40-42: Dispose the ActivitySource to prevent resource leaks.The
ActivitySourceis instantiated but never disposed. Consider implementingIDisposableorIAsyncDisposableonFakeHttpMessageHandlerto properly dispose of theActivitySource.🔎 Proposed fix
-public class FakeHttpMessageHandler : HttpMessageHandler +public class FakeHttpMessageHandler : HttpMessageHandler, IDisposable { public ActivitySource ActivitySource { get; } = new("Altinn.App.Tests.Common.Fixtures.FakeHttpMessageHandler"); + + protected override void Dispose(bool disposing) + { + if (disposing) + { + ActivitySource?.Dispose(); + } + base.Dispose(disposing); + }
199-231: Pass the CancellationToken to the endpoint handler.The
cancellationTokenparameter received inSendAsyncis not propagated to the endpoint'sHandlemethod. While the currentHandlesignature doesn't accept aCancellationToken, consider either:
- Adding
CancellationTokento theHandlesignature in theEndpointabstract class, or- Using the token to enable cancellation of endpoint matching/handling
54-64: Add input validation for constructor parameters.Consider validating that
minimumCallsis non-negative and thaturlPatternis not null or empty to catch configuration errors early in tests.🔎 Proposed validation
public UrlPatternEndpoint( HttpMethod method, string urlPattern, Func<HttpRequestMessage, Task<HttpResponseMessage>> handler, int minimumCalls ) { + ArgumentNullException.ThrowIfNull(method); + ArgumentException.ThrowIfNullOrWhiteSpace(urlPattern); + ArgumentNullException.ThrowIfNull(handler); + ArgumentOutOfRangeException.ThrowIfNegative(minimumCalls); + _method = method; _urlPattern = urlPattern; _handler = handler; _minimumCalls = minimumCalls; }test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (3)
151-170: Consider thread safety for the mock registry.The
_mocksdictionary is accessed without synchronization. IfMock<T>()is called concurrently from multiple threads during test setup, race conditions could occur.🔎 Proposed fix using ConcurrentDictionary
-private readonly Dictionary<Type, Mock> _mocks = []; +private readonly ConcurrentDictionary<Type, Mock> _mocks = new(); public Mock<T> Mock<T>() where T : class { var type = typeof(T); - if (_mocks.TryGetValue(type, out var existingMock)) - { - return (Mock<T>)existingMock; - } - var mock = new Mock<T>(MockBehavior.Strict); - _mocks[type] = mock; - Services.TryAddSingleton(mock.Object); - return mock; + return (Mock<T>)_mocks.GetOrAdd(type, _ => + { + var mock = new Mock<T>(MockBehavior.Strict); + Services.TryAddSingleton(mock.Object); + return mock; + }); }
293-317: Consider potential constructor exceptions.The
WrappedServiceProviderconstructor performs significant initialization (building the service provider, configuring OpenTelemetry) that could throw exceptions. While this is acceptable for test infrastructure, ensure that calling code handles potential failures gracefully.
433-444: Ensure FakeHttpMessageHandler disposal.The
FakeHttpMessageHandlercontains anActivitySourcethat should be disposed. Consider either:
- Disposing
FakeHttpMessageHandlerexplicitly inDispose/DisposeAsync, or- Ensuring
FakeHttpMessageHandlerimplementsIDisposableand is registered with a disposal scope🔎 Proposed disposal
public void Dispose() { _serviceProvider.Dispose(); if (!_dumpedTracesAndMetrics) { _dumpedTracesAndMetrics = true; DumpTracesAndMetrics(); } + _serviceCollection.FakeHttpMessageHandler.Dispose(); _loggerProvider.Dispose(); _tracerProvider.Dispose(); _meterProvider.Dispose(); }src/Altinn.App.Core/Internal/Data/IDataService.cs (1)
1-69: Extending IDataService with StorageAuthenticationMethod looks consistentAdding optional StorageAuthenticationMethod? parameters to GetByType, GetById, InsertJsonObject, UpdateJsonObject, and DeleteById aligns this interface with the rest of the storage APIs and keeps call sites source‑compatible by appending the parameters with defaults. You might optionally add
<param>XML docs for the new authenticationMethod arguments on InsertJsonObject/UpdateJsonObject for consistency with the other methods, but the interface itself looks good.test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs (1)
4-43: InstanceClientMockSi correctly mirrors extended IInstanceClient signaturesUpdating the mock methods to include optional StorageAuthenticationMethod? and CancellationToken parameters keeps it aligned with IInstanceClient while preserving existing behavior (the new parameters are safely ignored). If you ever need to test cancellation behavior, you could thread the ct into ReadJsonFile/WriteJsonFile, but that’s not required for current usage.
Also applies to: 70-75, 84-91, 106-110, 142-147, 226-230, 235-240, 264-270, 296-302, 333-339, 376-382, 419-425, 457-461
src/Altinn.App.Core/Constants/AltinnTaskTypes.cs (1)
1-47: LGTM! Clean centralization of task type constants.The internal static class appropriately centralizes task type identifiers. Documentation is clear and the internal visibility is correct for framework-internal use.
Optional: Minor documentation improvement
Line 24 reads "push update the instance" – consider "push updates to the instance" or "push an update to the instance" for clarity.
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (1)
303-309: Consider adding error handling for process advancement failures.Lines 303-309 call
RunProcessNextwithout try-catch. If the HTTP call fails, the webhook handler will throw an unhandled exception. Consider wrapping in try-catch and logging the failure while still returning success for the webhook (since payment status was updated successfully).🔎 Optional improvement
if ( paymentStatus == PaymentStatus.Paid && instance.Process.CurrentTask.AltinnTaskType == AltinnTaskTypes.Payment ) { - await RunProcessNext(instance, storageAuthenticationMethod); + try + { + await RunProcessNext(instance, storageAuthenticationMethod); + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to advance process for instance {InstanceId} after webhook payment completion.", instance.Id); + // Don't rethrow - payment status update succeeded, process can be advanced manually + } }
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Altinn.App.Api/Controllers/PaymentController.cs (1)
185-187: Add null guards for Process and CurrentTask in webhook listener.Lines 185-187 access
instance.Process.CurrentTask.ElementIdwithout checking ifProcessorCurrentTaskis null. This is inconsistent with the defensive checks added toGetPaymentInformation(lines 69-72) and could throwNullReferenceExceptionif the webhook arrives when the instance is not in an active task state.🔎 Proposed fix
+ if (instance.Process?.CurrentTask?.ElementId == null) + { + _logger.LogWarning( + "Instance {InstanceGuid} has no current task. Cannot process Nets webhook callback.", + instanceGuid + ); + return BadRequest("Instance has no current task"); + } + AltinnPaymentConfiguration? paymentConfiguration = _processReader .GetAltinnTaskExtension(instance.Process.CurrentTask.ElementId) ?.PaymentConfiguration;src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (1)
321-343: Add timeout to HttpClient in RunProcessNext.The
HttpClientcreated on line 324 uses the default timeout (100 seconds), which may be excessive for webhook-driven process advancement. A hung external app could block webhook processing indefinitely. Error handling has been improved with detailed logging andPaymentException, but an explicit timeout (e.g., 30 seconds) would further improve resilience.🔎 Recommended timeout
JwtToken token = await _authenticationTokenResolver.GetAccessToken(storageAuthenticationMethod); using var client = _httpClientFactory.CreateClient(); + client.Timeout = TimeSpan.FromSeconds(30); client.BaseAddress = new Uri(_generalSettings.FormattedExternalAppBaseUrl(_app));Based on past review comments.
🧹 Nitpick comments (2)
test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs (2)
162-162: Consider thread safety for concurrent setup scenarios.
_endpointsis not thread-safe. While typical test fixtures register endpoints before execution (avoiding concurrency), ifRegisterEndpointis called concurrently withSendAsync, race conditions could occur. This is likely acceptable for test fixtures but consider usingConcurrentBag<Endpoint>or documenting the thread-safety expectations if concurrent registration becomes a use case.
177-228: Use the importedMediaTypeHeaderValuefor consistency.Lines 195 and 221 use the fully qualified
System.Net.Http.Headers.MediaTypeHeaderValuewhenMediaTypeHeaderValueis already available via the import on Line 4.🔎 Proposed simplification
Content = new StringContent(content) { - Headers = { ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue(contentType) }, + Headers = { ContentType = new MediaTypeHeaderValue(contentType) }, },And similarly on line 221:
Content = new StringContent(System.Text.Json.JsonSerializer.Serialize(responseObject)) { Headers = { - ContentType = new System.Net.Http.Headers.MediaTypeHeaderValue("application/json"), + ContentType = new MediaTypeHeaderValue("application/json"), }, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Altinn.App.Api/Controllers/PaymentController.cs(4 hunks)src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cs(2 hunks)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs(4 hunks)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs(2 hunks)src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs(4 hunks)src/Altinn.App.Core/Features/Telemetry/Telemetry.Payment.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs(13 hunks)test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs(41 hunks)test/Altinn.App.Clients.Fiks.Tests/TestFixture.cs(2 hunks)test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs(9 hunks)test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs(15 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs(16 hunks)test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs(1 hunks)test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Features/Telemetry/Telemetry.Payment.cs
- src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.
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.
📚 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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs
📚 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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Clients.Fiks.Tests/TestFixture.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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:
test/Altinn.App.Clients.Fiks.Tests/TestFixture.cssrc/Altinn.App.Api/Controllers/PaymentController.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.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.Clients.Fiks.Tests/TestFixture.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.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:
test/Altinn.App.Clients.Fiks.Tests/TestFixture.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.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:
test/Altinn.App.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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:
test/Altinn.App.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.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.Clients.Fiks.Tests/TestFixture.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.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.Clients.Fiks.Tests/TestFixture.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.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 **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cstest/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs
🧬 Code graph analysis (6)
test/Altinn.App.Clients.Fiks.Tests/TestFixture.cs (1)
src/Altinn.App.Core/Constants/AltinnTaskTypes.cs (1)
AltinnTaskTypes(6-47)
src/Altinn.App.Api/Controllers/PaymentController.cs (5)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs (1)
NetsPaymentSettings(8-88)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (3)
Task(48-153)Task(156-165)Task(168-211)src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (7)
Task(55-151)Task(154-238)Task(240-319)Task(321-343)Task(346-363)Task(366-381)Task(383-414)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
Task(15-19)Task(24-28)Task(30-34)Task(39-39)Task(44-44)src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cs (1)
NetsCompleteWebhookPayload(8-39)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (1)
src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsNotifications.cs (2)
NetsNotifications(6-12)NetsWebHook(14-36)
src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cs (1)
src/Altinn.App.Core/Constants/AltinnTaskTypes.cs (1)
AltinnTaskTypes(6-47)
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (4)
src/Altinn.App.Core/Configuration/GeneralSettings.cs (2)
GeneralSettings(9-86)FormattedExternalAppBaseUrl(61-68)src/Altinn.App.Core/Features/Payment/Exceptions/PaymentException.cs (2)
PaymentException(8-16)PaymentException(14-15)src/Altinn.App.Core/Features/Payment/Models/PaymentInformation.cs (1)
PaymentInformation(6-27)src/Altinn.App.Core/Features/Telemetry/TelemetryActivityExtensions.cs (1)
Errored(421-428)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (2)
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
ApplicationMetadata(1385-1417)test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
ApplicationMetadata(209-215)
🪛 GitHub Actions: Build and Test on windows, macos and ubuntu
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
[error] 19-19: IDE0052: Private member '_app' can be removed as the value assigned to it is never read.
🪛 GitHub Check: Run dotnet build and test (macos-latest)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
[failure] 19-19:
Private member 'NetsPaymentProcessor._app' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)
[failure] 19-19:
Private member 'NetsPaymentProcessor._app' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)
🪛 GitHub Check: Run dotnet build and test (ubuntu-latest)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
[failure] 19-19:
Private member 'NetsPaymentProcessor._app' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)
[failure] 19-19:
Private member 'NetsPaymentProcessor._app' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)
⏰ 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: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (21)
test/Altinn.App.Clients.Fiks.Tests/TestFixture.cs (1)
10-10: LGTM! Good refactoring to centralized constants.The migration to use
AltinnTaskTypes.FiksArkivimproves maintainability by consolidating task type identifiers in a single location.Also applies to: 90-91
src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivHost.cs (1)
8-8: LGTM! Consistent use of centralized constants.The change to use
AltinnTaskTypes.FiksArkivaligns with the test fixture update and centralizes task type identifiers, improving code maintainability.Also applies to: 269-274
test/Altinn.App.Api.Tests/Controllers/InstancesController_ActiveInstancesTests.cs (1)
30-38: LGTM! Mock setups and verifications correctly updated.All
GetInstancesmock setups and verifications have been consistently updated to include the newStorageAuthenticationMethod?andCancellationTokenparameters. The nullable formIt.IsAny<StorageAuthenticationMethod?>()correctly matches the actual parameter type, addressing the concern from the previous review.Also applies to: 50-58, 94-103, 115-123, 159-167, 180-188, 222-230, 246-254, 289-297, 313-321, 355-363, 379-387
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs (1)
158-168: LGTM! Test mocks correctly updated for extended interface signatures.The test file has been properly updated to reflect the addition of optional
StorageAuthenticationMethod?andCancellationTokenparameters acrossIInstanceClientandIDataClientmethods. All mock setups and verifications consistently useIt.IsAny<StorageAuthenticationMethod?>()with the nullable matcher, correctly matching the interface signatures. The changes maintain test coverage across all CopyInstance scenarios (authorization checks, binary data handling, excluded types, etc.).Also applies to: 215-224, 272-281, 328-337, 402-434, 457-477, 551-582, 603-625, 664-718, 776-807, 828-879, 1000-1032, 1052-1078, 1231-1263, 1284-1309
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (2)
31-40: Constructor refactored to acceptApplicationMetadatadirectly.The constructor signature change from optional string parameters to requiring
ApplicationMetadatais a breaking change for test code, but aligns with the PR's goal to support webhook authentication. The defensive initialization ofTitleandDataTypesensures test fixtures have valid state.
48-48: Good defensive practice.Assigning
instance.AppIdfromAppMetadata.Idwhen null ensures consistency in test fixtures.test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs (2)
96-141: Well-implemented pattern matching logic.The
MatchUrlPatternmethod correctly handles both wildcard (*) and parameter ({param}) matching. The implementation properly:
- Advances through wildcards until
/or end- Skips parameter names in patterns while consuming path segments in URLs
- Ensures full match by verifying
j == url.Lengthat the endThe use of
ReadOnlySpan<char>is efficient for avoiding unnecessary allocations during pattern matching.
230-273: Solid implementation of request handling and diagnostics.The
SendAsyncimplementation provides excellent test support:
- Content buffering (line 241) enables multiple reads
- Activity tracking provides observability for debugging test failures
- Clear error messages distinguish between no matches and ambiguous matches
- Thread-safe
RequestResponsescollection captures all interactionstest/Altinn.App.Core.Tests/Internal/Process/ProcessEventHandlingTests.cs (2)
2-2: LGTM: Enum-based task types improve type safety.The migration from string literals to
AltinnTaskTypesconstants enhances maintainability and reduces the risk of typos in task type references.Also applies to: 247-247, 318-318, 462-462
118-128: LGTM: Mock updates correctly reflect expanded IInstanceClient signature.The addition of optional
StorageAuthenticationMethod?andCancellationTokenparameters to allUpdateProcessAndEventsmock setups and verifications is consistent and correct, usingIt.IsAny<T>()to match any value passed by the SUT.Also applies to: 137-148, 183-193, 202-213, 254-264, 273-284, 325-335, 344-355, 396-406, 415-426, 487-497, 507-518, 544-554, 563-574
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (2)
60-65: LGTM: Webhook key validation prevents unauthenticated callbacks.The early validation ensures that webhook registration only occurs when a secure key is configured, improving security posture.
101-112: LGTM: Webhook registration structure is correct.The
NetsNotificationspayload correctly configures the webhook with authorization header, callback URL, and event subscription for payment completion.test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (2)
493-502: LGTM: Dummy JWT helper addresses secret-scanning concerns.The
CreateDummyJwt()helper programmatically generates a JWT-like token for testing, avoiding hardcoded JWT literals that trigger secret scanners.Also applies to: 513-515
23-23: LGTM: Migration to IAuthenticationTokenResolver is consistent.All test methods correctly use the new
IAuthenticationTokenResolvermock instead of the deprecatedIUserTokenProvider, aligning with the authentication refactoring across the codebase.Also applies to: 57-63, 88-94, 128-134, 207-213, 243-249, 292-298, 366-372, 415-421, 469-475
src/Altinn.App.Api/Controllers/PaymentController.cs (3)
38-46: LGTM: Service locator pattern simplifies DI in controller.Resolving dependencies from
IServiceProviderwithin the constructor is acceptable here, especially given the growing number of dependencies for webhook support.
69-72: LGTM: Defensive null checks prevent NPE in GetPaymentInformation.The early guard for
instance.Process?.CurrentTask?.ElementIdpreventsNullReferenceExceptionif the instance is not in an active task state.
158-169: LGTM: Constant-time comparison mitigates timing attacks.Using
CryptographicOperations.FixedTimeEqualsprevents attackers from extracting the webhook key through timing analysis.test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs (2)
19-42: LGTM: DI-driven testing with HTTP stubs improves realism.The refactoring from direct
INetsClientmocking toMockedServiceCollectionwithHttpMessageHandler-based endpoint stubs more closely simulates production behavior and enables verification of actual HTTP request payloads. This aligns with the established testing patterns in the codebase.As per coding guidelines and learnings.
Also applies to: 57-88, 124-151
29-29: LGTM: Test data now includes webhook key and uses task type constants.Including
WebhookCallbackKeyin test settings and usingAltinnTaskTypes.Paymentinstead of string literals improves test coverage and type safety.Also applies to: 302-302
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (2)
61-150: LGTM: Payment start flow enhanced with validation, telemetry, and zero-sum handling.The expanded
StartPaymentmethod correctly validatesIOrderDetailsCalculatorpresence, wraps order calculation in telemetry, selects the appropriate processor, and skips processor invocation for zero-sum orders. Error handling via activity erroring is consistent with established patterns.
240-319: LGTM: Webhook handler integrates payment status check and process advancement.The
HandlePaymentCompletedWebhookmethod correctly retrieves payment information using ServiceOwner authentication, updates status via the processor, persists changes, and advances the process when payment is complete and task type matchesAltinnTaskTypes.Payment.
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
279-310: Inconsistent error handling returns null instead of throwing.Unlike other methods in this class that throw
PlatformHttpExceptionon non-OK responses,UpdateReadStatusreturnsnullwhen the update fails (line 308). This inconsistency requires callers to handle both exceptions and null returns, and the#nullable disable/#nullable restorepragmas suppress compiler warnings that would otherwise catch this.Consider aligning with the pattern used in other methods:
🔎 Proposed fix
if (response.StatusCode == HttpStatusCode.OK) { Instance instance = await JsonSerializerPermissive.DeserializeAsync<Instance>(response.Content, ct); return instance; } - _logger.LogError( - $"Could not update read status for instance {instanceOwnerPartyId}/{instanceGuid}. Request failed with status code {response.StatusCode}" - ); -#nullable disable - return null; -#nullable restore + _logger.LogError( + $"Could not update read status for instance {instanceOwnerPartyId}/{instanceGuid}. Request failed with status code {response.StatusCode}" + ); + throw await PlatformHttpException.CreateAsync(response);
♻️ Duplicate comments (3)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
328-328: Security scanner warning appears to be a false positive.The GitHub Advanced Security scanner flagged this line for potential XSS. However, this code serializes a
Substatusobject to JSON for an HTTP request body sent to the Platform Storage API, not for rendering in HTML or JavaScript. The serialized content does not flow to a web page or script context where XSS would be a concern.src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (1)
322-344: Add timeout to prevent indefinite hangs.Error handling has been properly added (lines 332-343), but the HTTP client still lacks a timeout. If the process advancement endpoint is slow or unresponsive, the webhook handler will hang indefinitely.
🔎 Recommended fix
private async Task RunProcessNext(Instance instance, StorageAuthenticationMethod storageAuthenticationMethod) { JwtToken token = await _authenticationTokenResolver.GetAccessToken(storageAuthenticationMethod); using var client = _httpClientFactory.CreateClient(); + client.Timeout = TimeSpan.FromSeconds(30); client.BaseAddress = new Uri(_generalSettings.FormattedExternalAppBaseUrl(_app)); client.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue( "Bearer", token.Value ); var response = await client.PutAsync($"instances/{instance.Id}/process/next", null); if (!response.IsSuccessStatusCode) { _logger.LogError( "Failed to advance process for instance {InstanceId} after payment completed webhook. Status code: {StatusCode}\n\n{content}", instance.Id, response.StatusCode, await response.Content.ReadAsStringAsync() ); throw new PaymentException( $"Failed to advance process for instance {instance.Id} after payment completed webhook." ); } }test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs (1)
42-46: Dispose the ActivitySource to prevent resource leaks.
ActivitySourceimplementsIDisposablebut is never disposed. While it's declared as a static field, it should still be properly disposed to prevent resource leaks, especially in test scenarios where the fixture may be created multiple times.Consider implementing
IDisposableonFakeHttpMessageHandlerand disposing theActivitySourceduring cleanup, or use a static disposal pattern with proper lifecycle management.Based on coding guidelines, "Remember to dispose IDisposable/IAsyncDisposable instances".
🧹 Nitpick comments (5)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (2)
25-25: Consider sealing the class.Since this class is
internaland implements an interface, consider marking itsealedto prevent unintended inheritance.🔎 Proposed change
-internal class InstanceClient : IInstanceClient +internal sealed class InstanceClient : IInstanceClientAs per coding guidelines: "Use sealed for classes unless inheritance is considered a valid use-case."
244-244: Consider using the provided cancellation token in error logging.While reading the error response with
CancellationToken.Noneensures error details are logged even when the operation is cancelled, it's inconsistent with cancellation token usage elsewhere in the file. Consider usingctfor consistency, or document whyCancellationToken.Noneis intentional here.src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (1)
30-38: Good security design: verify payment status with provider.The documentation correctly emphasizes that the implementation should call the provider for status verification rather than trusting the webhook payload alone. This is a critical security practice.
Consider adding a
CancellationTokenparameter for consistency with the other async methods in this interface. This would allow callers to cancel long-running webhook processing.src/Altinn.App.Api/Controllers/PaymentController.cs (2)
38-46: Consider using standard constructor injection instead of IServiceProvider.Using
IServiceProviderfor resolving dependencies is an anti-pattern known as "service locator." Standard constructor injection is preferred for testability and explicit dependency declaration.However, if this pattern is intentionally used to defer resolution or handle optional services, please document that reasoning.
🔎 Suggested refactor using constructor injection
- public PaymentController(IServiceProvider serviceProvider) + public PaymentController( + IInstanceClient instanceClient, + IProcessReader processReader, + ILogger<PaymentController> logger, + IOptions<NetsPaymentSettings> netsPaymentSettings, + IPaymentService paymentService, + AppImplementationFactory appImplementationFactory) { - _instanceClient = serviceProvider.GetRequiredService<IInstanceClient>(); - _processReader = serviceProvider.GetRequiredService<IProcessReader>(); - _logger = serviceProvider.GetRequiredService<ILogger<PaymentController>>(); - _netsPaymentSettings = serviceProvider.GetRequiredService<IOptions<NetsPaymentSettings>>(); - _paymentService = serviceProvider.GetRequiredService<IPaymentService>(); - _appImplementationFactory = serviceProvider.GetRequiredService<AppImplementationFactory>(); + _instanceClient = instanceClient; + _processReader = processReader; + _logger = logger; + _netsPaymentSettings = netsPaymentSettings; + _paymentService = paymentService; + _appImplementationFactory = appImplementationFactory; }
148-156: Consider returning 503 instead of 404 for missing configuration.Returning 404 "Not Found" when
WebhookCallbackKeyis not configured could inadvertently signal to an attacker that the endpoint exists but is misconfigured. A 503 "Service Unavailable" might be more appropriate for a configuration issue, as it doesn't reveal as much about the system state.Alternatively, you could return 200 OK (silent accept) to avoid giving any signal to potential attackers, since the webhook caller (Nets) won't act differently based on the response.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Altinn.App.Api/Controllers/PaymentController.cssrc/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cssrc/Altinn.App.Core/Features/Payment/Services/IPaymentService.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cssrc/Altinn.App.Core/Internal/Data/DataService.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cstest/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/PaymentProcessTaskTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- test/Altinn.App.Core.Tests/Features/Action/UniqueSignatureAuthorizerTests.cs
- src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs
- test/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/PaymentProcessTaskTests.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.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cssrc/Altinn.App.Core/Features/Payment/Services/IPaymentService.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.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.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs
🧠 Learnings (21)
📓 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: 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.
📚 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.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs
📚 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/Eformidling/Implementation/DefaultEFormidlingServiceTests.cstest/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cssrc/Altinn.App.Core/Internal/Data/DataService.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/Eformidling/Implementation/DefaultEFormidlingServiceTests.cssrc/Altinn.App.Core/Features/Payment/Services/IPaymentService.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cssrc/Altinn.App.Core/Internal/Data/DataService.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.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.Api/Controllers/PaymentController.cssrc/Altinn.App.Core/Internal/Data/IDataService.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.Api/Controllers/PaymentController.cstest/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.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/Features/Signing/SigningReceiptServiceTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cstest/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.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.Core.Tests/Features/Signing/SigningReceiptServiceTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Internal/Data/DataService.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.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.Core.Tests/Features/Signing/SigningReceiptServiceTests.cssrc/Altinn.App.Core/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cssrc/Altinn.App.Core/Internal/Data/DataService.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/Internal/Data/IDataService.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/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Internal/Data/DataService.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/Internal/Data/IDataService.cstest/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.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 **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cstest/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cstest/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.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.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: Include integration tests for platform service interactions
Applied to files:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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: Follow the AppFixture pattern for integration tests: Central orchestrator managing test lifecycle with feature-specific operations
Applied to files:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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 : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs
🧬 Code graph analysis (14)
test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
test/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cs (1)
Instance(184-195)test/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/PaymentProcessTaskTests.cs (1)
Instance(261-272)test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs (1)
Instance(294-305)test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (1)
ValidAltinnPaymentConfiguration(784-791)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPaymentConfiguration.cs (1)
ValidAltinnPaymentConfiguration(24-38)
src/Altinn.App.Api/Controllers/PaymentController.cs (5)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs (1)
NetsPaymentSettings(8-88)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
Task(15-19)Task(24-28)Task(34-38)Task(43-43)Task(48-48)src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cs (1)
NetsCompleteWebhookPayload(8-39)src/Altinn.App.Core/Internal/Process/ProcessTasks/PaymentProcessTask.cs (1)
AltinnPaymentConfiguration(102-116)src/Altinn.App.Core/Features/Payment/Exceptions/PaymentException.cs (2)
PaymentException(8-16)PaymentException(14-15)
test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (1)
src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cs (1)
src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (2)
test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs (4)
FakeHttpMessageHandler(42-283)Verify(34-34)Verify(152-160)Verify(276-282)test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (2)
StorageClientInterceptor(12-332)StorageClientInterceptor(31-40)
test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs (1)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)
test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs (2)
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (2)
StorageAuthenticationMethod(623-628)StorageAuthenticationMethod(630-631)src/Altinn.App.Core/Features/AuthenticationMethod.cs (5)
StorageAuthenticationMethod(85-85)StorageAuthenticationMethod(88-88)StorageAuthenticationMethod(91-92)StorageAuthenticationMethod(95-96)StorageAuthenticationMethod(100-103)
src/Altinn.App.Core/Internal/Data/DataService.cs (1)
src/Altinn.App.Core/Internal/Data/IDataService.cs (3)
Task(22-27)Task(38-43)Task(48-54)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (2)
src/Altinn.App.Core/Internal/Instances/IInstanceClient.cs (14)
Task(16-23)Task(28-32)Task(37-41)Task(46-50)Task(55-60)Task(71-77)Task(92-97)Task(108-114)Task(125-131)Task(145-151)Task(165-171)Task(184-199)Task(213-227)Task(238-244)src/Altinn.App.Core/Helpers/JsonSerializerPermissive.cs (1)
JsonSerializerPermissive(10-48)
🪛 GitHub Check: Run dotnet build and test (ubuntu-latest)
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs
[failure] 237-237:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 577-577:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 493-493:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 461-461:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 450-450:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 409-409:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 397-397:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 385-385:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 247-247:
An expression tree may not contain a call or invocation that uses optional arguments
[failure] 237-237:
An expression tree may not contain a call or invocation that uses optional arguments
⏰ 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). (4)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
🔇 Additional comments (32)
test/Altinn.App.Core.Tests/Features/Validators/LegacyValidationServiceTests/ValidationServiceTests.cs (1)
297-311: LGTM! Test mock correctly updated for nullable authentication parameter.The mock setup correctly reflects the API change that introduces optional
StorageAuthenticationMethod?parameters. UsingIt.IsAny<StorageAuthenticationMethod?>()properly matches the new nullable signature while maintaining test functionality.test/Altinn.App.Core.Tests/LayoutExpressions/FullTests/DataAccessorFixture.cs (1)
228-238: LGTM! Mock updated correctly for new authentication parameter.The mock setup has been correctly updated to accept the new nullable
StorageAuthenticationMethod?parameter, matching the updatedGetDataBytessignature. UsingIt.IsAny<StorageAuthenticationMethod?>()is appropriate for a general test fixture, allowing tests to pass any authentication method (or null).test/Altinn.App.Core.Tests/Internal/Process/ProcessTasks/Common/ProcessTaskDataLockerTests.cs (4)
52-61: LGTM! Mock verification correctly updated for nullable parameter.The change from
It.IsAny<StorageAuthenticationMethod>()toIt.IsAny<StorageAuthenticationMethod?>()correctly reflects the API signature update to make the authentication method optional.
92-101: LGTM! Mock verification correctly updated for nullable parameter.The change from
It.IsAny<StorageAuthenticationMethod>()toIt.IsAny<StorageAuthenticationMethod?>()correctly reflects the API signature update to make the authentication method optional.
132-141: LGTM! Mock verification correctly updated for nullable parameter.The change from
It.IsAny<StorageAuthenticationMethod>()toIt.IsAny<StorageAuthenticationMethod?>()correctly reflects the API signature update to make the authentication method optional.
172-181: LGTM! Mock verification correctly updated for nullable parameter.The change from
It.IsAny<StorageAuthenticationMethod>()toIt.IsAny<StorageAuthenticationMethod?>()correctly reflects the API signature update to make the authentication method optional.test/Altinn.App.Core.Tests/Features/Signing/SigningReceiptServiceTests.cs (2)
130-140: LGTM! Correctly adapted to nullable StorageAuthenticationMethod.The mock setup correctly uses
It.IsAny<StorageAuthenticationMethod?>()to match the new optional nullable parameter, maintaining test validity while supporting the webhook authentication changes.
408-418: LGTM! Consistent application of nullable parameter.The second mock setup is correctly updated with
It.IsAny<StorageAuthenticationMethod?>(), maintaining consistency across all GetDataBytes mocks in this test file.test/Altinn.App.Core.Tests/Internal/Process/ExpressionsExclusiveGatewayTests.cs (1)
278-288: LGTM – Mock correctly updated for nullable authentication parameter.The change from
It.IsAny<StorageAuthenticationMethod>()toIt.IsAny<StorageAuthenticationMethod?>()properly reflects the updated API signature where authentication methods are now optional. This maintains test compatibility with the broader webhook support changes.src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
25-58: Well-structured refactoring to support per-call authentication.The switch from
IUserTokenProvidertoIAuthenticationTokenResolverwith a default authentication method (StorageAuthenticationMethod.CurrentUser()) is a clean architectural improvement that enables webhook scenarios while maintaining backward compatibility.test/Altinn.App.Core.Tests/Eformidling/Implementation/DefaultEFormidlingServiceTests.cs (1)
184-194: LGTM!The mock setup correctly updated to use
StorageAuthenticationMethod?to match the updatedIDataClient.GetBinaryDatasignature.test/Altinn.App.Tests.Common/Fixtures/MockedServiceCollection.cs (3)
151-173: Well-designed mock factory pattern.The
Mock<T>()factory with lazy creation, thread-safe access, and automatic singleton registration in DI is a clean approach for centralized mock management. UsingMockBehavior.Strictensures all interactions are explicitly set up, which helps catch unintended calls.
268-272: LGTM!Calling
TryAddCommonServices()inBuildServiceProvider()ensures the common infrastructure is always wired up before building the provider.
299-322: Good telemetry integration for test diagnostics.The
WrappedServiceProviderproperly integrates OpenTelemetry for traces, metrics, and logs, which aids debugging test failures. The activity source registration forFakeHttpMessageHandleris a nice touch for tracing HTTP interactions.test/Altinn.App.Core.Tests/Internal/Data/DataServiceTests.cs (1)
42-52: LGTM!Mock setups correctly updated to use nullable
StorageAuthenticationMethod?matching the updatedIDataClientinterface signatures.test/Altinn.App.Api.Tests/Helpers/Patch/PatchServiceTests.cs (2)
88-101: LGTM!Mock setup correctly updated for
UpdateBinaryDatawith nullableStorageAuthenticationMethod?.
327-339: LGTM!Mock setup correctly updated for
GetDataByteswith nullableStorageAuthenticationMethod?.test/Altinn.App.Core.Tests/Internal/Pdf/PdfServiceTests.cs (1)
182-195: LGTM!Mock verification correctly updated for
InsertBinaryDatawith nullableStorageAuthenticationMethod?parameter.src/Altinn.App.Api/Controllers/PaymentController.cs (2)
158-169: Good security implementation with constant-time comparison.The use of
CryptographicOperations.FixedTimeEqualsprevents timing attacks when validating the authorization header. This is a proper security practice for webhook authentication.
185-192: Good defensive null check added.This addresses the potential
NullReferenceExceptionwheninstance.ProcessorCurrentTaskis null.test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (3)
53-78: Good test infrastructure refactoring.The fixture-based approach with
MockedServiceCollectionprovides a clean, reusable test setup. The shared_instanceandPaymentDataTypeIdconstants reduce duplication across tests.
591-637: Good coverage for webhook handling scenarios.The test
HandlePaymentCompletedWebhook_Calls_ProcessNextStep_WhenNetsSaysPaymentIsCompletedproperly verifies the happy path with HTTP endpoint registration for the process/next call.
639-691: Verify the commented-out endpoint registration is intentional.The commented-out
FakeHttpMessageHandler.RegisterEndpointblocks suggest these tests verify thatprocess/nextis NOT called. The test logic relies on the endpoint not being registered to fail if it's unexpectedly called. While this works, consider adding explicit verification or a comment explaining this pattern.src/Altinn.App.Core/Internal/Data/IDataService.cs (1)
1-81: LGTM!The interface changes are well-structured and maintain backward compatibility. All methods consistently add optional
authenticationMethodandctparameters with appropriate defaults, and the XML documentation has been updated accordingly.src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (3)
33-52: LGTM!Constructor properly follows DI patterns and correctly unwraps
IOptions<GeneralSettings>to store the value.
55-151: LGTM!StartPayment has been enhanced with telemetry, proper error handling, and zero-sum payment handling. The implementation validates required dependencies and handles edge cases appropriately.
241-320: Approve webhook handler.The webhook handler correctly retrieves and updates payment status using the provided authentication method. The logic appropriately mirrors
CheckAndStorePaymentStatuswhile passing authentication context through the data layer. TheAltinnTaskTypes.Paymentconstant is properly defined and correctly used.src/Altinn.App.Core/Internal/Data/DataService.cs (1)
1-154: LGTM!The implementation correctly threads the new
authenticationMethodandctparameters through all method calls. Cancellation tokens are properly passed to async operations including serialization, deserialization, and data client calls.test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs (4)
51-161: LGTM with note on pattern matching.The
UrlPatternEndpointimplementation is well-structured with thread-safe call counting. The pattern matching logic correctly handles wildcards (*) and path parameters ({param}) with efficientReadOnlySpanusage.Note: The pattern matching is intentionally simple per the documentation (lines 93-96) and handles the stated use cases correctly.
163-229: LGTM!The registration methods provide a flexible API for setting up endpoints with various response types. The use of
ConcurrentBagensures thread-safe registration, and the JSON endpoint helper is a convenient addition.
231-274: LGTM!The
SendAsyncimplementation provides good diagnostics with activity tagging, clear error messages when endpoints don't match, and proper content buffering for multiple reads. The endpoint matching logic correctly ensures exactly one endpoint matches per request.
276-314: LGTM!The
Verifymethod enables test assertions on endpoint invocation counts, and theRequestResponseclass provides comprehensive capture of request/response details for test validation.
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)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (1)
279-310: Reconsider the null return pattern inUpdateReadStatus.Lines 307-309 return
nullby disabling nullable reference types when the response status is not OK. This violates the non-nullable return typeTask<Instance>and creates ambiguity for callers.Consider throwing an exception (like other methods in this class) or changing the return type to
Task<Instance?>to make the null return explicit.🔎 Proposed fix: throw exception for consistency
if (response.StatusCode == HttpStatusCode.OK) { Instance instance = await JsonSerializerPermissive.DeserializeAsync<Instance>(response.Content, ct); return instance; } _logger.LogError( $"Could not update read status for instance {instanceOwnerPartyId}/{instanceGuid}. Request failed with status code {response.StatusCode}" ); -#nullable disable - return null; -#nullable restore + throw await PlatformHttpException.CreateAsync(response);
🧹 Nitpick comments (1)
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (1)
26-51: Remove unused internal Fixture class.The internal
Fixtureclass is no longer used by any tests in this file. All tests have been refactored to useMockedServiceCollection(initialized at line 53), making this class dead code.🔎 Proposed fix
- private sealed record Fixture(IServiceProvider ServiceProvider) : IDisposable - { - public PaymentService Service => (PaymentService)ServiceProvider.GetRequiredService<IPaymentService>(); - - public Mock<T> Mock<T>() - where T : class => Moq.Mock.Get(ServiceProvider.GetRequiredService<T>()); - - public static Fixture Create(bool addProcessor = true, bool addOrderDetailsCalculator = true) - { - var services = new ServiceCollection(); - services.AddLogging(logging => logging.AddProvider(NullLoggerProvider.Instance)); - services.AddAppImplementationFactory(); - services.AddSingleton(new Mock<IDataService>(MockBehavior.Strict).Object); - - if (addOrderDetailsCalculator) - services.AddSingleton(new Mock<IOrderDetailsCalculator>(MockBehavior.Strict).Object); - if (addProcessor) - services.AddSingleton(new Mock<IPaymentProcessor>(MockBehavior.Strict).Object); - - services.AddSingleton<IPaymentService, PaymentService>(); - - return new Fixture(services.BuildStrictServiceProvider()); - } - - public void Dispose() => (ServiceProvider as IDisposable)?.Dispose(); - } -
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.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.Core.Tests/Features/Payment/PaymentServiceTests.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-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/Features/Payment/PaymentServiceTests.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/Features/Payment/PaymentServiceTests.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/Features/Payment/PaymentServiceTests.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 **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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: Include integration tests for platform service interactions
Applied to files:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 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:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 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.Core.Tests/Features/Payment/PaymentServiceTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.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/Features/Payment/PaymentServiceTests.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.Core.Tests/Features/Payment/PaymentServiceTests.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.Core.Tests/Features/Payment/PaymentServiceTests.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: Follow the AppFixture pattern for integration tests: Central orchestrator managing test lifecycle with feature-specific operations
Applied to files:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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-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/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
⏰ 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). (5)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (6)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
1914-1936: Snapshot updates correctly mirror the intended public API changesThe added NETS webhook payload models, the validated
WebhookCallbackKeyonNetsPaymentSettings, and the extra optionalStorageAuthenticationMethod?+CancellationTokenparameters onIDataServiceandIInstanceClientall look consistent with the PR objectives. The snapshot appears to be correctly regenerated and in sync with the underlying code; no further action needed here.Based on learnings, *.verified.txt is treated as a generated snapshot and not a place to drive further API design suggestions.
Also applies to: 1949-1952, 3056-3063, 3211-3224
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (4)
25-32: LGTM! Class accessibility and authentication refactoring align with guidelines.The changes correctly:
- Make the class
internal sealedper coding guidelines- Replace
IUserTokenProviderwithIAuthenticationTokenResolverto enable webhook authentication- Initialize
_defaultAuthenticationMethodto preserve backward compatibilityConsumers should depend on the
IInstanceClientinterface.
39-58: LGTM! Constructor correctly updated for new authentication pattern.The constructor properly accepts
IAuthenticationTokenResolverand initializes the token resolver field.
328-328: Verify the XSS security alert.GitHub Advanced Security flagged line 328 for potential cross-site scripting. While this appears to be server-to-server API communication (serializing a
Substatusobject as JSON), please verify that thesubstatusparameter cannot contain untrusted user input that might be reflected in error responses or logs without proper sanitization.Based on past review comment from github-advanced-security bot.
61-421: Excellent refactoring for per-call authentication support.The implementation consistently applies the following pattern across all methods:
- Optional
StorageAuthenticationMethod? authenticationMethod = nullparameter- Optional
CancellationToken ct = defaultparameter- Token resolution via
_tokenResolver.GetAccessToken(authenticationMethod ?? _defaultAuthenticationMethod, ct)- Cancellation token propagation through HTTP calls and deserialization
This enables webhook authentication while preserving backward compatibility through the default authentication method.
As per learnings, adding optional parameters maintains source compatibility for consumers who rebuild via NuGet.
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (1)
605-758: Excellent webhook test coverage.The three new webhook tests provide comprehensive coverage of the payment completion webhook functionality:
- Verifies process advancement when payment is completed
- Ensures idempotency when process has already advanced
- Prevents advancement when payment status is not complete
The tests follow established patterns, use proper DI resolution, and verify expected behavior through mocks and HTTP endpoint registration.
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Tests.Common/Fixtures/FakeHttpMessageHandler.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs (1)
17-42: Consider making the classsealedand fieldreadonly.Per coding guidelines, classes should be
sealedunless inheritance is intended. Additionally,_netsPaymentSettingsis never reassigned and should bereadonly(also flagged by static analysis).🔎 Proposed fix
-public class NetsPaymentProcessorTests +public sealed class NetsPaymentProcessorTests { private static readonly JsonSerializerOptions _jsonSerializerOptions = new JsonSerializerOptions( JsonSerializerDefaults.Web ); private readonly MockedServiceCollection _fixture = new(); - private readonly NetsPaymentSettings _netsPaymentSettings = new NetsPaymentSettings + private static readonly NetsPaymentSettings _netsPaymentSettings = new NetsPaymentSettingstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (2)
653-660: Remove commented code.This commented-out code block should be removed. If the intent is to document that the endpoint is intentionally not registered, a brief comment would be clearer.
🔎 Proposed fix
- // Disable the process/next endpoint to ensure it is not called - // _fixture.FakeHttpMessageHandler.RegisterEndpoint( - // HttpMethod.Put, - // $"/{_instance.AppId}/instances/{_instance.Id}/process/next", - // HttpStatusCode.OK, - // "application/json", - // "{}" - // ); - + // Note: process/next endpoint is intentionally not registered to verify it's not called +
707-714: Remove commented code.This is the same pattern of commented-out code that should be cleaned up.
🔎 Proposed fix
- // Disable the process/next endpoint to ensure it is not called - // _fixture.FakeHttpMessageHandler.RegisterEndpoint( - // HttpMethod.Put, - // $"/{_instance.AppId}/instances/{_instance.Id}/process/next", - // HttpStatusCode.OK, - // "application/json", - // "{}" - // ); - + // Note: process/next endpoint is intentionally not registered to verify it's not called +
🧹 Nitpick comments (3)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (2)
430-489: Refactor test to use separate HttpResponseMessage instances.The test reuses a single
HttpResponseMessageand modifies itsContentproperty between calls. This pattern is fragile because:
- HttpContent can typically only be read once
- Shared response state across calls can lead to unexpected behavior
- The verification doesn't check endpoints or content, only request count
Additionally, line 480 declares an unused
urlvariable with a typo (extra quote).🔎 Recommended approach
Set up three separate response messages or use
SetupSequence:- HttpResponseMessage httpResponseMessage = new HttpResponseMessage - { - StatusCode = HttpStatusCode.OK, - Content = new StringContent(JsonConvert.SerializeObject(instance), Encoding.UTF8, "application/json"), - }; - - InitializeMocks([httpResponseMessage], ["datavalues"]); + HttpResponseMessage[] responses = Enumerable.Range(0, 3) + .Select(_ => new HttpResponseMessage + { + StatusCode = HttpStatusCode.OK, + Content = new StringContent(JsonConvert.SerializeObject(instance), Encoding.UTF8, "application/json"), + }) + .ToArray(); + + _handlerMock + .Protected() + .SetupSequence<Task<HttpResponseMessage>>( + "SendAsync", + ItExpr.Is<HttpRequestMessage>(p => p.RequestUri!.ToString().Contains("datavalues")), + ItExpr.IsAny<CancellationToken>() + ) + .ReturnsAsync(responses[0]) + .ReturnsAsync(responses[1]) + .ReturnsAsync(responses[2]);And remove the unused
urlvariable and the manual Content reassignments (lines 466-470, 472-476).
452-452: Optional: Use concrete type for consistency.This test declares
targetasIInstanceClient, while all other tests in the file use the concreteInstanceClienttype. Consider using the concrete type for consistency, or update all tests to use the interface.test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (1)
178-192: Consider using a more descriptive exception message.The exception message
"ddasdfg"at line 181 appears to be placeholder text. While it works for the test, a descriptive message would make test failures easier to understand.🔎 Proposed fix
_fixture .Mock<IPaymentProcessor>() .Setup(pp => pp.StartPayment(_instance, orderDetails, Language)) - .ThrowsAsync(new Exception("ddasdfg")) + .ThrowsAsync(new Exception("Payment processor failed to start payment")) .Verifiable(Times.Once); // Act & Assert await using var sp = _fixture.BuildServiceProvider(); var paymentService = sp.GetRequiredService<IPaymentService>(); var exception = await Assert.ThrowsAsync<Exception>(() => paymentService.StartPayment(_instance, paymentConfiguration, Language) ); - Assert.Equal("ddasdfg", exception.Message); + Assert.Equal("Payment processor failed to start payment", exception.Message);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs
🧠 Learnings (12)
📓 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-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/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs
📚 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/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cstest/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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 **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.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.Core.Tests/Features/Payment/PaymentServiceTests.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:
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Features/Payment/PaymentServiceTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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:
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs
🧬 Code graph analysis (1)
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (7)
src/Altinn.App.Core/Constants/AltinnTaskTypes.cs (1)
AltinnTaskTypes(6-47)src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (9)
PaymentService(19-416)PaymentService(33-52)Task(55-151)Task(154-238)Task(241-320)Task(322-344)Task(347-364)Task(367-382)Task(384-415)src/Altinn.App.Core/Features/Payment/Models/OrderDetails.cs (1)
OrderDetails(6-57)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
Task(15-19)Task(24-28)Task(34-38)Task(43-43)Task(48-48)src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPaymentConfiguration.cs (2)
ValidAltinnPaymentConfiguration(24-38)AltinnPaymentConfiguration(10-47)src/Altinn.App.Core/Features/Payment/Models/PaymentInformation.cs (1)
PaymentInformation(6-27)src/Altinn.App.Core/Features/Payment/Models/PaymentDetails.cs (1)
PaymentDetails(6-52)
⏰ 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). (5)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (12)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (3)
60-60: LGTM: Consistent refactor to IAuthenticationTokenResolver.All InstanceClient constructor calls have been properly updated to use
_authenticationTokenResolver.Objectinstead of the old_userTokenProvider.Object.Also applies to: 91-91, 132-132, 174-174, 212-212, 247-247, 296-296, 328-328, 370-370, 418-418, 533-533
554-563: LGTM: Proper fix for secret-scanning concern.The
CreateDummyJwt()helper successfully addresses the previous review comment by programmatically generating a JWT-like token instead of using hardcoded literals that trigger gitleaks.
574-576: LGTM: Mock setup properly refactored.The
InitializeMocksmethod correctly updates the token resolution mock to useGetAccessTokenwith the programmatically generated JWT.test/Altinn.App.Core.Tests/Features/Payment/Providers/Nets/NetsPaymentProcessorTests.cs (4)
44-88: LGTM!The test correctly validates the payment flow by registering an HTTP endpoint stub and verifying both the response content and the request payload. The use of
await usingfor proper service provider disposal is appropriate.
90-151: LGTM!This test properly validates order sum calculation by checking the serialized
Order.Amountin the request matchesexpectedSum. The DI-based fixture approach is well-implemented.
180-226: LGTM!The cancellation/termination test correctly verifies the HTTP PUT request to the terminate endpoint with proper URL and method assertions.
294-305: Good use of theAltinnTaskTypes.Paymentconstant.Using the constant instead of a string literal improves maintainability and type safety.
test/Altinn.App.Core.Tests/Features/Payment/PaymentServiceTests.cs (5)
24-51: Good class structure and DI setup.The class is properly marked
sealed, uses shared constants for test data, and the constructor correctly wires up the DI container with required services. The fixture-based approach centralizes test setup effectively.
577-623: LGTM!The webhook handler test properly sets up the mock for
GetPaymentStatus, registers the HTTP endpoint for process advancement, and verifies the complete webhook flow. Good coverage of the happy path.
625-677: Good edge case coverage.This test verifies that the process/next step is not called when the process has already advanced to a different task. The test correctly modifies
_instance.Process.CurrentTaskto simulate this scenario.
679-731: Good edge case coverage.This test verifies that process advancement is skipped when the payment status is not
Paid(returningCreatedinstead). This ensures the webhook handler correctly gates on payment completion.
733-777: LGTM!Helper methods are well-structured and provide consistent test data. The use of
PaymentDataTypeIdconstant inCreatePaymentConfigurationensures alignment with the fixture setup.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.cs
🧠 Learnings (9)
📓 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-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/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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 : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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/InstanceClientTests.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:
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.cs
🧬 Code graph analysis (1)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (15)
Task(61-87)Task(90-103)Task(106-129)Task(131-153)Task(156-183)Task(186-218)Task(221-247)Task(250-276)Task(279-310)Task(313-339)Task(342-368)Task(371-397)Task(400-421)InstanceClient(25-422)InstanceClient(42-58)
⏰ 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). (5)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (5)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (5)
4-4: LGTM: Required imports for authentication refactor.The new using directives support the transition from
IUserTokenProvidertoIAuthenticationTokenResolver.Also applies to: 8-8
23-24: LGTM: Mock setup aligns with authentication refactor.The replacement of
IUserTokenProviderwithIAuthenticationTokenResolveris consistent with the broader refactoring effort.
57-63: LGTM: InstanceClient constructor calls updated consistently.All test instantiations now correctly pass
_authenticationTokenResolver.Objectinstead of the deprecated token provider.Also applies to: 88-94, 128-134, 170-176, 208-214, 243-249, 292-298, 324-330, 366-372, 415-421, 452-458, 532-538
556-565: LGTM: Clean JWT generation helper.The
CreateDummyJwt()helper successfully addresses the secret-scanning concerns by generating a non-secret JWT programmatically instead of using hardcoded literals.
576-578: LGTM: Mock setup updated for new authentication interface.The
GetAccessTokensetup correctly replaces the deprecatedGetUserTokenand uses the newCreateDummyJwt()helper.
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (2)
47-48: Consider making default values constants.The default party ID and instance GUID are allocated on every call. Moving them to class-level constants would avoid repeated allocations.
🔎 Proposed refactor
+ private const int DefaultPartyId = 123456; + private static readonly Guid DefaultInstanceGuid = Guid.Parse("00000000-DEAD-FACE-BABE-000000000001"); + public void AddInstance(Instance instance) { - int defaultPartyId = 123456; - Guid defaultInstanceGuid = Guid.Parse("00000000-DEAD-FACE-BABE-000000000001"); instance.InstanceOwner ??= new InstanceOwner() { }; if (instance.InstanceOwner.PartyId is null && instance.Id is not null) { var idParts = instance.Id.Split('/'); Assert.Equal(2, idParts.Length); instance.InstanceOwner.PartyId = idParts[0]; } - instance.InstanceOwner.PartyId ??= defaultPartyId.ToString(); - instance.Id ??= $"{instance.InstanceOwner.PartyId}/{defaultInstanceGuid}"; + instance.InstanceOwner.PartyId ??= DefaultPartyId.ToString(); + instance.Id ??= $"{instance.InstanceOwner.PartyId}/{DefaultInstanceGuid}"; instance.Data ??= []; instance.AppId ??= AppMetadata.Id; _instances[instance.Id.ToLowerInvariant()] = instance; }
63-71: Add defensive error handling for consistency.Unlike
GetInstanceResponse(line 198), this method directly accesses dictionaries without validation. If the instance or data elements don't exist, it will throwKeyNotFoundException, which may obscure test setup issues.🔎 Proposed refactor
public (Instance instance, Dictionary<string, byte[]> data) GetInstanceAndData( int instanceOwnerPartyId, Guid instanceGuid ) { - var instance = _instances[$"{instanceOwnerPartyId}/{instanceGuid}"]; + if (!_instances.TryGetValue($"{instanceOwnerPartyId}/{instanceGuid}", out var instance)) + { + throw new InvalidOperationException($"Instance {instanceOwnerPartyId}/{instanceGuid} not found in test fixture"); + } + var data = instance.Data.ToDictionary(d => d.Id, d => _data[Guid.Parse(d.Id)]); return (instance, data); }Alternatively, for more robust handling:
public (Instance instance, Dictionary<string, byte[]> data) GetInstanceAndData( int instanceOwnerPartyId, Guid instanceGuid ) { - var instance = _instances[$"{instanceOwnerPartyId}/{instanceGuid}"]; - var data = instance.Data.ToDictionary(d => d.Id, d => _data[Guid.Parse(d.Id)]); + if (!_instances.TryGetValue($"{instanceOwnerPartyId}/{instanceGuid}", out var instance)) + { + throw new InvalidOperationException($"Instance {instanceOwnerPartyId}/{instanceGuid} not found in test fixture"); + } + + var data = new Dictionary<string, byte[]>(); + foreach (var dataElement in instance.Data) + { + if (!Guid.TryParse(dataElement.Id, out var dataId) || !_data.TryGetValue(dataId, out var dataBytes)) + { + throw new InvalidOperationException($"Data element {dataElement.Id} not found in test fixture for instance {instanceOwnerPartyId}/{instanceGuid}"); + } + data[dataElement.Id] = dataBytes; + } return (instance, data); }test/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cs (1)
369-369: Improve assertion precision for better test clarity.The assertion checks for
"12345"but the actualPartyIdis123456. While the test passes (substring match), it would be more precise to assert against the correct party ID or full instance ID.🔎 Suggested improvement
- Assert.Contains("Payment status is Paid for instance 12345", response.Value?.ToString()); + Assert.Contains($"Payment status is Paid for instance {_instance.Id}", response.Value?.ToString());src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (2)
240-320: Consider adding telemetry for webhook handling.The
StartPaymentmethod uses telemetry activity wrapping (line 61), butHandlePaymentCompletedWebhookdoes not. For consistency and better observability of webhook-driven flows, consider wrapping this method with a telemetry activity similar toStartPayment.💡 Example pattern
public async Task<string> HandlePaymentCompletedWebhook(...) { using var activity = _telemtry?.StartHandleWebhookActivity(); try { // existing logic } catch (Exception ex) { activity?.Errored(ex); throw; } }
28-28: Minor typo in field name.The field is named
_telemtryinstead of_telemetry. While consistent throughout the file, correcting the spelling would improve code quality.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Api/Controllers/PaymentController.cssrc/Altinn.App.Core/Features/Payment/Services/IPaymentService.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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:
src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cssrc/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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/Controllers/PaymentControllerTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs
🧠 Learnings (9)
📓 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: 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.
📚 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/Features/Payment/Services/IPaymentService.cssrc/Altinn.App.Api/Controllers/PaymentController.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.Api/Controllers/PaymentController.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Api/Controllers/PaymentController.cstest/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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.Api.Tests/Controllers/PaymentControllerTests.cs
📚 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.Api.Tests/Controllers/PaymentControllerTests.cstest/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.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/StorageClientInterceptor.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/StorageClientInterceptor.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:
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs
🧬 Code graph analysis (4)
src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (1)
src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnPaymentConfiguration.cs (1)
ValidAltinnPaymentConfiguration(24-38)
src/Altinn.App.Api/Controllers/PaymentController.cs (5)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs (1)
NetsPaymentSettings(8-88)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
Task(15-19)Task(24-28)Task(36-40)Task(45-45)Task(50-50)src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (7)
Task(55-151)Task(154-238)Task(241-320)Task(322-344)Task(347-364)Task(367-382)Task(384-415)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (3)
Task(45-150)Task(153-162)Task(165-208)src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cs (1)
NetsCompleteWebhookPayload(8-39)
test/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cs (4)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs (1)
NetsPaymentSettings(8-88)src/Altinn.App.Api/Controllers/PaymentController.cs (1)
PaymentController(38-46)src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (5)
Task(15-19)Task(24-28)Task(36-40)Task(45-45)Task(50-50)src/Altinn.App.Core/Features/Payment/Models/PaymentDetails.cs (1)
PaymentDetails(6-52)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (2)
src/Altinn.App.Core/Internal/Language/LanguageConst.cs (1)
LanguageConst(3-8)src/Altinn.App.Core/Helpers/InstantiationHelper.cs (1)
InstanceOwner(196-219)
⏰ 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). (5)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (8)
test/Altinn.App.Tests.Common/Fixtures/StorageClientInterceptor.cs (3)
31-40: LGTM!The constructor refactoring correctly initializes
AppMetadatawith sensible defaults for titles and data types, usingLanguageConstfor language keys as per established patterns.
104-105: LGTM!The routing correctly calls the renamed
GetInstanceResponsemethod.
196-210: LGTM!The renamed method maintains correct logic with defensive dictionary access and appropriate error responses.
src/Altinn.App.Core/Features/Payment/Services/IPaymentService.cs (1)
30-40: LGTM! Well-documented webhook handler method.The new
HandlePaymentCompletedWebhookmethod is properly documented and the signature aligns well with the PR's objective of adding webhook support with per-call authentication.test/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cs (1)
88-434: Excellent test coverage for webhook scenarios.The test suite comprehensively covers the webhook flow including authentication validation, configuration checks, and various task states. The use of helper methods and mock verification follows good testing practices.
src/Altinn.App.Api/Controllers/PaymentController.cs (2)
129-227: Well-implemented webhook listener with proper security controls.The webhook endpoint correctly implements:
- Constant-time authorization comparison to prevent timing attacks
- ServiceOwner authentication for system-level access
- Proper null checking for process state
- Graceful handling of edge cases (missing config returns 200 OK to prevent unnecessary retries)
The approach of using the webhook as a trigger to fetch authoritative payment status (rather than trusting the payload) is a sound security practice.
69-72: Good defensive check for process state.Adding validation that
CurrentTaskexists before accessing its properties prevents potentialNullReferenceExceptionand provides a clear error message.src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (1)
54-151: Well-structured payment initialization with comprehensive error handling.The
StartPaymentmethod properly:
- Validates required dependencies early
- Handles existing payments (already paid vs. already started)
- Wraps calculations with telemetry
- Gracefully handles zero-amount orders by skipping processor invocation
- Includes proper error reporting via telemetry
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs:
- Line 434: The test method
UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage has a typo in its
parameter name: rename the parameter from methodVerion to methodVersion in the
method signature and update any usages within the method body (and related test
attributes or invocations) to match the corrected name so the code compiles and
reads correctly.
♻️ Duplicate comments (2)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (1)
445-491: Resource leak: HttpResponseMessage not disposed.The
HttpResponseMessagecreated at line 445 is never disposed, and the test reuses the same response instance across multiple method invocations viaInitializeMocks. This pattern was flagged in previous review comments.As per coding guidelines, remember to dispose IDisposable instances. Based on learnings, similar resource management concerns were raised in prior reviews.
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (1)
276-276: Fix missing string interpolation prefix.This is the same issue flagged in the previous review. The return statement is missing the
$prefix, so{instance.Id}will be returned as literal text instead of the actual instance ID.🔎 Proposed fix
- return "Payment status is 'Skipped' for instance {instance.Id}. Won't ask payment processor for status."; + return $"Payment status is 'Skipped' for instance {instance.Id}. Won't ask payment processor for status.";
🧹 Nitpick comments (4)
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (4)
247-247: Make the log message more specific to webhook handling.The log message says "Checking payment status" but this method is specifically handling a webhook callback. Consider a more descriptive message like "Handling payment completed webhook for instance {InstanceId}."
🔎 Proposed fix
- _logger.LogInformation("Checking payment status for instance {InstanceId}.", instance.Id); + _logger.LogInformation("Handling payment completed webhook for instance {InstanceId}.", instance.Id);
288-293: Document why language is hardcoded to null in webhook handler.The
languageparameter is hardcoded tonullwhen callingGetPaymentStatus. Since webhooks are server-to-server callbacks without user context, this makes sense, but consider adding a comment to clarify this design decision.🔎 Suggested comment
+ // Language is null because webhooks are server-to-server calls without user context (PaymentStatus paymentStatus, PaymentDetails updatedPaymentDetails) = await paymentProcessor.GetPaymentStatus( instance, paymentDetails.PaymentId, totalPriceIncVat, language: null );
322-346: Good addition of HTTP timeout; consider adding CancellationToken support.The explicit 5-minute timeout (line 327) properly addresses the previous review comment and prevents indefinite hangs. The timeout value is appropriately generous for process engine calls.
As an optional improvement, consider adding a
CancellationTokenparameter to this method for better cancellation support, especially since it's an async operation that makes an external HTTP call.🔎 Optional refactor to add CancellationToken
- private async Task RunProcessNext(Instance instance, StorageAuthenticationMethod storageAuthenticationMethod) + private async Task RunProcessNext(Instance instance, StorageAuthenticationMethod storageAuthenticationMethod, CancellationToken cancellationToken = default) { JwtToken token = await _authenticationTokenResolver.GetAccessToken(storageAuthenticationMethod); using var client = _httpClientFactory.CreateClient(); // Be reasonably generous with timeout since this involves process engine calls client.Timeout = TimeSpan.FromMinutes(5); client.BaseAddress = new Uri(_generalSettings.FormattedExternalAppBaseUrl(_app)); client.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue( "Bearer", token.Value ); - var response = await client.PutAsync($"instances/{instance.Id}/process/next", null); + var response = await client.PutAsync($"instances/{instance.Id}/process/next", null, cancellationToken); if (!response.IsSuccessStatusCode) { _logger.LogError( "Failed to advance process for instance {InstanceId} after payment completed webhook. Status code: {StatusCode}\n\n{content}", instance.Id, response.StatusCode, await response.Content.ReadAsStringAsync() ); throw new PaymentException( $"Failed to advance process for instance {instance.Id} after payment completed webhook." ); } }Then update the call site on line 317:
- await RunProcessNext(instance, storageAuthenticationMethod); + await RunProcessNext(instance, storageAuthenticationMethod, cancellationToken);Note: This would require adding a
CancellationTokenparameter toHandlePaymentCompletedWebhookas well.
19-19: Consider sealing the class per coding guidelines.The coding guidelines specify: "Use sealed for classes unless inheritance is considered a valid use-case." Since
PaymentServiceis an internal service implementation, consider marking it assealedunless inheritance is intentionally supported.🔎 Proposed change
-internal class PaymentService : IPaymentService +internal sealed class PaymentService : IPaymentServiceBased on coding guidelines, services should be sealed by default.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cstest/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.cstest/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Controllers/PaymentControllerTests.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.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.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.Core.Tests/Implementation/InstanceClientTests.cs
🧠 Learnings (10)
📓 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: 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.
📚 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/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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 : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.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/InstanceClientTests.cssrc/Altinn.App.Core/Features/Payment/Services/PaymentService.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:
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.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.Core.Tests/Implementation/InstanceClientTests.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/Features/Payment/Services/PaymentService.cs
🧬 Code graph analysis (2)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs (15)
Task(61-87)Task(90-103)Task(106-129)Task(131-153)Task(156-183)Task(186-218)Task(221-247)Task(250-276)Task(279-310)Task(313-339)Task(342-368)Task(371-397)Task(400-421)InstanceClient(25-422)InstanceClient(42-58)
src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (6)
src/Altinn.App.Core/Configuration/GeneralSettings.cs (2)
GeneralSettings(9-86)FormattedExternalAppBaseUrl(61-68)src/Altinn.App.Core/Features/Payment/Exceptions/PaymentException.cs (2)
PaymentException(8-16)PaymentException(14-15)src/Altinn.App.Core/Features/Payment/Models/PaymentInformation.cs (1)
PaymentInformation(6-27)src/Altinn.App.Core/Features/Payment/Models/PaymentDetails.cs (1)
PaymentDetails(6-52)src/Altinn.App.Core/Features/Payment/Models/OrderDetails.cs (1)
OrderDetails(6-57)src/Altinn.App.Core/Constants/AltinnTaskTypes.cs (1)
AltinnTaskTypes(6-47)
🔇 Additional comments (5)
test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs (2)
556-565: LGTM: Dummy JWT generator addresses secret-scanning concern.The
CreateDummyJwthelper successfully resolves the previous issue with hardcoded JWT literals that triggered secret scanners. The implementation correctly generates a non-secret JWT-like token for testing purposes.
576-578: LGTM: Token resolver mock correctly configured.The
InitializeMocksmethod properly sets up theIAuthenticationTokenResolvermock using theCreateDummyJwthelper, aligning with the migration fromIUserTokenProvidertoIAuthenticationTokenResolver.src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (3)
33-52: LGTM: Constructor properly wired with new dependencies.The constructor correctly injects and assigns the new dependencies needed for webhook support and process advancement. The optional telemetry parameter follows the established pattern in the codebase.
55-151: LGTM: Well-structured payment initialization flow.The refactored StartPayment method now includes proper validation, telemetry instrumentation, and enhanced error handling. The zero-sum optimization (lines 121-125) and distinct logging for already-paid vs. already-started payments (lines 81-99) are particularly good additions.
312-318: Verify process advancement is complete for webhook flow.The PR description states "The implementation is incomplete: it currently does not perform process transitions or post events, because those previously happened in the frontend." However, this code does advance the process via
RunProcessNext. Please clarify:
- Does the
/instances/{id}/process/nextendpoint handle all necessary transitions and events?- Are there additional actions (e.g., posting platform events) that still need to be implemented for full webhook support?
| [InlineData(1)] | ||
| [InlineData(2)] | ||
| [InlineData(3)] | ||
| public async Task UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage(int methodVerion) |
There was a problem hiding this comment.
Fix parameter name typo.
The parameter name methodVerion should be methodVersion.
🔎 Proposed fix
- public async Task UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage(int methodVerion)
+ public async Task UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage(int methodVersion)
{
Guid instanceGuid = Guid.NewGuid();
int instanceOwnerId = 1337;
@@ -461,7 +461,7 @@
);
// Act
- switch (methodVerion)
+ switch (methodVersion)
{
case 1:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage(int methodVerion) | |
| public async Task UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage(int methodVersion) |
🤖 Prompt for AI Agents
In @test/Altinn.App.Core.Tests/Implementation/InstanceClientTests.cs around line
434, The test method UpdateDataValues_WithFullInstance_SuccessfullyCallsStorage
has a typo in its parameter name: rename the parameter from methodVerion to
methodVersion in the method signature and update any usages within the method
body (and related test attributes or invocations) to match the corrected name so
the code compiles and reads correctly.
|
/publish |
PR release:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Altinn.App.Api/Controllers/PaymentController.cs (3)
39-47: Consider using direct constructor injection instead of Service Locator pattern.The controller now uses
IServiceProviderto resolve all dependencies at runtime, which obscures the explicit dependencies and complicates testing. Direct constructor injection makes dependencies clearer and improves testability.🔎 Proposed refactor to use direct constructor injection
-public PaymentController(IServiceProvider serviceProvider) +public PaymentController( + IInstanceClient instanceClient, + IProcessReader processReader, + ILogger<PaymentController> logger, + IOptions<NetsPaymentSettings> netsPaymentSettings, + IPaymentService paymentService, + AppImplementationFactory appImplementationFactory) { - _instanceClient = serviceProvider.GetRequiredService<IInstanceClient>(); - _processReader = serviceProvider.GetRequiredService<IProcessReader>(); - _logger = serviceProvider.GetRequiredService<ILogger<PaymentController>>(); - _netsPaymentSettings = serviceProvider.GetRequiredService<IOptions<NetsPaymentSettings>>(); - _paymentService = serviceProvider.GetRequiredService<IPaymentService>(); - _appImplementationFactory = serviceProvider.GetRequiredService<AppImplementationFactory>(); + _instanceClient = instanceClient; + _processReader = processReader; + _logger = logger; + _netsPaymentSettings = netsPaymentSettings; + _paymentService = paymentService; + _appImplementationFactory = appImplementationFactory; }
198-217: Minor: Redundant null-conditional operator.Line 206 uses
instance.Process.CurrentTask?.ElementIdwith a null-conditional operator, butCurrentTaskwas already validated to be non-null at line 189. This is harmless but slightly redundant.🔎 Proposed simplification
_logger.LogInformation( "Payment configuration not found in AltinnTaskExtension for task {CurrentTask}. Cannot process Nets webhook callback for instance {InstanceId}. Likely the callback is for an old payment.", - instance.Process.CurrentTask?.ElementId, + instance.Process.CurrentTask.ElementId, instance.Id ); // If the current task is not a payment task, just log and return OK with a ProblemDetails that nets ignore, but might be logged. return Ok( new ProblemDetails() { Detail = - $"Payment configuration not found in AltinnTaskExtension for task {instance.Process.CurrentTask?.ElementId}", + $"Payment configuration not found in AltinnTaskExtension for task {instance.Process.CurrentTask.ElementId}", } );
143-229: Consider adding CancellationToken parameter for proper async cancellation.The webhook endpoint performs async operations but doesn't accept a
CancellationTokenparameter, preventing proper cancellation propagation if the request is aborted.🔎 Proposed enhancement
public async Task<IActionResult> PaymentWebhookListener( [FromRoute] string org, [FromRoute] string app, [FromRoute] int instanceOwnerPartyId, [FromRoute] Guid instanceGuid, [FromBody] NetsCompleteWebhookPayload webhookPayload, - [FromHeader(Name = "Authorization")] string authorizationHeader + [FromHeader(Name = "Authorization")] string authorizationHeader, + CancellationToken cancellationToken = default )Then pass
cancellationTokento async method calls likeGetInstanceandHandlePaymentCompletedWebhookif they support it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Api/Controllers/PaymentController.cstest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
🧰 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.Api/Controllers/PaymentController.cs
🧠 Learnings (9)
📓 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-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.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.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.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
📚 Learning: 2025-09-18T10:23:05.995Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json:7263-0
Timestamp: 2025-09-18T10:23:05.995Z
Learning: In PR #1357, ApplicationMetadata.apiScopes (now ApiScopesConfiguration) is currently unused at runtime; the schema change is acceptable without a transition mechanism for now.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
📚 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.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.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.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txtsrc/Altinn.App.Api/Controllers/PaymentController.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.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txtsrc/Altinn.App.Api/Controllers/PaymentController.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.Api/Controllers/PaymentController.cs
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/PaymentController.cs (4)
src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentSettings.cs (1)
NetsPaymentSettings(8-88)src/Altinn.App.Core/Features/Payment/Services/PaymentService.cs (7)
Task(55-151)Task(154-238)Task(241-320)Task(322-346)Task(349-366)Task(369-384)Task(386-417)src/Altinn.App.Core/Features/Payment/Processors/Nets/NetsPaymentProcessor.cs (3)
Task(45-150)Task(153-162)Task(165-208)src/Altinn.App.Core/Features/Payment/Processors/Nets/Models/NetsCompleteWebhookPayload.cs (1)
NetsCompleteWebhookPayload(8-39)
🔇 Additional comments (3)
src/Altinn.App.Api/Controllers/PaymentController.cs (3)
1-2: LGTM!The new imports and fields are appropriate for the webhook implementation. The cryptography imports enable constant-time comparison for secure authorization header validation, and the Nets-related imports support webhook payload handling.
Also applies to: 8-9, 15-17, 33-34
70-83: LGTM!The validation additions prevent potential
NullReferenceExceptionand provide clear error messages when the instance has no current task or payment configuration. Good defensive programming.
130-173: LGTM!The webhook listener endpoint properly implements security best practices:
- Uses
[AllowAnonymous]with explicit comment explaining authorization is handled via shared secret- Employs constant-time comparison to prevent timing attacks
- Validates configuration and authorization with appropriate logging and HTTP status codes
- Hides the internal endpoint from public API documentation
|
bjorntore
left a comment
bjorntore
left a comment
There was a problem hiding this comment.
LGTM!
Frontend deals OK with the new behaviour?
I don't know |
2 participants
Currently our nets implementation depends on the user visiting the instance (eg. via the return url from nets) before the instance is marked as completed. Some users (especially on mobile where they might be directed to the BankId app, does not open the browser tab again before the login has timed out).
Fixes
Changes
IInstanceClientandIDataServiceto have aStorageAutenicationMetodparameter on all methods (and update corresponding tests){org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/payment/nets-webhook-listenerfor nets to callPaymentService.HandlePaymentCompletedWebhook, that queries nets for payment info and calls process/next if relevant.payment.checkout.completedTesting
Policy.xmlallows the service owner the write action on the payment task.Unsolved issue
/process/nextDocumentation
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.