fix: Return distinct actions in GetAlinnActions #1298
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DialogEntityExtensions.cs (1)
23-24: Approved: Effective duplicate removal implementedThe changes successfully address the PR objective by eliminating duplicate action/resource tuples. This optimization will indeed simplify XACML requests and dialog tokens as intended.
For a minor performance improvement, consider using
DistinctByinstead ofGroupByandSelect. It's more concise and potentially more efficient for this use case.Here's a suggested optimization:
- .GroupBy(x => new { x.Name, x.AuthorizationAttribute }) - .Select(g => g.First()) // Remove duplicates by grouping + .DistinctBy(x => new { x.Name, x.AuthorizationAttribute })This change maintains the same functionality while potentially improving performance.
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DialogEntityExtensionsTests.cs (2)
46-51: Great job: Assertions significantly improved and more specific.The new assertions provide a much more thorough check of the
GetAltinnActionsmethod's output:
- Verifying the exact number of actions (9) instead of just non-emptiness.
- Checking for specific action names and their corresponding authorization attributes.
These changes align well with the updated test setup and likely reflect the expected behavior of the
GetAltinnActionsmethod, including deduplication of actions.For improved readability, consider extracting the expected action count to a constant:
const int ExpectedActionCount = 9; Assert.Equal(ExpectedActionCount, actions.Count);This would make it easier to update the expected count if the test setup changes in the future.
Line range hint
1-57: Excellent work: Test changes align well with PR objectives.The modifications to this test method strongly support the PR objectives:
- The test setup now includes duplicate actions, allowing verification of the deduplication process.
- The specific count assertion (9 actions) confirms that duplicates are indeed eliminated.
- The detailed assertions for various action/attribute combinations ensure correct handling of different scenarios.
These changes effectively verify that the
GetAltinnActionsmethod now returns distinct actions, which should lead to simplified XACML requests and dialog tokens as intended.To further enhance test coverage, consider adding an assertion to explicitly verify that no duplicate action/attribute combinations exist in the result:
Assert.Equal(actions.Count, actions.Distinct().Count());This would provide an additional safeguard against potential future regressions in the deduplication logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DialogEntityExtensions.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DialogEntityExtensionsTests.cs (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DialogEntityExtensions.cs (1)
23-24: Verify impact on downstream code and test coverageWhile the changes effectively remove duplicates and optimize the output, it's crucial to ensure that any downstream code relying on
GetAltinnActionscan handle the potentially reduced number of actions.Please run the following script to check for usages of
GetAltinnActionsin the codebase:Review the results to ensure that all usages are compatible with the new behavior.
Additionally, you mentioned in the PR description that relevant automated tests have been added. Could you please provide more information about these tests? It would be helpful to review them to ensure they cover both the new behavior and potential edge cases.
If you need help creating or expanding the test coverage for this change, I'd be happy to assist. Would you like me to propose some additional test cases?
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DialogEntityExtensionsTests.cs (2)
3-3: LGTM: Import statement added for new action types.The new import statement for
Digdir.Domain.Dialogporten.Domain.Dialogs.Entities.Actionsis appropriate and necessary for theDialogApiActionandDialogGuiActiontypes used in the updated test setup.
17-30: Excellent: Test setup enhanced with more comprehensive scenarios.The modifications to the
ApiActionsandGuiActionsproperties significantly improve the test coverage by including:
- Duplicate actions (e.g., multiple "read" actions)
- Actions with different authorization attributes
- New action types ("apiread" and "guiread")
The change in the first transmission's
AuthorizationAttributeto "bar" aligns well with the new API action setup.These changes will help ensure that the
GetAltinnActionsmethod correctly handles various scenarios, including deduplication and proper attribute assignment.Also applies to: 33-33
elsand
changed the title
🤖 I have created a release *beep* *boop* --- ## [1.26.0](v1.25.0...v1.26.0) (2024-10-22) ### Features * Add masstransit outbox system ([#1277](#1277)) ([bc04860](bc04860)) ### Bug Fixes * **infrastructure:** use correct networking for servicebus ([#1320](#1320)) ([4fb42bb](4fb42bb)) * Return distinct actions in GetAlinnActions ([#1298](#1298)) ([49948b2](49948b2)) * Upgraded Altinn.ApiClients.Maskinporten, specify TokenExchangeEnvironment ([#1328](#1328)) ([5156799](5156799)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
GetAltinnActions now removes duplicate action/resource tuples, which results in simpler XACML requests and dialog tokens.
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes