Add new Address field to views containing deprecated address #6205
Conversation
…to command-add-view
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added new Address field to views with deprecated address fields (
packages/twenty-server/src/database/commands/0-22-add-new-address-field-to-views-with-deprecated-address.command.ts) - Updated mock GraphQL queries and tests to include detailed address components (
packages/twenty-front/src/modules/favorites/hooks/__mocks__/useFavorites.ts,packages/twenty-front/src/modules/object-metadata/utils/__tests__/mapFieldMetadataToGraphQLQuery.test.tsx) - Modified metadata and decorators to support deprecated fields (
packages/twenty-server/src/engine/twenty-orm/decorators/workspace-field.decorator.ts,packages/twenty-server/src/engine/twenty-orm/decorators/workspace-is-deprecated.decorator.ts) - Updated test cases to reflect new address structure (
packages/twenty-server/test/company.e2e-spec.ts,packages/twenty-zapier/src/test/creates/crud_record.test.ts) - Ensured new Address field is displayed alongside deprecated field for user visibility (
packages/twenty-front/src/modules/object-record/hooks/__mocks__/useFindManyRecords.ts,packages/twenty-front/src/modules/sign-in-background-mock/constants/SignInBackgroundMockColumnDefinitions.ts)
28 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Reordered import statements for better readability (
packages/twenty-server/src/engine/api/graphql/workspace-query-runner/listeners/entity-events-to-db.listener.ts) - Renamed
isDeprecatedFieldtoshouldSkipFieldCreationfor clarity (packages/twenty-server/src/engine/workspace-manager/workspace-sync-metadata/comparators/workspace-field.comparator.ts)
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
| const activeWorkspaceIds = ( | ||
| await Promise.all( | ||
| workspaces.map(async (workspace) => { | ||
| const isActive = await this.workspaceIsActive(workspace); |
There was a problem hiding this comment.
What is the downside of performing the change on inactive workspaces besides the time to run this query? I'd say that as long as they have an existing workspace schema we should apply the migration. As it might be re-enabled?
There was a problem hiding this comment.
The downside is that inactive workspaces are not being synchronized with sync-metadata anymore, and may have subsequent issues making the command hard to apply or not applicable on them. We had the issues yesterday when running the command for the boolean fields on all the workspaces (= +8k while we have 375 active ones), at first I started fixing them but it was too time consuming and mostly time wasted on unused workspaces, I ended up only running the command for the active workspaces which was a bit painful too (I needed to run individual commands for each workspace id then to to execute lines 40 by 40)
|
|
||
| if (workspaceDataSource) { | ||
| try { | ||
| const newAddressField = await this.fieldMetadataRepository.findBy({ |
There was a problem hiding this comment.
It feels like you're close to have built a generic command.
Imo (opt1) is to make it fully generic and pass all params as options to the command, (opt2) is to isolate all variables at the very top of the script and make the script generic after that. Maybe (opt2) is good enough for now? We'll probably move towards (opt1) in a not-so-distant future
There was a problem hiding this comment.
yes good point! I will also take note of that on the doc
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced command to add new Address field to views with deprecated address (
packages/twenty-server/src/database/commands/0-22-add-new-address-field-to-views-with-deprecated-address.command.ts) - Iterates through workspaces to update views
- Includes logging for traceability and error handling
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added new
Addressfield inpackages/twenty-server/src/modules/company/standard-objects/company.workspace-entity.ts - Updated description of deprecated
Addressfield inpackages/twenty-server/src/modules/company/standard-objects/company.workspace-entity.ts
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced command to add new Address field to views with deprecated address field (
packages/twenty-server/src/database/commands/0-22-add-new-address-field-to-views-with-deprecated-address.command.ts) - Ensures workspace is active before updates
- Increments workspace cache version after updates
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
as per title, following introduction of new Address field, we want to display the new field next to the deprecated field, for users to notice the new field.