Skip to content

fix(admin-ui): unable to map permission to the role using GUI #2400#2403

Merged
moabu merged 8 commits intomainfrom
admin-ui-issue-2400
Oct 30, 2025
Merged

fix(admin-ui): unable to map permission to the role using GUI #2400#2403
moabu merged 8 commits intomainfrom
admin-ui-issue-2400

Conversation

@faisalsiddique4400
Copy link
Contributor

@faisalsiddique4400 faisalsiddique4400 commented Oct 30, 2025

fix(admin-ui): unable to map permission to the role using GUI #2400

Description

The Admin UI does not allow users to map a permission to a role from the Security → Mapping screen.
When selecting a role such as api-viewer and attempting to search and add a permission, the operation does not work.

Fix implemented

  • Corrected the mapping logic to ensure selected permissions are properly appended to the role.
  • Fixed UI event handling so the add operation updates the backend payload and UI list.
  • Verified that both search and add workflows now function without requiring a page reload.

Steps Verified

  1. Navigate to Home → Security → Mapping
  2. Select api-viewer (or any role)
  3. Search and add a new permission
  4. Mapping works as expected and permission appears in the list

🔗 Ticket

Closes: #2400

Summary by CodeRabbit

  • Refactor

    • Improved type safety and memoization for a typeahead input and centralized change handling.
    • Streamlined form state and initialization for permission management.
  • Behavioral Changes

    • Permission additions now merge with existing role permissions to avoid overwriting.
    • Permission picker disables adding new custom entries and binds to selected permissions.
  • UX

    • Dialog buttons updated: Accept shows a plus icon with "Add"; Cancel labeled "Cancel".

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

📝 Walkthrough

Walkthrough

Refactors GluuTypeAhead into a typed, memoized, Formik-aware component; updates MappingItem and MappingAddDialogForm to initialize and manage permission arrays correctly; changes MappingPage add flow to merge existing and new permissions into a payload before dispatch.

Changes

Cohort / File(s) Summary
Typed Typeahead
admin-ui/app/routes/Apps/Gluu/GluuTypeAhead.tsx
Add GluuTypeAheadProps interface; convert to memoized component; derive selectedValue with useMemo; centralize handleChange with Formik support; resolve labelKey; add default prop values; import FormikContextType.
Mapping form state & usage
admin-ui/plugins/admin/components/Mapping/MappingItem.js
Initialize Formik with mappingAddPermissions: [] (array) instead of object; remove hardcoded value={[]} from GluuTypeAhead so it binds to Formik state.
Permission selection & dialog behavior
admin-ui/plugins/admin/components/Mapping/MappingAddDialogForm.js
Reset selection on modal close; make getPermissionsForSearch accept optional role and filter out already-assigned permissions; add effects to recalc permissions when apiRole or mapping change; disable adding new entries in TypeAhead; adjust button labels/icons.
Add mapping payload merge
admin-ui/plugins/admin/components/Mapping/MappingPage.js
onAddConfirmed(mappingData) now merges mappingData.permissions with existing permissions for the same role, builds payload { role, permissions: mergedPermissions }, calls buildPayload with that payload and dispatches addNewRolePermissions({ data: payload }).

Sequence Diagram(s)

sequenceDiagram
    participant UI as User UI
    participant Dialog as MappingAddDialogForm
    participant Formik as Formik State
    participant TypeAhead as GluuTypeAhead
    participant Page as MappingPage
    participant Store as Redux (actions)

    rect #f0f8ff
    Note over UI,Dialog: Open "Add Permission" dialog
    UI->>Dialog: open(apiRole)
    Dialog->>Formik: init form (mappingAddPermissions: [])
    Dialog->>Dialog: getPermissionsForSearch(apiRole) -> filter existing
    end

    rect #f6fff0
    Note over Formik,TypeAhead: Select permissions via TypeAhead
    TypeAhead->>Formik: read value (formik.values.mappingAddPermissions)
    UI->>TypeAhead: select permissions
    TypeAhead->>Formik: handleChange -> setFieldValue('mappingAddPermissions', newValue)
    end

    rect #fff8f0
    Note over Page,Store: Confirm add -> merge and dispatch
    Dialog->>Page: onAddConfirmed(mappingData)
    Page->>Page: compute existing permissions for role -> mergedPermissions
    Page->>Store: dispatch addNewRolePermissions({ data: { role, permissions: mergedPermissions } })
    Store->>Page: ack
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay extra attention to:
    • GluuTypeAheadProps typing, default prop values, and handleChange Formik interactions.
    • useMemo usage for selectedValue to ensure selection updates correctly.
    • getPermissionsForSearch filtering logic and its effects/dependencies in MappingAddDialogForm.js.
    • onAddConfirmed merge logic in MappingPage.js to avoid duplicating or dropping permissions.

Suggested reviewers

  • syntrydy
  • moabu

Poem

🐰 In fields of code I hop and peek,
I typed the typeahead sleek and meek,
Arrays no longer stuck and gray,
Permissions join the proper way,
Hoppity—now mapping saves the day! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix(admin-ui): unable to map permission to the role using GUI #2400" directly and clearly describes the primary objective of this changeset. The title uses the conventional commit format (fix), concisely states the problem that was fixed (inability to map permissions to roles via the GUI), and references the corresponding issue number. The title is specific enough that a teammate scanning history would immediately understand that this PR addresses and resolves the permission mapping bug in the Admin UI.
Linked Issues Check ✅ Passed The code changes comprehensively address all coding requirements from linked issue #2400. The GluuTypeAhead component refactoring improves typing and Formik integration to support proper state management for permission selection. MappingItem.js and MappingAddDialogForm.js changes fix form state initialization and permission filtering logic, enabling users to search and select permissions. MappingPage.js now correctly merges new permissions with existing ones and constructs the proper backend payload. Together, these changes enable users to successfully search for and add permissions to roles through the GUI, which directly fulfills the issue's expected behavior requirement.
Out of Scope Changes Check ✅ Passed All changes in this pull request are directly related to resolving the permission mapping bug. The GluuTypeAhead component improvements, MappingItem form state adjustments, MappingAddDialogForm permission filtering logic, and MappingPage backend payload construction are all necessary components of fixing the reported issue. There are no modifications to unrelated features, utilities, or infrastructure; every change contributes specifically to enabling the permission-to-role mapping workflow through the GUI.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch admin-ui-issue-2400

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eae25f9 and 7454513.

📒 Files selected for processing (2)
  • admin-ui/plugins/admin/components/Mapping/MappingAddDialogForm.js (4 hunks)
  • admin-ui/plugins/admin/components/Mapping/MappingPage.js (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/plugins/admin/components/Mapping/MappingPage.js (1)
admin-ui/app/utils/PermChecker.ts (1)
  • buildPayload (153-160)
admin-ui/plugins/admin/components/Mapping/MappingAddDialogForm.js (1)
admin-ui/plugins/admin/components/Mapping/MappingPage.js (3)
  • modal (30-30)
  • mapping (25-25)
  • selectedTheme (36-36)
🔇 Additional comments (8)
admin-ui/plugins/admin/components/Mapping/MappingAddDialogForm.js (7)

25-31: LGTM! Good state cleanup practice.

The reset effect properly clears the form state when the modal closes, preventing stale data from persisting when the dialog is reopened.


33-48: LGTM! Efficient permission filtering logic.

The refactored function correctly filters out already-assigned permissions for a given role using a Set for efficient lookups. This is a key part of the fix enabling proper permission addition.


49-51: LGTM! Proper permission refresh logic.

The two effects work together correctly:

  • The first ensures permissions are populated when loaded from the backend
  • The second recalculates filtered permissions when a role is selected or when mappings change

This ensures the searchable permissions list stays in sync with the current state.

Also applies to: 61-63


65-73: LGTM! Clean role extraction logic.

The effect correctly extracts role names from the roles array with proper null-safety using optional chaining.


93-101: Verify the role selection display behavior.

The value prop is hardcoded to an empty array, which means the selected role won't be displayed visually in the component after selection. While the onChange handler correctly updates apiRole state, users won't see their selection reflected in the UI.

If the intent is to show the selected role, consider:

-            value={[]}
+            value={apiRole ? [apiRole] : []}

If this is intentional (e.g., a search-only field that doesn't display selections), please confirm the expected UX behavior.


102-114: LGTM! Proper TypeAhead configuration.

Setting allowNew={false} correctly prevents users from creating invalid permissions via the UI, and binding value={selectedPermissions} ensures the component reflects the current selection state.


116-134: LGTM! Improved button UX.

The button label changes improve clarity: using "Add" with a plus icon for the accept action and "Cancel" for dismissal is more intuitive than the previous labels.

admin-ui/plugins/admin/components/Mapping/MappingPage.js (1)

63-71: LGTM! This is the critical fix for the permission mapping bug.

The updated logic correctly merges existing permissions with newly added ones using a Set for deduplication, rather than replacing them. This ensures that:

  • Adding permissions to an existing role preserves previously assigned permissions
  • Duplicate permissions are automatically filtered out
  • New roles (where existing is undefined) work correctly with the || [] fallback

This addresses the core issue described in #2400 where adding permissions via the GUI failed.


Comment @coderabbitai help to get the list of available commands and usage tips.

@faisalsiddique4400
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@duttarnab duttarnab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

-- The feature of adding new role-scope mapping is not working.

Base automatically changed from admin-ui-issue-2401 to main October 30, 2025 15:36
@mo-auto mo-auto added comp-admin-ui Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality labels Oct 30, 2025
@sonarqubecloud
Copy link

@moabu moabu merged commit 7630f1a into main Oct 30, 2025
7 checks passed
@moabu moabu deleted the admin-ui-issue-2400 branch October 30, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-admin-ui Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(admin-ui): unable to map permission to the role using GUI

4 participants