Aman-Mittal commented on code in PR #823:
URL:
https://github.com/apache/fineract-backoffice-ui/pull/823#discussion_r4385329365
##########
src/app/features/settings/global-configurations.component.ts:
##########
@@ -62,6 +64,8 @@ import { TooltipDirective } from
'../../shared/directives/tooltip.directive';
[checked]="!!config['enabled']"
(ionChange)="onToggleConfig(config)"
[attr.aria-label]="'Toggle ' + config['name']"
+ [attr.data-testid]="'config-toggle-' + config['id']"
+ appRequiresPermission="UPDATE_CONFIGURATION"
Review Comment:
Nit: Global Configurations has no unit test file, so this gate (and the one
at L81) is covered only by the Playwright spec. A small
`global-configurations.component.test.ts` would pin it down cheaply:
- without `UPDATE_CONFIGURATION`, the toggle and edit are disabled and an
`ionChange` doesn't reach `putConfigurationsConfigId`;
- with it, they're enabled.
Non-blocking.
##########
src/app/features/security/roles/roles-list.component.ts:
##########
@@ -73,8 +73,10 @@ import { ButtonComponent } from
'../../../ui/button/button.component';
emphasis="quiet"
[label]="'COMMON.EDIT' | appTranslate"
icon="create-outline"
+ [attr.data-testid]="'role-edit-' + role.id"
[appTooltip]="'COMMON.EDIT' | appTranslate"
(click)="onEditRole(role)"
+ *appHasPermission="'UPDATE_ROLE'"
Review Comment:
Two notes, both non-blocking:
- **Hidden vs disabled.** Users and roles *hide* Edit (`*appHasPermission`),
while configurations *disable* the toggle with a hint
(`appRequiresPermission`). Each matches its neighbours on the same screen, and
hiding a toggle would also hide the setting's value, so the split makes sense.
A sentence in the PR description saying so would help the next person.
- **Existing gap, follow-up.** The role editor saves the description
(`UPDATE_ROLE`) and then calls `PUT /roles/{id}/permissions`. Fineract maps
that to `updateRolePermissions` (action `PERMISSIONS`, entity `ROLE`), which
needs **`PERMISSIONS_ROLE`**. A user with `UPDATE_ROLE` but not
`PERMISSIONS_ROLE` sees Edit, saves, and gets a partial save followed by a 403.
Gating on `UPDATE_ROLE` matches the current route guard, so this isn't for this
PR, but it's worth an issue (gate the permissions section, or the save, on
`PERMISSIONS_ROLE`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
