feat: implement policy summary for app policy #14888
Conversation
📝 WalkthroughWalkthroughThis pull request updates several aspects of the policy editor and related UI elements. It increases the maximum width for the settings modal, corrects a typographical error in localization text, and refactors the policy editor by removing deprecated components and tests while introducing new components for a tabbed interface. Additionally, it adjusts key generation in a summary component and modifies the behavior of a utility function, with corresponding changes to its tests. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
nkylstad
changed the title
github-actions
bot
added
solution/studio/designer
![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 comments (1)
frontend/packages/policy-editor/src/PolicyEditor.tsx (1)
59-59: 💡 Verification agent🧩 Analysis chain
Address TODO comment regarding rule ID generation.
There's an unresolved TODO comment about whether the rule ID should be hardcoded. This should be addressed to ensure proper implementation.
🏁 Script executed:
#!/bin/bash # Search for related issues to understand context of the TODO gh issue view 10893 || echo "Issue #10893 not found or not accessible" # Check if there are any recent updates or discussions on this issue gh issue list --search "10893 in:comments" --limit 5Length of output: 773
Update Rule ID Generation Based on Closed Issue #10893
The current implementation still hardcodes part of the rule ID:
`${resourceType}:${usageType === 'app' ? 'example' : resourceId}:ruleid:${pr.ruleId}` // TODO - find out if ID should be hardcoded. Issue: #10893Since Issue #10893 ("Find out what the ruleId for a rule in a policy should look like") is now closed, please update the implementation to follow the finalized decision. If the current behavior is confirmed as correct, remove the TODO comment; otherwise, revise the rule ID generation logic accordingly.
- File:
frontend/packages/policy-editor/src/PolicyEditor.tsx(line 59)
🧹 Nitpick comments (4)
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsx (1)
1-49: Well-structured tests for PolicyRulesEditor componentThe test file provides good coverage for the basic functionality of the PolicyRulesEditor component, including:
- Verifying correct display of policy rules
- Testing the add rule button functionality
Suggestions for improvement:
- Consider adding tests for error handling scenarios (since there's a state variable for error display mentioned in the summary)
- Add a test case for empty policy rules state
- Test any filtering or sorting capabilities if present in the component
// Add additional test cases for more comprehensive coverage: + it('displays error state correctly when an error occurs', () => { + // Mock implementation with error state + mockPolicyEditorContextValue.hasErrors = true; + renderPolicyRulesEditor(); + + // Assert error state display + expect(screen.getByText(/error/i)).toBeInTheDocument(); + }); + + it('handles empty policy rules gracefully', () => { + // Setup empty rules + const emptyPolicy = {...mockPolicy, rules: []}; + mockPolicyEditorContextValue.policy = emptyPolicy; + renderPolicyRulesEditor(); + + // Assert empty state message is shown + expect(screen.getByText(/no policy rules/i)).toBeInTheDocument(); + });frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.tsx (1)
13-21: Consider adding error boundary or error handling for child components.While the component structure is clean, there's no explicit error handling for the child components. If either
PolicyCardRulesorAddPolicyRuleButtonthrows an error, it could affect the entire component.Consider adding error boundaries around child components:
return ( <div> + <ErrorBoundary fallback={<div>Something went wrong with policy rules</div>}> <PolicyCardRules showErrorsOnAllRulesAboveNew={showErrorsOnAllRulesAboveNew} /> <div className={classes.addCardButtonWrapper}> <AddPolicyRuleButton onClick={handleClickAddButton} /> </div> + </ErrorBoundary> </div> );Note: You'll need to import ErrorBoundary from a suitable source or create your own.
frontend/packages/policy-editor/src/components/PolicyEditorTabs/PolicyEditorTabs.test.tsx (1)
9-30: Consider adding additional test cases for edge conditions.While the current tests cover the basic functionality, consider adding tests for potential edge cases, such as:
- Testing accessibility features (keyboard navigation between tabs)
- Testing that content updates correctly when switching between tabs multiple times
- Testing behavior when context values change
frontend/packages/policy-editor/src/PolicyEditor.test.tsx (1)
35-49: Consider adding interaction tests for tab functionality.While these tests verify the presence/absence of UI elements based on different usage types, they don't test the actual interaction with tabs (clicking between tabs, correct content display on tab switch).
+ it('switches between tabs correctly when clicked', async () => { + const user = userEvent.setup(); + renderPolicyEditor(); + + // Verify initial tab content is visible + const summaryTab = screen.getByText(textMock('policy_editor.rules_summary')); + const editTab = screen.getByText(textMock('policy_editor.rules_edit')); + + // Initially summary tab should be selected + expect(screen.getByText(textMock('policy_editor.summary_heading'))).toBeInTheDocument(); + + // Click on the edit tab + await user.click(editTab); + + // Verify edit tab content is now visible + expect(screen.getByText(textMock('policy_editor.card_button_text'))).toBeInTheDocument(); + + // Switch back to summary tab + await user.click(summaryTab); + + // Verify summary content is visible again + expect(screen.getByText(textMock('policy_editor.summary_heading'))).toBeInTheDocument(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/app-development/layout/PageHeader/SubHeader/SettingsModalButton/SettingsModal/SettingsModal.module.css(1 hunks)frontend/language/src/nb.json(1 hunks)frontend/packages/policy-editor/src/PolicyEditor.module.css(0 hunks)frontend/packages/policy-editor/src/PolicyEditor.test.tsx(1 hunks)frontend/packages/policy-editor/src/PolicyEditor.tsx(2 hunks)frontend/packages/policy-editor/src/components/PolicyEditorTabs/PolicyEditorTabs.test.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicyEditorTabs/PolicyEditorTabs.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.module.css(1 hunks)frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx(1 hunks)frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts(1 hunks)frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/policy-editor/src/PolicyEditor.module.css
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.module.css
- frontend/language/src/nb.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (22)
frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx (1)
39-42: Improved key uniqueness for StudioTag componentsThe addition of
subjectto the key prop (${subject}-${action}-${subResource}) is a good improvement that ensures better uniqueness for React's reconciliation process, especially in scenarios where the same action-subResource combination might appear across different subjects.frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts (1)
56-57:✅ Verification successful
Behavior change in filterDefaultAppLimitations function
This is a significant change in behavior. Previously, the function would return an empty array when no default limitations were found, but now it returns the original resources array. This change seems more logically correct since if there are no default limitations to filter out, the original data should be preserved.
Verify that this behavior change doesn't break any existing functionality that might expect an empty array when no default limitations are found.
🏁 Script executed:
#!/bin/bash # Find all usages of filterDefaultAppLimitations to check impact of behavior change # Look for files that call this function rg --type=typescript "filterDefaultAppLimitations" -A 2 -B 2 --no-filenameLength of output: 109
I'll run a revised search that doesn't rely on the built‐in "typescript" file type. Please run the following new shell script to check for any usages of the
filterDefaultAppLimitationsfunction:If this search returns results, review the contexts to ensure that returning the original
appResourcesarray (instead of an empty array) when no default limitations are found does not break any dependent functionalities. If no usages are found, it's less likely that this behavior change will negatively impact the codebase, but please manually verify the tests and any edge-case scenarios.
🏁 Script executed:
#!/bin/bash # Search for usages of filterDefaultAppLimitations in TypeScript files using a glob pattern rg "filterDefaultAppLimitations" --glob="*.ts" -A 2 -B 2 --no-filenameLength of output: 1258
Action Required: Confirm Test Coverage for Updated Filtering Behavior
The current tests confirm that:
- When no default limitations are present in the resource list, the function returns the original
appResourcesarray.- In scenarios where default limitations exist, the filtering correctly produces an empty array (or removes them accordingly).
While the implemented change is logically sound and well-covered by the tests, please take an extra moment to manually verify edge cases or any integrations that might implicitly expect an empty array. Based on our review, there’s no immediate concern, and the changes seem safe.
frontend/app-development/layout/PageHeader/SubHeader/SettingsModalButton/SettingsModal/SettingsModal.module.css (1)
2-2: Significant increase in modal widthThe maximum width of the settings modal has been increased from 64rem to 150rem (more than doubling the previous size), while maintaining the responsive behavior with the 80vw constraint. This change likely accommodates the new PolicySummary component mentioned in the PR objectives.
Verify that:
- The increased width doesn't cause any layout issues on standard displays
- The content properly fills the expanded space without appearing too sparse
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsx (1)
42-48: Helper function enhances test readabilityThe
renderPolicyRulesEditorhelper function is a good practice that encapsulates the rendering logic with the context provider, making the test cases more readable and maintainable.frontend/packages/policy-editor/src/components/PolicyEditorTabs/PolicyEditorTabs.tsx (2)
1-5: Well-structured imports with clear dependencies.The imports are organized logically, with React core imports first, followed by component library imports, and then project-specific components.
7-23: Clean implementation of tabbed interface with proper internationalization.The component is well-structured with:
- Proper use of the
useTranslationhook for internationalization- Clear tab structure with descriptive values ('summary' and 'rules')
- Appropriate setting of 'summary' as the default tab
- Clean component composition using the imported tab components
This implementation follows React best practices for functional components and provides good separation of concerns between the tab structure and the content components.
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.tsx (2)
1-5: Appropriate imports with clear dependencies.The imports are well-organized, including React's useState hook, component imports, and CSS module import.
6-12: Well-implemented state management for error handling.The component correctly uses the useState hook to track whether errors should be shown on rules above a newly added rule. The handler function is concise and follows best practices.
frontend/packages/policy-editor/src/components/PolicyEditorTabs/PolicyEditorTabs.test.tsx (4)
1-8: Comprehensive test setup with appropriate imports.The imports are well-organized, including testing libraries, the component under test, context provider, and necessary mocks.
9-13: Basic test coverage for default tab selection.The test correctly verifies that the summary tab is selected by default, which is an important behavior to validate.
15-21: Good user interaction testing for tab switching.The test appropriately uses userEvent to simulate user interaction with the tabs and verifies the expected content is displayed after switching tabs.
24-30: Good use of helper function for consistent component rendering.The helper function encapsulates the rendering logic with the necessary context provider, making the tests more maintainable and reducing code duplication.
frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts (3)
113-113: Test description correctly updated to reflect new function behavior.The test description has been appropriately updated to reflect the changed behavior of the
filterDefaultAppLimitationsfunction.
121-121: Updated test assertion aligns with function behavior change.The assertion has been correctly modified to check that the function returns the input array instead of an empty array when no default app limitations are present.
113-122:❓ Verification inconclusive
Verify impact of behavior change across the application.
The change in the
filterDefaultAppLimitationsfunction behavior from returning an empty array to returning the input array could impact other parts of the application that depend on this function.
🏁 Script executed:
#!/bin/bash # Description: Find all usages of filterDefaultAppLimitations to assess impact of behavior change echo "Looking for usages of filterDefaultAppLimitations function..." rg -A 3 -B 3 "filterDefaultAppLimitations" --type ts --type tsx echo "Checking for potential affected components..." rg -A 5 "appResources.*filter" --type ts --type tsxLength of output: 407
Manual Verification Required: Confirm Impact of
filterDefaultAppLimitationsChangeThe behavior of
filterDefaultAppLimitationsnow returns the input array instead of an empty array when no default limitations are present. An initial automated search for its usages failed due to file type filtering issues (unrecognized file type: tsx).
- Please re-run the search across both
.tsand.tsxfiles. For example, you might use:# Search in both TypeScript file types fd -e ts -e tsx -x rg -A 3 -B 3 "filterDefaultAppLimitations" fd -e ts -e tsx -x rg -A 5 "appResources.*filter"- Verify manually that no other parts of the application rely on the previous behavior (i.e., an empty array being returned).
frontend/packages/policy-editor/src/PolicyEditor.test.tsx (2)
38-43: Good test implementation for the app usage type.This test correctly verifies that the component renders a tabbed interface when the
usageTypeis set to 'app', checking for the presence of key elements in the UI.
45-49: Good test implementation for the resource usage type.This test appropriately verifies the alternate rendering path when
usageTypeis 'resource', ensuring the tab interface is not present and the rules view is properly displayed.frontend/packages/policy-editor/src/PolicyEditor.tsx (5)
22-23: Good modularization of components.The addition of dedicated components for policy rules editing and tab navigation improves the code's modularity and separation of concerns.
93-93: Clean conditional rendering implementation.The conditional rendering based on
usageTypeis concise and readable, providing different experiences for app and resource contexts.
99-102:❓ Verification inconclusive
Address TODO comment about resource type determination.
There's another TODO comment about how the resource type should be determined. This should be addressed to ensure the implementation is as intended.
🏁 Script executed:
#!/bin/bash # Search for related issues to understand context of the TODO gh issue view 10880 || echo "Issue #10880 not found or not accessible" # Check if there are any recent updates or discussions on this issue gh issue list --search "10880 in:comments" --limit 5Length of output: 728
Confirm Resource Type Determination Approach
The issue (#10880) is closed, but the acceptance criteria indicate that the correct resource type hasn’t been finalized. Please verify whether the current hardcoded values meet the intended design. If these values (i.e.
'urn:altinn'for apps and'urn:altinn:resource'for resources) are still acceptable, remove the TODO; otherwise, update the implementation based on any final decision or documented requirements.
54-67:❓ Verification inconclusive
Verify policy saving logic with new component structure.
The policy saving logic appears unchanged, but ensure it still properly integrates with the new
PolicyRulesEditorandPolicyEditorTabscomponents. Make sure these components can properly trigger thehandleSavePolicyfunction with the correct data.
🏁 Script executed:
#!/bin/bash # Check how the new components interact with the save functionality echo "Checking PolicyRulesEditor for save functionality:" rg -n "savePolicy|handleSave" frontend/packages/policy-editor/src/components/PolicyRulesEditor/ echo "Checking PolicyEditorTabs for save functionality:" rg -n "savePolicy|handleSave" frontend/packages/policy-editor/src/components/PolicyEditorTabs/Length of output: 587
Action Required: Verify Integration of Save Functionality in PolicyEditorTabs
The current tests confirm that PolicyRulesEditor calls the save function (as seen in its test file), which is great. However, we detected no direct references to
savePolicyorhandleSavein PolicyEditorTabs. Please ensure that:
- PolicyEditorTabs is correctly wired to trigger the
handleSavePolicyfunction or otherwise integrate with the save logic.- The absence of direct references in PolicyEditorTabs is intentional (e.g., if it delegates the save operation in a non-obvious way) and is covered by existing tests.
It’s recommended to manually verify or add tests for PolicyEditorTabs to confirm it dispatches the save correctly when needed.
1-102:✅ Verification successful
Include unit tests for refactored components.
With the introduction of
PolicyRulesEditorandPolicyEditorTabscomponents, ensure these new components have adequate test coverage to maintain the overall test coverage of the application.
🏁 Script executed:
#!/bin/bash # Check if test files exist for the new components echo "Checking for PolicyRulesEditor tests:" ls -la frontend/packages/policy-editor/src/components/PolicyRulesEditor/*.test.tsx || echo "Test file not found" echo "Checking for PolicyEditorTabs tests:" ls -la frontend/packages/policy-editor/src/components/PolicyEditorTabs/*.test.tsx || echo "Test file not found"Length of output: 647
Unit Test Coverage Verified for New Components
The new components already include unit tests:
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsxfrontend/packages/policy-editor/src/components/PolicyEditorTabs/PolicyEditorTabs.test.tsxNo further action is needed regarding unit tests at this time. Please ensure that these tests continue to cover all relevant scenarios as changes are made.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsx (3)
28-39: Test name doesn't match assertion.The test name suggests that it checks if the rule list length increases, but it only verifies that the
savePolicyfunction was called, without confirming the actual list length change.Consider adding an assertion that actually verifies the list length increases, or rename the test to better reflect what it's actually testing:
- it('increases the rule list length when add rule button is clicked', async () => { + it('calls savePolicy when add rule button is clicked', async () => {Alternatively, if you want to verify the rule list length increases, you should update the mock to reflect the change and add an assertion:
// After the click await user.click(addButton); // Update the mock to simulate what would happen in the real component mockPolicyEditorContextValue.policy.rules = [...mockPolicy.rules, newMockRule]; // Re-render to reflect changes rerender(); // Now check that an additional rule appears const updatedLabelCount = screen.queryAllByText( textMock('policy_editor.rule_card_sub_resource_title') ).length; expect(updatedLabelCount).toEqual(mockPolicy.rules.length + 1);
42-48: Consider placing the helper function before tests.While JavaScript allows function declarations to be hoisted, it's a good practice to define helper functions before using them for better readability.
Consider moving the
renderPolicyRulesEditorfunction above the test suite.
1-49: Consider adding more test cases.The test file currently only tests basic functionality. Consider adding more test cases for error handling, edge cases, or specific interactions with policy rules.
Additional test cases to consider:
- Testing error states
- Testing editing existing rules
- Testing deletion of rules
- Testing validation of rules
- Testing the behavior when there are no rules
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (3)
frontend/packages/policy-editor/src/components/PolicyRulesEditor/PolicyRulesEditor.test.tsx (3)
1-9: Good imports and setup.The file correctly imports the necessary components, mocks, and testing utilities needed for the test suite.
11-17: Mock data is well structured.The mock policy object provides all required properties for testing purposes.
19-26: Test verifies correct rendering of policy rules.The test effectively verifies that the component renders the correct number of policy rules based on the mock data.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14888 +/- ##
=======================================
Coverage 95.80% 95.80%
=======================================
Files 1928 1930 +2
Lines 25201 25216 +15
Branches 2878 2879 +1
=======================================
+ Hits 24143 24158 +15
Misses 797 797
Partials 261 261 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Implement new PolicySummary component to show a summary of the policy for apps:
Verification
Documentation
Summary by CodeRabbit
Style
New Features
Bug Fixes
Tests
PolicyEditorandPolicyRulesEditorcomponents, ensuring accurate rendering and functionality.