-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Transform record phone field metadata #12706
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Major refactoring of phone number handling to fix incorrect '+1' prefix being added to phone numbers when updated via API, with improved validation and transformation logic.
- Added new
transform-phones-value.util.ts
with comprehensive validation using libphonenumber-js for country codes and phone formats - Modified
RecordInputTransformerService
to properly handle phone field transformations, preventing unwanted '+1' prefix - Added integration tests in
create-one-field-metadata-phone.integration-spec.ts
covering various phone number formats and edge cases - Enhanced phone metadata types in
phones.composite-type.ts
with improved type safety and validation - Implemented proper validation in
validate-default-value-for-type.util.ts
for phone number field metadata
7 files reviewed, 10 comments
Edit PR Review Bot Settings | Greptile
...ion/metadata/suites/field-metadata/phone/create-one-field-metadata-phone.integration-spec.ts
Outdated
Show resolved
Hide resolved
...rver/src/engine/core-modules/record-transformer/services/record-input-transformer.service.ts
Outdated
Show resolved
Hide resolved
...rver/src/engine/core-modules/record-transformer/services/record-input-transformer.service.ts
Outdated
Show resolved
Hide resolved
...y-server/src/engine/metadata-modules/field-metadata/composite-types/phones.composite-type.ts
Show resolved
Hide resolved
...-server/src/engine/api/graphql/workspace-query-runner/factories/query-runner-args.factory.ts
Outdated
Show resolved
Hide resolved
...-server/src/engine/api/graphql/workspace-query-runner/factories/query-runner-args.factory.ts
Outdated
Show resolved
Hide resolved
...wenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts
Outdated
Show resolved
Hide resolved
...wenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts
Outdated
Show resolved
Hide resolved
...wenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts
Outdated
Show resolved
Hide resolved
...wenty-server/src/engine/core-modules/record-transformer/utils/transform-phones-value.util.ts
Outdated
Show resolved
Hide resolved
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:46643 This environment will automatically shut down when the PR is closed or after 5 hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Enhanced phone number validation with extensive error handling and utility functions for data cleanup and transformation.
- Added new error codes in
record-transformer.exception.ts
for specific phone validation failures like conflicting country/calling codes - Introduced
removeUndefinedFields
utility inpackages/twenty-shared/src/utils
to prevent data corruption during phone number processing - Implemented comprehensive test coverage with specific validation cases including empty payloads and international formats
- Added
recordTransformerGraphqlApiExceptionHandler
to properly surface phone-related errors to the API layer - Removed debug console.log statements throughout codebase for cleaner production code
9 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# Introduction close twentyhq#12343 Adding a transform step for any field phone in order to infer country code and calling code from the number if they're provided ## Edges cases ```ts RecordTransformerExceptionCode.INVALID_PHONE_NUMBER: RecordTransformerExceptionCode.INVALID_PHONE_COUNTRY_CODE: RecordTransformerExceptionCode.CONFLICTING_PHONE_COUNTRY_CODE: RecordTransformerExceptionCode.CONFLICTING_PHONE_CALLING_CODE: RecordTransformerExceptionCode.CONFLICTING_PHONE_CALLING_CODE_AND_COUNTRY_CODE: RecordTransformerExceptionCode.INVALID_PHONE_CALLING_CODE: RecordTransformerExceptionCode.INVALID_URL: ``` ## Coverage Note: Will handle REST api integration testing pivot and UPDATE operation later in the afternoon, critical bug appeared that I prefer handling before improving this PR coverage, also would be too many updates Note2: Haven't fuzzed all of the string inputs, would seem overkill for such a use case, to be debated ```ts PASS test/integration/metadata/suites/field-metadata/phone/create-one-field-metadata-phone.integration-spec.ts (23.609 s) Phone field metadata tests suite ✓ It should succeed create primary phone field (1397 ms) ✓ It should succeed create primary phone field with number and other information (930 ms) ✓ It should succeed create primary phone field with full international format and other information (893 ms) ✓ It should succeed create primary phone field with full international and infer other information from it but not the countryCode as its shared (825 ms) ✓ It should succeed create primary phone field with full international and infer other information from it (818 ms) ✓ It should succeed create primary phone field with empty payload (827 ms) ✓ It should succeed create additional phone field with number and other information (894 ms) ✓ It should succeed create additional phone field with full international format and other information (1024 ms) ✓ It should succeed create additional phone field with full international and infer other information from it but not the countryCode as its shared (808 ms) ✓ It should succeed create additional phone field with full international and infer other information from it (751 ms) ✓ It should succeed create additional phone field with empty payload (739 ms) ✓ It should fail to create primary phone field without country or calling code at all (776 ms) ✓ It should fail to create primary phone field with invalid country code (782 ms) ✓ It should fail to create primary phone field with invalid calling code (858 ms) ✓ It should fail to create primary phone field with conflicting country code and calling code (872 ms) ✓ It should fail to create primary phone field with invalid phone number format (1489 ms) ✓ It should fail to create primary phone field with conflicting phone number country code (1425 ms) ✓ It should fail to create primary phone field with conflicting phone number calling code (1553 ms) ✓ It should fail to create primary phone field without country or calling code at all (814 ms) ✓ It should fail to create primary phone field with invalid country code (813 ms) ✓ It should fail to create primary phone field with invalid calling code (742 ms) ✓ It should fail to create primary phone field with conflicting country code and calling code (783 ms) ✓ It should fail to create primary phone field with invalid phone number format (731 ms) ✓ It should fail to create primary phone field with conflicting phone number country code (947 ms) ✓ It should fail to create primary phone field with conflicting phone number calling code (822 ms) Test Suites: 1 passed, 1 total Tests: 25 passed, 25 total Snapshots: 14 passed, 14 total Time: 23.627 s ```
Introduction
close #12343
Adding a transform step for any field phone in order to infer country code and calling code from the number if they're provided
Edges cases
Coverage
Note: Will handle REST api integration testing pivot and UPDATE operation later in the afternoon, critical bug appeared that I prefer handling before improving this PR coverage, also would be too many updates
Note2: Haven't fuzzed all of the string inputs, would seem overkill for such a use case, to be debated