-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: when field metadata SELECT type is edited update view groups #8344
Changes from 3 commits
6c67f58
6422e42
de7b154
30be593
caeb443
c365275
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,13 +21,15 @@ import { | |
FieldMetadataException, | ||
FieldMetadataExceptionCode, | ||
} from 'src/engine/metadata-modules/field-metadata/field-metadata.exception'; | ||
import { FieldMetadataRelatedRecordsService } from 'src/engine/metadata-modules/field-metadata/services/field-metadata-related-records.service'; | ||
import { assertDoesNotNullifyDefaultValueForNonNullableField } from 'src/engine/metadata-modules/field-metadata/utils/assert-does-not-nullify-default-value-for-non-nullable-field.util'; | ||
import { | ||
computeColumnName, | ||
computeCompositeColumnName, | ||
} from 'src/engine/metadata-modules/field-metadata/utils/compute-column-name.util'; | ||
import { generateNullable } from 'src/engine/metadata-modules/field-metadata/utils/generate-nullable'; | ||
import { isCompositeFieldMetadataType } from 'src/engine/metadata-modules/field-metadata/utils/is-composite-field-metadata-type.util'; | ||
import { isSelectFieldMetadataType } from 'src/engine/metadata-modules/field-metadata/utils/is-select-field-metadata-type.util'; | ||
import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity'; | ||
import { assertMutationNotOnRemoteObject } from 'src/engine/metadata-modules/object-metadata/utils/assert-mutation-not-on-remote-object.util'; | ||
import { | ||
|
@@ -83,6 +85,7 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit | |
private readonly workspaceMetadataVersionService: WorkspaceMetadataVersionService, | ||
private readonly twentyORMGlobalManager: TwentyORMGlobalManager, | ||
private readonly fieldMetadataValidationService: FieldMetadataValidationService, | ||
private readonly fieldMetadataRelatedRecordsService: FieldMetadataRelatedRecordsService, | ||
) { | ||
super(fieldMetadataRepository); | ||
} | ||
|
@@ -311,7 +314,7 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit | |
FieldMetadataEntity, | ||
); | ||
|
||
const [existingFieldMetadata] = await fieldMetadataRepository.find({ | ||
const existingFieldMetadata = await fieldMetadataRepository.findOne({ | ||
where: { | ||
id, | ||
workspaceId: fieldMetadataInput.workspaceId, | ||
|
@@ -325,7 +328,7 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit | |
); | ||
} | ||
|
||
const [objectMetadata] = await this.objectMetadataRepository.find({ | ||
const objectMetadata = await this.objectMetadataRepository.findOne({ | ||
where: { | ||
id: existingFieldMetadata.objectMetadataId, | ||
workspaceId: fieldMetadataInput.workspaceId, | ||
|
@@ -418,6 +421,18 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit | |
); | ||
} | ||
|
||
console.log('updatedFieldMetadata', updatedFieldMetadata); | ||
if ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Remove console.log statements before merging to production |
||
updatedFieldMetadata.isActive && | ||
isSelectFieldMetadataType(updatedFieldMetadata.type) | ||
) { | ||
console.log('updateRelatedViewGroups'); | ||
await this.fieldMetadataRelatedRecordsService.updateRelatedViewGroups( | ||
existingFieldMetadata, | ||
updatedFieldMetadata, | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider wrapping updateRelatedViewGroups in try/catch to handle potential errors gracefully |
||
|
||
if ( | ||
fieldMetadataInput.name || | ||
updatableFieldInput.options || | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,158 @@ | ||
import { Injectable } from '@nestjs/common'; | ||
|
||
import { In } from 'typeorm'; | ||
|
||
import { | ||
FieldMetadataComplexOption, | ||
FieldMetadataDefaultOption, | ||
} from 'src/engine/metadata-modules/field-metadata/dtos/options.input'; | ||
import { FieldMetadataEntity } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; | ||
import { isSelectFieldMetadataType } from 'src/engine/metadata-modules/field-metadata/utils/is-select-field-metadata-type.util'; | ||
import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager'; | ||
import { ViewGroupWorkspaceEntity } from 'src/modules/view/standard-objects/view-group.workspace-entity'; | ||
import { ViewWorkspaceEntity } from 'src/modules/view/standard-objects/view.workspace-entity'; | ||
|
||
type Differences<T> = { | ||
created: T[]; | ||
updated: { old: T; new: T }[]; | ||
deleted: T[]; | ||
}; | ||
|
||
@Injectable() | ||
export class FieldMetadataRelatedRecordsService { | ||
constructor( | ||
private readonly twentyORMGlobalManager: TwentyORMGlobalManager, | ||
) {} | ||
|
||
public async updateRelatedViewGroups( | ||
oldFieldMetadata: FieldMetadataEntity, | ||
newFieldMetadata: FieldMetadataEntity, | ||
) { | ||
if ( | ||
!isSelectFieldMetadataType(newFieldMetadata.type) || | ||
!isSelectFieldMetadataType(oldFieldMetadata.type) | ||
) { | ||
return; | ||
} | ||
|
||
const view = await this.getFieldMetadataView(newFieldMetadata); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider wrapping all database operations in a transaction to ensure atomicity |
||
|
||
const { created, updated, deleted } = this.getOptionsDifferences( | ||
oldFieldMetadata.options, | ||
newFieldMetadata.options, | ||
); | ||
|
||
const viewGroupRepository = | ||
await this.twentyORMGlobalManager.getRepositoryForWorkspace<ViewGroupWorkspaceEntity>( | ||
newFieldMetadata.workspaceId, | ||
'viewGroup', | ||
); | ||
|
||
const maxPosition = view.viewGroups.reduce( | ||
(max, viewGroup) => Math.max(max, viewGroup.position), | ||
0, | ||
); | ||
|
||
/** | ||
* Create new view groups for the new options | ||
*/ | ||
const viewGroupsToCreate = created.map((option, index) => | ||
viewGroupRepository.create({ | ||
fieldMetadataId: newFieldMetadata.id, | ||
fieldValue: option.value, | ||
position: maxPosition + index, | ||
isVisible: true, | ||
viewId: view.id, | ||
}), | ||
); | ||
|
||
await viewGroupRepository.insert(viewGroupsToCreate); | ||
|
||
/** | ||
* Update existing view groups for the updated options | ||
*/ | ||
for (const { old: oldOption, new: newOption } of updated) { | ||
const viewGroup = view.viewGroups.find( | ||
(viewGroup) => viewGroup.fieldValue === oldOption.value, | ||
); | ||
|
||
if (!viewGroup) { | ||
throw new Error('View group not found'); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Error should include more context like fieldMetadataId and fieldValue to help debugging |
||
|
||
await viewGroupRepository.update( | ||
{ | ||
id: viewGroup.id, | ||
}, | ||
{ | ||
fieldValue: newOption.value, | ||
}, | ||
); | ||
} | ||
|
||
/** | ||
* Delete view groups for the deleted options | ||
*/ | ||
const valuesToDelete = deleted.map((option) => option.value); | ||
|
||
await viewGroupRepository.delete({ | ||
fieldMetadataId: newFieldMetadata.id, | ||
fieldValue: In(valuesToDelete), | ||
}); | ||
} | ||
|
||
private getOptionsDifferences( | ||
oldOptions: (FieldMetadataDefaultOption | FieldMetadataComplexOption)[], | ||
newOptions: (FieldMetadataDefaultOption | FieldMetadataComplexOption)[], | ||
) { | ||
const differences: Differences< | ||
FieldMetadataDefaultOption | FieldMetadataComplexOption | ||
> = { | ||
created: [], | ||
updated: [], | ||
deleted: [], | ||
}; | ||
|
||
const oldOptionsMap = new Map( | ||
oldOptions.map((option) => [option.id, option]), | ||
); | ||
const newOptionsMap = new Map( | ||
newOptions.map((option) => [option.id, option]), | ||
); | ||
|
||
for (const newOption of newOptions) { | ||
const oldOption = oldOptionsMap.get(newOption.id); | ||
|
||
if (!oldOption) { | ||
differences.created.push(newOption); | ||
} else if (oldOption.value !== newOption.value) { | ||
differences.updated.push({ old: oldOption, new: newOption }); | ||
} | ||
} | ||
|
||
for (const oldOption of oldOptions) { | ||
if (!newOptionsMap.has(oldOption.id)) { | ||
differences.deleted.push(oldOption); | ||
} | ||
} | ||
|
||
return differences; | ||
} | ||
|
||
private async getFieldMetadataView( | ||
fieldMetadata: FieldMetadataEntity, | ||
): Promise<ViewWorkspaceEntity> { | ||
const viewRepository = | ||
await this.twentyORMGlobalManager.getRepositoryForWorkspace<ViewWorkspaceEntity>( | ||
fieldMetadata.workspaceId, | ||
'view', | ||
); | ||
|
||
return await viewRepository.findOneOrFail({ | ||
where: { | ||
kanbanFieldMetadataId: fieldMetadata.id, | ||
}, | ||
relations: ['viewGroups'], | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Add error handling if view is not found for the given kanbanFieldMetadataId |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { FieldMetadataType } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; | ||
|
||
export const isSelectFieldMetadataType = ( | ||
type: FieldMetadataType, | ||
): type is FieldMetadataType.SELECT => { | ||
return type === FieldMetadataType.SELECT; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope this is not equivalent :p typeORM makes two queries for a findOne and one for find!