-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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: #15474 retrieve zoom usersettings and update zoom integration #15489
fix: #15474 retrieve zoom usersettings and update zoom integration #15489
Conversation
@vijayraghav-io is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add community label" took an action on this PR • (06/19/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (06/19/24)1 reviewer was added to this PR based on Keith Williams's automation. |
all checks passed |
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.
Thanks for your contribution @vijayraghav-io. I just have one blocking comment.
export type ZoomUserSettings = z.infer<typeof zoomUserSettingsSchema>; | ||
|
||
/** @link https://developers.zoom.us/docs/api/rest/reference/user/methods/#operation/userSettings */ | ||
export const zoomUserSettingsSchema = z.object({ |
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 noticed we're only using the password and auto record settings. When making the API call can we filter for just those two properties.
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.
sure
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.
@joeauyeung
updated at LOC - 302, 349 as per doc - https://developers.zoom.us/docs/api/rest/reference/user/methods/#operation/userSettings
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.
Awesome, can we also then clean up zoomUserSettingsSchema
to only the fields we're filtering on. It'll help make reading the file easier.
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 can clean up.
I thought in future if any more settings is required to be retrieved , we already have the schema defined, need not again refer or search the zoom api doc.
Please share your thoughts.
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.
@joeauyeung , updated
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.
Our thought process is we only want code that is being used. If we need to access different user settings we can always add to the schema in the future.
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.
ok
…474_zoomUserSettings
…yraghav-io/cal.com into fix_#15474_zoomUserSettings
let userSettings: ZoomUserSettings | undefined; | ||
try { | ||
const filterResp = "default_password_for_scheduled_meetings,auto_recording"; | ||
const responseBody = await fetchZoomApi(`users/me/settings?custom_query_fields=${filterResp}`); | ||
userSettings = zoomUserSettingsSchema.parse(responseBody); | ||
} catch (err) { | ||
console.error(err); | ||
/* If user settings could not be retrieved due to error in like - api scope configurations, | ||
just continue create meeting with default settings*/ | ||
} |
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.
Since we're repeating this functionality, can we abstract this into its own function? Maybe we can call that function inside of translate event.
Also we have to add a new scope to our Zoom app. If a user has not reauthenticated then that means this will always throw an error. can we add proper logging if that is the case?
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.
@joeauyeung updated
Hi @joeauyeung , all comments are addressed, reminding you on this PR. |
@joeauyeung can you take a look again 🙏 |
This PR is being marked as stale due to inactivity. |
@joeauyeung , can you please review the required changes done and approve |
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 for the delay, every thing looks good @vijayraghav-io. Thanks again for your contribution
Thankyou! @joeauyeung 🙏 |
I guess the specific scopes are not enabled/added here for cal app to access the zoom user settings. https://developers.zoom.us/docs/api/rest/reference/user/ma/#operation/userSettings https://developers.zoom.us/docs/integrations/oauth-scopes-overview/ |
What does this PR do?
This is generic implementation with zoomUserSettingsSchema defined , any future zoomUserSettings retrieval and corresponding logic update for create/update meetings or any.. can be easily extended.
Mandatory Tasks (DO NOT REMOVE)