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

Add delete name column from standard object tables #7470

Merged

Conversation

Weiko
Copy link
Member

@Weiko Weiko commented Oct 7, 2024

Following this #7428 we now need to fix existing workspaces thanks to this migration command.

This command will fetch all standard objects and their fields then filter out tables that don't have that column OR objects that have an existing fieldMetadata "name" of type TEXT and delete the rest.

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 introduces a new command to delete the 'name' column from standard object tables in existing workspaces as part of the upgrade process to version 0.31.

  • Added DeleteNameColumnStandardObjectTablesCommand in packages/twenty-server/src/database/commands/upgrade-version/0-31/0-31-delete-name-column-standard-object-tables.command.ts
  • Integrated new command into UpgradeTo0_31Command execution flow in packages/twenty-server/src/database/commands/upgrade-version/0-31/0-31-upgrade-version.command.ts
  • Updated UpgradeTo0_31CommandModule to include the new command in packages/twenty-server/src/database/commands/upgrade-version/0-31/0-31-upgrade-version.module.ts
  • Command checks for existing 'name' column and field metadata before deletion, with dry run option and error handling

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

Comment on lines +72 to +75
const nameColumnExists = await queryRunner?.hasColumn(
standardObject.nameSingular,
'name',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Ensure queryRunner is not null before using optional chaining

Copy link
Collaborator

@ijreilly ijreilly left a comment

Choose a reason for hiding this comment

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

lgtm !

continue;
}

if (!nameColumnExists) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a usecase in which we expect to have the fieldMetadata but not the column? or would that just not be our problem here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point however that shouldn't be possible or at least that should be part of the sync-metadata health-check (which is broken though...)

@Weiko Weiko merged commit 7bdbbcf into main Oct 7, 2024
7 of 8 checks passed
@Weiko Weiko deleted the c--add-delete-name-column-standard-object-tables-command branch October 7, 2024 13:20
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
Following this twentyhq#7428 we now need
to fix existing workspaces thanks to this migration command.

This command will fetch all standard objects and their fields then
filter out tables that don't have that column OR objects that have an
existing fieldMetadata "name" of type TEXT and delete the rest.
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.

2 participants