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

Migrate fields of deprecated type LINK to type LINKS #6332

Merged
merged 12 commits into from
Jul 23, 2024
Merged

Conversation

ijreilly
Copy link
Collaborator

@ijreilly ijreilly commented Jul 19, 2024

Closes #5909.

Adding a command to migrate fields of type Link to fields of type Links, including their data.

if (position > acc) {
return position;
}
if (!createdFieldIsAlreadyInView) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this as we have no constraint elsewhere to forbid adding an already present field in a view - not sure if there is a usecase where that would make sense?
on my side I needed this in case of a rollback where we would restore the initial field by creating it again which would trigger the field's addition to the view, while it was still present in the view (from when it was originally created as we don't delete fields from viewFields when we delete the field - not sure why we don't do it?), creating a duplicated presence in the view.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK adding the same field twice is currently not possible on frontend side. I guess that means we do not have any use case.

About your second question, I do we still see the viewFields when we delete le field currently? I don't get how this would work

@ijreilly ijreilly changed the title Migrate link Migrate fields of deprecated type LINK to type LINKS Jul 19, 2024
@ijreilly ijreilly marked this pull request as ready for review July 19, 2024 17:45
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

  • Enhanced handling of view fields in packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata.service.ts
  • Added check to prevent duplicate field entries in views
  • Used lodash.isempty to verify view existence
  • Imported ViewFieldWorkspaceEntity for typecasting view fields

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

_passedParam: string[],
options: MigrateLinkFieldsToLinksCommandOptions,
): Promise<void> {
this.logger.log('running');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a more explicit log?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or remove it since this is bellow already

if (position > acc) {
return position;
}
if (!createdFieldIsAlreadyInView) {
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK adding the same field twice is currently not possible on frontend side. I guess that means we do not have any use case.

About your second question, I do we still see the viewFields when we delete le field currently? I don't get how this would work

private readonly logger = new Logger(ViewService.name);
constructor(private readonly twentyORMManager: TwentyORMManager) {}

async addFieldToViewsContainingOldField({
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO functions in services should be more generic. Input should be either:

  • addFieldToViews(workspaceId, newFieldId, viewFilter)
    - addFieldToViews(workspaceId, newFieldId, views). Then you will need another function findViews(workspaceId, filter)

Copy link
Contributor

@thomtrp thomtrp left a comment

Choose a reason for hiding this comment

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

🔥

@ijreilly ijreilly merged commit 8d33264 into main Jul 23, 2024
6 checks passed
@ijreilly ijreilly deleted the migrate-link branch July 23, 2024 07:57
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.

[Timebox] Migrate Link Field Type to the new Links field type
2 participants