feat(policy editor): app policy summary component #14757
Conversation
|
Warning Rate limit exceeded@nkylstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces several enhancements across the policy editor module. It adds new CSS classes, multiple React components for summarizing policy rules and subjects, and corresponding types. Additionally, new test files have been created to validate these components. The changes also include an expanded set of utility functions and constants for managing policy rules, along with comprehensive unit tests. These updates extend functionality and improve test coverage for policy management in the UI. Changes
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
CodeRabbit Configuration File (
|
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: 4
🧹 Nitpick comments (8)
frontend/packages/policy-editor/src/types/index.ts (1)
5-5: Consider adding JSDoc comments for the URN type.The
AppSubResourceDefaultLimitationTypetype would benefit from documentation explaining the significance of these URN values.+/** URN type for defining the scope of sub-resource limitations in app policies */ export type AppSubResourceDefaultLimitationType = 'urn:altinn:org' | 'urn:altinn:app';frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx (1)
31-47: Consider memoizing the action mapping logic.The action mapping and rendering logic could benefit from memoization to prevent unnecessary re-renders.
+const MemoizedActionCell = React.memo(({ action, actionsForRole }: { action: string; actionsForRole: AppPolicyActionMap }) => ( + <StudioTable.Cell> + <div className={classes.limitationsCell}> + {actionsForRole[action] + ? actionsForRole[action].split(', ').map((r, i) => ( + <StudioTag size='small' key={i} color='info'> + {r} + </StudioTag> + )) + : '-'} + </div> + </StudioTable.Cell> +)); {actions.map((action, index) => ( - <StudioTable.Cell key={index}> - <div className={classes.limitationsCell}> - {actionsForRole[action] - ? actionsForRole[action].split(', ').map((r, i) => { - return ( - <StudioTag size='small' key={i} color='info'> - {r} - </StudioTag> - ); - }) - : '-'} - </div> - </StudioTable.Cell> + <MemoizedActionCell key={action} action={action} actionsForRole={actionsForRole} /> ))}frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx (1)
37-46: Move inline styles to CSS modules.For better maintainability and consistency, move the inline styles to a CSS module.
+// PolicyRuleSummary.module.css +.subjectContainer { + display: flex; + flex-wrap: wrap; + gap: 4px; +} -<div style={{ display: 'flex', flexWrap: 'wrap', gap: 4 }}> +<div className={classes.subjectContainer}>frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.tsx (1)
8-54: Consider memoizing extracted values.The
extractAllUniqueActionsandextractAllUniqueSubjectsfunctions are called multiple times. Consider memoizing their results.export function PolicySummary() { const { policyRules } = usePolicyEditorContext(); const { t } = useTranslation(); + const uniqueActions = React.useMemo(() => extractAllUniqueActions(policyRules), [policyRules]); + const uniqueSubjects = React.useMemo(() => extractAllUniqueSubjects(policyRules), [policyRules]); + return ( // ... existing JSX - {extractAllUniqueActions(policyRules).map((action) => ( + {uniqueActions.map((action) => ( // ... action rendering ))} // ... more JSX - {extractAllUniqueSubjects(policyRules).map((subject) => ( + {uniqueSubjects.map((subject) => ( <PolicyRuleSubjectSummary subject={subject} - actions={extractAllUniqueActions(policyRules)} + actions={uniqueActions} /> ))} // ... remaining JSX ); }frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.test.tsx (1)
54-62: Improve test coverage for PolicyRuleSubjectSummary component.The current test suite only verifies basic rendering. Consider adding test cases for:
- Different subject types (role codes, organizations)
- Various action combinations
- Error states and edge cases
frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.test.tsx (1)
12-25: Consider extracting mock data to a shared test utility.The mock subjects are duplicated across test files. Consider moving them to a shared test utility file, similar to how
mockAction1,mockAction2are imported frompolicyActionMocks.frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts (2)
68-78: Consider memoizing the extracted subjects list.The
extractAllUniqueSubjectsfunction processes the rules array on every call. Consider memoizing the result to improve performance when the rules haven't changed.+import { useMemo } from 'react'; -export const extractAllUniqueSubjects = (rules: PolicyRuleCard[]): string[] => { +export const useExtractAllUniqueSubjects = (rules: PolicyRuleCard[]): string[] => { + return useMemo(() => { const subjects: string[] = []; rules.forEach((rule) => { rule.subject.forEach((s) => { if (!subjects.includes(s.toLowerCase())) { subjects.push(s.toLowerCase()); } }); }); return subjects; + }, [rules]); };
146-167: Consider using a more type-safe approach for action mapping.The
mapActionsForRolefunction uses a plain object for the result. Consider using aMapor defining a strict type for better type safety.- const result = {}; + const result = new Map<string, string>(); filterRulesWithSubject(policyRules, subject).forEach((rule) => { const covers = rule.resources .map((r) => getSubResourceDisplayText(r, usageType, t).concat(` (${rule.ruleId})`)) .join(', '); rule.actions.forEach((action) => { - if (!result[action]) { - result[action] = `${covers}`; + if (!result.has(action)) { + result.set(action, covers); } else { - result[action] += `, ${covers}`; + result.set(action, `${result.get(action)}, ${covers}`); } }); }); return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.module.css(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.test.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.test.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.tsx(1 hunks)frontend/packages/policy-editor/src/types/index.ts(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)frontend/packages/policy-editor/src/utils/AppPolicyUtils/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.module.css
- frontend/packages/policy-editor/src/utils/AppPolicyUtils/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Testing
- GitHub Check: CodeQL
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (2)
frontend/packages/policy-editor/src/types/index.ts (1)
1-3: LGTM! Well-defined type for action mapping.The
AppPolicyActionMaptype provides a flexible structure for mapping policy actions.frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts (1)
13-491: Well-structured and comprehensive test suite!The test suite provides excellent coverage with:
- Clear test descriptions
- Good edge case coverage
- Thorough validation of functionality
...es/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.tsx
Outdated
Show resolved
Hide resolved
...licy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.test.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14757 +/- ##
==========================================
+ Coverage 95.79% 95.80% +0.01%
==========================================
Files 1928 1932 +4
Lines 25141 25235 +94
Branches 2868 2877 +9
==========================================
+ Hits 24083 24176 +93
Misses 798 798
- Partials 260 261 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts (3)
59-85: Make the test description more descriptive.Consider renaming the test description to better describe the scenario being tested, e.g., "should return an empty list when searching for a non-existent subject".
- it('should return an empty list if no rules have the provided subject', () => { + it('should return an empty list when searching for a non-existent subject', () => {
169-173: Make the test description more descriptive.Consider renaming the test description to better describe the scenario being tested, e.g., "should return an empty list when given an empty list of policy rules".
- it('should return an empty list if the list of policy rules is empty', () => { + it('should return an empty list when given an empty list of policy rules', () => {
307-372: Add error cases for invalid inputs.The
mapActionsForRolefunction's test suite could benefit from additional test cases for invalid inputs:
- Invalid resource types
- Malformed resource arrays
- Empty or invalid actions
Consider adding these test cases:
it('should handle invalid resource types gracefully', () => { const rules: PolicyRuleCard[] = [{ ruleId: '1', actions: ['read'], resources: [[{ type: 'invalid:type', id: 'test' }]], subject: ['subject1'], description: 'test' }]; const mappedActions = mapActionsForRole(rules, 'subject1', 'app', textMock); expect(mappedActions).toEqual({}); }); it('should handle empty actions gracefully', () => { const rules: PolicyRuleCard[] = [{ ruleId: '1', actions: [], resources: [[{ type: 'urn:altinn:app', id: '[app]' }]], subject: ['subject1'], description: 'test' }]; const mappedActions = mapActionsForRole(rules, 'subject1', 'app', textMock); expect(mappedActions).toEqual({}); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.test.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.tsx(1 hunks)frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/policy-editor/src/components/PolicySummary/PolicySummary.tsx
- frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.test.tsx
- frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx
⏰ 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 (2)
frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts (2)
212-265: Well-structured test cases with good coverage!The test cases for
getSubResourceDisplayTextare well-structured and cover various scenarios:
- No limitations
- App sub-resource with limitations
- Generic sub-resource with limitations
The test descriptions are clear, and the assertions are specific and meaningful.
493-569: Excellent test coverage for case-insensitive subject matching!The test cases for
getSubjectDisplayNameare comprehensive and cover important scenarios:
- Regular subject lookup
- Non-existent subject fallback
- Service owner special case
- Case-insensitive matching
The test descriptions are clear, and the assertions verify the expected behavior thoroughly.
.../packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx
Outdated
Show resolved
Hide resolved
.../packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx
Outdated
Show resolved
Hide resolved
.../packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx
Outdated
Show resolved
Hide resolved
...es/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx
Outdated
Show resolved
Hide resolved
...es/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.ts (2)
79-86: Fix inconsistency in JSDoc documentation.The JSDoc description for the
delimiterparameter (line 82) mentions that it "Defaults to ', '" (with a space after the comma), but the actual implementation in code (line 85) uses just ',' without a space as the default value.Update the JSDoc to match the actual implementation:
- * @param delimiter The delimiter to use between items. Defaults to ', '. + * @param delimiter The delimiter to use between items. Defaults to ','.
88-97: Fix incorrect parameter name in JSDoc documentation.The JSDoc has an incorrect parameter name. The first parameter is named "array" in the documentation but it should be "string" to match the actual parameter name.
Update the parameter name in the JSDoc:
- * @param array The array of interest. + * @param string The string to split into an array.frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts (2)
338-350: Add test case for whitespace trimming functionality.The
getArrayFromStringimplementation inArrayUtils.tstrims whitespace from each item, but there's no test case specifically verifying this behavior.Add a test case to verify that whitespace is properly trimmed:
it('returns an empty array when the input string is empty', () => { expect(ArrayUtils.getArrayFromString('')).toEqual([]); }); + + it('trims whitespace from each item in the array', () => { + expect(ArrayUtils.getArrayFromString(' a , b , c ')).toEqual(['a', 'b', 'c']); + });
343-345: Test case description could be more precise.The test description states "returns an array with the input string as the only element when no separator is found", but this might be slightly misleading. The string doesn't have to be "not found" - it's more that no splitting occurs because the delimiter doesn't exist in the input.
Consider rephrasing the test description for clarity:
- it('returns an array with the input string as the only element when no separator is found', () => { + it('returns an array with the input string as the only element when the delimiter is not present in the string', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts(1 hunks)frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.ts(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.module.css(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSummary.tsx
🔇 Additional comments (1)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts (1)
324-336: The test suite covers all essential scenarios well.The test cases for the
toStringmethod cover the primary use cases: default delimiter behavior, empty array handling, and custom delimiter support.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx (2)
18-26: Consider memoizing the actionsForRole mapping.The
mapActionsForRoleoperation is potentially expensive and is being recalculated on every render. Since it depends only on the context and props, consider using React.useMemo to optimize performance.- const actionsForRole = mapActionsForRole(policyRules, subject, usageType, t); + const actionsForRole = React.useMemo( + () => mapActionsForRole(policyRules, subject, usageType, t), + [policyRules, subject, usageType, t] + );
36-44: Implement safe access to actionsForRole using optional chaining.Direct property access with
actionsForRole[action]could potentially cause errors if the action key doesn't exist in the object. Using optional chaining would make the code more robust.- {actionsForRole[action] + {actionsForRole?.[action] ? ArrayUtils.getArrayFromString(actionsForRole[action]).map((subResource) => { // ...Also consider adding a null check for the second reference to
actionsForRole[action]within the map function:- ? ArrayUtils.getArrayFromString(actionsForRole[action]).map((subResource) => { + ? ArrayUtils.getArrayFromString(actionsForRole?.[action] || '').map((subResource) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts(1 hunks)frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typechecking and linting
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (4)
frontend/libs/studio-pure-functions/src/ArrayUtils/ArrayUtils.test.ts (2)
324-336: Tests look comprehensive and well-written.The test cases for the
toStringmethod cover the essential scenarios including default delimiter behavior, empty array handling, and custom delimiter usage.
338-354: Good coverage of edge cases for the getArrayFromString method.The test suite for
getArrayFromStringcovers all important scenarios including basic parsing, handling single items without separators, empty strings, and whitespace trimming. This ensures the utility function behaves as expected in various conditions.frontend/packages/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx (2)
13-16: Props interface is clean and focused.The type definition is concise and clearly represents the data required for this component.
32-48: Use unique keys for array items.Using array indices as keys can lead to rendering issues. Consider using a more stable unique identifier.
But I see you're now using
key={action}for theStudioTable.Cellwhich is good. However, ensureactionvalues are indeed unique in your application context to avoid potential key collisions.
...es/policy-editor/src/components/PolicySummary/PolicyRuleSummary/PolicyRuleSubjectSummary.tsx
Show resolved
Hide resolved
|
Skipping manual test because these components are not actually implemented anywhere yet. |
nkylstad
added
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/language/src/nb.json (1)
731-736: Review and Minor Correction for Summary Component Translations
The new keys for the policy summary component are implemented as follows:
"policy_editor.summary_heading": "Oversikt over tilganger som er satt opp for appen""policy_editor.summary_info_about": "I tabellen under kan du se en oversikt over hvilke roller som er gitt tilganger i appens tilgangspolicy, oghvilke rettigheter som er satt for de ulike rollene. Rettigheter kan settes for hele tjenesten, eller for spesifikke deler av tjenesten, som en oppgave i prosessen. Regelen som rettigheten er satt på vises i parentes.""policy_editor.summary_info_edit": "Gå til fanen \"Rediger regler\" for å se detaljer om de enkelte reglene, og for å legge til eller fjerne regler.""policy_editor.summary_table.header_rolecategory": "Kategori""policy_editor.summary_table.header_rolecode": "Kode""policy_editor.summary_table.header_rolename": "Navn"There is a minor typographical error in
"policy_editor.summary_info_about": the word"oghvilke"should be"og hvilke".Please consider applying the following diff to correct this minor issue:
-"policy_editor.summary_info_about": "I tabellen under kan du se en oversikt over hvilke roller som er gitt tilganger i appens tilgangspolicy, oghvilke rettigheter som er satt for de ulike rollene. Rettigheter kan settes for hele tjenesten, eller for spesifikke deler av tjenesten, som en oppgave i prosessen. Regelen som rettigheten er satt på vises i parentes.", +"policy_editor.summary_info_about": "I tabellen under kan du se en oversikt over hvilke roller som er gitt tilganger i appens tilgangspolicy, og hvilke rettigheter som er satt for de ulike rollene. Rettigheter kan settes for hele tjenesten, eller for spesifikke deler av tjenesten, som en oppgave i prosessen. Regelen som rettigheten er satt på vises i parentes.",frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts (4)
38-45: Consider using.some()to avoid creating intermediate arrays.
Instead of mapping each subject to lowercase and then checking.includes(...), you could streamline the check:-return rules.filter((rule) => - rule.subject.map((s) => s.toLowerCase()).includes(subject.toLowerCase()), -); +return rules.filter((rule) => + rule.subject.some((s) => s.toLowerCase() === subject.toLowerCase()), +);This avoids allocating a new array for each rule, though performance gains may be minor unless dealing with very large rule sets.
68-78: Optimize subject extraction using a Set.
Using a Set would simplify uniqueness checks and improve efficiency:-export const extractAllUniqueSubjects = (rules: PolicyRuleCard[]): string[] => { - const subjects: string[] = []; - rules.forEach((rule) => { - rule.subject.forEach((s) => { - if (!subjects.includes(s.toLowerCase())) { - subjects.push(s.toLowerCase()); - } - }); - }); - return subjects; +export const extractAllUniqueSubjects = (rules: PolicyRuleCard[]): string[] => { + const foundSubjects = new Set<string>(); + rules.forEach((rule) => { + rule.subject.forEach((s) => foundSubjects.add(s.toLowerCase())); + }); + return Array.from(foundSubjects); };
85-95: Extract actions more cleanly using a Set.
This can mirror the approach suggested for subjects:-export const extractAllUniqueActions = (rules: PolicyRuleCard[]): string[] => { - const actions: string[] = []; - rules.forEach((rule) => { - rule.actions.forEach((a) => { - if (!actions.includes(a)) { - actions.push(a); - } - }); - }); - return actions; +export const extractAllUniqueActions = (rules: PolicyRuleCard[]): string[] => { + const foundActions = new Set<string>(); + rules.forEach((rule) => { + rule.actions.forEach((a) => foundActions.add(a)); + }); + return Array.from(foundActions); };
139-167: Consider storing action coverage in a more structured format.
Currently, each action stores a comma-separated list of resource coverage strings. For future extensibility (e.g., sorting or filtering resources), you might store them in a data structure, then generate a string at the presentation layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/language/src/nb.json(3 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 skipped from review as they are similar to previous changes (1)
- frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (5)
frontend/language/src/nb.json (3)
660-660: New Translation Entry for Whole Service
The new key"policy_editor._subresource_covers.whole_service": "Hele tjenesten",is clearly added to support the policy summary component. The translation is concise and consistent with similar entries.
708-710: Review of Role Category Translations
The keys related to role categories are defined as follows:
"policy_editor.role_category.altinn_org": "Organisasjon""policy_editor.role_category.altinn_rolecode": "Rolle""policy_editor.role_category.unknown": "Ukjent"These translations are clear and appropriate for their intended context in the policy editor.
725-726: Validation of Rule Editor Keys
The keys"policy_editor.rules_edit": "Regler"and"policy_editor.rules_summary": "Oversikt"are succinctly set. Please confirm that these brief terms fully capture their intended actions without causing confusion in the UI. Overall, they seem consistent with the rest of the localization entries.frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts (2)
9-14: Constants look good.
The default limitations and subject code/name constants are straightforward and seem appropriate for labeling service owners in the policy editor context.
22-30: Implementation ofgetSubjectDisplayNameis clear and intuitive.
Using case-insensitive matching for subject IDs is a solid approach, and falling back to the subject if none is found provides robust handling of unexpected cases.
frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/policy-editor/src/utils/AppPolicyUtils/AppPolicyUtils.ts
Show resolved
Hide resolved
Description
Created a summary component for app policy, that can be used to create an overview of the roles, actions and resources set in the policy.
Example with the default app policy:
Implementing the component in the policy editor will be done in a separate PR.
Verification
Summary by CodeRabbit
New Features
PolicyRuleSubjectSummaryandPolicySummarycomponents to ensure proper rendering and functionality.Style
PolicyRuleSubjectSummaryandPolicyRuleSummarycomponents.