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

Create command to migrate message channel sync stage enum #6280

Merged
merged 6 commits into from
Jul 16, 2024

Conversation

bosiraphael
Copy link
Contributor

@bosiraphael bosiraphael commented Jul 16, 2024

Create command to migrate message channel sync stage enum

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

  • Added 0-22-update-message-channel-sync-stage-enum.command.ts to migrate syncStage enum for messageChannel (packages/twenty-server/src/database/commands/0-22-update-message-channel-sync-stage-enum.command.ts)
  • Updated database-command.module.ts to include UpdateMessageChannelSyncStageEnumCommand (packages/twenty-server/src/database/commands/database-command.module.ts)

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@bosiraphael bosiraphael changed the title Create command to migrate sync stage enum Create command to migrate message channel sync stage enum Jul 16, 2024
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

(updates since last review)

  • Removed FieldMetadataType.PROBABILITY from simple field types in mapFieldMetadataToGraphqlQuery function (packages/twenty-server/src/engine/api/rest/core/query-builder/utils/map-field-metadata-to-graphql-query.utils.ts)
  • Removed handling of PROBABILITY field type in computeInputFields and getTypeFromFieldMetadataType functions (packages/twenty-zapier/src/utils/computeInputFields.ts)
  • Removed PROBABILITY enum value from FieldMetadataType enum (packages/twenty-zapier/src/utils/data.types.ts)

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

if (options.workspaceId) {
workspaceIds = [options.workspaceId];
} else {
workspaceIds = (await this.workspaceRepository.find()).map(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not only run it active workspaces like we do now, for sync-metadata and other recent commands?
otherwise it will run on +8k workspaces which may take long + will fail on numerous workspaces

this.logger.log(
chalk.red(`Running command on workspace ${workspaceId} failed`),
);
throw error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

so if it fails for any workspace, the command will stop altogether. Don't we want to continue to the next workspace instead? it's what we do elsewhere

@ijreilly
Copy link
Collaborator

@bosiraphael
https://github.com/twentyhq/twenty/blob/main/packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/commands/sync-workspace-metadata.command.ts
What I find interesting

  • loops on active workspaces only (you can reuse the same util)
  • try / catch block for each workspace, to log the error then continue

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

(updates since last review)

  • Updated message channel sync stage enum
  • Replaced workspace repository with workspace status service for fetching active workspace IDs (packages/twenty-server/src/database/commands/0-22-update-message-channel-sync-stage-enum.command.ts)

No major changes found since last review.

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

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

(updates since last review)

  • Added error handling for data source metadata retrieval
  • Enhanced logging for migration process errors
  • Ensured transaction rollback on failure
  • Updated syncStage field metadata with new options
  • Incremented workspace cache version

File: packages/twenty-server/src/database/commands/0-22-update-message-channel-sync-stage-enum.command.ts

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Approved!

@charlesBochet charlesBochet merged commit 3566e0b into main Jul 16, 2024
6 checks passed
@charlesBochet charlesBochet deleted the create-command-to-migrate-sync-stage-enum branch July 16, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants