[BUG] Record settings not saved (#9762)

# Introduction
By initially fixing this Fixes #9381, discovered other behavior that
have been fix.
Overall we encountered a bug that corrupts a workspace and make the
browser + api crash
This issue https://github.com/twentyhq/core-team-issues/issues/25
suggests a refactor that has final save button instead of auto-save

## `labelIdentifierFieldMetadataId` form default value
The default value resulted in being undefined, resulting in react hook
form `labelIdentifierFieldMetadataId` is required field error.

### Fix
Setting default value fallback to `null`  as field is `nullable`

## `SettingsDataModelObjectSettingsFormCard` never triggers form
Unless I'm mistaken in production touching any fields within
`SettingsDataModelObjectSettingsFormCard` would never trigger form
submission until you also modify `SettingsDataModelObjectAboutForm`
fields

### Fix
Provide and apply `onblur` that triggers the form on both
`SettingsDataModelObjectSettingsFormCard` inputs

## Wrong default `labelIdentifierFieldMetadataItem` on first page render
When landing on the page for the first time, if a custom
`labelIdentifierFieldMetadataItem` has been set it won't be computed
within the `PreviewCard`.
Occurs when `labelIdentifierFieldMetadataId` form default value is
undefined, due to `any` injection.

### Fix
In the `getLabelIdentifierFieldMetadataItem` check the
`labelIdentifierFieldMetadataIdFormValue` definition, if undefined
fallback to current `objectMetadata` identifier

---------

Co-authored-by: Charles Bochet <charles@twenty.com>
This commit is contained in:
Paul Rastoin
2025-01-22 16:32:57 +01:00
committed by GitHub
parent 80c9ebfd4e
commit 8ab01ebef4
22 changed files with 161 additions and 89 deletions

View File

@ -8,6 +8,7 @@ import { getJestMetadataAndApolloMocksWrapper } from '~/testing/jest/getJestMeta
jest.mock('react-router-dom', () => ({
useParams: jest.fn(),
useSearchParams: jest.fn(),
Link: jest.fn(),
}));
jest.mock('@/auth/hooks/useAuth', () => ({

View File

@ -7,6 +7,7 @@ import { getJestMetadataAndApolloMocksWrapper } from '~/testing/jest/getJestMeta
jest.mock('react-router-dom', () => ({
useParams: jest.fn(),
useSearchParams: jest.fn(),
Link: jest.fn(),
}));
jest.mock('@/auth/hooks/useAuth', () => ({

View File

@ -81,6 +81,6 @@ export const responseData = {
isActive: true,
createdAt: '',
updatedAt: '',
labelIdentifierFieldMetadataId: '',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
imageIdentifierFieldMetadataId: '',
};

View File

@ -36,6 +36,6 @@ export const responseData = {
isActive: true,
createdAt: '',
updatedAt: '',
labelIdentifierFieldMetadataId: '',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
imageIdentifierFieldMetadataId: '',
};

View File

@ -50,6 +50,6 @@ export const responseData = {
isActive: true,
createdAt: '',
updatedAt: '',
labelIdentifierFieldMetadataId: '',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
imageIdentifierFieldMetadataId: '',
};

View File

@ -5,9 +5,14 @@ import { FieldMetadataItem } from './FieldMetadataItem';
export type ObjectMetadataItem = Omit<
GeneratedObject,
'__typename' | 'fields' | 'dataSourceId' | 'indexMetadatas'
| '__typename'
| 'fields'
| 'dataSourceId'
| 'indexMetadatas'
| 'labelIdentifierFieldMetadataId'
> & {
__typename?: string;
fields: FieldMetadataItem[];
labelIdentifierFieldMetadataId: string;
indexMetadatas: IndexMetadataItem[];
};

View File

@ -1,11 +1,23 @@
import { isLabelIdentifierField } from '@/object-metadata/utils/isLabelIdentifierField';
describe('isLabelIdentifierField', () => {
it('should work as expected', () => {
it('should not find unknown labelIdentifier', () => {
const res = isLabelIdentifierField({
fieldMetadataItem: { id: 'fieldId', name: 'fieldName' },
objectMetadataItem: {},
objectMetadataItem: {
labelIdentifierFieldMetadataId: 'unknown',
},
});
expect(res).toBe(false);
});
it('should find known labelIdentifier', () => {
const res = isLabelIdentifierField({
fieldMetadataItem: { id: 'fieldId', name: 'fieldName' },
objectMetadataItem: {
labelIdentifierFieldMetadataId: 'fieldId',
},
});
expect(res).toBe(true);
});
});

View File

@ -1,5 +1,5 @@
import { objectMetadataItemSchema } from '@/object-metadata/validation-schemas/objectMetadataItemSchema';
import { ObjectMetadataItemsQuery } from '~/generated-metadata/graphql';
import { ObjectMetadataItem } from '../types/ObjectMetadataItem';
export const mapPaginatedObjectMetadataItemsToObjectMetadataItems = ({
@ -8,16 +8,24 @@ export const mapPaginatedObjectMetadataItemsToObjectMetadataItems = ({
pagedObjectMetadataItems: ObjectMetadataItemsQuery | undefined;
}) => {
const formattedObjects: ObjectMetadataItem[] =
pagedObjectMetadataItems?.objects.edges.map((object) => ({
...object.node,
fields: object.node.fields.edges.map((field) => field.node),
indexMetadatas: object.node.indexMetadatas?.edges.map((index) => ({
...index.node,
indexFieldMetadatas: index.node.indexFieldMetadatas?.edges.map(
(indexField) => indexField.node,
),
})),
})) ?? [];
pagedObjectMetadataItems?.objects.edges.map((object) => {
const labelIdentifierFieldMetadataId =
objectMetadataItemSchema.shape.labelIdentifierFieldMetadataId.parse(
object.node.labelIdentifierFieldMetadataId,
);
return {
...object.node,
fields: object.node.fields.edges.map((field) => field.node),
labelIdentifierFieldMetadataId,
indexMetadatas: object.node.indexMetadatas?.edges.map((index) => ({
...index.node,
indexFieldMetadatas: index.node.indexFieldMetadatas?.edges.map(
(indexField) => indexField.node,
),
})),
};
}) ?? [];
return formattedObjects;
};

View File

@ -1,3 +1,4 @@
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems';
import { objectMetadataItemSchema } from '../objectMetadataItemSchema';
@ -15,16 +16,37 @@ describe('objectMetadataItemSchema', () => {
expect(result).toEqual(validObjectMetadataItem);
});
it('fails for an invalid object metadata item that has null labelIdentifier', () => {
// Given
const validObjectMetadataItem = generatedMockObjectMetadataItems.find(
(item) => item.nameSingular === 'company',
);
expect(validObjectMetadataItem).not.toBeUndefined();
if (validObjectMetadataItem === undefined)
throw new Error('Should never occurs');
// When
const result = objectMetadataItemSchema.safeParse({
...validObjectMetadataItem,
labelIdentifierFieldMetadataId: null,
});
// Then
expect(result.success).toEqual(false);
});
it('fails for an invalid object metadata item', () => {
// Given
const invalidObjectMetadataItem = {
const invalidObjectMetadataItem: Partial<
Record<keyof ObjectMetadataItem, unknown>
> = {
createdAt: 'invalid date',
dataSourceId: 'invalid uuid',
fields: 'not an array',
icon: 'invalid icon',
isActive: 'not a boolean',
isCustom: 'not a boolean',
isSystem: 'not a boolean',
labelIdentifierFieldMetadataId: 'not a uuid',
labelPlural: 123,
labelSingular: 123,
namePlural: 'notCamelCase',
@ -41,4 +63,22 @@ describe('objectMetadataItemSchema', () => {
// Then
expect(result.success).toBe(false);
});
it('should fail to parse empty string as LabelIdentifier', () => {
const emptyString = '';
const result =
objectMetadataItemSchema.shape.labelIdentifierFieldMetadataId.safeParse(
emptyString,
);
expect(result.success).toBe(false);
});
it('should succeed to parse valid uuid as LabelIdentifier', () => {
const validUuid = '20202020-ae24-4871-b445-10cc8872cb10';
const result =
objectMetadataItemSchema.shape.labelIdentifierFieldMetadataId.safeParse(
validUuid,
);
expect(result.success).toBe(true);
});
});

View File

@ -20,7 +20,7 @@ export const objectMetadataItemSchema = z.object({
isCustom: z.boolean(),
isRemote: z.boolean(),
isSystem: z.boolean(),
labelIdentifierFieldMetadataId: z.string().uuid().nullable(),
labelIdentifierFieldMetadataId: z.string().uuid(),
labelPlural: metadataLabelSchema(),
labelSingular: metadataLabelSchema(),
namePlural: camelCaseStringSchema,

View File

@ -17,12 +17,13 @@ const mockObjectMetadataItem: ObjectMetadataItem = {
labelSingular: 'Company',
labelPlural: 'Companies',
isCustom: false,
labelIdentifierFieldMetadataId: '20202020-dd4a-4ea4-bb7b-1c7300491b65',
isActive: true,
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
fields: [
{
id: 'field-1',
id: '20202020-fed9-4ce5-9502-02a8efaf46e1',
name: 'amount',
label: 'Amount',
type: FieldMetadataType.NUMBER,
@ -32,7 +33,7 @@ const mockObjectMetadataItem: ObjectMetadataItem = {
updatedAt: new Date().toISOString(),
} as FieldMetadataItem,
{
id: 'field-2',
id: '20202020-dd4a-4ea4-bb7b-1c7300491b65',
name: 'name',
label: 'Name',
type: FieldMetadataType.TEXT,

View File

@ -18,6 +18,7 @@ const objectMetadataItem: ObjectMetadataItem = {
updatedAt: '2021-01-01',
nameSingular: 'object1',
namePlural: 'object1s',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
icon: 'icon',
isActive: true,
isSystem: false,

View File

@ -18,7 +18,7 @@ describe('buildRecordGqlFieldsAggregateForView', () => {
isActive: true,
isSystem: false,
isRemote: false,
labelIdentifierFieldMetadataId: null,
labelIdentifierFieldMetadataId: '06b33746-5293-4d07-9f7f-ebf5ad396064',
imageIdentifierFieldMetadataId: null,
isLabelSyncedWithName: true,
fields: [

View File

@ -25,6 +25,7 @@ describe('useLimitPerMetadataItem', () => {
labelSingular: 'labelSingular',
namePlural: 'namePlural',
nameSingular: 'nameSingular',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
updatedAt: 'updatedAt',
isLabelSyncedWithName: false,
fields: [],

View File

@ -9,6 +9,7 @@ describe('generateAggregateQuery', () => {
id: 'test-id',
labelSingular: 'Company',
labelPlural: 'Companies',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
isCustom: false,
isActive: true,
createdAt: new Date().toISOString(),
@ -46,6 +47,7 @@ describe('generateAggregateQuery', () => {
id: 'test-id',
labelSingular: 'Person',
labelPlural: 'People',
labelIdentifierFieldMetadataId: '20202020-72ba-4e11-a36d-e17b544541e1',
isCustom: false,
isActive: true,
createdAt: new Date().toISOString(),

View File

@ -28,6 +28,7 @@ export const useFieldPreviewValue = ({
relationObjectMetadataItem: relationObjectMetadataItem ?? {
fields: [],
labelSingular: '',
labelIdentifierFieldMetadataId: '20202020-1000-4629-87e5-9a1fae1cc2fd',
nameSingular: CoreObjectNameSingular.Company,
},
skip:

View File

@ -23,7 +23,6 @@ import { SnackBarVariant } from '@/ui/feedback/snack-bar-manager/components/Snac
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import { navigationMemorizedUrlState } from '@/ui/navigation/states/navigationMemorizedUrlState';
import styled from '@emotion/styled';
import isEmpty from 'lodash.isempty';
import pick from 'lodash.pick';
import { useSetRecoilState } from 'recoil';
import { useNavigateSettings } from '~/hooks/useNavigateSettings';
@ -70,6 +69,7 @@ export const ObjectSettings = ({ objectMetadataItem }: ObjectSettingsProps) => {
mode: 'onTouched',
resolver: zodResolver(objectEditFormSchema),
});
const { isDirty } = formConfig.formState;
const setNavigationMemorizedUrl = useSetRecoilState(
navigationMemorizedUrlState,
@ -124,7 +124,7 @@ export const ObjectSettings = ({ objectMetadataItem }: ObjectSettingsProps) => {
const handleSave = async (
formValues: SettingsDataModelObjectEditFormValues,
) => {
if (isEmpty(formConfig.formState.dirtyFields) === true) {
if (!isDirty) {
return;
}
try {
@ -202,6 +202,7 @@ export const ObjectSettings = ({ objectMetadataItem }: ObjectSettingsProps) => {
description="Choose the fields that will identify your records"
/>
<SettingsDataModelObjectSettingsFormCard
onBlur={() => formConfig.handleSubmit(handleSave)()}
objectMetadataItem={objectMetadataItem}
/>
</Section>

View File

@ -219,6 +219,7 @@ export const SettingsDataModelObjectAboutForm = ({
value={value ?? undefined}
onChange={(nextValue) => onChange(nextValue ?? null)}
disabled={disableEdition}
onBlur={onBlur}
/>
)}
/>

View File

@ -1,7 +1,7 @@
import styled from '@emotion/styled';
import { useMemo } from 'react';
import { Controller, useFormContext } from 'react-hook-form';
import { IconCircleOff, isDefined, useIcons } from 'twenty-ui';
import { IconCircleOff, useIcons } from 'twenty-ui';
import { z } from 'zod';
import { LABEL_IDENTIFIER_FIELD_METADATA_TYPES } from '@/object-metadata/constants/LabelIdentifierFieldMetadataTypes';
@ -19,11 +19,16 @@ export const settingsDataModelObjectIdentifiersFormSchema =
export type SettingsDataModelObjectIdentifiersFormValues = z.infer<
typeof settingsDataModelObjectIdentifiersFormSchema
>;
export type SettingsDataModelObjectIdentifiers =
keyof SettingsDataModelObjectIdentifiersFormValues;
type SettingsDataModelObjectIdentifiersFormProps = {
objectMetadataItem: ObjectMetadataItem;
defaultLabelIdentifierFieldMetadataId: string;
onBlur: () => void;
};
const LABEL_IDENTIFIER_FIELD_METADATA_ID: SettingsDataModelObjectIdentifiers =
'labelIdentifierFieldMetadataId';
const IMAGE_IDENTIFIER_FIELD_METADATA_ID: SettingsDataModelObjectIdentifiers =
'imageIdentifierFieldMetadataId';
const StyledContainer = styled.div`
display: flex;
@ -32,12 +37,11 @@ const StyledContainer = styled.div`
export const SettingsDataModelObjectIdentifiersForm = ({
objectMetadataItem,
defaultLabelIdentifierFieldMetadataId,
onBlur,
}: SettingsDataModelObjectIdentifiersFormProps) => {
const { control } =
useFormContext<SettingsDataModelObjectIdentifiersFormValues>();
const { getIcon } = useIcons();
const labelIdentifierFieldOptions = useMemo(
() =>
getActiveFieldMetadataItems(objectMetadataItem)
@ -65,41 +69,37 @@ export const SettingsDataModelObjectIdentifiersForm = ({
{[
{
label: 'Record label',
fieldName: 'labelIdentifierFieldMetadataId' as const,
fieldName: LABEL_IDENTIFIER_FIELD_METADATA_ID,
options: labelIdentifierFieldOptions,
defaultValue: objectMetadataItem.labelIdentifierFieldMetadataId,
},
{
label: 'Record image',
fieldName: 'imageIdentifierFieldMetadataId' as const,
fieldName: IMAGE_IDENTIFIER_FIELD_METADATA_ID,
options: imageIdentifierFieldOptions,
defaultValue: null,
},
].map(({ fieldName, label, options }) => (
].map(({ fieldName, label, options, defaultValue }) => (
<Controller
key={fieldName}
name={fieldName}
control={control}
defaultValue={
fieldName === 'labelIdentifierFieldMetadataId'
? isDefined(objectMetadataItem[fieldName])
? objectMetadataItem[fieldName]
: defaultLabelIdentifierFieldMetadataId
: objectMetadataItem[fieldName]
}
render={({ field: { onBlur, onChange, value } }) => {
return (
<Select
label={label}
disabled={!objectMetadataItem.isCustom || !options.length}
fullWidth
dropdownId={`${fieldName}-select`}
emptyOption={emptyOption}
options={options}
value={value}
onChange={onChange}
onBlur={onBlur}
/>
);
}}
defaultValue={defaultValue}
render={({ field: { onChange, value } }) => (
<Select
label={label}
disabled={!objectMetadataItem.isCustom || !options.length}
fullWidth
dropdownId={`${fieldName}-select`}
emptyOption={emptyOption}
options={options}
value={value}
onChange={(value) => {
onChange(value);
onBlur();
}}
/>
)}
/>
))}
</StyledContainer>

View File

@ -1,21 +1,18 @@
import styled from '@emotion/styled';
import { useMemo } from 'react';
import { useFormContext } from 'react-hook-form';
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { getLabelIdentifierFieldMetadataItem } from '@/object-metadata/utils/getLabelIdentifierFieldMetadataItem';
import { SettingsDataModelCardTitle } from '@/settings/data-model/components/SettingsDataModelCardTitle';
import { SettingsDataModelFieldPreviewCard } from '@/settings/data-model/fields/preview/components/SettingsDataModelFieldPreviewCard';
import { SettingsDataModelObjectSummary } from '@/settings/data-model/objects/components/SettingsDataModelObjectSummary';
import {
SettingsDataModelObjectIdentifiersForm,
SettingsDataModelObjectIdentifiersFormValues,
} from '@/settings/data-model/objects/forms/components/SettingsDataModelObjectIdentifiersForm';
import { SettingsDataModelObjectIdentifiersForm } from '@/settings/data-model/objects/forms/components/SettingsDataModelObjectIdentifiersForm';
import { Trans } from '@lingui/react/macro';
import { Card, CardContent } from 'twenty-ui';
type SettingsDataModelObjectSettingsFormCardProps = {
objectMetadataItem: ObjectMetadataItem;
onBlur: () => void;
};
const StyledFieldPreviewCard = styled(SettingsDataModelFieldPreviewCard)`
@ -38,22 +35,15 @@ const StyledObjectSummaryCardContent = styled(CardContent)`
export const SettingsDataModelObjectSettingsFormCard = ({
objectMetadataItem,
onBlur,
}: SettingsDataModelObjectSettingsFormCardProps) => {
const { watch: watchFormValue } =
useFormContext<SettingsDataModelObjectIdentifiersFormValues>();
const labelIdentifierFieldMetadataIdFormValue = watchFormValue(
'labelIdentifierFieldMetadataId',
);
const labelIdentifierFieldMetadataItem = useMemo(
() =>
getLabelIdentifierFieldMetadataItem({
fields: objectMetadataItem.fields,
labelIdentifierFieldMetadataId: labelIdentifierFieldMetadataIdFormValue,
}),
[labelIdentifierFieldMetadataIdFormValue, objectMetadataItem],
);
const labelIdentifierFieldMetadataItem = useMemo(() => {
return getLabelIdentifierFieldMetadataItem({
fields: objectMetadataItem.fields,
labelIdentifierFieldMetadataId:
objectMetadataItem.labelIdentifierFieldMetadataId,
});
}, [objectMetadataItem]);
return (
<Card fullWidth>
@ -80,9 +70,7 @@ export const SettingsDataModelObjectSettingsFormCard = ({
<CardContent>
<SettingsDataModelObjectIdentifiersForm
objectMetadataItem={objectMetadataItem}
defaultLabelIdentifierFieldMetadataId={
labelIdentifierFieldMetadataItem?.id
}
onBlur={onBlur}
/>
</CardContent>
</Card>

View File

@ -2868,7 +2868,7 @@ export const mockedStandardObjectMetadataQueryResult: ObjectMetadataItemsQuery =
"isSystem": false,
"createdAt": "2024-11-06T08:55:38.993Z",
"updatedAt": "2024-11-06T08:55:38.993Z",
"labelIdentifierFieldMetadataId": null,
"labelIdentifierFieldMetadataId": "7896a006-eb14-481e-8197-661b7009a22e",
"imageIdentifierFieldMetadataId": null,
"shortcut": null,
"isLabelSyncedWithName": false,

View File

@ -1,14 +1,23 @@
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { objectMetadataItemSchema } from '@/object-metadata/validation-schemas/objectMetadataItemSchema';
import { mockedStandardObjectMetadataQueryResult } from '~/testing/mock-data/generated/mock-metadata-query-result';
export const generatedMockObjectMetadataItems: ObjectMetadataItem[] =
mockedStandardObjectMetadataQueryResult.objects.edges.map((edge) => ({
...edge.node,
fields: edge.node.fields.edges.map((edge) => edge.node),
indexMetadatas: edge.node.indexMetadatas.edges.map((index) => ({
...index.node,
indexFieldMetadatas: index.node.indexFieldMetadatas?.edges.map(
(indexField) => indexField.node,
),
})),
}));
mockedStandardObjectMetadataQueryResult.objects.edges.map((edge) => {
const labelIdentifierFieldMetadataId =
objectMetadataItemSchema.shape.labelIdentifierFieldMetadataId.parse(
edge.node.labelIdentifierFieldMetadataId,
);
return {
...edge.node,
fields: edge.node.fields.edges.map((edge) => edge.node),
labelIdentifierFieldMetadataId,
indexMetadatas: edge.node.indexMetadatas.edges.map((index) => ({
...index.node,
indexFieldMetadatas: index.node.indexFieldMetadatas?.edges.map(
(indexField) => indexField.node,
),
})),
};
});