Migrate domainName field from text type to links type #6410

ijreilly · 2024-07-25T14:23:46Z

Closes #5759.

greptile-apps

PR Summary

The pull request migrates the domainName field from a text type to a links type, allowing for multiple domain values.

CommandMenu.tsx: Updated to use getCompanyDomainName for fetching company logos.
Company.ts: Changed domainName field to support both string and link object types.
mapFavorites.ts: Adapted to use getCompanyDomainName for avatar URLs.
0-23-migrate-domain-to-links.command.ts: Added migration command for domainName field.
quick-actions.service.ts: Updated enrichCompany method to handle new linked field type.

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

greptile-apps

PR Summary

(updates since last review)

The pull request migrates the domainName field from a text type to a links type, enhancing the ability to handle multiple domain values.

packages/twenty-server/src/database/commands/upgrade-version/0-23/0-23-migrate-domain-to-links.command.ts: Introduces a comprehensive migration command with error handling and rollback mechanisms.
packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts: Integrates TwentyORMGlobalManager for repository operations, improving data access consistency.
packages/twenty-server/src/engine/twenty-orm/factories/workspace-datasource.factory.ts: Simplifies cache version logic using nullish coalescing operator.
packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/services/workspace-sync-object-metadata-identifiers.service.ts: Adds null check for standardId to enhance metadata retrieval robustness.
packages/twenty-front/src/modules/object-record/utils/sanitizeRecordInput.ts: Removes getCompanyDomainName dependency, simplifying domainName sanitization logic.

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

Weiko · 2024-07-29T08:48:49Z

...ty-server/src/database/commands/upgrade-version/0-23/0-23-migrate-domain-to-links.command.ts

+    dataSourceMetadata: DataSourceEntity;
+  }) {
+    await workspaceQueryRunner.query(
+      `UPDATE "${dataSourceMetadata.schema}"."company" SET "${targetColumnName}" = CASE WHEN "${sourceColumnName}" LIKE 'https://%' THEN "${sourceColumnName}" ELSE 'https://' || "${sourceColumnName}" END;`,


is 'https' a requirement for LINKS fields? I'm wondering if we really want to do that otherwise? Also it might overwrite to "https://http://www.google.com" (unlikely though)

Yes I think we really want to have that https:// prefix, we add it in the front-end.
Maybe I could replace LIKE 'https://%' with LIKE 'http' to be extra safe

Weiko · 2024-07-29T08:51:41Z

...ty-server/src/database/commands/upgrade-version/0-23/0-23-migrate-domain-to-links.command.ts

+          });
+
+          // Backfill primaryLinkLabel with empty string
+          await this.backfillColumnWithEmptyString({


Do we need to set empty string? I thought this column would have a defaultValue already

Oh you are right ! I added the defaultValue logic after, so until then I needed this. removing it now thanks

Weiko · 2024-07-29T09:09:42Z

packages/twenty-server/src/utils/getCompanyDomainName.ts

@@ -0,0 +1,8 @@
+// temporary, to remove once domainName has been fully migrated to Links type
+export const getCompanyDomainName = (company: any) => {


Seems this one is not aligned with getCompanyDomainName from front package

right, thanks

Weiko · 2024-07-29T09:10:24Z

...r/src/engine/metadata-modules/field-metadata/interfaces/field-metadata-settings.interface.ts

@@ -10,6 +10,10 @@ type FieldMetadataDefaultSettings = {
  isForeignKey?: boolean;
 };

+type FieldMetadataLinksSettings = {
+  isDomain?: boolean;


Do we want to introduce this in this PR? I don't see where this is used

Right it's not actually

Weiko · 2024-07-29T09:11:21Z

packages/twenty-server/src/engine/core-modules/quick-actions/quick-actions.service.ts

@@ -159,7 +161,7 @@ export class QuickActionsService {
    }

    const enrichedData = await this.intelligenceService.enrichCompany(
-      company.domainName,
+      getCompanyNameFromDomainName(getCompanyDomainName(company.domainName)),


It used to work with domainName only, why do we want the name of the company now?

its a mistake, thanks for spotting this. (i think this service is not in use anymore by the way)

bosiraphael · 2024-07-29T10:04:33Z

packages/twenty-server/src/modules/company/repositories/company.repository.ts

      VALUES ($1, $2, $3, $4, $5)`,
      [
        companyToCreate.id,
-        companyToCreate.domainName,
+        'https://' + companyToCreate.domainName,


Why do we add 'https://' here but we don't in getExistingCompaniesByDomainNames? The get and the create should have a consistant api, either companyToCreate and domainNames should both contain https:// or they shouldn't.

I had not paid attention to that, thanks.
I am replacing the query to compare the domain names without the 'https://' or 'http://' prefix; this seems safer than adding the prefix the domainNames and comparing. (although when we migrate the existing data we are adding https:// so all existing data should have a https:// prefix but still)

yet to test though, waiting on your help on how to trigger sync again

bosiraphael · 2024-07-29T10:05:26Z

packages/twenty-server/src/engine/workspace-manager/standard-objects-prefill-data/company.ts

@@ -23,7 +23,7 @@ export const companyPrefillData = async (
    .values([
      {
        name: 'Airbnb',
-        domainName: 'airbnb.com',
+        domainNamePrimaryLinkUrl: 'airbnb.com',


https:// ?

Weiko

LGTM, great job @ijreilly! 👏

bosiraphael

LGTM

ijreilly added 3 commits July 24, 2024 17:35

Introduce domain name helper to handle both domainName types

7ed0286

Merge branch 'main' of github.com:twentyhq/twenty into migrate-domain

c3183f6

Migrate domainName text field to links type

f535704

ijreilly requested a review from bosiraphael July 25, 2024 14:23

greptile-apps bot reviewed Jul 25, 2024

View reviewed changes

ijreilly added 2 commits July 25, 2024 17:35

Merge branch 'main' of github.com:twentyhq/twenty into migrate-domain

674ba58

Backfill primaryLinkLabel

221af97

ijreilly marked this pull request as draft July 25, 2024 15:38

Fix sanitized input

5be2a3b

ijreilly marked this pull request as ready for review July 25, 2024 16:02

Add log to sync workspace metadata

f21b759

greptile-apps bot reviewed Jul 25, 2024

View reviewed changes

Fix stories

d4ddb07

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

ijreilly added 7 commits July 26, 2024 16:06

Fix sign-in and prefill data

65265de

Fix demo seeds

d7664f1

Merge branch 'main' of github.com:twentyhq/twenty into migrate-domain

5319937

fixes

cf59dea

Merge branch 'main' of github.com:twentyhq/twenty into migrate-domain

25ebb28

fix

e9e0f3f

Fix getCompanyDomainName for relations

6426534

Weiko reviewed Jul 29, 2024

View reviewed changes

Weiko added -PR: awaiting author and removed -PR: awaiting review labels Jul 29, 2024

bosiraphael reviewed Jul 29, 2024

View reviewed changes

ijreilly added 5 commits July 29, 2024 12:40

Fix issues with https:// prefix

e9be361

Merge branch 'main' of github.com:twentyhq/twenty into migrate-domain

1ddb585

Adapt tests

b1e70d1

Adapt filter to domain name

fb42a42

Handle Links composite fields at import

d94a64d

ijreilly added -PR: awaiting review and removed -PR: awaiting author labels Jul 29, 2024

Set size for new viewField

09cbdb0

Weiko approved these changes Jul 30, 2024

View reviewed changes

bosiraphael approved these changes Jul 30, 2024

View reviewed changes

ijreilly merged commit 8e35eda into main Jul 30, 2024
14 checks passed

ijreilly deleted the migrate-domain branch July 30, 2024 09:47

ijreilly mentioned this pull request Jul 31, 2024

Support for multiple values in the Email field #6261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate domainName field from text type to links type #6410

Migrate domainName field from text type to links type #6410

ijreilly commented Jul 25, 2024

greptile-apps bot left a comment

greptile-apps bot left a comment

Weiko Jul 29, 2024

ijreilly Jul 29, 2024

Weiko Jul 29, 2024

ijreilly Jul 29, 2024

Weiko Jul 29, 2024

ijreilly Jul 29, 2024

Weiko Jul 29, 2024

ijreilly Jul 29, 2024

Weiko Jul 29, 2024

ijreilly Jul 29, 2024

bosiraphael Jul 29, 2024

ijreilly Jul 29, 2024

ijreilly Jul 29, 2024

ijreilly Jul 29, 2024

bosiraphael Jul 29, 2024

Weiko left a comment

bosiraphael left a comment

		@@ -0,0 +1,8 @@
		// temporary, to remove once domainName has been fully migrated to Links type
		export const getCompanyDomainName = (company: any) => {

Migrate domainName field from text type to links type #6410

Migrate domainName field from text type to links type #6410

Conversation

ijreilly commented Jul 25, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weiko left a comment

Choose a reason for hiding this comment

bosiraphael left a comment

Choose a reason for hiding this comment