-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Update unique fields on standard field - include soft deleted records #14562
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
base: main
Are you sure you want to change the base?
Conversation
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:11057 This environment will automatically shut down when the PR is closed or after 5 hours. |
5aa7acc
to
47ae5b0
Compare
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.
Greptile Overview
Summary
This PR fixes a critical issue with CSV uploads when mapping multiple unique identifiers by updating the unique field constraint logic to include soft-deleted records.
Key Changes:
- Database indexes: Removed
"deletedAt" IS NULL
constraint from unique indexes so soft-deleted records are included in uniqueness checks - Upsert logic: Enhanced GraphQL resolver to restore soft-deleted records by setting
deletedAt: null
during upserts - Contact/company creation: Added restoration logic for soft-deleted contacts and companies instead of treating them as conflicts
- Database migration: Added v1.7 upgrade command to deduplicate existing unique field violations
- Testing: Comprehensive integration tests covering soft-delete restoration scenarios
The changes ensure that when users upload CSV data with existing unique identifiers from soft-deleted records, those records are restored rather than causing constraint violations.
Confidence Score: 4/5
- This PR is generally safe to merge with minor syntax issues to address
- Score reflects solid implementation of soft-delete restoration logic with comprehensive testing, but contains syntax errors that should be fixed before merge
- packages/twenty-server/src/database/commands/upgrade-version-command/1-7/1-7-deduplicate-unique-fields.command.ts needs syntax fixes
Important Files Changed
File Analysis
Filename | Score | Overview |
---|---|---|
packages/twenty-server/src/engine/workspace-manager/workspace-migration-builder/factories/utils/workspace-migration-index.factory.utils.ts | 5/5 | Removed "deletedAt" IS NULL constraint from unique indexes to include soft-deleted records in uniqueness checks |
packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts | 4/5 | Enhanced upsert logic to restore soft-deleted records by setting deletedAt: null and updated query to include soft-deleted records |
packages/twenty-server/src/modules/contact-creation-manager/services/create-company.service.ts | 4/5 | Added restoration logic for soft-deleted companies, including filtering and updating deleted companies with deletedAt: null |
packages/twenty-server/src/database/commands/upgrade-version-command/1-7/1-7-deduplicate-unique-fields.command.ts | 3/5 | New command to deduplicate unique fields for existing data, handling workspace members, companies and people with duplicate constraints |
Sequence Diagram
sequenceDiagram
participant U as User
participant API as GraphQL API
participant R as CreateManyResolver
participant DB as Database
participant CS as ContactService
participant COS as CompanyService
U->>API: CSV Upload with unique identifiers
API->>R: createMany with upsert: true
R->>DB: Query existing records (including soft-deleted)
DB->>R: Return existing records
alt Record exists and is soft-deleted
R->>DB: Update record with deletedAt: null
DB->>R: Restored record
else Record exists and not deleted
R->>DB: Update existing record
DB->>R: Updated record
else Record doesn't exist
R->>DB: Insert new record
DB->>R: New record
end
alt Contact creation workflow
CS->>DB: Find soft-deleted contacts by email
DB->>CS: Soft-deleted contacts
CS->>COS: Create/restore companies for contacts
COS->>DB: Restore soft-deleted companies
DB->>COS: Restored companies
CS->>DB: Restore contacts with company associations
DB->>CS: Restored contacts
end
R->>API: Return processed records
API->>U: Success response
15 files reviewed, 4 comments
...r/src/database/commands/upgrade-version-command/1-7/1-7-deduplicate-unique-fields.command.ts
Outdated
Show resolved
Hide resolved
...r/src/database/commands/upgrade-version-command/1-7/1-7-deduplicate-unique-fields.command.ts
Outdated
Show resolved
Hide resolved
constructor( | ||
@InjectRepository(Workspace) | ||
protected readonly workspaceRepository: Repository<Workspace>, | ||
protected readonly twentyORMGlobalManager: TwentyORMGlobalManager, |
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.
runOnWorkspace is already providing an ORM Manager that is managed by the command itself (handle injection and destruction
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.
actually I'm wrong here!
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.
that's the way an upgrade command should be instantiated
...r/src/database/commands/upgrade-version-command/1-7/1-7-deduplicate-unique-fields.command.ts
Outdated
Show resolved
Hide resolved
|
||
return this.createContactService.createPeople( | ||
const restoredContacts = | ||
await this.restorePeopleAndRestoreOrCreateCompanies( |
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.
I would have kept one function that's doing both
Done :
fixes #14443