-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Import - add duplicate check on import #12810
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
Conversation
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.
PR Summary
Added uniqueness constraint validation during spreadsheet imports to prevent duplicate entries across both simple and composite fields.
- Implemented
spreadsheetImportComputeUnicityRowHook
inspreadsheet-import/utils/spreadsheetImportGetUnicityRowHook.ts
to validate unique constraints during import - Added
isIncludedInUniqueConstraint
field to composite field configurations insettings/data-model/constants/SettingsCompositeFieldTypeConfigs.ts
for primary identifiers - Enhanced
useOpenObjectRecordsSpreadsheetImportDialog
to integrate uniqueness checks through rowHook in import dialog options
3 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
...ont/src/modules/object-record/spreadsheet-import/utils/spreadsheetImportGetUnicityRowHook.ts
Outdated
Show resolved
Hide resolved
...ont/src/modules/object-record/spreadsheet-import/utils/spreadsheetImportGetUnicityRowHook.ts
Outdated
Show resolved
Hide resolved
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:5010 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
||
uniqueConstraintFields.forEach((uniqueConstraint) => { | ||
const uniqueConstraintRow = uniqueConstraint | ||
.map((field) => row?.[field] || '') |
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.
Remark: Unless I'm mistaken we should not be case sensitive here and try to trim most of the fields ?
...ont/src/modules/object-record/spreadsheet-import/utils/spreadsheetImportGetUnicityRowHook.ts
Outdated
Show resolved
Hide resolved
}); | ||
} | ||
}); | ||
} |
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.
Nitpick: we might wanna display the failing value along the failing field ?
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.
mmmmh 🤔 not sure it is useful as error message is display when at hover on value
<img width="1217" alt="Screenshot 2025-06-24 at 11 43 03" src="https://github.com/user-attachments/assets/de2bc12e-5e25-484f-a034-f52b0f237a3e" /> Test : - Add duplicate on id or on primaryEmail for people or primaryUrlLink on companies closes twentyhq/core-team-issues#909
Test :
closes twentyhq/core-team-issues#909