WEB-859: Update Role Permission Search Field - #3381
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
View Role Component src/app/system/roles-and-permissions/view-role/view-role.component.html, view-role.component.scss, view-role.component.ts |
Restructured permissions UI with toolbar layout containing conditional grouping label/search-results summary and Select/Deselect buttons; added search field with icon and clear button; implemented two-panel layout (grouping list + permissions list) with alternate search-results panel; added search filtering logic with properties searchText, filteredPermissions, isSearchActive and methods filterPermissions(), clearSearch(); updated card actions styling and container spacing; added MatIcon imports and styling for toolbar, search UI, scrollable panels, permission badges, and no-results state. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'WEB-859: Update Role Permission Search Field' accurately describes the main change in the changeset—adding a search field for filtering permissions with supporting UI/UX improvements. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
📝 Coding Plan
- Generate coding plan for human review comments
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 help to get the list of available commands and usage tips.
|
Hi @alberto-art3ch and @IOhacker, |
![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 (2)
src/app/system/roles-and-permissions/view-role/view-role.component.scss (2)
189-203: Avoid double horizontal spacing in submit actions.At Line 192
gap: 8pxis already set on the flex container, but buttons still applymargin-right(Line 198–Line 202). This duplicates spacing and makes layout tuning harder.♻️ Suggested cleanup
.submit-actions { display: flex; justify-content: flex-end; gap: 8px; padding: 16px 0 0; margin: 0; border-top: 1px solid $light-grey; button { - margin-right: 8px; - - &:last-child { - margin-right: 0; - } + margin-right: 0; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/system/roles-and-permissions/view-role/view-role.component.scss` around lines 189 - 203, The .submit-actions flex container already uses gap: 8px, so remove the redundant per-button horizontal margins: delete the button { margin-right: 8px; &:last-child { margin-right: 0; } } rules in the .submit-actions block (or at minimum remove margin-right from button) so spacing is controlled solely by gap; target the .submit-actions and its nested button selector to make the change.
39-196: Use shared spacing/theme tokens for the new permission UI styles.New styles add many hardcoded values (for example Line 44, Line 45, Line 70, Line 82, Line 86, Line 142, Line 173, Line 177–Line 180, Line 193). Please switch these to project spacing/theme variables to keep design consistency and simplify future theming.
As per coding guidelines
src/**/*.{scss,html}: Stick to the 8px grid system for visual design and spacingandLeverage SCSS variables defined in src/main.scss and src/theme/mifosx-theme.scss rather than generating custom classes and explicit pixel values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/system/roles-and-permissions/view-role/view-role.component.scss` around lines 39 - 196, Several selectors in view-role.component.scss (.permission-toolbar, .permission-toolbar-left h3, .permission-toolbar-right, .search-count, .search-field-container, .permissions-panel, .grouping-list, .display-permissions, .list-permission, .content div(.header), .search-results-panel, .permission-grouping-badge, .no-results, .submit-actions, etc.) use hardcoded pixel values and custom sizes; replace those hardcoded spacing/size/color values with the project’s spacing and theme variables from src/main.scss and src/theme/mifosx-theme.scss (use the defined spacing tokens that follow the 8px grid and the theme color variables instead of $light-grey/$dark-grey literals where appropriate), consolidate repeated sizes (e.g., repeated 480px/16px/8px/24px/48px/2.5rem/0.75rem) into the corresponding spacing/typography tokens, and ensure paddings/margins/gaps align to the 8px grid while keeping the same visual layout for selectors like .permission-toolbar, .list-permission, .permission-grouping-badge, .no-results mat-icon and .submit-actions.
🤖 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/app/system/roles-and-permissions/view-role/view-role.component.html`:
- Around line 94-102: Bulk action buttons are visible during a search and can
modify the wrong permission set; hide them while search is active. Update the
template condition around the permission toolbar buttons (the block rendering
the Select All/Deselect All buttons that calls selectAll() and deselectAll()) to
also require that search is not active (e.g., change the guard from !isDisabled
to !isDisabled && !isSearching or use a getter like isSearching => !!searchQuery
|| filteredPermissions?.length > 0), and apply the same visibility change to the
other toolbar area that shows permissions (the section using filteredPermissions
around lines 159–182) so bulk actions are not shown when
filteredPermissions/search mode is in effect. Ensure you reference selectAll,
deselectAll and filteredPermissions when making these changes.
- Around line 117-119: The icon-only clear-search button lacks an accessible
name; update the button element that calls clearSearch() (the <button ...
(click)="clearSearch()"> with the <mat-icon>close</mat-icon>) to include an
aria-label (e.g., aria-label="Clear search") or an i18n-friendly label
(aria-label="{{ 'roles.clearSearch' | translate }}") so screen readers announce
its purpose—ensure the attribute is added to the same button that invokes
clearSearch().
- Around line 88-89: The template in the ViewRoleComponent uses three missing
translation keys—labels.text.Search Results, labels.inputs.Search Permissions,
and labels.text.No permissions found—so either run the translation extractor
(npm run translations:extract) to add them automatically or manually add these
keys with appropriate translations into all locale JSON files (i.e., every
translations locale file) so the strings render correctly for all locales.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.ts`:
- Around line 241-266: In filterPermissions, the input is only trimmed for the
empty check but the matching uses the untrimmed searchValue, so leading/trailing
spaces (e.g. " read ") fail to match; fix by normalizing the input up
front—create a trimmed value (e.g., searchValueTrimmed = searchValue.trim()),
assign this.searchText = searchValueTrimmed, use that trimmed value to build
lowerSearch and for the empty-check/early return, and then continue matching
against lowerSearch in the existing loop (references: function
filterPermissions, variables this.searchText, lowerSearch,
this.filteredPermissions, this.groupings, this.tempPermissionUIData).
---
Nitpick comments:
In `@src/app/system/roles-and-permissions/view-role/view-role.component.scss`:
- Around line 189-203: The .submit-actions flex container already uses gap: 8px,
so remove the redundant per-button horizontal margins: delete the button {
margin-right: 8px; &:last-child { margin-right: 0; } } rules in the
.submit-actions block (or at minimum remove margin-right from button) so spacing
is controlled solely by gap; target the .submit-actions and its nested button
selector to make the change.
- Around line 39-196: Several selectors in view-role.component.scss
(.permission-toolbar, .permission-toolbar-left h3, .permission-toolbar-right,
.search-count, .search-field-container, .permissions-panel, .grouping-list,
.display-permissions, .list-permission, .content div(.header),
.search-results-panel, .permission-grouping-badge, .no-results, .submit-actions,
etc.) use hardcoded pixel values and custom sizes; replace those hardcoded
spacing/size/color values with the project’s spacing and theme variables from
src/main.scss and src/theme/mifosx-theme.scss (use the defined spacing tokens
that follow the 8px grid and the theme color variables instead of
$light-grey/$dark-grey literals where appropriate), consolidate repeated sizes
(e.g., repeated 480px/16px/8px/24px/48px/2.5rem/0.75rem) into the corresponding
spacing/typography tokens, and ensure paddings/margins/gaps align to the 8px
grid while keeping the same visual layout for selectors like
.permission-toolbar, .list-permission, .permission-grouping-badge, .no-results
mat-icon and .submit-actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2252c691-c182-4855-9961-4ec4ed0e11df
📒 Files selected for processing (3)
src/app/system/roles-and-permissions/view-role/view-role.component.htmlsrc/app/system/roles-and-permissions/view-role/view-role.component.scsssrc/app/system/roles-and-permissions/view-role/view-role.component.ts
| <strong>{{ 'labels.text.Search Results' | translate }}</strong> | ||
| <span class="search-count">({{ filteredPermissions.length }})</span> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new translation keys are present in locale resources.
# Expected: each key appears in translation JSON files (or gets generated by extraction workflow).
rg -n --type=json "labels\\.text\\.Search Results|labels\\.inputs\\.Search Permissions|labels\\.text\\.No permissions found"
# Optional project workflow check (read-only guidance):
# npm run translations:extractRepository: openMF/web-app
Length of output: 40
🏁 Script executed:
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.html | sed -n '85,95p'Repository: openMF/web-app
Length of output: 596
🏁 Script executed:
# Check if there are any locale/i18n files in the project
fd -e json -p "locale|i18n|translation" | head -20Repository: openMF/web-app
Length of output: 512
🏁 Script executed:
# Search more broadly for where translation keys might be defined
rg -l "labels\.text\." --type=json | head -10Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.html | sed -n '105,112p'Repository: openMF/web-app
Length of output: 434
🏁 Script executed:
cat -n src/app/system/roles-and-permissions/view-role/view-role.component.html | sed -n '162,168p'Repository: openMF/web-app
Length of output: 419
🏁 Script executed:
# Check the structure and content of a translation file
cat src/assets/translations/en-US.json | head -50Repository: openMF/web-app
Length of output: 4653
🏁 Script executed:
# Search specifically for the keys mentioned in all translation files
rg "Search Results|Search Permissions|No permissions found" src/assets/translations/Repository: openMF/web-app
Length of output: 40
Add missing translation keys to locale files or run translation extraction.
The template uses three translation keys that do not exist in any locale file:
labels.text.Search Results(line 88)labels.inputs.Search Permissions(line 108)labels.text.No permissions found(line 165)
Without these keys in src/assets/translations/*.json, users will see untranslated key strings. Run npm run translations:extract to generate the missing keys, or manually add them to all locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.html`
around lines 88 - 89, The template in the ViewRoleComponent uses three missing
translation keys—labels.text.Search Results, labels.inputs.Search Permissions,
and labels.text.No permissions found—so either run the translation extractor
(npm run translations:extract) to add them automatically or manually add these
keys with appropriate translations into all locale JSON files (i.e., every
translations locale file) so the strings render correctly for all locales.
There was a problem hiding this comment.
Will fix this in the next push!
There was a problem hiding this comment.
@YousufFFFF, sounds good! When you push the fix, remember to also run npm run translations:extract to ensure all locale files are updated with the new keys (labels.text.Search Results, labels.inputs.Search Permissions, and labels.text.No permissions found). I'll verify them in the next review round. 👍
🧠 Learnings used
Learnt from: shubhamkumar9199
Repo: openMF/web-app PR: 3367
File: src/app/savings/gsim-account/gsim-account.component.html:17-17
Timestamp: 2026-03-13T09:33:25.634Z
Learning: In `src/app/savings/gsim-account/gsim-account.component.html`, the `mat-card-header` / `mat-card-title` elements were intentionally removed from the Group Summary card because `mat-card-header` introduces flex layout behavior that caused horizontal overflow and alignment issues with the `layout-row-wrap` container. A plain `<h3>` heading inside `mat-card-content` is used instead as a deliberate workaround. Do not flag this as a violation of the Angular Material usage guideline.
Learnt from: CR
Repo: openMF/web-app PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-11T06:23:30.388Z
Learning: Applies to src/app/**/*.{html,ts} : Run `npm run translations:extract` to extract i18n variables whenever new strings are added to code
Learnt from: CR
Repo: openMF/web-app PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-11T06:23:30.388Z
Learning: Applies to src/app/**/*.{html,ts} : Use proper i18n variables from `ngx-translate/core` for all user-facing strings instead of hardcoded text
| <div class="permission-toolbar-right"> | ||
| @if (!isDisabled) { | ||
| <div class="flex-25 layout-row layout-xs-column layout-align-end gap-5px select-deselect"> | ||
| <button mat-raised-button color="primary" (click)="selectAll()"> | ||
| {{ 'labels.buttons.Select All' | translate }} | ||
| </button> | ||
| <button mat-raised-button color="default" (click)="deselectAll()"> | ||
| {{ 'labels.buttons.Deselect All' | translate }} | ||
| </button> | ||
| </div> | ||
| <button mat-raised-button color="primary" (click)="selectAll()"> | ||
| {{ 'labels.buttons.Select All' | translate }} | ||
| </button> | ||
| <button mat-raised-button color="default" (click)="deselectAll()"> | ||
| {{ 'labels.buttons.Deselect All' | translate }} | ||
| </button> | ||
| } |
There was a problem hiding this comment.
Prevent bulk actions from modifying the wrong permission set during search.
At Line 95, bulk buttons remain visible in search mode, but search view (Line 159–Line 182) shows filteredPermissions while bulk methods still operate on the current grouping list. This can change hidden checkboxes unexpectedly.
✅ Minimal safe fix (hide bulk actions during search mode)
- `@if` (!isDisabled) {
+ `@if` (!isDisabled && !isSearchActive) {
<button mat-raised-button color="primary" (click)="selectAll()">
{{ 'labels.buttons.Select All' | translate }}
</button>
<button mat-raised-button color="default" (click)="deselectAll()">
{{ 'labels.buttons.Deselect All' | translate }}
</button>
}Also applies to: 159-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.html`
around lines 94 - 102, Bulk action buttons are visible during a search and can
modify the wrong permission set; hide them while search is active. Update the
template condition around the permission toolbar buttons (the block rendering
the Select All/Deselect All buttons that calls selectAll() and deselectAll()) to
also require that search is not active (e.g., change the guard from !isDisabled
to !isDisabled && !isSearching or use a getter like isSearching => !!searchQuery
|| filteredPermissions?.length > 0), and apply the same visibility change to the
other toolbar area that shows permissions (the section using filteredPermissions
around lines 159–182) so bulk actions are not shown when
filteredPermissions/search mode is in effect. Ensure you reference selectAll,
deselectAll and filteredPermissions when making these changes.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the current state of the file before confirming.
🏁 Script executed:
#!/bin/bash
rg -n "isDisabled|isSearchActive" src/app/system/roles-and-permissions/view-role/view-role.component.html
Length of output: 127
@YousufFFFF, it looks like the fix hasn't been applied in the current branch yet — line 95 still reads:
`@if` (!isDisabled) {The suggested guard needs to be updated to also check !isSearchActive:
`@if` (!isDisabled && !isSearchActive) {
<button mat-raised-button color="primary" (click)="selectAll()">
{{ 'labels.buttons.Select All' | translate }}
</button>
<button mat-raised-button color="default" (click)="deselectAll()">
{{ 'labels.buttons.Deselect All' | translate }}
</button>
}Without this guard, clicking Select All or Deselect All while a search is active will still operate on the current grouping's permission list (not the filtered results), silently changing checkboxes that aren't visible on screen. Could you apply this change?
🧠 Learnings used
Learnt from: shubhamkumar9199
Repo: openMF/web-app PR: 3367
File: src/app/savings/gsim-account/gsim-account.component.html:17-17
Timestamp: 2026-03-13T09:33:25.634Z
Learning: In `src/app/savings/gsim-account/gsim-account.component.html`, the `mat-card-header` / `mat-card-title` elements were intentionally removed from the Group Summary card because `mat-card-header` introduces flex layout behavior that caused horizontal overflow and alignment issues with the `layout-row-wrap` container. A plain `<h3>` heading inside `mat-card-content` is used instead as a deliberate workaround. Do not flag this as a violation of the Angular Material usage guideline.
| <button matSuffix mat-icon-button (click)="clearSearch()"> | ||
| <mat-icon>close</mat-icon> | ||
| </button> |
There was a problem hiding this comment.
Add an accessible name to the clear-search icon button.
The icon-only button at Line 117 has no aria-label, so screen readers may not announce its purpose.
♿ Suggested fix
- <button matSuffix mat-icon-button (click)="clearSearch()">
+ <button
+ matSuffix
+ mat-icon-button
+ (click)="clearSearch()"
+ [attr.aria-label]="'labels.buttons.Clear' | translate"
+ >
<mat-icon>close</mat-icon>
</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.html`
around lines 117 - 119, The icon-only clear-search button lacks an accessible
name; update the button element that calls clearSearch() (the <button ...
(click)="clearSearch()"> with the <mat-icon>close</mat-icon>) to include an
aria-label (e.g., aria-label="Clear search") or an i18n-friendly label
(aria-label="{{ 'roles.clearSearch' | translate }}") so screen readers announce
its purpose—ensure the attribute is added to the same button that invokes
clearSearch().
| filterPermissions(searchValue: string) { | ||
| this.searchText = searchValue; | ||
| if (!searchValue || searchValue.trim() === '') { | ||
| this.isSearchActive = false; | ||
| this.filteredPermissions = []; | ||
| return; | ||
| } | ||
| this.isSearchActive = true; | ||
| const lowerSearch = searchValue.toLowerCase(); | ||
| this.filteredPermissions = []; | ||
| for (const grouping of this.groupings) { | ||
| const group = this.tempPermissionUIData[grouping]; | ||
| if (group) { | ||
| for (const perm of group.permissions) { | ||
| const readableName = perm.code.replace(/_/g, ' ').toLowerCase(); | ||
| if (readableName.includes(lowerSearch) || perm.code.toLowerCase().includes(lowerSearch)) { | ||
| this.filteredPermissions.push({ | ||
| code: perm.code, | ||
| id: perm.id, | ||
| grouping | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Normalize trimmed input before matching in permission search.
At Line 243 you trim only for empty-check, but matching uses untrimmed text (Line 249). Inputs like " read " can incorrectly return no results.
🔎 Suggested fix
filterPermissions(searchValue: string) {
this.searchText = searchValue;
- if (!searchValue || searchValue.trim() === '') {
+ const normalizedSearch = searchValue?.trim().toLowerCase() ?? '';
+ if (!normalizedSearch) {
this.isSearchActive = false;
this.filteredPermissions = [];
return;
}
this.isSearchActive = true;
- const lowerSearch = searchValue.toLowerCase();
this.filteredPermissions = [];
for (const grouping of this.groupings) {
const group = this.tempPermissionUIData[grouping];
if (group) {
for (const perm of group.permissions) {
const readableName = perm.code.replace(/_/g, ' ').toLowerCase();
- if (readableName.includes(lowerSearch) || perm.code.toLowerCase().includes(lowerSearch)) {
+ if (readableName.includes(normalizedSearch) || perm.code.toLowerCase().includes(normalizedSearch)) {
this.filteredPermissions.push({
code: perm.code,
id: perm.id,
grouping
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| filterPermissions(searchValue: string) { | |
| this.searchText = searchValue; | |
| if (!searchValue || searchValue.trim() === '') { | |
| this.isSearchActive = false; | |
| this.filteredPermissions = []; | |
| return; | |
| } | |
| this.isSearchActive = true; | |
| const lowerSearch = searchValue.toLowerCase(); | |
| this.filteredPermissions = []; | |
| for (const grouping of this.groupings) { | |
| const group = this.tempPermissionUIData[grouping]; | |
| if (group) { | |
| for (const perm of group.permissions) { | |
| const readableName = perm.code.replace(/_/g, ' ').toLowerCase(); | |
| if (readableName.includes(lowerSearch) || perm.code.toLowerCase().includes(lowerSearch)) { | |
| this.filteredPermissions.push({ | |
| code: perm.code, | |
| id: perm.id, | |
| grouping | |
| }); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| filterPermissions(searchValue: string) { | |
| this.searchText = searchValue; | |
| const normalizedSearch = searchValue?.trim().toLowerCase() ?? ''; | |
| if (!normalizedSearch) { | |
| this.isSearchActive = false; | |
| this.filteredPermissions = []; | |
| return; | |
| } | |
| this.isSearchActive = true; | |
| this.filteredPermissions = []; | |
| for (const grouping of this.groupings) { | |
| const group = this.tempPermissionUIData[grouping]; | |
| if (group) { | |
| for (const perm of group.permissions) { | |
| const readableName = perm.code.replace(/_/g, ' ').toLowerCase(); | |
| if (readableName.includes(normalizedSearch) || perm.code.toLowerCase().includes(normalizedSearch)) { | |
| this.filteredPermissions.push({ | |
| code: perm.code, | |
| id: perm.id, | |
| grouping | |
| }); | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/system/roles-and-permissions/view-role/view-role.component.ts` around
lines 241 - 266, In filterPermissions, the input is only trimmed for the empty
check but the matching uses the untrimmed searchValue, so leading/trailing
spaces (e.g. " read ") fail to match; fix by normalizing the input up
front—create a trimmed value (e.g., searchValueTrimmed = searchValue.trim()),
assign this.searchText = searchValueTrimmed, use that trimmed value to build
lowerSearch and for the empty-check/early return, and then continue matching
against lowerSearch in the existing loop (references: function
filterPermissions, variables this.searchText, lowerSearch,
this.filteredPermissions, this.groupings, this.tempPermissionUIData).
|
This is good but this is for role only. But the next is when you create/edit the role and look for the permissions |
Sure! Just went through your attached video on jira will try to implement the same for permissions too! |
Roles.and.Permissions.-.Google.Chrome.2026-03-15.04-26-59.mp4Is this alligned with what you had in mind @IOhacker ? |
|
@YousufFFFF yes. It is the functionality expected. Just the buttons to cancel or apply them after their selection |
|
@IOhacker Cancel and submit buttons exists here. |
|
@YousufFFFF could you please review the code rabbit comments and add the language translations? |
|
Hey @IOhacker, |
|
@IOhacker I have opened a new PR that adds the additional things that we discussed here with the language translations! |
2 participants
Description
This PR adds a search and filtering feature for permissions in the
view-rolecomponent to improve usability when managing role permissions.Key Changes
Search Functionality
filterPermissions()andclearSearch()methods to handle search logic and reset the view.UI Improvements
Component Updates
view-role.component.tsto include search state management and filtering logic.MatIcon,MatIconButton) for the search UI.Styling Enhancements
These improvements make it easier to locate specific permissions in roles with a large number of permission entries.
Related Issues
Closes #WEB-859
After Video
Home.-.Google.Chrome.2026-03-15.03-18-31.mp4
Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit