- Notifications
You must be signed in to change notification settings - Fork11.2k
fix: delegation credential error webhooks + refactor repeated code#25232
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…into helper methods- Added private triggerDelegationCredentialError method in Office365CalendarService class- Added triggerDelegationCredentialError helper function in TeamsVideoApiAdapter- Replaced all 4 instances in Office365CalendarService with helper method call- Replaced all 4 instances in TeamsVideoApiAdapter with helper function call- Keeps code DRY by eliminating repeated if statement and webhook trigger logicCo-Authored-By: morgan@cal.com <morgan@cal.com>
vercelbot commentedNov 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The latest updates on your projects. Learn more aboutVercel for GitHub. |
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.
No issues found across 6 files
Uh oh!
There was an error while loading.Please reload this page.
emrysal 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.
Happy with this. Better error handling; more centralized - and the query looks sane (search for index by teamId first, some filters after)
b8530e2 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
What does this PR do?
This PR contains two sets of changes:
1. Original Changes (Delegation Credential Error Webhook Fixes)
orgIdtodelegationCredentialIdfor more accurate organization resolutionWebhookRepository.findByOrgIdAndTrigger()method for fetching webhooks by organization and trigger eventdelegationCredentialIdandorganizationIdfieldsdelegatedToIdcheck before triggering webhooks2. Refactoring Changes (Code DRY Improvements)
triggerDelegationCredentialError()private method inOffice365CalendarServiceclasstriggerDelegationCredentialError()helper function inTeamsVideoApiAdapterMandatory Tasks (DO NOT REMOVE)
How should this be tested?
Testing the webhook fixes:
DELEGATION_CREDENTIAL_ERRORtrigger at the organization leveldelegationCredentialIdandorganizationIdTesting the refactoring:
Human Review Checklist
Critical items to review:
Webhook Logic Change: Verify the change from passing
orgIddirectly to looking up the delegation credential first is correct. Does this properly handle all edge cases?New Repository Method: Review
WebhookRepository.findByOrgIdAndTrigger()- is the query correct? Does it properly filter by organization and trigger event?Payload Changes: The webhook payload now includes
delegationCredentialIdandorganizationId. Is this a breaking change for existing webhook consumers? Should this be documented?GoogleCalendar Fix: Verify the addition of
credential.delegatedToIdcheck in GoogleCalendar is correct and consistent with other integrations.Refactoring Correctness: Verify the extracted helper methods maintain exactly the same behavior as the original repeated code. Check that:
Edge Cases: What happens if
delegationCredentialis not found in the database? The code returns early, but is this the correct behavior?Session Info: