Deprecate address standard field #6087
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Updated 'Address' field type in
/packages/twenty-front/src/modules/sign-in-background-mock/constants/SignInBackgroundMockColumnDefinitions.tsand/packages/twenty-front/src/modules/sign-in-background-mock/constants/SignInBackgroundMockFilterDefinitions.ts - Added
isDeprecatedproperty toWorkspaceFieldMetadataArgsin/packages/twenty-server/src/engine/twenty-orm/interfaces/workspace-field-metadata-args.interface.ts - Introduced
WorkspaceIsDeprecateddecorator in/packages/twenty-server/src/engine/twenty-orm/decorators/workspace-is-deprecated.decorator.ts - Deprecated 'address' field in company prefill data in
/packages/twenty-server/src/engine/workspace-manager/standard-objects-prefill-data/company.ts - Modified synchronization logic to handle deprecated fields in
/packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/services/workspace-sync-field-metadata.service.ts
17 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
| @@ -108,10 +114,6 @@ export class WorkspaceFieldComparator { | |||
| const findField = ( | |||
| field: ComputedPartialFieldMetadata | FieldMetadataEntity, | |||
| ) => { | |||
| if (field.isCustom) { | |||
There was a problem hiding this comment.
I don't understand what this was for, we were comparing name with ids (fieldName is an id)
There was a problem hiding this comment.
It's because custom fields don't have standard ids so I guess the comparison was made with field.name instead.
It's more confusing to have field.standardId === fieldName since as you said one is an id and the other one is a text 🤔 (Maybe @magrinj has more context on this part, I'm wondering if we really can remove this code. I'll double check on my side)
| /** | ||
| * Create field metadata | ||
| */ | ||
| const createdFieldMetadataCollection = await fieldMetadataRepository.save( |
There was a problem hiding this comment.
Inverted create and update because when deprecating a field and creating a new one with the deprecated field's former name, we first need the deprecated field to be renamed before we can create a new one with the previous name
ijreilly
changed the title
| @@ -41,7 +41,7 @@ export class EntityEventsToDbListener { | |||
| // .... | |||
|
|
|||
| private async handle(payload: ObjectRecordBaseEvent) { | |||
| if (!payload.objectMetadata.isAuditLogged) { | |||
| if (!payload.objectMetadata?.isAuditLogged) { | |||
There was a problem hiding this comment.
Mmm, objectMetadata should always be defined I believe, did you have an issue locally?
There was a problem hiding this comment.
Yes I had it, but I think it was when there was an error with the refacto that was fixed afterwards. I will remove it
...c/engine/workspace-manager/workspace-sync-metadata/comparators/workspace-field.comparator.ts
Outdated
Show resolved
Hide resolved
| @@ -108,10 +114,6 @@ export class WorkspaceFieldComparator { | |||
| const findField = ( | |||
| field: ComputedPartialFieldMetadata | FieldMetadataEntity, | |||
| ) => { | |||
| if (field.isCustom) { | |||
There was a problem hiding this comment.
It's because custom fields don't have standard ids so I guess the comparison was made with field.name instead.
It's more confusing to have field.standardId === fieldName since as you said one is an id and the other one is a text 🤔 (Maybe @magrinj has more context on this part, I'm wondering if we really can remove this code. I'll double check on my side)
| @WorkspaceField({ | ||
| standardId: COMPANY_STANDARD_FIELD_IDS.address_deprecated, | ||
| type: FieldMetadataType.TEXT, | ||
| label: 'Address (deprecated) ', |
There was a problem hiding this comment.
@Bonapara @FelixMalfait are you ok with this? or would you want a different naming? (it's the label so they can still change it again)
There was a problem hiding this comment.
Address (Deprecated) seems good to me! Ideally, I wonder if we could also prepend the field description with:
"This standard field has been deprecated and migrated as a custom field. Please consider using the new 'address' field type."
Fix a small bug introduced in #6087 preventing database reset
Closes #5916