-
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
Deduplicate-emails #10355
Deduplicate-emails #10355
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
This PR implements case-insensitive email deduplication across the contact management system, ensuring that variations of the same email address (e.g., '[email protected]' and '[email protected]') are treated as identical.
- Added case-insensitive email handle comparison in
packages/twenty-server/src/modules/contact-creation-manager/utils/get-unique-contacts-and-handles.util.ts
usingtoLocaleLowerCase()
- Implemented URL normalization and email case standardization in
packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/query-runner-args.factory.ts
- Added test cases in
get-unique-contacts-and-handles.spec.ts
to verify case-insensitive email deduplication - Enhanced error handling for both URL and email processing in the query runner factory
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
...-server/src/engine/api/graphql/workspace-query-runner/factories/query-runner-args.factory.ts
Outdated
Show resolved
Hide resolved
...-server/src/engine/api/graphql/workspace-query-runner/factories/query-runner-args.factory.ts
Outdated
Show resolved
Hide resolved
…runner/factories/query-runner-args.factory.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
… into deduplicate-emails
try { | ||
newPrimaryLinkUrl = new URL(newPrimaryLinkUrl).toString(); | ||
} catch { | ||
newPrimaryLinkUrl = value?.primaryLinkUrl; |
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.
suggestion for long term: should this rather be part of the ORM layer so services (like messaging) or custom apps can also benefit from it?
suggestion for long term: this seems tied to validating the input + casting it. I think we need a robust approach here for FE and BE behavior. Ok with this fix for now
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 will do the util
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.
must: this could also be extracted to an util and tested separately :)
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.
✅
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.
As discussed orally:
- let's also do secondaryLinks?
- check company creation, do we lowercase email when extracting the domain from an email?
Thanks @guillim for your contribution! |
Following User request to remove duplicate emails