Discard empty and null links in Links fields (#12188)

This PR has several objectives: - Ignore invalid and empty links in the frontend - Ignore empty links when creating or updating a link field in the backend - Throw an error when trying to create or update a link field with an invalid link The logic is mostly the same in the frontend and the backend: we take the initial primaryLink and the secondaryLinks, we discard all the empty links (with `url === '' || url === null`), and the primaryLink becomes the first remaining link. ## Frontend There are three parts in the frontend where we have to remove the empty links: - LinksDisplay - LinksFieldInput - isFieldValueEmpty; used in RecordInlineCell ## Backend I put the logic in `packages/twenty-server/src/engine/core-modules/record-transformer/services/record-input-transformer.service.ts` as it's used by the REST API, the GraphQL API, and by Create Record and Update Record actions in the workflows.
2025-05-23 11:13:10 +02:00
parent 75e4a5d19b
commit ec9d8e4e95
17 changed files with 544 additions and 31 deletions
--- a/packages/twenty-front/src/modules/object-record/record-field/mocks/fieldDefinitions.ts
+++ b/packages/twenty-front/src/modules/object-record/record-field/mocks/fieldDefinitions.ts
@ -1,11 +1,12 @@
 import { formatFieldMetadataItemAsFieldDefinition } from '@/object-metadata/utils/formatFieldMetadataItemAsFieldDefinition';
 import { FieldDefinition } from '@/object-record/record-field/types/FieldDefinition';
 import {
-    FieldActorMetadata,
-    FieldFullNameMetadata,
-    FieldRatingMetadata,
-    FieldSelectMetadata,
-    FieldTextMetadata,
+  FieldActorMetadata,
+  FieldFullNameMetadata,
+  FieldLinksMetadata,
+  FieldRatingMetadata,
+  FieldSelectMetadata,
+  FieldTextMetadata,
 } from '@/object-record/record-field/types/FieldMetadata';
 import { FieldMetadataType } from '~/generated-metadata/graphql';
 import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems';
@ -111,3 +112,19 @@ export const actorFieldDefinition: FieldDefinition<FieldActorMetadata> = {
    objectMetadataNameSingular: 'person',
  },
 };
+
+export const linksFieldDefinition: FieldDefinition<FieldLinksMetadata> = {
+  fieldMetadataId,
+  label: 'Links',
+  iconName: 'IconLink',
+  type: FieldMetadataType.LINKS,
+  defaultValue: {
+    primaryLinkUrl: null,
+    primaryLinkLabel: null,
+    secondaryLinks: [],
+  },
+  metadata: {
+    fieldName: 'links',
+    objectMetadataNameSingular: 'company',
+  },
+};
--- a/packages/twenty-front/src/modules/object-record/record-field/meta-types/input/components/stories/LinksFieldInput.stories.tsx
+++ b/packages/twenty-front/src/modules/object-record/record-field/meta-types/input/components/stories/LinksFieldInput.stories.tsx
@ -416,3 +416,31 @@ export const Cancel: Story = {
    expect(cancelJestFn).toHaveBeenCalledTimes(1);
  },
 };
+
+export const InvalidUrls: Story = {
+  args: {
+    value: {
+      primaryLinkUrl: 'lydia,com',
+      primaryLinkLabel: 'Invalid URL',
+      secondaryLinks: [
+        { url: 'wikipedia', label: 'Missing Protocol' },
+        { url: '\\invalid', label: 'Invalid Characters' },
+      ],
+    },
+  },
+  play: async ({ canvasElement }) => {
+    const canvas = within(canvasElement);
+
+    const input = await canvas.findByPlaceholderText('URL');
+    expect(input).toBeVisible();
+    expect(input).toHaveValue('');
+
+    await waitFor(() => {
+      expect(canvas.queryByRole('link')).toBeNull();
+    });
+
+    expect(canvas.queryByText('Invalid URL')).not.toBeInTheDocument();
+    expect(canvas.queryByText('Missing Protocol')).not.toBeInTheDocument();
+    expect(canvas.queryByText('Invalid Characters')).not.toBeInTheDocument();
+  },
+};
--- a/packages/twenty-front/src/modules/object-record/record-field/meta-types/input/utils/tests/getFieldLinkDefinedLinks.test.ts
+++ b/packages/twenty-front/src/modules/object-record/record-field/meta-types/input/utils/tests/getFieldLinkDefinedLinks.test.ts
@ -158,5 +158,33 @@ describe('getFieldLinkDefinedLinks', () => {
        }),
      ).toEqual([]);
    });
+
+    it('should filter out secondary links and primary link with invalid URLs', () => {
+      expect(
+        getFieldLinkDefinedLinks({
+          primaryLinkUrl: 'lydia,com',
+          primaryLinkLabel: 'Invalid Primary',
+          secondaryLinks: [
+            {
+              url: 'lydia,com',
+              label: 'Invalid URL',
+            },
+            {
+              url: 'wikipedia',
+              label: 'Missing Protocol',
+            },
+            {
+              url: 'https://twenty.com',
+              label: 'Valid URL',
+            },
+          ],
+        }),
+      ).toEqual([
+        {
+          url: 'https://twenty.com',
+          label: 'Valid URL',
+        },
+      ]);
+    });
  });
 });
--- a/packages/twenty-front/src/modules/object-record/record-field/meta-types/input/utils/getFieldLinkDefinedLinks.ts
+++ b/packages/twenty-front/src/modules/object-record/record-field/meta-types/input/utils/getFieldLinkDefinedLinks.ts
@ -1,6 +1,6 @@
 import { FieldLinksValue } from '@/object-record/record-field/types/FieldMetadata';
 import { isNonEmptyString } from '@sniptt/guards';
-import { isDefined } from 'twenty-shared/utils';
+import { isDefined, isValidUrl } from 'twenty-shared/utils';

 export const getFieldLinkDefinedLinks = (fieldValue: FieldLinksValue) => {
  return [
@ -23,5 +23,6 @@ export const getFieldLinkDefinedLinks = (fieldValue: FieldLinksValue) => {
        label: link.label,
      };
    })
-    .filter(isDefined);
+    .filter(isDefined)
+    .filter(({ url }) => isValidUrl(url));
 };
--- a/packages/twenty-front/src/modules/object-record/record-field/types/guards/isFieldLinksValue.ts
+++ b/packages/twenty-front/src/modules/object-record/record-field/types/guards/isFieldLinksValue.ts
@ -1,15 +1,14 @@
-import { absoluteUrlSchema } from 'twenty-shared/utils';
 import { z } from 'zod';
 import { FieldLinksValue } from '../FieldMetadata';

 export const linksSchema = z.object({
  primaryLinkLabel: z.string().nullable(),
-  primaryLinkUrl: absoluteUrlSchema.or(z.string().length(0)).nullable(),
+  primaryLinkUrl: z.string().nullable(),
  secondaryLinks: z
    .array(
      z.object({
        label: z.string().nullable(),
-        url: absoluteUrlSchema.nullable(),
+        url: z.string().nullable(),
      }),
    )
    .nullable(),
--- a/packages/twenty-front/src/modules/object-record/record-field/utils/tests/isFieldValueEmpty.test.ts
+++ b/packages/twenty-front/src/modules/object-record/record-field/utils/tests/isFieldValueEmpty.test.ts
@ -2,6 +2,7 @@ import {
  booleanFieldDefinition,
  fieldMetadataId,
  fullNameFieldDefinition,
+  linksFieldDefinition,
  relationFieldDefinition,
  selectFieldDefinition,
 } from '@/object-record/record-field/__mocks__/fieldDefinitions';
@ -112,4 +113,104 @@ describe('isFieldValueEmpty', () => {
      }),
    ).toBe(false);
  });
+
+  it('should return correct value for links field', () => {
+    // Empty cases
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: null,
+          primaryLinkLabel: null,
+          secondaryLinks: [],
+        },
+      }),
+    ).toBe(true);
+
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: null,
+      }),
+    ).toBe(true);
+
+    // Valid primary link only
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: 'https://www.twenty.com',
+          primaryLinkLabel: 'Twenty Website',
+          secondaryLinks: [],
+        },
+      }),
+    ).toBe(false);
+
+    // Valid secondary link only
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: null,
+          primaryLinkLabel: null,
+          secondaryLinks: [
+            { url: 'https://docs.twenty.com', label: 'Documentation' },
+          ],
+        },
+      }),
+    ).toBe(false);
+
+    // Invalid primary link but valid secondary link
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: 'lydia,com',
+          primaryLinkLabel: 'Invalid URL',
+          secondaryLinks: [
+            { url: 'https://docs.twenty.com', label: 'Documentation' },
+          ],
+        },
+      }),
+    ).toBe(false);
+
+    // Valid primary link but invalid secondary link
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: 'https://www.twenty.com',
+          primaryLinkLabel: 'Twenty Website',
+          secondaryLinks: [{ url: 'wikipedia', label: 'Invalid URL' }],
+        },
+      }),
+    ).toBe(false);
+
+    // All invalid links
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: 'lydia,com',
+          primaryLinkLabel: 'Invalid URL',
+          secondaryLinks: [{ url: 'wikipedia', label: 'Invalid URL' }],
+        },
+      }),
+    ).toBe(true);
+
+    // Multiple secondary links with mix of valid and invalid
+    expect(
+      isFieldValueEmpty({
+        fieldDefinition: linksFieldDefinition,
+        fieldValue: {
+          primaryLinkUrl: null,
+          primaryLinkLabel: null,
+          secondaryLinks: [
+            { url: 'wikipedia', label: 'Invalid URL' },
+            { url: 'https://docs.twenty.com', label: 'Documentation' },
+          ],
+        },
+      }),
+    ).toBe(false);
+  });
 });
--- a/packages/twenty-front/src/modules/object-record/record-field/utils/isFieldValueEmpty.ts
+++ b/packages/twenty-front/src/modules/object-record/record-field/utils/isFieldValueEmpty.ts
@ -1,5 +1,6 @@
 import { isArray, isNonEmptyArray, isString } from '@sniptt/guards';

+import { getFieldLinkDefinedLinks } from '@/object-record/record-field/meta-types/input/utils/getFieldLinkDefinedLinks';
 import { FieldDefinition } from '@/object-record/record-field/types/FieldDefinition';
 import { FieldMetadata } from '@/object-record/record-field/types/FieldMetadata';
 import { isFieldActor } from '@/object-record/record-field/types/guards/isFieldActor';
@ -116,9 +117,14 @@ export const isFieldValueEmpty = ({
  }

  if (isFieldLinks(fieldDefinition)) {
-    return (
-      !isFieldLinksValue(fieldValue) || isValueEmpty(fieldValue.primaryLinkUrl)
-    );
+    if (!isFieldLinksValue(fieldValue)) {
+      return true;
+    }
+
+    const definedLinks = getFieldLinkDefinedLinks(fieldValue);
+    const isFieldLinksEmpty = definedLinks.length === 0;
+
+    return isFieldLinksEmpty;
  }

  if (isFieldActor(fieldDefinition)) {
--- a/packages/twenty-front/src/modules/ui/field/display/components/stories/LinksDisplay.stories.tsx
+++ b/packages/twenty-front/src/modules/ui/field/display/components/stories/LinksDisplay.stories.tsx
@ -180,3 +180,25 @@ export const AutomaticLabelFromURL: Story = {
    expect(secondaryLink).toHaveAttribute('href', 'https://test.example.com');
  },
 };
+
+export const InvalidLinks: Story = {
+  args: {
+    value: {
+      primaryLinkUrl: 'wikipedia',
+      primaryLinkLabel: 'Invalid URL',
+      secondaryLinks: [{ url: 'lydia,com', label: 'Invalid URL with comma' }],
+    },
+  },
+  play: async ({ canvasElement }) => {
+    const canvas = within(canvasElement);
+
+    await waitFor(() => {
+      expect(canvas.queryByRole('link')).toBeNull();
+    });
+
+    expect(canvas.queryByText('Invalid URL')).not.toBeInTheDocument();
+    expect(
+      canvas.queryByText('Invalid URL with comma'),
+    ).not.toBeInTheDocument();
+  },
+};