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.

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

Support for multiple values in the Phone field #6260

Closed
Tracked by #6445
Bonapara opened this issue Jul 15, 2024 · 4 comments
Closed
Tracked by #6445

Support for multiple values in the Phone field #6260

Bonapara opened this issue Jul 15, 2024 · 4 comments
Assignees
Labels
scope: back+front Issues requiring full-stack knowledge

Comments

@Bonapara
Copy link
Member

Bonapara commented Jul 15, 2024

Current Behavior

Currently, our system allows for only a single phone number to be added to a phone field. However, individuals often have multiple phone numbers, which may vary significantly. To accommodate this, we need a more flexible solution than adding multiple fields like "Phone 2, Phone 3, Phone X."

Desired Behavior

We should adapt our data structure to allow for multiple phone numbers in a single phone field, similar to our previous enhancement for multiple links. The proposed data structure is as follows:

phones {
  primaryPhone {
    number
    countryCode
  }
  additionalPhones 
}

This structure will enable us to store a primary phone and an array of additional phone numbers. The implementation should mirror the approach used in the multiple links field update.

CleanShot 2024-07-15 at 14 55 10

Figma

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=28562-74913&t=hrDkyWQ3cs5iGjno-11

@charlesBochet
Copy link
Member

Discuss with @ijreilly before tackling this one

Copy link
Contributor

gitstart-app bot commented Jul 16, 2024

Here is the GitStart Ticket for this issue: https://clients.gitstart.com/twenty/5449/tickets/TWNTY-6260

@ijreilly
Copy link
Collaborator

ijreilly commented Jul 24, 2024

Some technical hints here -

Currently we have fields of type Phone that can be used for customized fields, and a standard field of type Text called "phone" on the standard object "person". We need to migrate both the Phone type custom fields and the phone standard field to the new Phones type.

I would recommend first tackling #6261 to get familiar with the issue

  1. (PR 1) Introduce new “Phones” composite field type (similar to “Links”)
    After this step is merged it is no longer possible to create custom field with the “old” phone type, but they are still part of the created standard fields, are still displaying correctly etc.
  • Create the new field type (FieldMetadataType.Phones)
  • Handle field display (inspiration: LinksFieldDisplay)
  • Handle field update (inspiration: LinksFieldInput). inputted value (ex: +33601020304) should be stored as the phone number while the countryCode should be inferred from input using the lib react-phone-number-input that we use for the current phone field input.
  • Prevent creation of new fields with "Phone" type (example PR)
  • To test: reset of db, creation of a new workspace, creation and filling of phones...
  1. (PR 2) Update standard fields of type "Phone" to the new type
    After this step is merged, newly created workspaces will directly benefit from the new type, while the existing workspaces need to have their migration to the new type done before they can have their metadata synchronized again; so this should only be merged and deployed when we are ready to do the migrations (step 3).
  • Update standard objects' standard phone fields of type Phone. I think you only need to update person's email field here, updating type from Text to Phones. (example PR)
  1. (PR 2 - same as step 2) Write script to replace "phone" fields with "phones" fields, handling the data migration
    After this step is merged, we can run the command on all active workspaces to migrate their data (twenty team will take care of running the command).
  • Example migration script - full PR. In addition here you need to migrate the standard phone field on person from type Text to Phones.
  • We want to migrate the current value of phone field to the new fields' phones.primaryPhone. countryCode has to be backfilled.
  • This type of command is now usually added to a group of commands that need to be run to allow for the upgrade to a new version of twenty, eg upgrade:0-23. You can probably add this to a future 0.24 command as this will not be ready by the time we release 0.23. For instance for 0.23 the command to run is yarn command:prod upgrade-0.23
  • To test: migration of data, sync-metadata on migrated workspaces (command yarn command:prod workspace:sync-metadata), running command multiple times of the same workspace that initially required a migration, running command simulating errors to ensure rollback is working correctly

ijreilly pushed a commit that referenced this issue Sep 11, 2024
### Description

- This is the first PR on Phones field;


- We are introducing new field type(Phones)


- We are Forbidding creation of Phone field


- We Added support for filtering and sorting on Phones field


- We are using the same display mode as used on the Links field type
(chips), check the Domain field of the Company object


- We are also using the same logic of the link when editing the field

**How to Test**

1. Checkout to TWNTY-6260 branch
2. Reset database using "npx nx database:reset twenty-server" command
3. Add custom field of type Phones in settings/data-model

**Loom Video:**\

<https://www.loom.com/share/3c981260be254dcf851256d020a20ab0?sid=58507361-3a3b-452c-9de8-b5b1abda70ac>

### Refs

#6260

Co-authored-by: gitstart-twenty <gitstart-twenty@users.noreply.github.com>
@FelixMalfait
Copy link
Member

Closing the issue as the PR was merged. @gitstart-twenty please create a new issue if they are any followups to do! Thanks!

ijreilly added a commit that referenced this issue Sep 24, 2024
This PR was created by [GitStart](https://gitstart.com/) to address the
requirements from this ticket:
[TWNTY-6260](https://clients.gitstart.com/twenty/5449/tickets/TWNTY-6260).
This ticket was imported from:
[TWNTY-6260](#6260)

 --- 

### Description

This is the second PR on TWNTY-6260 which handles data migration of
Phone field to Phones field.\
\
How to Test?\
 Follow the below steps:

- On the main branch, 
- go to
`packages/twenty-server/src/database/typeorm-seeds/workspace/people.ts`
and change any person's phone number to a string with characters for
example: "test invalid phone", and then reset the DB.
  - reset database using `npx nx database:reset twenty-server`
- This is to make sure that invalid numbers will be handled properly. We
should use the invalid value itself to avoid removing data and see how
the behavior is on the front end. should be the same as in the main, the
display shows the invalid value, but the input is empty when you click,
and then you can update.
- Checkout to `TWNTY-6260-phone-migration` branch
- Rebuild typescript using `npx nx build twenty-server`
- Run command `yarn command:prod upgrade-0.32` to do migration
- Run both backend and frontend to see the migrated field

### Demo

- **Loom Video:**\

<https://www.loom.com/share/4b9bcb423cee447d8ad09852a83b27da?sid=ed74ecaa-0339-4575-acdc-a863e95e94fd>

### Refs

#6260

---------

Co-authored-by: gitstart-twenty <gitstart-twenty@users.noreply.github.com>
Co-authored-by: Marie Stoppa <marie.stoppa@essec.edu>
Co-authored-by: Weiko <corentin@twenty.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: back+front Issues requiring full-stack knowledge
Projects
Status: ✅ Done
Development

No branches or pull requests

5 participants