Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Phone country code unique #backend #9035

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Phone country code unique #backend #9035

wants to merge 21 commits into from

Conversation

guillim
Copy link
Contributor

@guillim guillim commented Dec 12, 2024 •
edited
Loading

fix #8775

@guillim guillim self-assigned this Dec 12, 2024
@guillim guillim linked an issue Dec 12, 2024 that may be closed by this pull request
Phone country codes not unique #8775
Open
@guillim guillim changed the title Phone country code unique Phone country code unique #backend Dec 17, 2024
@guillim guillim marked this pull request as ready for review December 17, 2024 13:04
greptile-apps[bot]
greptile-apps bot reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds functionality to handle phone country codes and calling codes separately, addressing the issue where multiple countries can share the same calling code (e.g., +1 for US and Canada).

  • Added new primaryPhoneCallingCode field across the system to store international dialing codes (e.g., '+33'), while primaryPhoneCountryCode now stores ISO country codes (e.g., 'FR')
  • Created migration command PhoneCallingCodeCommand that converts existing calling codes to proper country codes and backfills the new calling code field
  • Added validation and default value handling for the new calling code field in metadata definitions and DTOs
  • Updated GraphQL queries, OpenAPI specs, and mock data to support the new phone number structure
  • Ensured migration runs before metadata sync to maintain data consistency during upgrade

13 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile

packages/twenty-server/src/database/commands/database-command.module.ts Outdated Show resolved Hide resolved
.../twenty-server/src/database/commands/upgrade-version/0-40/0-40-phone-calling-code.command.ts Outdated Show resolved Hide resolved
.../twenty-server/src/database/commands/upgrade-version/0-40/0-40-phone-calling-code.command.ts Show resolved Hide resolved
.../twenty-server/src/database/commands/upgrade-version/0-40/0-40-phone-calling-code.command.ts Show resolved Hide resolved
packages/twenty-server/src/engine/api/__mocks__/object-metadata-item.mock.ts Show resolved Hide resolved
packages/twenty-server/src/database/typeorm-seeds/workspace/people.ts Show resolved Hide resolved
packages/twenty-server/src/database/typeorm-seeds/metadata/fieldsMetadata.ts Outdated Show resolved Hide resolved
packages/twenty-server/src/database/typeorm-seeds/metadata/fieldsMetadata.ts Show resolved Hide resolved
...ages/twenty-server/src/engine/core-modules/open-api/utils/__tests__/components.utils.spec.ts Show resolved Hide resolved
packages/twenty-server/src/engine/core-modules/open-api/utils/components.utils.ts Show resolved Hide resolved
@github-actions GitHub Actions

This comment was marked as spam.

Sign in to view
Weiko
Weiko reviewed Dec 18, 2024
View reviewed changes
.../src/database/commands/upgrade-version/0-40/0-40-phone-calling-code-create-column.command.ts
where: {
workspaceId,
type: FieldMetadataType.PHONES,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can load objectMetadata as a relation here to avoid making N queries in the loop below

.../src/database/commands/upgrade-version/0-40/0-40-phone-calling-code-create-column.command.ts

for (const phoneFieldMetadata of phonesFieldMetadata) {
if (
isDefined(phoneFieldMetadata) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplified

if (isDefined(phoneFieldMetadata?.name))

.../src/database/commands/upgrade-version/0-40/0-40-phone-calling-code-create-column.command.ts
isDefined(phoneFieldMetadata) &&
isDefined(phoneFieldMetadata.name)
) {
const [objectMetadata] = await this.objectMetadataRepository.find({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, once the relation is loaded you can directly do

phoneFieldMetadata.objectMetadata

to retrieve the objectMetadata

.../src/database/commands/upgrade-version/0-40/0-40-phone-calling-code-create-column.command.ts
`P1 Step 1 - Let's find the "nameSingular" of this objectMetadata: ${objectMetadata.nameSingular || 'not found'}`,
);

if (!objectMetadata || !objectMetadata.nameSingular) continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!objectMetadata || !objectMetadata.nameSingular) continue;
if (!objectMetadata?.nameSingular) continue;

Although you probably won't need this check once you'll load the object from field

.../src/database/commands/upgrade-version/0-40/0-40-phone-calling-code-create-column.command.ts
workspaceId: workspaceId,
isNullable: true,
defaultValue: "''",
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
},
} satisfies Partial<FieldMetadataEntity>,

...nty-front/src/modules/settings/data-model/fields/preview/utils/getPhonesFieldPreviewValue.ts
return getCountries().includes(str as CountryCode);
};

export const countryCodeToCallingCode = (countryCode: string): string => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

export const countryCodeToCallingCode = (countryCode: string): string => {
  if (!countryCode || !isStrCountryCodeGuard(countryCode)) {
    return `+${DEFAULT_PHONE_CALLING_CODE}`;
  }

  const callingCode = getCountryCallingCode(countryCode);

  return callingCode ? `+${callingCode}` : `+${DEFAULT_PHONE_CALLING_CODE}`;
};

Can we use if blocks in general? I think it's more readable this way

...ront/src/modules/object-record/record-field/meta-types/input/components/PhonesFieldInput.tsx
@@ -60,22 +59,23 @@ export const PhonesFieldInput = ({

const phones = createPhonesFromFieldValue(fieldValue);

const defaultCallingCode =
const defaultCountry =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, why are we mixing calling code and country code there?

...ules/settings/data-model/fields/forms/phones/components/SettingsDataModelFieldPhonesForm.tsx
@@ -37,12 +41,13 @@ export const SettingsDataModelFieldPhonesForm = ({
.sort((a, b) => a.countryName.localeCompare(b.countryName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

  const countries = [
    { label: 'No country', value: '' },
    ...useCountries()
      .sort((a, b) => a.countryName.localeCompare(b.countryName))
      .map((country) => ({
        label: `${country.countryName} (+${country.callingCode})`,
        value: country.countryCode,
      })),
  ];

then you don't need the unshift below. Wdyt?

...ules/settings/data-model/fields/forms/phones/components/SettingsDataModelFieldPhonesForm.tsx
@@ -73,6 +78,11 @@ export const SettingsDataModelFieldPhonesForm = ({
...value,
primaryPhoneCountryCode:
applySimpleQuotesToString(newPhoneCountryCode),
primaryPhoneCallingCode: applySimpleQuotesToString(
countryCodeToCallingCode(
newPhoneCountryCode as CountryCode,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure this type assertion is bringing much value

.../twenty-server/src/database/commands/upgrade-version/0-40/0-40-phone-calling-code.command.ts
const records = await repository.find();

for (const record of records) {
// this.logger.log( `P1 Step 2 - obj.nameSingular: ${objectMetadata.nameSingular} (objId: ${objectMetadata.id}) - record.id: ${record.id} - phoneFieldMetadata.name: ${phoneFieldMetadata.name}` );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

Weiko
Weiko reviewed Dec 18, 2024
View reviewed changes
Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments! Also I think it would be nice to implement dryRun in your commands so we can safely test them 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@Weiko Weiko Weiko left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@guillim guillim

Labels
-PR: awaiting author -PR: wip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Phone country codes not unique
3 participants
@guillim @Weiko @charlesBochet