[FIX] Optimistically compute position only for objectMetadataItem…

… that has the field (#10510) # Introduction In this PR #10493 introduced a regression on optimistic cache for record creation, by expecting a `position` `fieldMetadataItem` on every `ObjectMetadataItem` which is a wrong assertion Some `Tasks` and `ApiKeys` do not have one ## Fix Dynamically compute optimistic record input position depending on current `ObjectMetadataItem` ## Refactor Refactored a failing test following [jest each](https://jestjs.io/docs/api#describeeachtablename-fn-timeout) pattern to avoid error prone duplicated env tests. Created a "standard' and applied it to an other test also using `it.each`.
twentyhq · Feb 26, 2025 · ea0d3c6 · ea0d3c6
1 parent e6355c7
commit ea0d3c6
Show file tree

Hide file tree

Showing 11 changed files with 210 additions and 115 deletions.
diff --git a/...eckObjectMetadataItemHasFieldCreatedBy.ts → ...ls/hasObjectMetadataItemFieldCreatedBy.ts b/...eckObjectMetadataItemHasFieldCreatedBy.ts → ...ls/hasObjectMetadataItemFieldCreatedBy.ts
@@ -1,7 +1,7 @@
 import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
 import { FieldMetadataType } from 'twenty-shared';
 
-export const checkObjectMetadataItemHasFieldCreatedBy = (
+export const hasObjectMetadataItemFieldCreatedBy = (
   objectMetadataItem: ObjectMetadataItem,
 ) =>
   objectMetadataItem.fields.some(

diff --git a/...ages/twenty-front/src/modules/object-metadata/utils/hasObjectMetadataItemPositionField.ts b/...ages/twenty-front/src/modules/object-metadata/utils/hasObjectMetadataItemPositionField.ts
@@ -0,0 +1,10 @@
+import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
+import { FieldMetadataType } from 'twenty-shared';
+
+export const hasObjectMetadataItemPositionField = (
+  objectMetadataItem: ObjectMetadataItem,
+) =>
+  !objectMetadataItem.isRemote &&
+  objectMetadataItem.fields.some(
+    (field) => field.type === FieldMetadataType.POSITION,
+  );
diff --git a/packages/twenty-front/src/modules/object-metadata/utils/hasPositionField.ts b/packages/twenty-front/src/modules/object-metadata/utils/hasPositionField.ts
diff --git a/packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts b/packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts
@@ -6,7 +6,7 @@ import { triggerDestroyRecordsOptimisticEffect } from '@/apollo/optimistic-effec
 import { currentWorkspaceMemberState } from '@/auth/states/currentWorkspaceMemberState';
 import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
 import { useObjectMetadataItems } from '@/object-metadata/hooks/useObjectMetadataItems';
-import { checkObjectMetadataItemHasFieldCreatedBy } from '@/object-metadata/utils/checkObjectMetadataItemHasFieldCreatedBy';
+import { hasObjectMetadataItemFieldCreatedBy } from '@/object-metadata/utils/hasObjectMetadataItemFieldCreatedBy';
 import { useCreateOneRecordInCache } from '@/object-record/cache/hooks/useCreateOneRecordInCache';
 import { deleteRecordFromCache } from '@/object-record/cache/utils/deleteRecordFromCache';
 import { getObjectTypename } from '@/object-record/cache/utils/getObjectTypename';
@@ -49,7 +49,7 @@ export const useCreateManyRecords = <
   });
 
   const objectMetadataHasCreatedByField =
-    checkObjectMetadataItemHasFieldCreatedBy(objectMetadataItem);
+    hasObjectMetadataItemFieldCreatedBy(objectMetadataItem);
 
   const computedRecordGqlFields =
     recordGqlFields ?? generateDepthOneRecordGqlFields({ objectMetadataItem });

diff --git a/packages/twenty-front/src/modules/object-record/hooks/useCreateOneRecord.ts b/packages/twenty-front/src/modules/object-record/hooks/useCreateOneRecord.ts
@@ -7,7 +7,6 @@ import { triggerDestroyRecordsOptimisticEffect } from '@/apollo/optimistic-effec
 import { currentWorkspaceMemberState } from '@/auth/states/currentWorkspaceMemberState';
 import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
 import { useObjectMetadataItems } from '@/object-metadata/hooks/useObjectMetadataItems';
-import { checkObjectMetadataItemHasFieldCreatedBy } from '@/object-metadata/utils/checkObjectMetadataItemHasFieldCreatedBy';
 import { useCreateOneRecordInCache } from '@/object-record/cache/hooks/useCreateOneRecordInCache';
 import { deleteRecordFromCache } from '@/object-record/cache/utils/deleteRecordFromCache';
 import { getObjectTypename } from '@/object-record/cache/utils/getObjectTypename';
@@ -16,8 +15,8 @@ import { RecordGqlOperationGqlRecordFields } from '@/object-record/graphql/types
 import { generateDepthOneRecordGqlFields } from '@/object-record/graphql/utils/generateDepthOneRecordGqlFields';
 import { useCreateOneRecordMutation } from '@/object-record/hooks/useCreateOneRecordMutation';
 import { useRefetchAggregateQueries } from '@/object-record/hooks/useRefetchAggregateQueries';
-import { FieldActorForInputValue } from '@/object-record/record-field/types/FieldMetadata';
 import { ObjectRecord } from '@/object-record/types/ObjectRecord';
+import { computeOptimisticCreateRecordBaseRecordInput } from '@/object-record/utils/computeOptimisticCreateRecordBaseRecordInput';
 import { computeOptimisticRecordFromInput } from '@/object-record/utils/computeOptimisticRecordFromInput';
 import { getCreateOneRecordMutationResponseField } from '@/object-record/utils/getCreateOneRecordMutationResponseField';
 import { sanitizeRecordInput } from '@/object-record/utils/sanitizeRecordInput';
@@ -46,9 +45,6 @@ export const useCreateOneRecord = <
     objectNameSingular,
   });
 
-  const objectMetadataHasCreatedByField =
-    checkObjectMetadataItemHasFieldCreatedBy(objectMetadataItem);
-
   const computedRecordGqlFields =
     recordGqlFields ?? generateDepthOneRecordGqlFields({ objectMetadataItem });
 
@@ -84,25 +80,14 @@ export const useCreateOneRecord = <
       id: idForCreation,
     };
 
-    const baseOptimisticRecordInputCreatedBy:
-      | { createdBy: FieldActorForInputValue }
-      | undefined = objectMetadataHasCreatedByField
-      ? {
-          createdBy: {
-            source: 'MANUAL',
-            context: {},
-          },
-        }
-      : undefined;
     const optimisticRecordInput = computeOptimisticRecordFromInput({
       cache: apolloClient.cache,
       currentWorkspaceMember: currentWorkspaceMember,
       objectMetadataItem,
       objectMetadataItems,
       recordInput: {
-        ...baseOptimisticRecordInputCreatedBy,
+        ...computeOptimisticCreateRecordBaseRecordInput(objectMetadataItem),
         ...recordInput,
-        position: Number.NEGATIVE_INFINITY,
         id: idForCreation,
       },
     });

diff --git a/...c/modules/object-record/object-sort-dropdown/utils/__tests__/turnSortsIntoOrderBy.test.ts b/...c/modules/object-record/object-sort-dropdown/utils/__tests__/turnSortsIntoOrderBy.test.ts
@@ -1,11 +1,23 @@
 import { FieldMetadataItem } from '@/object-metadata/types/FieldMetadataItem';
 import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
+import { RecordGqlOperationOrderBy } from '@/object-record/graphql/types/RecordGqlOperationOrderBy';
 import { turnSortsIntoOrderBy } from '@/object-record/object-sort-dropdown/utils/turnSortsIntoOrderBy';
 import { RecordSort } from '@/object-record/record-sort/types/RecordSort';
+import { FieldMetadataType } from '~/generated-metadata/graphql';
+import { EachTestingContext } from '~/types/EachTestingContext';
 
-const objectMetadataItem: ObjectMetadataItem = {
+const objectMetadataItemWithPositionField: ObjectMetadataItem = {
   id: 'object1',
-  fields: [],
+  fields: [
+    {
+      name: 'name',
+      updatedAt: '2021-01-01',
+      createdAt: '2021-01-01',
+      id: '20202020-18b3-4099-86e3-c46b2d5d42f2',
+      type: FieldMetadataType.POSITION,
+      label: 'label',
+    },
+  ],
   indexMetadatas: [],
   createdAt: '2021-01-01',
   updatedAt: '2021-01-01',
@@ -22,81 +34,126 @@ const objectMetadataItem: ObjectMetadataItem = {
   isLabelSyncedWithName: true,
 };
 
-describe('turnSortsIntoOrderBy', () => {
-  it('should sort by recordPosition if no sorts', () => {
-    const fields = [{ id: 'field1', name: 'createdAt' }] as FieldMetadataItem[];
-    expect(turnSortsIntoOrderBy({ ...objectMetadataItem, fields }, [])).toEqual(
-      [
+type PartialFieldMetadaItemWithRequiredId = Pick<FieldMetadataItem, 'id'> &
+  Partial<Omit<FieldMetadataItem, 'id'>>;
+const getMockFieldMetadataItem = (
+  overrides: PartialFieldMetadaItemWithRequiredId,
+): FieldMetadataItem => ({
+  name: 'name',
+  updatedAt: '2021-01-01',
+  createdAt: '2021-01-01',
+  type: FieldMetadataType.TEXT,
+  label: 'label',
+  ...overrides,
+});
+
+type TurnSortsIntoOrderTestContext = EachTestingContext<{
+  fields: PartialFieldMetadaItemWithRequiredId[];
+  expected: RecordGqlOperationOrderBy;
+  sort: RecordSort[];
+  objectMetadataItemOverrides?: Partial<Omit<ObjectMetadataItem, 'fields'>>;
+}>;
+
+const turnSortsIntoOrderByTestUseCases: TurnSortsIntoOrderTestContext[] = [
+  {
+    title: 'It should sort by recordPosition if no sorts',
+    context: {
+      fields: [{ id: 'field1', name: 'field1' }],
+      sort: [],
+      expected: [
         {
           position: 'AscNullsFirst',
         },
       ],
-    );
-  });
-
-  it('should create OrderByField with single sort', () => {
-    const sorts: RecordSort[] = [
-      {
-        id: 'id',
-        fieldMetadataId: 'field1',
-        direction: 'asc',
-      },
-    ];
-    const fields = [{ id: 'field1', name: 'field1' }] as FieldMetadataItem[];
-    expect(
-      turnSortsIntoOrderBy({ ...objectMetadataItem, fields }, sorts),
-    ).toEqual([{ field1: 'AscNullsFirst' }, { position: 'AscNullsFirst' }]);
-  });
-
-  it('should create OrderByField with multiple sorts', () => {
-    const sorts: RecordSort[] = [
-      {
-        id: 'id',
-        fieldMetadataId: 'field1',
-        direction: 'asc',
-      },
-      {
-        id: 'id',
-        fieldMetadataId: 'field2',
-        direction: 'desc',
+    },
+  },
+  {
+    title: 'It should create OrderByField with single sort',
+    context: {
+      fields: [{ id: 'field1', name: 'field1' }],
+      sort: [
+        {
+          id: 'id',
+          fieldMetadataId: 'field1',
+          direction: 'asc',
+        },
+      ],
+      expected: [{ field1: 'AscNullsFirst' }, { position: 'AscNullsFirst' }],
+    },
+  },
+  {
+    title: 'It should create OrderByField with multiple sorts',
+    context: {
+      fields: [
+        { id: 'field1', name: 'field1' },
+        { id: 'field2', name: 'field2' },
+      ],
+      sort: [
+        {
+          id: 'id',
+          fieldMetadataId: 'field1',
+          direction: 'asc',
+        },
+        {
+          id: 'id',
+          fieldMetadataId: 'field2',
+          direction: 'desc',
+        },
+      ],
+      expected: [
+        { field1: 'AscNullsFirst' },
+        { field2: 'DescNullsLast' },
+        { position: 'AscNullsFirst' },
+      ],
+    },
+  },
+  {
+    title: 'It should ignore if field not found',
+    context: {
+      fields: [],
+      sort: [
+        {
+          id: 'id',
+          fieldMetadataId: 'invalidField',
+          direction: 'asc',
+        },
+      ],
+      expected: [{ position: 'AscNullsFirst' }],
+    },
+  },
+  {
+    title: 'It should not return position for remotes',
+    context: {
+      fields: [],
+      sort: [
+        {
+          id: 'id',
+          fieldMetadataId: 'invalidField',
+          direction: 'asc',
+        },
+      ],
+      expected: [],
+      objectMetadataItemOverrides: {
+        isRemote: true,
       },
-    ];
-    const fields = [
-      { id: 'field1', name: 'field1' },
-      { id: 'field2', name: 'field2' },
-    ] as FieldMetadataItem[];
-    expect(
-      turnSortsIntoOrderBy({ ...objectMetadataItem, fields }, sorts),
-    ).toEqual([
-      { field1: 'AscNullsFirst' },
-      { field2: 'DescNullsLast' },
-      { position: 'AscNullsFirst' },
-    ]);
-  });
+    },
+  },
+];
 
-  it('should ignore if field not found', () => {
-    const sorts: RecordSort[] = [
-      {
-        id: 'id',
-        fieldMetadataId: 'invalidField',
-        direction: 'asc',
-      },
-    ];
-    expect(turnSortsIntoOrderBy(objectMetadataItem, sorts)).toEqual([
-      { position: 'AscNullsFirst' },
-    ]);
-  });
+describe('turnSortsIntoOrderBy', () => {
+  it.each<TurnSortsIntoOrderTestContext>(turnSortsIntoOrderByTestUseCases)(
+    '.$title',
+    ({ context: { fields, sort, expected, objectMetadataItemOverrides } }) => {
+      const newFields = fields.map(getMockFieldMetadataItem);
+      const objectMetadataItemWithNewFields = {
+        ...objectMetadataItemWithPositionField,
+        ...objectMetadataItemOverrides,
+        fields: [...objectMetadataItemWithPositionField.fields, ...newFields],
+      };
 
-  it('should not return position for remotes', () => {
-    const sorts: RecordSort[] = [
-      {
-        id: 'id',
-        fieldMetadataId: 'invalidField',
-        direction: 'asc',
-      },
-    ];
-    expect(
-      turnSortsIntoOrderBy({ ...objectMetadataItem, isRemote: true }, sorts),
-    ).toEqual([]);
-  });
+      expect(
+        turnSortsIntoOrderBy(objectMetadataItemWithNewFields, sort),
+      ).toEqual(expected);
+    },
+  );
 });
diff --git a/...twenty-front/src/modules/object-record/object-sort-dropdown/utils/turnSortsIntoOrderBy.ts b/...twenty-front/src/modules/object-record/object-sort-dropdown/utils/turnSortsIntoOrderBy.ts
@@ -1,13 +1,13 @@
 import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
 
-import { hasPositionField } from '@/object-metadata/utils/hasPositionField';
 import { RecordGqlOperationOrderBy } from '@/object-record/graphql/types/RecordGqlOperationOrderBy';
 import { isDefined } from 'twenty-shared';
 import { mapArrayToObject } from '~/utils/array/mapArrayToObject';
 import { isUndefinedOrNull } from '~/utils/isUndefinedOrNull';
 
 import { FieldMetadataItem } from '@/object-metadata/types/FieldMetadataItem';
 import { getOrderByForFieldMetadataType } from '@/object-metadata/utils/getOrderByForFieldMetadataType';
+import { hasObjectMetadataItemPositionField } from '@/object-metadata/utils/hasObjectMetadataItemPositionField';
 import { RecordSort } from '@/object-record/record-sort/types/RecordSort';
 import { OrderBy } from '@/types/OrderBy';
 
@@ -35,7 +35,7 @@ export const turnSortsIntoOrderBy = (
     })
     .filter(isDefined);
 
-  if (hasPositionField(objectMetadataItem)) {
+  if (hasObjectMetadataItemPositionField(objectMetadataItem)) {
     const positionOrderBy = [
       {
         position: 'AscNullsFirst',

diff --git a/...wenty-front/src/modules/object-record/record-index/hooks/useRecordBoardRecordGqlFields.ts b/...wenty-front/src/modules/object-record/record-index/hooks/useRecordBoardRecordGqlFields.ts
@@ -2,7 +2,7 @@ import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadata
 import { CoreObjectNameSingular } from '@/object-metadata/types/CoreObjectNameSingular';
 import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
 import { getObjectMetadataIdentifierFields } from '@/object-metadata/utils/getObjectMetadataIdentifierFields';
-import { hasPositionField } from '@/object-metadata/utils/hasPositionField';
+import { hasObjectMetadataItemPositionField } from '@/object-metadata/utils/hasObjectMetadataItemPositionField';
 import { generateDepthOneRecordGqlFields } from '@/object-record/graphql/utils/generateDepthOneRecordGqlFields';
 import { recordBoardVisibleFieldDefinitionsComponentSelector } from '@/object-record/record-board/states/selectors/recordBoardVisibleFieldDefinitionsComponentSelector';
 import { recordGroupFieldMetadataComponentState } from '@/object-record/record-group/states/recordGroupFieldMetadataComponentState';
@@ -58,7 +58,9 @@ export const useRecordBoardRecordGqlFields = ({
         true,
       ]),
     ),
-    ...(hasPositionField(objectMetadataItem) ? { position: true } : undefined),
+    ...(hasObjectMetadataItemPositionField(objectMetadataItem)
+      ? { position: true }
+      : undefined),
     ...identifierQueryFields,
     noteTargets: generateDepthOneRecordGqlFields({
       objectMetadataItem: noteTargetObjectMetadataItem,

diff --git a/...nty-front/src/modules/object-record/utils/computeOptimisticCreateRecordBaseRecordInput.ts b/...nty-front/src/modules/object-record/utils/computeOptimisticCreateRecordBaseRecordInput.ts
@@ -0,0 +1,24 @@
+import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
+import { hasObjectMetadataItemFieldCreatedBy } from '@/object-metadata/utils/hasObjectMetadataItemFieldCreatedBy';
+import { hasObjectMetadataItemPositionField } from '@/object-metadata/utils/hasObjectMetadataItemPositionField';
+import { FieldActorForInputValue } from '@/object-record/record-field/types/FieldMetadata';
+import { ObjectRecord } from '@/object-record/types/ObjectRecord';
+
+export const computeOptimisticCreateRecordBaseRecordInput = (
+  objectMetadataItem: ObjectMetadataItem,
+) => {
+  const baseRecordInput: Partial<ObjectRecord> = {};
+
+  if (hasObjectMetadataItemFieldCreatedBy(objectMetadataItem)) {
+    baseRecordInput.createdBy = {
+      source: 'MANUAL',
+      context: {},
+    } satisfies FieldActorForInputValue;
+  }
+
+  if (hasObjectMetadataItemPositionField(objectMetadataItem)) {
+    baseRecordInput.position = Number.NEGATIVE_INFINITY;
+  }
+
+  return baseRecordInput;
+};
diff --git a/packages/twenty-front/src/types/EachTestingContext.ts b/packages/twenty-front/src/types/EachTestingContext.ts
@@ -0,0 +1,4 @@
+export type EachTestingContext<T> = {
+  title: string;
+  context: T;
+};