#6094 Prevent creating a custom field with an existing name #6100

deval2498 · 2024-07-02T21:58:43Z

Fixes #6094
Description: Added logic inside SettingsObjectNewFieldStep2.tsx to prevent form submission
Current Behaviours:

greptile-apps

PR Summary

Added logic to prevent form submission if a custom field name already exists in packages/twenty-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx
Maintains a list of existing field names for validation
Displays an error message when a duplicate name is detected

_{1 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Added error icon next to label input in packages/twenty-front/src/modules/settings/data-model/fields/forms/components/SettingsDataModelFieldAboutForm.tsx
Introduced state management for existing field names in packages/twenty-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx
Implemented validation checks to prevent duplicate field names in packages/twenty-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx
Disabled save button and displayed error message for duplicate field names in packages/twenty-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx

_{2 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

ijreilly

Thank you for your contribution @deval2498!
The code is functional but I added a few indications to try to follow our best practices!

ijreilly · 2024-07-04T10:11:09Z

...y-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx

  useEffect(() => {
    if (!activeObjectMetadataItem) {
      navigate(AppPath.NotFound);
    }
+    else {
+      setExistingFieldNames(activeObjectMetadataItem.fields.map(field => field.name));


One of our best practices is to avoid using useEffect as much as possible as they trigger rerenders.
In our case here since the list of field names (activeObjectMetadataItem.fields.map(field => field.name)) is not supposed to change often we don't need to be reactive and to store them in a state :)

ijreilly · 2024-07-04T11:56:52Z

...y-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx


  const formConfig = useForm<SettingsDataModelNewFieldFormValues>({
    mode: 'onTouched',
    resolver: zodResolver(settingsFieldFormSchema),
  });

+  const [existingFieldNames, setExistingFieldNames] = useState<string[]>([]);
+  const [validateNameField, setValidateNameField] = useState(true)


we want to try to use react-hook-form helpers instead of implementing a custom side-validation. Here it would mean relying on react-hook-form useForm's resolver (used L61): settingsFieldFormSchema can be updated to a function accepting additional limitations through arguments. This should prevent the user from saving as formConfig.formState.isValid will be false.

ijreilly · 2024-07-04T11:59:21Z

...y-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx

@@ -202,7 +219,10 @@ export const SettingsObjectNewFieldStep2 = () => {
                title="Name and description"
                description="The name and description of this field"
              />
-              <SettingsDataModelFieldAboutForm />
+              <SettingsDataModelFieldAboutForm isErrorField={!validateNameField}/>
+              {!validateNameField && (


Here too, we should try to use react-hook-form to get the errors and display them at field-level

deval2498 · 2024-07-04T12:05:38Z

On it. Thanks for this review

deval2498 · 2024-07-07T18:45:35Z

Updated the code as per the guidelines. Updated:

FieldMetadataItemSchema to accept an array of strings, so that we can throw an error directly from zod resolver
Used inbuilt error controls of Controller in SettingsDataModelFieldAboutForm
Updated types to incorporate the changes made to FieldMetadataItemSchema
Cuurent Behaviour:

greptile-apps

PR Summary

(updates since last review)

Added validation logic in packages/twenty-front/src/pages/settings/data-model/SettingsObjectNewField/SettingsObjectNewFieldStep2.tsx to prevent duplicate field names.
Updated packages/twenty-front/src/modules/object-metadata/validation-schemas/fieldMetadataItemSchema.ts to include existingLabels parameter for label uniqueness validation.
Modified packages/twenty-front/src/modules/object-metadata/validation-schemas/metadataLabelSchema.ts to check for unique metadata labels.
Introduced maximum length constraints for field names in packages/twenty-front/src/modules/settings/data-model/fields/forms/components/SettingsDataModelFieldAboutForm.tsx.
Updated packages/twenty-front/src/modules/settings/data-model/fields/forms/validation-schemas/settingsFieldFormSchema.ts to accept existingLabels for validation.

_{83 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-07T18:33:11Z

packages/twenty-front/src/modules/object-metadata/validation-schemas/metadataLabelSchema.ts

+      },
+    )
+    .refine((label) => {
+      try{if(!existingLabels) {


🪶 style: Consider adding a space after try for consistency and readability.

Suggested change

try{if(!existingLabels) {

try { if (!existingLabels) {

greptile-apps · 2024-07-07T18:35:14Z

.../src/modules/settings/data-model/fields/forms/components/SettingsDataModelFieldAboutForm.tsx

+  const validateLabel = () => {
+    trigger('label')
+  }
+  console.log(errors.label, "errors near label")


🪶 style: Remove console.log statement before merging.

greptile-apps

PR Summary

(updates since last review)

Enhanced metadataLabelSchema.ts to ensure label uniqueness.
Removed console.log and added error message for non-unique labels in SettingsDataModelFieldAboutForm.tsx.

No major changes found since last review.

_{2 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

…e-used

…orm-submission-if-name-used

ijreilly · 2024-07-15T13:42:46Z

great work @deval2498 , thanks for your contribution :) i allowed myself to add some minor improvements.
I think this also fixes #5428 :) great

there are some remaining issues from the tests (probably some code to adapt), do you want to have a look or shall i take over? @deval2498

deval2498 · 2024-07-15T14:48:51Z

Will take a look and update you on this, thanks for helping here

Bonapara · 2024-07-15T15:27:21Z

Hi @deval2498, thanks for the PR! Could you remove the red error icon on the right side of the field and add a Border Danger around the field?

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=3007-69739&t=hrDkyWQ3cs5iGjno-11

…orm-submission-if-name-used

deval2498 · 2024-07-17T09:15:47Z

@ijreilly color exists in theme.border.color.danger, unable to perform tests on my machine heap running out of memory would be great if you can help:)

ijreilly

great work @deval2498 this will be very useful ! :)

greptile-apps bot reviewed Jul 2, 2024

View reviewed changes

greptile-apps bot reviewed Jul 3, 2024

View reviewed changes

charlesBochet added the -PR: awaiting review label Jul 3, 2024

charlesBochet assigned ijreilly Jul 3, 2024

ijreilly reviewed Jul 4, 2024

View reviewed changes

changes to show name taken on label

7677b01

deval2498 force-pushed the 6094-prevent-form-submission-if-name-used branch from a1405c3 to 7677b01 Compare July 7, 2024 18:31

deval2498 added 2 commits July 8, 2024 00:10

removed console logs and fixed refine schema for label

bac8965

cleaning up metadatalabelschema

1dd0c73

greptile-apps bot reviewed Jul 7, 2024

View reviewed changes

deval2498 requested a review from ijreilly July 8, 2024 07:42

deval2498 and others added 3 commits July 9, 2024 11:58

Merge branch 'twentyhq:main' into 6094-prevent-form-submission-if-nam…

1222bec

…e-used

Improve error message handling

e3f2b28

Merge branch 'main' of github.com:twentyhq/twenty into 6094-prevent-f…

0e2a95f

…orm-submission-if-name-used

Add error message for non formattable labels

0b01a26

ijreilly mentioned this pull request Jul 16, 2024

Improve Error Message for Duplicate Field Creation #6117

Closed

deval2498 and others added 4 commits July 16, 2024 21:31

test fixes

b2a66e1

Merge branch 'main' of github.com:twentyhq/twenty into 6094-prevent-f…

421ab5b

…orm-submission-if-name-used

Add red border and remove icon if error

d62b6eb

Introduce light red color to comply with figma

ed7b2db

Use theme.border.color.danger instead of introducing new color

6d2a08a

ijreilly approved these changes Jul 17, 2024

View reviewed changes

ijreilly merged commit 94c2358 into twentyhq:main Jul 17, 2024
7 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6094 Prevent creating a custom field with an existing name #6100

#6094 Prevent creating a custom field with an existing name #6100

deval2498 commented Jul 2, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot left a comment

ijreilly left a comment

ijreilly Jul 4, 2024

ijreilly Jul 4, 2024

ijreilly Jul 4, 2024

deval2498 commented Jul 4, 2024

deval2498 commented Jul 7, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jul 7, 2024

greptile-apps bot Jul 7, 2024

greptile-apps bot left a comment

ijreilly commented Jul 15, 2024 •

edited

Loading

deval2498 commented Jul 15, 2024

Bonapara commented Jul 15, 2024

deval2498 commented Jul 17, 2024 •

edited

Loading

ijreilly left a comment

#6094 Prevent creating a custom field with an existing name #6100

#6094 Prevent creating a custom field with an existing name #6100

Conversation

deval2498 commented Jul 2, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

ijreilly left a comment

Choose a reason for hiding this comment

ijreilly Jul 4, 2024

Choose a reason for hiding this comment

ijreilly Jul 4, 2024

Choose a reason for hiding this comment

ijreilly Jul 4, 2024

Choose a reason for hiding this comment

deval2498 commented Jul 4, 2024

deval2498 commented Jul 7, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 7, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 7, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

ijreilly commented Jul 15, 2024 • edited Loading

deval2498 commented Jul 15, 2024

Bonapara commented Jul 15, 2024

deval2498 commented Jul 17, 2024 • edited Loading

ijreilly left a comment

Choose a reason for hiding this comment

deval2498 commented Jul 2, 2024 •

edited

Loading

deval2498 commented Jul 7, 2024 •

edited

Loading

ijreilly commented Jul 15, 2024 •

edited

Loading

deval2498 commented Jul 17, 2024 •

edited

Loading