[connectors] Launch workflows from ZendeskConnectorManager #8346
Conversation
aubin-tchoi
force-pushed
the
zendesk/client
branch
from
537061e
to
893373b
Compare
aubin-tchoi
requested review from
Fraggle,
fontanierh,
tdraier and
flvndvd
and removed request for
Fraggle and
fontanierh
| const workflowStartResult = await launchZendeskSyncWorkflow({ | ||
| connectorId: connector.id, | ||
| }); |
There was a problem hiding this comment.
nit but we usually prefer to pass proper type, here you can pass Connector.
| @@ -86,15 +105,52 @@ export class ZendeskConnectorManager extends BaseConnectorManager<null> { | |||
| } | |||
|
|
|||
| async stop(): Promise<Result<undefined, Error>> { | |||
| throw new Error("Method not implemented."); | |||
| const result = await stopZendeskSyncWorkflow(this.connectorId); | |||
| return result.isErr() ? result : new Ok(undefined); | |||
There was a problem hiding this comment.
Why not directly returning result?
There was a problem hiding this comment.
types do not match here, but this is an error in stopZendeskSyncWorkflow, will fix it and return result here, thanks for spotting
| "[Zendesk] Error resuming the sync workflow." | ||
| ); | ||
| } | ||
| return new Ok(undefined); |
There was a problem hiding this comment.
Seems weird to not return the error in this case 🤔
There was a problem hiding this comment.
true, i'll change that
| } | ||
| await connector.markAsUnpaused(); | ||
|
|
||
| const brandIds = await ZendeskBrandResource.fetchAllBrandIds({ |
There was a problem hiding this comment.
Why do we only resume with the brandIds?
There was a problem hiding this comment.
the brands are the top level collections, syncing them syncs everything
Co-authored-by: Flavien David <[email protected]>
| } | ||
|
|
||
| const dataSourceConfig = dataSourceConfigFromConnector(connector); | ||
| try { |
There was a problem hiding this comment.
Why do we need the try/catch if we have a Result type? Also in many other places there is no try/catch that wraps this call.
There was a problem hiding this comment.
my bad, changed that in launchZendeskSyncWorkflow a while ago and got a little bit confused here
There was a problem hiding this comment.
goal here is indeed to always use the Result type by catching it if necessary in the deepest level possible and return it in launchZendeskSyncWorkflow and then in resume.
| await connector.markAsPaused(); | ||
| const result = await this.stop(); | ||
|
|
||
| return result.isErr() ? result : new Ok(undefined); |
There was a problem hiding this comment.
Same here? Can we simply return result?
| }); | ||
| const result = await launchZendeskSyncWorkflow({ connector, brandIds }); | ||
|
|
||
| return result.isErr() ? result : new Ok(undefined); |
There was a problem hiding this comment.
Here types don't match since we pass the workflow ID in the Ok of launchZendeskSyncWorkflow, turns out we don't need this workflow ID so i'll just change launchZendeskSyncWorkflow and return result everywhere appliable
| } | ||
|
|
||
| const dataSourceConfig = dataSourceConfigFromConnector(connector); | ||
| const result = await launchZendeskSyncWorkflow({ connector }); |
There was a problem hiding this comment.
No need to wrap this in parameter in an object.
The rule is:
- Native types that are very easy to mix up -> Wrap in an object to have specific keys
- None-native types, use them as-is
Description
ZendeskConfigurations(fetchByIdmistook forfetchByConnectorId).Risk
Deploy Plan