-
Notifications
You must be signed in to change notification settings - Fork 114
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
Adding folder nodes mapping the Intercom hierarchy #9469
Adding folder nodes mapping the Intercom hierarchy #9469
Conversation
99917fb
to
be8e198
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.
First batch of comments
connectors/migrations/20241219_backfill_intercom_data_source_folders.ts
Outdated
Show resolved
Hide resolved
connectors/migrations/20241219_backfill_intercom_data_source_folders.ts
Outdated
Show resolved
Hide resolved
connectors/migrations/20241219_backfill_intercom_data_source_folders.ts
Outdated
Show resolved
Hide resolved
connectors/src/connectors/intercom/lib/conversation_permissions.ts
Outdated
Show resolved
Hide resolved
connectors/src/connectors/intercom/lib/conversation_permissions.ts
Outdated
Show resolved
Hide resolved
connectors/src/connectors/intercom/lib/help_center_permissions.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.
On the right path 👍 Left some coms
Make sure to test
Also checking: did you carefully track all model calls (to not miss any crud point)?
connectors/src/connectors/intercom/lib/help_center_permissions.ts
Outdated
Show resolved
Hide resolved
connectors/src/connectors/intercom/lib/conversation_permissions.ts
Outdated
Show resolved
Hide resolved
… revoke and strategy.detroy
19315b4
to
77eb440
Compare
// Add folder node for teams | ||
await upsertIntercomTeamsFolderActivity({ | ||
connectorId, | ||
}); | ||
|
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.
we're in for a ride again @philipperolet :)
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.
intercom is webhook-based for the conversations
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.
oh wait actually we're good, I thiiink webhooks rely on non-temporal code
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 guess we can stop all the workflows, deploy and resume them, which will trigger full resyncs for all the Help Centers but should be ok
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 far as I can tell the only workflow triggered by webhooks is stopIntercomSyncWorkflow -> Seems fine if we miss some
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.
this workflow is triggered by a webhook on /uninstall
, it's clearly not fine at all if we miss some but this one is more easily reconstructible (we need the connectorId in the webhook and we're good to go)
connectors/src/connectors/intercom/temporal/sync_help_center.ts
Outdated
Show resolved
Hide resolved
@@ -52,6 +55,11 @@ export async function intercomSyncWorkflow({ | |||
}) { | |||
await saveIntercomConnectorStartSync({ connectorId }); | |||
|
|||
// Add folder node for teams | |||
await upsertIntercomTeamsFolderActivity({ |
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.
exceptionally, just this once, we could discuss moving that into the syncTeamOnly activity
To be discussed with @overmode @philipperolet @aubin-tchoi vs the difficulty of stopping / restarting all intercom workflows for this change
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.
cc #9469 (comment) I think we can get away with batch stop + batch resume
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.
@aubin-tchoi says it shouldn't be a problem, I will check the code too, leaving it there until that
Description
Add folder nodes for TeamId, Teams, CollectionId and HelpCenter
Risk
Break Intercom connector
Test
Connect locally, do a few operations, delete then restore connector.
Deploy Plan
20241219_backfill_intercom_data_source_folders.ts