[permissions V2] Upsert object and setting permissions #11101
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements a comprehensive permissions V2 system with object and setting-level permission management, introducing new DTOs, services, and database schema changes.
- Introduces
ObjectPermissionDTOandSettingPermissionDTOwith corresponding services for granular CRUD operations on objects and settings - Renames permission tables from plural to singular form (
objectPermissions->objectPermission,settingsPermissions->settingPermission) with proper cascade delete behavior - Adds new mutations in
RoleResolverfor upserting object and setting permissions, protected byPermissionsV2feature flag - Implements proper error handling with new exception codes
OBJECT_METADATA_NOT_FOUNDandINVALID_SETTING - Adds unique constraints on permission tables to prevent duplicate entries (
objectMetadataId+roleIdandsetting+roleId)
Note: There appears to be a mismatch between the PR description (mentioning avatar URLs) and the actual code changes (implementing permissions V2).
17 file(s) reviewed, 18 comment(s)
Edit PR Review Bot Settings | Greptile
...es/twenty-server/src/engine/metadata-modules/object-permission/dtos/object-permission.dto.ts
Show resolved
Hide resolved
packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.entity.ts
Outdated
Show resolved
Hide resolved
...-server/src/engine/metadata-modules/object-permission/dtos/upsert-object-permission-input.ts
Outdated
Show resolved
Hide resolved
...enty-server/src/database/typeorm/metadata/migrations/1742488572894-renamePermissionTables.ts
Show resolved
Hide resolved
...-server/src/engine/metadata-modules/object-permission/dtos/upsert-object-permission-input.ts
Outdated
Show resolved
Hide resolved
.../twenty-server/src/engine/metadata-modules/setting-permission/dtos/setting-permission.dto.ts
Show resolved
Hide resolved
...erver/src/engine/metadata-modules/setting-permission/dtos/upsert-setting-permission-input.ts
Show resolved
Hide resolved
...s/twenty-server/src/engine/metadata-modules/setting-permission/setting-permission.service.ts
Show resolved
Hide resolved
Comment on lines
+61
to
+74
| if (error.message.includes('violates foreign key constraint')) { | ||
| const role = await this.roleRepository.findOne({ | ||
| where: { | ||
| id: input.roleId, | ||
| }, | ||
| }); | ||
|
|
||
| if (!isDefined(role)) { | ||
| throw new PermissionsException( | ||
| PermissionsExceptionMessage.ROLE_NOT_FOUND, | ||
| PermissionsExceptionCode.ROLE_NOT_FOUND, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
style: Role validation should happen before the upsert attempt to avoid unnecessary database operations
There was a problem hiding this comment.
we are not doing so to avoid unnecessary calls
...s/twenty-server/src/engine/metadata-modules/setting-permission/setting-permission.service.ts
Outdated
Show resolved
Hide resolved
ijreilly
changed the title
Also fixes workflows Fixes #11073
…1071) # Introduction closes #11030, might not fix the issue as it seems to be related to the passed record to `identifierChipGeneratorPerObject` about to dig deeper in this generator code => found nothing revelant doubled check each `RecordChip` invocation that could provide undefined record Fixed wrong default generated `RecordChipData` signature ## Reproducibility I've only been able to reproduce the bug in production using the very same opportunity than within the issue, but not all the time https://crm.twenty-internal.com/objects/opportunities?viewId=b709d3d1-2dd2-455d-ba73-784f3ab00883 ```json // Removed timelineActivities to prevent linking ids { "data": { "opportunity": { "__typename": "Opportunity", "closeDate": null, "company": null, "companyId": null, "createdAt": "2024-06-17T09:45:22.357Z", "deletedAt": null, "id": "006a22dd-6bd6-4247-a24b-42fb164cd48c", "name": "test", "pointOfContact": null, "pointOfContactId": null, "position": 0, "probability": "0", "stage": "NEW_STAGE", "updatedAt": "2025-03-20T16:27:51.927Z", "amount": { "__typename": "Currency", "amountMicros": null, "currencyCode": "USD" }, "attachments": { "__typename": "AttachmentConnection", "edges": [] }, "createdBy": { "__typename": "Actor", "source": "MANUAL", "workspaceMemberId": null, "name": "", "context": {} }, "favorites": { "__typename": "FavoriteConnection", "edges": [] }, "taskTargets": { "__typename": "TaskTargetConnection", "edges": [] }, "noteTargets": { "__typename": "NoteTargetConnection", "edges": [ { "__typename": "NoteTargetEdge", "node": { "__typename": "NoteTarget", "appEventId": null, "companyId": null, "createdAt": "2025-01-22T17:11:07.801Z", "deletedAt": null, "feedbackId": null, "id": "2e8eca1c-e2c2-425a-93fc-ef2aeb65f410", "issueId": null, "listingId": null, "noteId": "ab586b51-6931-4a4a-9c24-0d16226211b2", "opportunityId": "006a22dd-6bd6-4247-a24b-42fb164cd48c", "personId": null, "somethingId": null, "testId": null, "updatedAt": "2025-01-22T17:11:07.801Z" } } ] }, } } } ```
… values (#11081) closes #11013 Fixes the issue where users couldn't delete all text in select field options. Removed the error throw for empty strings in the computeOptionValueFromLabel function, allowing proper text deletion. This error handling was redundant since the form validation already prevents submission with empty values.
Added 'rediss' in protocols array so that url doesn't fail isUrl constraint
Added rediss protocol. Co-authored-by: Félix Malfait <[email protected]>
Don't have access to push on #9942, so close it and open new PR here <img width="244" alt="Screenshot 2025-02-18 at 11 09 39" src="https://github.com/user-attachments/assets/4bc1b436-147a-4d17-88c8-2aff0fffd06a" /> <img width="246" alt="Screenshot 2025-02-18 at 11 09 51" src="https://github.com/user-attachments/assets/3d7b2972-ab7e-4e3b-a177-658325a3bb70" /> Ok for RecordInlineCell / RecordTableCell and EmailsFieldInput / LinksFieldInput. I think it's too complex for a so small issue (agree with you khuddite) closes #9778 on top of #9942 from @khuddite --------- Co-authored-by: khuddite <[email protected]> Co-authored-by: khuddite <[email protected]> Co-authored-by: Charles Bochet <[email protected]>
Put workflows on top of navigation commands in the command menu.
- Update skeleton loader padding and make it take only one line - Update rich text editor padding to align with the fields inside the side panel <img width="163" alt="Capture d’écran 2025-03-21 à 11 57 00" src="https://github.com/user-attachments/assets/5964404a-3a32-4d7e-b96f-3377949430bf" />
## Context Currency picker was not working properly, clicking a value was triggering the clickOutsideListener of the parent and was closing the select without saving. We are now toggling the click outside listener based on the state of the currency picker dropdown This also means the UX changed a bit, now choosing a value or clicking outside only closes the select (allowing you to choose the amount as well) and only enter OR clicking outside will save
This PR essentially focuses on a refactor of the component hierarchy and naming in advanced filter dropdown, to make it more readable and easy to maintain. This refactor was required because this area of the code is recursive, so it's better to see the same abstract components in the recursion, instead of trying to guess whether we have the same components than the level above or not. Also keep in mind that this refactor is meant to separate the advanced filter code path from the view bar simple filter code path, while reusing what's reusable, so here we have a first attempt at finding the sweet spot, that we'll be able to duplicate on other filter dropdown use cases. - We now use AdvancedFilterRecordFilterGroupRow and AdvancedFilterRecordFilterRow to make it clearer in the advanced filter dropdown recursion where we are. - Children component of AdvancedFilterRecordFilterRow have been abstracted at the same level to make reading easier - The field selection dropdown is now in a self-explanatory component that follows the same naming pattern as other dropdowns in the app : AdvancedFilterFieldSelectDrodownButton, together with AdvancedFilterFieldSelectDrodownContent. - The field selection search in the filter dropdown is now a standalone component : AdvancedFilterFieldSelectSearchInput - The UI container of a row has been abstracted in a new AdvancedFilterDropdownRow Miscellaneous : - Renamed a bunch of view filter old naming to record filter naming.
## After  <img width="825" alt="Capture d’écran 2025-03-21 à 15 43 14" src="https://github.com/user-attachments/assets/87818683-9cb0-4264-a6c3-f0420b0ae34d" />
Add filters by visibleRecordGroup to compute totalCount ## Before see #11067 ## After  - After hiding `New` and `Meeting` columns: <img width="1280" alt="image" src="https://github.com/user-attachments/assets/a2ae1728-ea11-4e2d-86e5-02778b3c42c0" /> - With filtering <img width="1275" alt="image" src="https://github.com/user-attachments/assets/263d8865-9cba-4b46-84a0-e9270b29109b" /> - Works also in groupBy view <img width="1280" alt="image" src="https://github.com/user-attachments/assets/c1ec171f-6eec-45db-aafc-2bd2d1de8841" />
## Context Reverting back the removal of "Log out" button. While it is now accessible from the workspace picker, suspended workspaces don't have access to that picker and are locked in the settings pages so I'm adding back the log out button in the settings menu <img width="225" alt="Screenshot 2025-03-21 at 15 52 40" src="https://github.com/user-attachments/assets/d5453868-d043-49e9-9207-2cfdd65838da" />
The old `useListenClickOutside` API allowed us to pass a hotkeyScope as a parameter, the click outside was triggered only if the current hotkey scope matched the parameter. We don't want this anymore. This fixes a few bugs related to hotkey scopes inside the side panel.
## Optimization: Efficient Health Check Query This PR optimizes the workspace health check system by replacing the N+1 query pattern with efficient database queries. ### Key Improvements - **Eliminated N+1 Query Problem**: Instead of fetching all workspaces and then querying each one individually for pending migrations (which caused slowness in production), we now use a single optimized query to directly identify workspaces with pending migrations - **Better Performance**: Reduced the number of database queries from potentially hundreds/thousands (previous implementation) to just 2 fixed queries regardless of workspace count - **Full Coverage Instead of Sampling**: Rather than implementing a cap on workspace checks at 100 (which was a workaround for performance issues), this solution addresses the root cause by optimizing the query pattern. We can now efficiently check all workspaces with pending migrations without performance penalties. @FelixMalfait This addresses the "always eager-load when you can" feedback by handling the problem at the database level rather than just applying a limit. The optimized query should solve both the performance issues and provide more accurate health status information.
Added a new job to check for changed files before executing the CI workflow. Integrated Tinybird local service, updated environment variables, and refined the CI steps for better functionality and clarity.
Done - Replace global search in multi record picker and single record picker To do - refactor SingleRecordPicker to match MultipleRecordPicker - next 1:1 - items in this issue twentyhq/core-team-issues#643 closes twentyhq/core-team-issues#535
Adding a way to switch to enterprise plan during onboarding <img width="409" alt="Screenshot 2025-03-21 at 17 03 19" src="https://github.com/user-attachments/assets/7a8c9ebd-3d77-4875-a141-c30fa5119eff" />
- create a form filler component - send the response on submit - put back a field name. We need it for the step output - validate a form is well set before activation TODO: - we need to refresh to see the form submitted. We need to discuss about a strategy - the response is not saved in the step settings. We need a new endpoint to update workflow run step https://github.com/user-attachments/assets/0f34a6cd-ed8c-4d9a-a1d4-51455cc83443
## What This PR aims to make sure all application exceptions are captured through react-error-boundaries Once merged we will have: - Root Level: AppErrorBoundary at the highest level (full screen) ==> this one needs to be working in any case, not relying on Theme, was not working - Route Level: AppErrorBoundary in DefaultLayout (full screen) ==> this was missing and it seems that error are not propagated outside of the router, making errors triggered in CommandMenu or NavigationDrawer missing - Page Level: AppErrorBoundary in DefaultLayout write around the Page itself (lower than CommandMenu + NavigationDrawer) - Manually triggered: example in ClientConfigProvider ## Screenshots App level (ex throw in IconsProvider) <img width="1512" alt="image" src="https://github.com/user-attachments/assets/18a14815-a203-4edf-b931-43068c3436ec" /> Route level (ex throw in CommandMenu) <img width="1512" alt="image" src="https://github.com/user-attachments/assets/ca066627-14c7-438e-a432-f0999a1f3b84" /> Page level (ex throw in RecordTable) <img width="1512" alt="image" src="https://github.com/user-attachments/assets/ffeaa935-02af-4762-8859-7a0ccf8b77e1" /> Manually Triggered (clientConfig, ex when backend is not up) <img width="1512" alt="image" src="https://github.com/user-attachments/assets/062d6d84-097a-4ed9-b6ce-763b8c27c659" />
Fixes many bug regarding TableCell and InlineCells
…bles (#11104) When using postgres cloud databse connection string with protocol 'postgresql', it throws error "ERROR PG_DATABASE_URL must be a URL address". 
…construct` (#11083) # Introduction In this PR we've migrated `twenty-shared` from a `vite` app [libary-mode](https://vite.dev/guide/build#library-mode) to a [preconstruct](https://preconstruct.tools/) "atomic" application ( in the future would like to introduce preconstruct to handle of all our atomic dependencies such as `twenty-emails` `twenty-ui` etc it will be integrated at the monorepo's root directly, would be to invasive in the first, starting incremental via `twenty-shared`) For more information regarding the motivations please refer to nor: - twentyhq/core-team-issues#587 - twentyhq/core-team-issues#281 (comment) close twentyhq/core-team-issues#589 close twentyhq/core-team-issues#590 ## How to test In order to ease the review this PR will ship all the codegen at the very end, the actual meaning full diff is `+2,411 −114` In order to migrate existing dependent packages to `twenty-shared` multi barrel new arch you need to run in local: ```sh yarn tsx packages/twenty-shared/scripts/migrateFromSingleToMultiBarrelImport.ts && \ npx nx run-many -t lint --fix -p twenty-front twenty-ui twenty-server twenty-emails twenty-shared twenty-zapier ``` Note that `migrateFromSingleToMultiBarrelImport` is idempotent, it's atm included in the PR but should not be merged. ( such as codegen will be added before merging this script will be removed ) ## Misc - related opened issue preconstruct preconstruct/preconstruct#617 ## Closed related PR - #11028 - #10993 - #10960 ## Upcoming enhancement: ( in others dedicated PRs ) - 1/ refactor generate barrel to export atomic module instead of `*` - 2/ generate barrel own package with several files and tests - 3/ Migration twenty-ui the same way - 4/ Use `preconstruct` at monorepo global level ## Conclusion As always any suggestions are welcomed !
## Description - this PR fixes issue #11092 - Added ScrollWrapper and scroll in Settings sidebar while maintaining the exit Settings fixed. ## Added Behaviour https://github.com/user-attachments/assets/c8db0f6d-986e-46f3-85d6-bb3028c56e5f --------- Co-authored-by: ehconitin <[email protected]> Co-authored-by: Félix Malfait <[email protected]>
In this PR, I'm: - fixing the root cause (we should not try to render a RecordChip if the record is not defined in RelationFromMany Display) - fixing related typing issues - we won't be able to catch the issue from TS perspective as ObjectRecord is a Record of string, any
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes twentyhq/core-team-issues#639