Allow to edit labels of standard objects #10922

AFCMS · 2025-03-15T18:42:39Z

This PR is a work in progress.

Still left to fix:

When disabling synchronization of labels / api names, the edited labels should be set to the English version. Currently the client just send the localized versions together with the isLabelSyncedWithName change. Could be an easy fix.
Sometimes flipping the switch don't trigger the update function, may be a regression as it seems to affect the custom objects too.
There is a frontend problem where the labels inputs don't reflect the changes made. When enabling back synchronisation after editing labels, they are correctly back to their base values (backend, navigation breadcrumb, etc) but the label inputs still have the old values (switching pages will put them back to normal). I suspect this could be linked to the above problem.
API names are still displayed for standard objects per (kept them for debugging, trivial fix)
SettingsDataModelObjectAboutForm have a disableEdition parameter which is now used only for a few fields, not sure if it's worth keeping because it's a bit misleading since it doesn't "disable" much?
I don't know what these do, but I have seen "Remote" object types. Not sure if they work with my patch or not (I don't know how to test them)
Make it work with metadata synchronisation

What should work:

Disabling synchronization of standard objects should work, label inputs should no longer be disabled
Modifying labels should work
Enabling back synchronization should reset back the labels to the base value and disable the label inputs again (minus the mentioned display bug)
The synchronisation switch should still work as expected for custom objects
Creating custom objects should still work (it uses the same form)

FelixMalfait

Good start!!! Now what happens if I run yarn command:prod workspace:sync-metadata, will it get erased? What should we do next?

I don't know what these do, but I have seen "Remote" object types. Not sure if they work with my patch or not (I don't know how to test them)

Remote objects are foreign tables that live on a different server. In that case we shouldn't be able to rename the API. I don't have a strong opinion on whether label name should be editable or not in that case, do what makes the code simpler (as long as you cannot edit api name)

I haven't tested the code yet, will test more soon after you make updates. Thank you!

FelixMalfait · 2025-03-16T16:53:41Z

packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts

+    if (
+      objectMetadata.isCustom ||
+      (objectMetadata.isLabelSyncedWithName === false &&
+        ['labelPlural', 'labelSingular'].includes(labelKey))


We should probably let people edit the description all of the time too. Sorry this should have been in the initial spec. There is a product flaw here in the way we define what gets overwritten by new standard metadata changes, I think it's fine to put it under the same toggle but this is something we might want to reconsider in the future

Would also allowing to change the icon make sense?

Yes great point! Icon is similar to description

Editing icon and description is now possible, but the sync switch should at the very least have a different label.

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

FelixMalfait · 2025-03-16T17:06:48Z

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

+              );
+            }
+          }
+        } else {


We most often try avoid using elseor elseif and favor early returns which makes the code more readable (if x return y; if z return d;). We also avoid nesting if structures within another

...rc/modules/settings/data-model/objects/forms/components/SettingsDataModelObjectAboutForm.tsx

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

FelixMalfait · 2025-03-16T17:17:13Z

...rc/modules/settings/data-model/objects/forms/components/SettingsDataModelObjectAboutForm.tsx

-                        if (value === true) {
+                        if (
+                          value === true &&
+                          (!objectMetadataItem || objectMetadataItem?.isCustom)


If I set the value to true for a standard object, then the Label should be updated back to its value as you rightly pointed out on Discord. Is that done purely server-side and then it's updated with the response? I'm reviewing the code but haven't tested it yet

It should be purely server-side, since translations of labels are server-side. Label inputs should updated with the response (currently the update are propagated to the rest of the app, but not the label inputs, as I mentioned in the PR description)

...les/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx

AFCMS · 2025-03-17T20:33:25Z

About the form problem mentioned in the PR description:

I have determined that when you flip the switch two times in a row, the second time when the handleSave function is called, it doesn't find any "dirty" fields and logically don't save anything. No idea why there are no dirty fields yet.

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

charlesBochet · 2025-03-19T10:50:02Z

...nty-server/src/engine/metadata-modules/utils/get-standard-object-metadata-from-definition.ts

+
+const standardObjectMap = new Map<string, WorkspaceEntityMetadataArgs>();
+
+standardObjectMetadataDefinitions.forEach((entity) => {


this does not feel right, this will perform a side effect when importing the utils

I get why you wouldn't want side effects but I think this is efficient from a performance perspective. How would you do this?

See #10922 (comment) ; it's my comment that Louis into this direction. I didn't like the previous solution

Digging a bit more:

I'm also not sure what is specific to standard vs custom. It feels that they should be handled the same way

We would usually load this kind of information from the database metadata (or actually the redis metadata) as this will differ from what we have in the "definitions" (aka "workspace-entities") as long as the sync metadata has not been run

If we really want to parse the workspace entity (which I'm not convinced we shoud!), I would have a dedicated singleton service handled by nest container to do that. This way it's only instanciated once

If for some reason we don't want to use a nest js service, and want to do raw javascript, I would introduce another util with a very explicit naming: initStandardObjectMetadataFromDefMap instead of a getStandardObjectMetadataFromDefinition which I don't expect to create a new Map ==> this is likely to end up in a memory leak IMO!

I would prefer to go with 1) and 2), then if we can't 3) and then last solution 4)

FelixMalfait · 2025-03-20T11:37:18Z

Also lake sure ton configure your linter in VSCode so you don't break the CI :)

charlesBochet · 2025-03-20T11:42:37Z

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

+            objectMetadata.standardId
+          ) {
+            const standardObjectMetadata =
+              getStandardObjectMetadataFromDefinition(


TBH, I would load the metadata from the cache here and filter on standards (actually we only need to filter with standardId)

Signed-off-by: AFCMS <[email protected]>

Do not send the label changes when switching `isLabelSyncedWithName` to false

…nition`

FelixMalfait · 2025-03-24T10:30:46Z

@greptileai review

greptile-apps

PR Summary

This PR adds support for editing labels of standard objects while preserving API names through a new standardOverrides mechanism in both frontend and backend.

Added standardOverrides JSONB column to fieldMetadata and objectMetadata tables via migration 1742736630054 to store overridden labels/descriptions
Modified SettingsDataModelObjectAboutForm and SettingsDataModelFieldIconLabelForm to allow label editing when isLabelSyncedWithName is false
Added validation in before-update-one-object.hook.ts to prevent API name changes for standard objects while allowing label updates
Renamed resolveTranslatableString to resolveOverridableString in services to handle overridden values
Needs testing with workspace:sync-metadata command to ensure overrides persist during synchronization

_{💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!}

_{19 file(s) reviewed, 15 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-03-24T10:31:25Z

...les/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx

+      if (
+        formValues.isLabelSyncedWithName === false &&
+        isLabelSyncedWithName === true
+      ) {


logic: Missing check for when isLabelSyncedWithName changes from false to true - could leave labels in an inconsistent state

greptile-apps · 2025-03-24T10:31:26Z

.../modules/settings/data-model/fields/forms/components/SettingsDataModelFieldIconLabelForm.tsx

                          disabled={
-                            disabled || (isLabelSyncedWithName ?? false)
+                            disabled ||
+                            (isLabelSyncedWithName ?? false) ||
+                            !fieldMetadataItem?.isCustom
                          }


style: API name field's disabled state should be simplified to avoid potential bugs. Consider extracting this logic into a separate function for better maintainability.

greptile-apps · 2025-03-24T10:31:43Z

...rc/modules/settings/data-model/objects/forms/components/SettingsDataModelObjectAboutForm.tsx

+                        if (
+                          value === true &&
+                          (!objectMetadataItem || objectMetadataItem?.isCustom)
+                        ) {


logic: Redundant condition check - !objectMetadataItem || objectMetadataItem?.isCustom. The optional chaining is unnecessary since we already check if objectMetadataItem is undefined

Suggested change

if (

value === true &&

(!objectMetadataItem || objectMetadataItem?.isCustom)

) {

if (

value === true &&

(!objectMetadataItem || objectMetadataItem.isCustom)

) {

packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.entity.ts

packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts

packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.entity.ts

greptile-apps · 2025-03-24T10:33:55Z

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

+    const isUpdatingLabelsWhenSynced =
+      (instance.update.labelSingular || instance.update.labelPlural) &&
+      objectMetadata.isLabelSyncedWithName &&
+      instance.update.isLabelSyncedWithName !== false &&
+      (instance.update.labelSingular !== objectMetadata.labelSingular ||
+        instance.update.labelPlural !== objectMetadata.labelPlural);


logic: This condition may miss edge cases where labelSingular/Plural are set to empty strings or other falsy values. Consider using isDefined() here for consistency.

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts

ijreilly

Will resume review when you ping me @FelixMalfait !

.../modules/settings/data-model/fields/forms/components/SettingsDataModelFieldIconLabelForm.tsx

ijreilly · 2025-03-24T10:17:20Z

...les/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx

+      if (
+        formValues.isLabelSyncedWithName === false &&
+        isLabelSyncedWithName === true
+      ) {


what if in the same form we update isLabelSyncedWithName value from true to false + the label value ? from this code I feel we would only update isLabelSyncedWithName then return?

ijreilly · 2025-03-24T10:31:31Z

...rc/modules/settings/data-model/objects/forms/components/SettingsDataModelObjectAboutForm.tsx

                          fillNamePluralFromLabelPlural(labelPlural);
                          fillNameSingularFromLabelSingular(labelSingular);
                        }
                        onNewDirtyField?.();
+
+                        // Server-side side effect when isLabelSyncedWithName is changed
+                        if (


No call is sent when we are only updating isLabelSyncedWithName value
(recording does not show Network tab but I checked and no call was sent)

Enregistrement.de.l.ecran.2025-03-24.a.11.28.15.mov

@FelixMalfait did you create a ticket for this one in the end ? or was it fixed ?

packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.resolver.ts

packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts

ijreilly · 2025-03-24T10:37:02Z

...enty-server/src/engine/metadata-modules/field-metadata/hooks/before-update-one-field.hook.ts

+    fieldMetadata: FieldMetadataEntity,
+  ): UpdateOneInputType<T> {
+    const update: StandardFieldUpdate = {};
+    const allowedFields = ['isActive', 'isLabelSyncedWithName'];


updatableFields? fieldsAllowedForUpdate? (trying to find a more precise naming)

ijreilly · 2025-03-24T10:44:31Z

...enty-server/src/engine/metadata-modules/field-metadata/hooks/before-update-one-field.hook.ts

+    update: StandardFieldUpdate,
+  ): void {
+    if (!isDefined(instance.update.description)) {
+      return;


can we not update to an empty description ? Maybe we jsut want to evaluate
if(!('description' in instance.update)) ?

greptile-apps

PR Summary

(updates since last review)

Based on the recent changes and previous reviews, I'll focus on new developments and unaddressed points:

This PR continues work on standard object label editing, with significant progress on the standardOverrides implementation and validation logic.

Added proper validation in before-update-one-field.hook.ts to handle label synchronization and overrides for standard fields
Introduced type-safe FieldStandardOverridesDTO and ObjectStandardOverridesDTO for managing override data
Potential SQL injection vulnerability in field-metadata.service.ts createViewAndViewFields needs attention
The any type casting for standardOverrides in multiple services should be replaced with proper types
Transaction handling in deleteOneField needs review for multiple queryRunner safety

_{21 file(s) reviewed, 7 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-front/src/modules/object-metadata/graphql/mutations.ts

greptile-apps · 2025-03-24T14:17:18Z

.../modules/settings/data-model/fields/forms/components/SettingsDataModelFieldIconLabelForm.tsx

@@ -143,7 +141,7 @@ export const SettingsDataModelFieldIconLabelForm = ({
                }
              }}
              error={getErrorMessageFromError(errors.label?.message)}
-              disabled={disabled}
+              disabled={isLabelSyncedWithName === true}


logic: Label input's disabled state is incorrect - should be isLabelSyncedWithName === true && !fieldMetadataItem?.isCustom to allow editing standard object labels when sync is disabled

greptile-apps · 2025-03-24T14:17:22Z

...les/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx

-        idToUpdate: objectMetadataItem.id,
-        updatePayload: formValues,
-      });
+      await updateObjectMetadata(formValues);

      formConfig.reset(undefined, { keepValues: true });


logic: Calling reset with keepValues: true after a successful update may prevent the form from reflecting server-side changes

greptile-apps · 2025-03-24T14:17:23Z

...les/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx

+    if (!objectMetadataItem.isCustom) {
+      const {
+        nameSingular: _,
+        namePlural: __,
+        ...payloadWithoutNames
+      } = updatePayload;
+
+      return updateOneObjectMetadataItem({
+        idToUpdate: objectMetadataItem.id,
+        updatePayload: payloadWithoutNames,
+      });
+    }


style: Destructuring with _ and __ is not a clear naming convention. Consider using more descriptive names like 'unusedSingular' and 'unusedPlural'

greptile-apps · 2025-03-24T14:17:33Z

...rc/modules/settings/data-model/objects/forms/components/SettingsDataModelObjectAboutForm.tsx

+                            {
+                              keepDirty: true,
+                            },


style: Keeping dirty state during reset could lead to unexpected form behavior and validation issues

packages/twenty-server/src/engine/metadata-modules/field-metadata/dtos/field-metadata.dto.ts

packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.module.ts

…request

ijreilly

huge work done here! :)
the update logic is not the simplest between isLabelSyncedWithName, overrides, etc. I think it would be better protected if covered by integration tests.
We already have integration tests for custom object renaming (rename-custom-object.integration-spec.ts), I think we should add more !

ijreilly · 2025-03-25T10:22:55Z

...les/settings/data-model/object-details/components/SettingsUpdateDataModelObjectAboutForm.tsx

+
+  const handleError = (error: unknown) => {
+    // eslint-disable-next-line no-console
+    console.error(error);


was this intended or part of local debug?

I wasn't sure but it was already there so I left it. Might not be needed indeed...

ijreilly · 2025-03-25T10:36:08Z

...rc/modules/settings/data-model/objects/forms/components/SettingsDataModelObjectAboutForm.tsx

                          fillNamePluralFromLabelPlural(labelPlural);
                          fillNameSingularFromLabelSingular(labelSingular);
                        }
                        onNewDirtyField?.();
+
+                        // Server-side side effect when isLabelSyncedWithName is changed
+                        if (


@FelixMalfait did you create a ticket for this one in the end ? or was it fixed ?

ijreilly · 2025-03-25T10:43:38Z

packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.resolver.ts

+    return this.objectMetadataService.resolveOverridableString(
+      objectMetadata,
+      'icon',
+      context.req.headers['x-locale'],


does icon also vary depending on locale ? I don't see why it should

Yes indeed the icon should remain the same for all locales (cf next PR)

FelixMalfait reviewed Mar 16, 2025

View reviewed changes

charlesBochet reviewed Mar 19, 2025

View reviewed changes

...ty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts Outdated Show resolved Hide resolved

charlesBochet reviewed Mar 19, 2025

View reviewed changes

charlesBochet added -PR: wip -PR: awaiting author labels Mar 19, 2025

charlesBochet reviewed Mar 20, 2025

View reviewed changes

AFCMS added 9 commits March 22, 2025 15:53

Allow to edit labels of standard objects

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

658c0c3

Signed-off-by: AFCMS <[email protected]>

Create utility function, optimise

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

9573773

getEntityMetadataByStandardId => `getStandardObjectMetadataFromDefi…

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

fef4f63

…nition`

Remove the need to disable ESLint warnings

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

3b92c83

Simplify disabled checks

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

47f2a81

Less nested if/else part 1

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

22fe3dc

Fix linter issue

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

ddd1ba3

Simplify conditions

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

Loading
Loading status checks…

6838a85

AFCMS force-pushed the standard_obj_label_edit branch from 033df7f to 6838a85 Compare March 22, 2025 14:53

AFCMS and others added 12 commits March 22, 2025 16:47

Allow to edit icon and description

Verified

This commit was signed with the committer’s verified signature.

AFCMS

GPG key ID: E953045F82DE352D

Verified
Learn about vigilant mode

Loading
Loading status checks…

e0f81f0

Quick proof of concept for alternative method

Loading
Loading status checks…

7b0de6a

Refactor to be more explicit (not re-read this code yet, wip)

78ec8eb

Remove unused code

9f70451

Change how hook is used

Loading
Loading status checks…

263f689

Fix label update when toggle is changed

Loading
Loading status checks…

4eb28c0

Merge branch 'main' into standard_obj_label_edit

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

39f4986

Lint migration file

18a4e2a

Icon overwrite and start preparing for field overwrite

Loading
Loading status checks…

a9fd262

Change path for isDefined after merge

Loading
Loading status checks…

b3a4878

Begin support for field overwrite

Loading
Loading status checks…

ffb6011

Disallow editing field api names for standard objects

Loading
Loading status checks…

25d632c

Fix field overwrite

Loading
Loading status checks…

983760d

greptile-apps bot reviewed Mar 24, 2025

View reviewed changes

ijreilly reviewed Mar 24, 2025

View reviewed changes

FelixMalfait and others added 9 commits March 24, 2025 11:58

Refactor frontend code to simplify

8e0a7d2

Remove disabled prop

fd7e285

Simplify code

Loading
Loading status checks…

0207129

Code refactoring

637aa29

Same refactoring but for fieldMetadata

ed4aacc

Fix typing

Loading
Loading status checks…

514f810

Greptile fixes

Loading
Loading status checks…

938dc6b

Better naming (Marie's comment)

Loading
Loading status checks…

af72c3e

Merge branch 'main' into standard_obj_label_edit

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

6420d04

FelixMalfait marked this pull request as ready for review March 24, 2025 14:16

greptile-apps bot reviewed Mar 24, 2025

View reviewed changes

FelixMalfait added 5 commits March 24, 2025 16:24

Attempt to fix bug where change isLabelSyncedWithName doesnt trigger …

Loading
Loading status checks…

e575db8

…request

Attempt to fix bug where change isLabelSyncedWithName doesnt trigger …

Loading
Loading status checks…

edceb55

…request

Minor review fixes

Loading
Loading status checks…

086ef57

Fix mocks

Loading
Loading status checks…

09de95a

Lint

Loading
Loading status checks…

fc02d6a

FelixMalfait force-pushed the standard_obj_label_edit branch from 0fd497c to fc02d6a Compare March 24, 2025 19:06

FelixMalfait merged commit 52cf6f4 into twentyhq:main Mar 24, 2025
45 checks passed

AFCMS deleted the standard_obj_label_edit branch March 24, 2025 20:07

ijreilly reviewed Mar 25, 2025

View reviewed changes

FelixMalfait mentioned this pull request Mar 25, 2025

Migrate overwrite system of options/settings for standard object twentyhq/core-team-issues#665

Open


		const standardObjectMap = new Map<string, WorkspaceEntityMetadataArgs>();

		standardObjectMetadataDefinitions.forEach((entity) => {

Allow to edit labels of standard objects #10922

Allow to edit labels of standard objects #10922

Conversation

AFCMS commented Mar 15, 2025 • edited Loading

FelixMalfait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixMalfait Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AFCMS commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixMalfait commented Mar 20, 2025

Choose a reason for hiding this comment

FelixMalfait commented Mar 24, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

ijreilly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijreilly Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Mar 24, 2025

Choose a reason for hiding this comment

ijreilly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AFCMS commented Mar 15, 2025 •

edited

Loading

FelixMalfait Mar 17, 2025 •

edited

Loading

ijreilly Mar 24, 2025 •

edited

Loading