feat: created by email calendar #6536
Conversation
magrinj
force-pushed
the
feat/created-by-messaging
branch
from
1c5c2da
to
95c4861
Compare
| /** | ||
| * Pick only the keys that match the Type `U` | ||
| */ | ||
| -export type PickKeysByType<T, U> = string & keyof { |
There was a problem hiding this comment.
TypeORM doesn't handle column type that can be nullable...
| @@ -111,7 +110,7 @@ export class CompanyWorkspaceEntity extends BaseWorkspaceEntity { | |||
| icon: 'IconMap', | |||
| }) | |||
| @WorkspaceIsNullable() | |||
| address: Address; | |||
| address: AddressMetadata; | |||
There was a problem hiding this comment.
Missing and incorrect metadata types
| const companyDomainNameColumnName = | ||
| domainNameFieldMetadata?.type === FieldMetadataType.LINKS | ||
| ? 'domainNamePrimaryLinkUrl' | ||
| : 'domainName'; |
There was a problem hiding this comment.
TBC: Dropping support of old column domainName typed as a simple string, as migration should be applied.
cc @ijreilly
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements support for EMAIL and CALENDAR sources in the createdBy composite field, following up on issue #6324. Key changes include:
- Updated CreateCompanyAndContactService to handle new ACTOR field type and FieldActorSource
- Removed specific repository files for companies and persons, shifting to TwentyORMGlobalManager
- Modified CompanyWorkspaceEntity to include createdBy field of type ActorMetadata
- Added utility function computeDisplayName for handling full name display
- Updated various services and jobs to include FieldActorSource.EMAIL and FieldActorSource.CALENDAR
- Applied a custom patch to typeorm package to handle nullable fields in PickKeysByType
These changes improve record creation source handling and provide more detailed information about record creators across different sources.
17 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| const alreadyCreatedContacts = await personRepository.find({ | ||
| where: { | ||
| email: Any(uniqueHandles), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
style: Using Any() in TypeORM query may have performance implications for large datasets. Consider using IN operator if possible.
| // FixMe: TypeORM typing issue... id is always returned when using save | ||
| recordId: createdPerson.id!, |
There was a problem hiding this comment.
logic: Using non-null assertion (!) here might hide potential runtime errors. Consider handling the case where id could be undefined.
| {}, | ||
| // Avoid creating duplicate companies | ||
| const uniqueCompanies = uniqBy(companies, 'domainName'); | ||
| const conditions = uniqueCompanies.map((companyToCreate) => ({ |
There was a problem hiding this comment.
It's actually run in a job, but I agree, maybe we can optimize this, I've try to improve it a bit already
| @@ -30,6 +31,7 @@ export class CreateCompanyAndContactJob { | |||
| displayName: contact.displayName, | |||
| })), | |||
| workspaceId, | |||
| FieldActorSource.EMAIL, | |||
There was a problem hiding this comment.
This one is also emit from CalendarSaveEventsService so it won't always be EMAIL
| const personRepository = | ||
| await this.twentyORMGlobalManager.getRepositoryForWorkspace( | ||
| workspaceId, | ||
| PersonWorkspaceEntity, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Can't we use twentyORMManager instead of twentyORMGlobalManager here?
| // In some jobs the accountOwner is not populated | ||
| if (!connectedAccount.accountOwner) { | ||
| const workspaceMemberRepository = | ||
| await this.twentyORMGlobalManager.getRepositoryForWorkspace( | ||
| workspaceId, | ||
| WorkspaceMemberWorkspaceEntity, | ||
| ); | ||
|
|
||
| const workspaceMember = await workspaceMemberRepository.findOne({ | ||
| where: { | ||
| id: connectedAccount.accountOwnerId, | ||
| }, | ||
| }); | ||
|
|
||
| if (!workspaceMember) { | ||
| throw new Error( | ||
| `Workspace member with id ${connectedAccount.accountOwnerId} not found in workspace ${workspaceId}`, | ||
| ); | ||
| } | ||
|
|
||
| const companyDomainNameColumnName = | ||
| domainNameFieldMetadata?.type === FieldMetadataType.LINKS | ||
| ? 'domainNamePrimaryLinkUrl' | ||
| : 'domainName'; | ||
| connectedAccount.accountOwner = workspaceMember; | ||
| } |
There was a problem hiding this comment.
We can modify the code to always pass the connected account with the account owner to the job. But it will be easier once all the raw queries are replaced.
| transactionManager?: EntityManager, | ||
| ): Promise<string> { | ||
| const companyId = v4(); | ||
| const companyRepository = | ||
| await this.twentyORMGlobalManager.getRepositoryForWorkspace( |
There was a problem hiding this comment.
twentyORMManager instead?
This PR is a followup of #6324 to add support of EMAIL and CALENDAR source for the created by composite field.