Skip to content

Feat 17408 : Add remove option for object permissions rule#17601

Open
LTan-101104 wants to merge 13 commits intotwentyhq:mainfrom
LTan-101104:feat-17408
Open

Feat 17408 : Add remove option for object permissions rule#17601
LTan-101104 wants to merge 13 commits intotwentyhq:mainfrom
LTan-101104:feat-17408

Conversation

@LTan-101104
Copy link

This PR aims to fix: #17408

Main modification includes adding a new column for dropdown in the object permission rule table (containing options for editing and removal). Removal logic is implemented using existing pattern with hook useResetObjectPermission (relies on existing hooks useUpsertFieldPermissionInDraftRole and useUpsertObjectPermissionInDraftRole).

Feel free to suggest any necessary changes. Functionality (unrestricted access is allowed when permission removal is applied) is already tested.

Demo video: https://drive.google.com/file/d/1M4RYHw-JEhDdJksKkL3MY_VyXAV3aS9I/view?usp=sharing

Copilot AI review requested due to automatic review settings January 31, 2026 15:49
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2026

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against 9bbc940

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 6 files

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 31, 2026

Greptile Overview

Greptile Summary

This PR adds a removal option for object permission rules in the settings UI, enabling users to reset permissions back to unrestricted access.

Key Changes:

  • Added a new options dropdown component (SettingsRolePermissionsObjectLevelTableRowOptionsDropdown) with "Remove rule" and "Edit" menu items
  • Implemented useResetObjectPermission hook that sets all object and field permissions to null (unrestricted access)
  • Updated table grid layout to accommodate the new options column
  • The remove functionality correctly resets both object-level permissions (canReadObjectRecords, canUpdateObjectRecords, canSoftDeleteObjectRecords, canDestroyObjectRecords) and all associated field-level permissions

Issues Found:

  • Debug/TODO comments were left in SettingsRolePermissionsObjectLevelObjectFieldPermissionTableRow.tsx that should be removed before merging
  • Minor code style issues with comment formatting
  • The dropdown follows the existing pattern from AdvancedFilterRecordFilterOptionsDropdown, maintaining consistency with the codebase

Confidence Score: 4/5

  • Safe to merge after removing debug comments - no functional issues found
  • The implementation follows existing patterns correctly and the logic is sound. Score is 4/5 only because of the debug/TODO comments that need cleanup before merging
  • Clean up comments in SettingsRolePermissionsObjectLevelObjectFieldPermissionTableRow.tsx before merging

Important Files Changed

Filename Overview
packages/twenty-front/src/modules/settings/roles/role-permissions/object-level-permissions/components/SettingsRolePermissionsObjectLevelTableRowOptionsDropdown.tsx New dropdown component for remove/edit actions - has style issues with unnecessary comments
packages/twenty-front/src/modules/settings/roles/role-permissions/object-level-permissions/field-permissions/components/SettingsRolePermissionsObjectLevelObjectFieldPermissionTableRow.tsx Added debug/TODO comments that should be removed - no functional changes
packages/twenty-front/src/modules/settings/roles/role-permissions/object-level-permissions/hooks/useResetObjectPermission.ts New hook implementing permission reset logic - follows existing patterns correctly

Sequence Diagram

sequenceDiagram
    participant User
    participant Dropdown as SettingsRolePermissionsObjectLevelTableRowOptionsDropdown
    participant Hook as useResetObjectPermission
    participant UpsertObjHook as useUpsertObjectPermissionInDraftRole
    participant UpsertFieldHook as useUpsertFieldPermissionInDraftRole
    participant RecoilState as settingsDraftRoleFamilyState

    User->>Dropdown: Click "..." options button
    Dropdown->>User: Show dropdown menu
    User->>Dropdown: Click "Remove rule"
    Dropdown->>Hook: resetObjectPermission(objectMetadataId)
    
    Hook->>RecoilState: Read current draft role state
    RecoilState-->>Hook: Return fieldPermissions & objectPermissions
    
    Hook->>Hook: Filter field permissions for object
    Hook->>Hook: Find existing object permission
    Hook->>Hook: Create reset permissions (all null)
    
    alt No existing object permission
        Hook->>UpsertObjHook: upsertObjectPermissionInDraftRole(newObjectPermission)
        UpsertObjHook->>RecoilState: Update with new object permission
    else Existing object permission
        Hook->>UpsertObjHook: upsertObjectPermissionInDraftRole(updatedObjectPermission)
        UpsertObjHook->>RecoilState: Update existing object permission
    end
    
    loop For each field permission
        Hook->>UpsertFieldHook: upsertFieldPermissionInDraftRole(resetFieldPermission)
        UpsertFieldHook->>RecoilState: Update field permission to null
    end
    
    RecoilState-->>User: UI updates to show unrestricted access
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds the ability to remove object permission rules from roles, addressing a gap where users could add permission rules but not delete them. The implementation follows existing patterns in the codebase by creating a dropdown menu with "Remove rule" and "Edit" options, similar to the AdvancedFilterRecordFilterOptionsDropdown.

Changes:

  • Adds useResetObjectPermission hook to reset object and field permissions to null values
  • Implements SettingsRolePermissionsObjectLevelTableRowOptionsDropdown component with remove and edit functionality
  • Adjusts table grid layout to accommodate the new options dropdown column

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
useResetObjectPermission.ts New hook that resets object and field permissions for a given object by setting all permission flags to null
SettingsRolePermissionsObjectLevelTableRowOptionsDropdown.tsx New dropdown component providing remove and edit options for object permission rows
SettingsRolePermissionsObjectLevelTableRow.tsx Integrates the new options dropdown into the permission table row
SettingsRolePermissionsObjectLevelTableHeader.tsx Adds empty header cell for the new options column
ObjectLevelPermissionTableGridAutoColumns.ts Adjusts grid column widths to accommodate the new options column
SettingsRolePermissionsObjectLevelObjectFieldPermissionTableRow.tsx Adds documentation comments explaining permission handling behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

'canDestroyObjectRecords',
];

export const useResetObjectPermission = (roleId: string) => {
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

The hook parameter structure is inconsistent with the similar hook useUpsertObjectPermission. While useUpsertObjectPermission uses destructured object parameters ({ roleId }: { roleId: string }), this hook uses a direct parameter (roleId: string). For consistency with the codebase, consider updating to use the same parameter pattern as useUpsertObjectPermission.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2026

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:31732

This environment will automatically shut down when the PR is closed or after 5 hours.

@LTan-101104
Copy link
Author

@Bonapara Ready for review.

@etiennejouan etiennejouan self-assigned this Feb 5, 2026
Copy link
Contributor

@etiennejouan etiennejouan left a comment

Choose a reason for hiding this comment

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

Hi @LTan-101104 it works well ! Thanks and Congrats for your first contribution on Twenty repo 👏

I left small comments regarding design. Do not hesitate to reach me if not clear.

accent="danger"
/>
</DropdownMenuItemsContainer>
<DropdownMenuItemsContainer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick : Could you invert "Remove rule" and "Edit" to fit with design ? #17408 (comment)

objectPermissionDetailUrl={navigationUrl}
/>
</TableCell>
<TableCell align="right">
Copy link
Contributor

Choose a reason for hiding this comment

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

To remove cf design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rule Deletion not possible in Role Menue

2 participants