[FIX] Optimistically compute position only for objectMetadataItem that has the field (#10510)

# Introduction In this PR https://github.com/twentyhq/twenty/pull/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`.
2025-02-26 17:04:52 +01:00
parent e6355c7c49
commit ea0d3c605f
11 changed files with 215 additions and 120 deletions
--- a/packages/twenty-front/src/modules/object-metadata/utils/checkObjectMetadataItemHasFieldCreatedBy.ts
+++ b/packages/twenty-front/src/modules/object-metadata/utils/checkObjectMetadataItemHasFieldCreatedBy.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(
--- a/packages/twenty-front/src/modules/object-metadata/utils/hasObjectMetadataItemPositionField.ts
+++ b/packages/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,
+  );
--- a/packages/twenty-front/src/modules/object-metadata/utils/hasPositionField.ts
+++ b/packages/twenty-front/src/modules/object-metadata/utils/hasPositionField.ts
@ -1,4 +0,0 @@
-import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
-
-export const hasPositionField = (objectMetadataItem: ObjectMetadataItem) =>
-  !objectMetadataItem.isRemote;
--- 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 });
--- 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,
      },
    });
--- a/packages/twenty-front/src/modules/object-record/object-sort-dropdown/utils/tests/turnSortsIntoOrderBy.test.ts
+++ b/packages/twenty-front/src/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',
        },
      ],
-    );
-  });
+    },
+  },
+  {
+    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,
+      },
+    },
+  },
+];

-  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' }]);
-  });
+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 create OrderByField with multiple sorts', () => {
-    const sorts: RecordSort[] = [
-      {
-        id: 'id',
-        fieldMetadataId: 'field1',
-        direction: 'asc',
-      },
-      {
-        id: 'id',
-        fieldMetadataId: 'field2',
-        direction: 'desc',
-      },
-    ];
-    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' },
-    ]);
-  });
-
-  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);
+    },
+  );
 });
--- a/packages/twenty-front/src/modules/object-record/object-sort-dropdown/utils/turnSortsIntoOrderBy.ts
+++ b/packages/twenty-front/src/modules/object-record/object-sort-dropdown/utils/turnSortsIntoOrderBy.ts
@ -1,6 +1,5 @@
 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';
@ -8,6 +7,7 @@ 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',
--- a/packages/twenty-front/src/modules/object-record/record-index/hooks/useRecordBoardRecordGqlFields.ts
+++ b/packages/twenty-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,
--- a/packages/twenty-front/src/modules/object-record/utils/computeOptimisticCreateRecordBaseRecordInput.ts
+++ b/packages/twenty-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;
+};
--- 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;
+};
--- a/packages/twenty-front/src/utils/array/tests/buildRecordFromKeysWithSameValue.test.ts
+++ b/packages/twenty-front/src/utils/array/tests/buildRecordFromKeysWithSameValue.test.ts
@ -1,23 +1,40 @@
+import { EachTestingContext } from '~/types/EachTestingContext';
 import { buildRecordFromKeysWithSameValue } from '~/utils/array/buildRecordFromKeysWithSameValue';

+type BuildRecordFromKeysWithSameValueTestContext = EachTestingContext<{
+  array: string[];
+  expected: Record<string, boolean | string>;
+  arg?: boolean | string;
+}>;
+
+const buildRecordFromKeysWithSameValueTestUseCases: BuildRecordFromKeysWithSameValueTestContext[] =
+  [
+    {
+      title: 'It should create record from array and fill with boolean',
+      context: {
+        array: ['foo', 'bar'] as const,
+        expected: { foo: true, bar: true },
+        arg: true,
+      },
+    },
+    {
+      title: 'It should create record from array and fill with string',
+      context: {
+        array: ['foo', 'bar'],
+        expected: { foo: 'oui', bar: 'oui' },
+        arg: 'oui',
+      },
+    },
+    {
+      title: 'It should create empty record from empty array',
+      context: { array: [], expected: {}, arg: undefined },
+    },
+  ];
 describe('buildRecordFromKeysWithSameValue', () => {
-  test.each([
-    { array: [], expected: {}, arg: undefined },
-    {
-      array: ['foo', 'bar'],
-      expected: { foo: 'oui', bar: 'oui' },
-      arg: 'oui',
-    },
-    {
-      array: ['foo', 'bar'] as const,
-      expected: { foo: true, bar: true },
-      arg: true,
-    },
-  ])(
-    '.buildRecordFromKeysWithSameValue($array, $arg)',
-    ({ array, arg, expected }) => {
-      const result = buildRecordFromKeysWithSameValue(array, arg);
-      expect(result).toEqual(expected);
-    },
-  );
+  test.each<BuildRecordFromKeysWithSameValueTestContext>(
+    buildRecordFromKeysWithSameValueTestUseCases,
+  )('.$title', ({ context: { array, arg, expected } }) => {
+    const result = buildRecordFromKeysWithSameValue(array, arg);
+    expect(result).toEqual(expected);
+  });
 });