-
Notifications
You must be signed in to change notification settings - Fork 11.6k
chore: Sync Services to update external tools #3814
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
Conversation
| try { | ||
| this.service = new service(); | ||
| } catch (e) { | ||
| this.log.warn("Couldn't instantiate sync service:", (e as Error).message); |
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.
Not having the requirements to instance a service part of a sync service will just result in a warning. So for example, if Close.com or Sendgrid API Keys are missing, a warning will come up to the console and no other method will be called resulting in no errors whatsoever
| return this.service !== undefined; | ||
| } | ||
|
|
||
| async getUserLastBooking(user: { email: string }): Promise<{ booking: { createdAt: Date } | null } | null> { |
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 is to provide common logic to all Sync Services. As they all need to get the last booking for a user, this is available from the class they extend from
| import SendgridService from "./SendgridService"; | ||
|
|
||
| const services: ISyncServices[] = [ | ||
| //CloseComService, This service gets a special treatment after deciding it shouldn't get the same treatment as Sendgrid |
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.
Close.com does not behave the same as Sendgrid, as the definitions changed to only register contacts within teams, so it was excluded from here and implemented specific Close.com functions in SyncServiceManager.
zomars
left a comment
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.
Looking good @leog nice work
| export default async function handler(req: NextApiRequest, res: NextApiResponse) { | ||
| if (req.method === "POST") { | ||
| if (!req.headers["x-helpscout-signature"]) { |
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.
Sorry @zomars, couldn't make it a default handler as I couldn't find a way to disable the automatic bodyParser from it.
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.
@leog the defaultHandler doesn't add bodyParser at all. You can still configure it at config level
zomars
left a comment
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.

What does this PR do?
Introduces the concept of Sync Services, used to sync external tools with key information of Cal.com users when they interact with the app, such as HelpScout, Close.com and SendGrid. See #3633 for more details on what's needed.
Note that the work for this was implemented above #3709, which means it needs to be merged before this PR gets merged.Implements #3633.
Please be sure to also review closely related PRs in other apps. Once this PR is merged, they also need to be merged.
Type of change
How should this be tested?