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

[Emails migration] Fix email field migration #7065

Merged
merged 5 commits into from
Sep 16, 2024
Merged

Conversation

ijreilly
Copy link
Collaborator

Fix email field migration

  • Remove deprecated field of type Email
  • Add standard emails field on person to person views in position 4

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 pull request addresses issues with email field migration in the Twenty application, focusing on improving the robustness of the process and ensuring proper data handling.

  • Introduces new FixEmailFieldsToEmailsCommand to remove deprecated email fields and add the new 'emails' field to person views
  • Updates UpgradeTo0_30Command to include the new fix command in the upgrade process
  • Adds getViewsIdsForObjectMetadataId method to ViewService for retrieving view IDs associated with specific object metadata
  • Implements error handling and logging throughout the migration process for better debugging and monitoring
  • Ensures compatibility with the new email field structure across workspaces

4 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings

Comment on lines 75 to 77
workspaceQueryRunner = workspaceDataSource.createQueryRunner();

await workspaceQueryRunner.connect();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider wrapping the query runner in a try-finally block to ensure it's always released, even if an error occurs during connection.

Comment on lines +151 to +158
positions: personViewsIds.reduce((acc, personView) => {
if (!personView.id) {
return acc;
}
acc[personView.id] = 4;

return acc;
}, []),
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: This reduce function initializes 'acc' as an array but uses it as an object. Consider initializing it as an object: personViewsIds.reduce((acc, personView) => { ... }, {})

});
this.logger.log(chalk.green(`Added emails to view ${workspaceId}.`));
} catch (error) {
await workspaceQueryRunner.release();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: workspaceQueryRunner is released here and again in the finally block. Consider removing this line to avoid potential errors.

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.

LGTM

@charlesBochet charlesBochet merged commit dcac832 into main Sep 16, 2024
6 checks passed
@charlesBochet charlesBochet deleted the fix-email-migration branch September 16, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants