feat(fiks-arkiv): configurable case file classifications and document format codes#1765
Open
danielskovli wants to merge 3 commits into
Open
feat(fiks-arkiv): configurable case file classifications and document format codes#1765danielskovli wants to merge 3 commits into
danielskovli wants to merge 3 commits into
Conversation
… format codes - Optional CaseFileClassifications on FiksArkivMetadataSettings. - Optional FormatCode override per document (e.g. PDF/A). - Variantformat changed from Produksjonsformat (P) to Arkivformat (A). - IFiksArkivConfigResolver: GetInstanceOwnerClassification replaced with GetCaseFileClassifications (breaking). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR extends Fiks Arkiv support to handle multiple case-file classifications and configurable document format codes. A new FiksArkivClassification model allows specifying classifications in settings, which are combined with the instance-owner classification and applied to outgoing arkivmelding payloads. Document format codes can now be overridden per datatype, with MessagePayloadWrapper resolving the final format via explicit code, file extension, or filename. ChangesClassification and Format Code Configuration
Configuration Resolver and Payload Generation
Test Coverage and Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
danielskovli
added
feature
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs (1)
417-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove blocking
GetAwaiter().GetResult()in test code. Line 417 intest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.csusesAuthenticated.User user => KlassifikasjonFactory.CreateUser(user).GetAwaiter().GetResult(),, which violates the guideline “Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task”. Refactor the helper/fixture to be async andawaitthe factory result instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs` at line 417, The test uses a blocking call in the expression Authenticated.User user => KlassifikasjonFactory.CreateUser(user).GetAwaiter().GetResult(); remove the blocking .GetAwaiter().GetResult() by making the test helper/fixture async: change the provider to return a Task<User> (or make the factory call inside an async setup method) and await KlassifikasjonFactory.CreateUser(user) instead; update callers in FiksArkivDefaultPayloadGeneratorTest (and any usages of Authenticated.User) to await the Task or run within an async test/setup method so no synchronous blocking APIs (.Result/.Wait/GetAwaiter().GetResult) are used.
🧹 Nitpick comments (2)
test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cs (1)
539-547: ⚡ Quick winAssert the owner classification explicitly at index 0.
Line 539-547 verifies appended configured classifications, but it doesn’t directly assert that the owner classification remains first. A direct assertion on
result[0]would harden this contract.Proposed assertion addition
Assert.Equal(3, result.Count); + Assert.NotNull(result[0].KlassifikasjonssystemID); + Assert.NotNull(result[0].KlasseID); Assert.Equal("configured-system-1", result[1].KlassifikasjonssystemID); Assert.Equal("configured-class-1", result[1].KlasseID);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cs` around lines 539 - 547, Add an explicit assertion that the owner classification remains at index 0 in the test FiksArkivConfigResolverTest by asserting properties on result[0] (e.g., KlassifikasjonssystemID, KlasseID, Tittel and ErSkjermet) before the existing assertions that check configured entries; this ensures the owner classification is still first and hardens the test contract.test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/Models/FiksArkivSettingsTest.cs (1)
14-17: ⚡ Quick winAdd whitespace validation cases for
ClassificationIdandTitle.Line 14-17 already checks required-field behavior, but
ClassificationIdandTitleonly test empty-string inputs, not whitespace-only values. Adding whitespace cases closes an easy regression gap.Proposed test additions
[InlineData("sys", "", "title", null, "ClassificationId configuration is required")] + [InlineData("sys", " ", "title", null, "ClassificationId configuration is required")] [InlineData("sys", "cls", "", null, "Title configuration is required")] + [InlineData("sys", "cls", " ", null, "Title configuration is required")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/Models/FiksArkivSettingsTest.cs` around lines 14 - 17, The tests in FiksArkivSettingsTest currently validate whitespace-only SystemId but not ClassificationId or Title; add InlineData cases for whitespace-only values for those fields so the test method (the InlineData-decorated test) asserts the same "required" errors for ClassificationId and Title when they are strings of whitespace. Specifically, add entries analogous to [InlineData(" ", "cls", "title", null, "SystemId configuration is required")] but targeting ClassificationId (e.g., [InlineData("sys", " ", "title", null, "ClassificationId configuration is required")]) and Title (e.g., [InlineData("sys", "cls", " ", null, "Title configuration is required")]) so FiksArkivSettingsTest covers whitespace-only validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Altinn.App.Clients.Fiks/FiksArkiv/Models/MessagePayloadWrapper.cs`:
- Around line 9-15: The GetFileFormat method writes FileFormatCode directly into
Format.KodeProperty which can include surrounding whitespace; trim
FileFormatCode before assigning it. Update the logic in GetFileFormat (the
method that returns Format with KodeProperty) to use FileFormatCode.Trim() (or
check trimmed value for IsNullOrWhiteSpace) and fall back to
Payload.GetDotlessFileExtension() when the trimmed value is empty, so
KodeProperty never contains leading/trailing spaces.
---
Outside diff comments:
In
`@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs`:
- Line 417: The test uses a blocking call in the expression Authenticated.User
user => KlassifikasjonFactory.CreateUser(user).GetAwaiter().GetResult(); remove
the blocking .GetAwaiter().GetResult() by making the test helper/fixture async:
change the provider to return a Task<User> (or make the factory call inside an
async setup method) and await KlassifikasjonFactory.CreateUser(user) instead;
update callers in FiksArkivDefaultPayloadGeneratorTest (and any usages of
Authenticated.User) to await the Task or run within an async test/setup method
so no synchronous blocking APIs (.Result/.Wait/GetAwaiter().GetResult) are used.
---
Nitpick comments:
In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cs`:
- Around line 539-547: Add an explicit assertion that the owner classification
remains at index 0 in the test FiksArkivConfigResolverTest by asserting
properties on result[0] (e.g., KlassifikasjonssystemID, KlasseID, Tittel and
ErSkjermet) before the existing assertions that check configured entries; this
ensures the owner classification is still first and hardens the test contract.
In
`@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/Models/FiksArkivSettingsTest.cs`:
- Around line 14-17: The tests in FiksArkivSettingsTest currently validate
whitespace-only SystemId but not ClassificationId or Title; add InlineData cases
for whitespace-only values for those fields so the test method (the
InlineData-decorated test) asserts the same "required" errors for
ClassificationId and Title when they are strings of whitespace. Specifically,
add entries analogous to [InlineData(" ", "cls", "title", null, "SystemId
configuration is required")] but targeting ClassificationId (e.g.,
[InlineData("sys", " ", "title", null, "ClassificationId configuration is
required")]) and Title (e.g., [InlineData("sys", "cls", " ", null, "Title
configuration is required")]) so FiksArkivSettingsTest covers whitespace-only
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65047a5d-114f-48ad-b00c-823dc7b2eb5c
📒 Files selected for processing (22)
src/Altinn.App.Clients.Fiks/Extensions/FiksArkivClassificationExtensions.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivConfigResolver.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivDefaultPayloadGenerator.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/IFiksArkivConfigResolver.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/Models/FiksArkivSettings.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/Models/MessagePayloadWrapper.cstest/Altinn.App.Clients.Fiks.Tests/Extensions/ListExtensionsTests.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivConfigResolverTest.GetCaseFileClassifications_ReturnsOwnerClassification_ForKnownAuthenticationTypes.Org.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivConfigResolverTest.GetCaseFileClassifications_ReturnsOwnerClassification_ForKnownAuthenticationTypes.ServiceOwner.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivConfigResolverTest.GetCaseFileClassifications_ReturnsOwnerClassification_ForKnownAuthenticationTypes.SystemUser.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivConfigResolverTest.GetCaseFileClassifications_ReturnsOwnerClassification_ForKnownAuthenticationTypes.User.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.1.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.2.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.3.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.4.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.5.verified.txttest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/Models/FiksArkivDocumentsTest.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/Models/FiksArkivSettingsTest.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/Models/MessagePayloadWrapperTest.cstest/Altinn.App.Clients.Fiks.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
martinothamar
approved these changes
May 22, 2026
Contributor
martinothamar
left a comment
martinothamar
left a comment
There was a problem hiding this comment.
generally LGTM, though not sure it is safe to do PDF/A by default?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes and extends the archive document generator to comply with requirements from IKTA:
CaseFileClassificationsonFiksArkivMetadataSettings.FormatCodeoverride per document (e.g.PDF/A).Variantformatchanged fromProduksjonsformat(P) toArkivformat(A).IFiksArkivConfigResolver.GetInstanceOwnerClassificationreplaced withGetCaseFileClassifications.Related
Summary by CodeRabbit