-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix missing events during account connection #9825
base: main
Are you sure you want to change the base?
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 adds webhook event emission functionality for connected accounts and channels in both Google and Microsoft API services, addressing the missing webhook events issue (#8765).
- Added
workspaceEventEmitter.emitDatabaseBatchEvent
calls in/packages/twenty-server/src/engine/core-modules/auth/services/google-apis.service.ts
for connected accounts, message channels, and calendar channels - Added identical event emission functionality in
/packages/twenty-server/src/engine/core-modules/auth/services/microsoft-apis.service.ts
for consistent behavior - Events now properly include before/after states for updates and full record data for creations
- Added
ObjectMetadataEntity
repository injection to both services to fetch required metadata for events - Events are emitted within transactions to ensure data consistency
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
...connectedAccount, | ||
...updatedConnectedAccount.raw[0], | ||
}, |
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.
logic: spreading updatedConnectedAccount.raw[0] might override fields from the original connectedAccount object that weren't included in the update. Consider only spreading the specific fields that were updated
@@ -114,7 +121,27 @@ export class GoogleAPIsService { | |||
manager, | |||
); | |||
|
|||
await messageChannelRepository.save( | |||
const connectedAccountMetadata = |
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.
First read:
- Shouldn't we also check the workspaceId?
Second read: (optional)
- i think it's fine as we are in the write path but we could also leverage the cached metadata (through WorkspaceCacheStorageService) to avoid performing additional queries on the database.
Third read: (optional)
I would actually remove objectMetadata from the Event payload completly (so no need to fetch it anymore):
- as we already have objectMetadataNameSingular as part of the BatchEvent.
- Also looks a bit heavy to force the caller to provide the whole objectMetadata everytime. I think there is only a few places we will need to have more than the nameSingular (maybe timelineActivity need the id for instance), we could leave it to timelineActivity to fetch the objectMetadata based on the nameSingular
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.
Agree with the comments. I'm not sure how far I can go as part of this PR.
1 should be done for sure (I copied the query from somewhere else so I will check it's probably missing in other places). As this pattern is already used elsewhere, 2 and 3 might require large refactoring that shouldn't be part of this PR - in that case should I create a followup issue that details (3) and merge with (1) 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.
yes LGTM
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.
Makes sense to add these events emts are we are still not emitting them from the ORM layer.
I've left a comment in 3 parts. Part 1 seems mandatory to address if I'm not mistaken, Part 2 and 3 optional but could be nice!
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 tested the events emission and it works correctly. LGTM. Also agree with @charlesBochet comments.
Fix #8765