Remove extraneous senders from Fiks Arkiv Payload#1685
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved internal sender support: deleted AltinnOrgNo constant, removed CreateInternalSender and GetServiceOwnerParty, eliminated service-owner lookup/usage from payload generation, updated tests and snapshots to omit internal/internal-service korrespondansepart entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivDefaultPayloadGenerator.cs`:
- Around line 127-131: The code was refactored to only add instanceOwnerParty to
journalEntry.Korrespondansepart (see FiksArkivDefaultPayloadGenerator where
instanceOwnerParty is used) and IFiksArkivConfigResolver.GetServiceOwnerParty
must remain on the interface, but the test mock for that method is now unused;
remove the unused mock setup in FiksArkivDefaultPayloadGeneratorTest (around the
previous line ~282) so the test no longer configures GetServiceOwnerParty, and
ensure the test asserts only against instanceOwnerParty being added to
journalEntry.Korrespondansepart without referencing the removed sender mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7f7c2e5-2f3b-4850-96c8-79ed611ca903
📒 Files selected for processing (7)
src/Altinn.App.Clients.Fiks/Constants/FiksArkivConstants.cssrc/Altinn.App.Clients.Fiks/Factories/KorrespondansepartFactory.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivDefaultPayloadGenerator.cstest/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.txt
💤 Files with no reviewable changes (6)
- test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.2.verified.txt
- test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.4.verified.txt
- src/Altinn.App.Clients.Fiks/Constants/FiksArkivConstants.cs
- src/Altinn.App.Clients.Fiks/Factories/KorrespondansepartFactory.cs
- test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.3.verified.txt
- test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/.Verify/FiksArkivDefaultPayloadGeneratorTest.GeneratePayload_GeneratesCorrectPayload.1.verified.txt
src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivDefaultPayloadGenerator.cs
Show resolved
Hide resolved
tina-ahm
added
feature
2 tasks
- Update tests
Contributor
There was a problem hiding this comment.
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)
186-216: 🛠️ Refactor suggestion | 🟠 MajorUnused
serviceOwnerPartyparameter should be removed.The
serviceOwnerPartyparameter (line 194) is accepted byTestCase.Createbut is never passed toPayloadGeneratorFixture.Create(lines 202-211). This results in unnecessary allocations at lines 61, 83, 95, and 123 whereFactories.ServiceOwnerParty(...)is called but discarded.Additionally, the
Factories.ServiceOwnerPartyhelper (lines 351-352) appears to be unused after this refactor.♻️ Proposed fix to remove dead code
Remove the parameter from
TestCase.Create:public static TestCase Create( string testIdentifier, string fiksArkivMessageType, IEnumerable<string> expectedAttachmentFilenames, FiksArkivDataTypeSettings primaryDocumentSettings, IReadOnlyList<FiksArkivDataTypeSettings>? attachmentSettings, FiksArkivDocumentMetadata? archiveDocumentMetadata, Korrespondansepart recipientParty, - Korrespondansepart serviceOwnerParty, Korrespondansepart? instanceOwnerParty, Klassifikasjon instanceOwnerClassification, string applicationTitle = "Test app", string appId = "ttd/test-app" )Remove
serviceOwnerPartyarguments from all test case calls (lines 61, 83, 95, 123), and delete the unusedFactories.ServiceOwnerPartymethod (lines 351-352).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs` around lines 186 - 216, Remove the unused serviceOwnerParty parameter from TestCase.Create and stop passing it into PayloadGeneratorFixture.Create; update all callers that currently pass Factories.ServiceOwnerParty(...) (the test case calls at the locations referenced) to omit that argument, and delete the now-unused Factories.ServiceOwnerParty helper; specifically edit the TestCase.Create signature to remove the serviceOwnerParty parameter, remove the corresponding argument in calls to PayloadGeneratorFixture.Create, and remove the Factories.ServiceOwnerParty method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs`:
- Around line 186-216: Remove the unused serviceOwnerParty parameter from
TestCase.Create and stop passing it into PayloadGeneratorFixture.Create; update
all callers that currently pass Factories.ServiceOwnerParty(...) (the test case
calls at the locations referenced) to omit that argument, and delete the
now-unused Factories.ServiceOwnerParty helper; specifically edit the
TestCase.Create signature to remove the serviceOwnerParty parameter, remove the
corresponding argument in calls to PayloadGeneratorFixture.Create, and remove
the Factories.ServiceOwnerParty method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c57f16ac-ee62-4924-b2d2-c55ff7f94278
📒 Files selected for processing (5)
src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivConfigResolver.cssrc/Altinn.App.Clients.Fiks/FiksArkiv/IFiksArkivConfigResolver.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cstest/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cstest/Altinn.App.Clients.Fiks.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
💤 Files with no reviewable changes (4)
- src/Altinn.App.Clients.Fiks/FiksArkiv/IFiksArkivConfigResolver.cs
- test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivConfigResolverTest.cs
- src/Altinn.App.Clients.Fiks/FiksArkiv/FiksArkivConfigResolver.cs
- test/Altinn.App.Clients.Fiks.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs (1)
164-170: The test is correct as written; thenull!suppressions don't pose a robustness risk.The
GeneratePayloadmethod validates themessageTypeat the start (line 65 ofFiksArkivDefaultPayloadGenerator.cs) and throws immediately if unsupported, before accessing any fixture state. This negative-path test correctly exercises that guard without depending on later validation.Using factory methods instead of
null!would be more idiomatic C# and improve readability, but it is not necessary for correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs` around lines 164 - 170, The test GeneratePayload_ThrowsException_ForUnsupportedMessageType is correct but uses null! suppressions that reduce readability; replace the null! arguments passed into PayloadGeneratorFixture.Create and the null! in Factories.Instance(...) with the existing factory/helper methods that produce valid dummy fixtures or defaults (use PayloadGeneratorFixture.Create's normal factory overloads and Factories.Instance helper) so the test still triggers the early messageType validation in GeneratePayload but uses explicit, readable test doubles instead of null! assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs`:
- Around line 164-170: The test
GeneratePayload_ThrowsException_ForUnsupportedMessageType is correct but uses
null! suppressions that reduce readability; replace the null! arguments passed
into PayloadGeneratorFixture.Create and the null! in Factories.Instance(...)
with the existing factory/helper methods that produce valid dummy fixtures or
defaults (use PayloadGeneratorFixture.Create's normal factory overloads and
Factories.Instance helper) so the test still triggers the early messageType
validation in GeneratePayload but uses explicit, readable test doubles instead
of null! assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 666b0b5c-4d8f-42e6-b62f-6aeaad390af0
📒 Files selected for processing (1)
test/Altinn.App.Clients.Fiks.Tests/FiksArkiv/FiksArkivDefaultPayloadGeneratorTest.cs
|
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
Removed the extraneous senders (service owner as "Avsender" and Altinn Studio as "Intern avsender", respectively) so that, by default, only the instance owner is set as "Avsender" in the generated payload for Fiks Arkiv.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit