fix(office365video): use PATCH for updateMeeting and implement deleteMeeting#29508
fix(office365video): use PATCH for updateMeeting and implement deleteMeeting#29508NamanYadav11 wants to merge 1 commit into
Conversation
…Meeting updateMeeting was calling POST /onlineMeetings, creating a new meeting on every reschedule instead of updating the existing one via PATCH. deleteMeeting was a no-op that returned an empty array without making any HTTP request, leaving the remote Teams meeting alive after deletion. Also updates existing updateMeeting tests (which were asserting the broken POST behavior) and adds deleteMeeting tests covering success, 404, and error. Fixes calcom#29498
|
Welcome to Cal.diy, @NamanYadav11! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughThe PR updates the Office365 Teams video adapter to properly handle meeting lifecycle operations. The 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app-store/office365video/lib/VideoApiAdapter.ts`:
- Around line 233-236: Replace uses of HttpError in the new non‑tRPC error
branches with ErrorWithCode: add imports "ErrorCode" and "ErrorWithCode" from
"`@calcom/lib/errorCodes`" and "`@calcom/lib/errors`" respectively, then change
throws like the ones in VideoApiAdapter (e.g. the meeting update/delete branches
that currently throw new HttpError(...)) to throw new
ErrorWithCode(ErrorCode.BadRequest, "<message>") or use
ErrorWithCode.Factory.BadRequest("<message>") so all new error paths use
ErrorWithCode instead of HttpError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fd6aacf-5944-4197-8de6-a5d58efc4ec1
📒 Files selected for processing (2)
packages/app-store/office365video/lib/VideoApiAdapter.test.tspackages/app-store/office365video/lib/VideoApiAdapter.ts
| throw new HttpError({ | ||
| statusCode: 400, | ||
| message: "Meeting ID is required to update a Teams meeting", | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how ErrorWithCode is imported/constructed across the repo,
# then migrate this file's newly added throws consistently.
rg -n --type=ts -C2 '\bErrorWithCode\b'
rg -n --type=ts -C2 'new HttpError\(' packages/app-store/office365video/lib/VideoApiAdapter.tsRepository: calcom/cal.diy
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/app-store/office365video/lib/VideoApiAdapter.ts"
echo "== Imports in $FILE (first 120 lines) =="
sed -n '1,120p' "$FILE"
echo
echo "== Error usage in $FILE (ErrorWithCode / HttpError) =="
rg -n --type=ts '\bErrorWithCode\b' "$FILE" || true
rg -n --type=ts '\bHttpError\b' "$FILE" || true
rg -n --type=ts 'new HttpError\(' "$FILE" || true
echo
echo "== Relevant sections (around 220-310) =="
sed -n '210,310p' "$FILE"Repository: calcom/cal.diy
Length of output: 8861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/app-store/office365video/lib/VideoApiAdapter.ts"
echo "== ErrorWithCode usage in $FILE =="
rg -n --type=ts '\bErrorWithCode\b' "$FILE" || true
echo
echo "== ErrorCode values used in typical ErrorWithCode throws (spot check) =="
ls -1 packages/lib | head -n 50 || true
# Try common errorCodes locations
rg -n --type=ts "enum ErrorCode|export (const )?ErrorCode|ErrorCode\.(BadRequest|InternalServerError|NotFound)" packages/lib/errorCodes* packages/lib/*errorCodes* 2>/dev/null || true
rg -n --type=ts "BadRequest|InternalServerError|NotFound" packages/lib/errorCodes* 2>/dev/null || true
echo
echo "== HttpError definition/contract =="
rg -n --type=ts "export (class|function) HttpError|class HttpError" packages -S -g'*.ts' -g'*.tsx' | head -n 50
if [ -f "packages/lib/http-error.ts" ]; then
sed -n '1,200p' packages/lib/http-error.ts
elif [ -f "packages/lib/http-error/index.ts" ]; then
sed -n '1,200p' packages/lib/http-error/index.ts
fi
echo
echo "== How updateMeeting/deleteMeeting errors are consumed (search for office365_video usage) =="
rg -n --type=ts "office365_video" packages | head -n 50
echo
echo "== Call sites for updateMeeting/deleteMeeting =="
rg -n --type=ts "updateMeeting\\s*:\\s*async|updateMeeting\\s*\\(" packages | head -n 50
rg -n --type=ts "deleteMeeting\\s*:\\s*async|deleteMeeting\\s*\\(" packages | head -n 50
echo
echo "== Search for handling of HttpError vs ErrorWithCode near video adapters =="
rg -n --type=ts "instanceof HttpError|instanceof ErrorWithCode" packages/features packages/app-store packages/lib | head -n 80Repository: calcom/cal.diy
Length of output: 11879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how conferencing layer handles errors thrown by video adapters
sed -n '1,220p' packages/features/conferencing/lib/videoClient.ts
echo
echo "== Other VideoApiAdapter implementations using HttpError? =="
rg -n --type=ts "import \\{ HttpError \\} from \"`@calcom/lib/http-error`\"" packages/app-store/*/lib/VideoApiAdapter.ts || true
rg -n --type=ts "\bnew HttpError\(" packages/app-store/*/lib/VideoApiAdapter.ts || true
echo
echo "== Error mapping helpers (client/server) =="
sed -n '1,220p' packages/lib/getClientErrorFromUnknown.ts
sed -n '1,220p' packages/lib/server/getServerErrorFromUnknown.tsRepository: calcom/cal.diy
Length of output: 16378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' packages/lib/errors.tsRepository: calcom/cal.diy
Length of output: 3191
Use ErrorWithCode (not HttpError) in the new non‑tRPC error branches of VideoApiAdapter.ts.
packages/app-store/office365video/lib/VideoApiAdapter.ts currently throws new HttpError({ statusCode: 400, ... }) (and other HttpError instances) for the new meeting update/delete error paths (lines 233-236, 248-251, 280-283, 291-294). Switch these to ErrorWithCode using import { ErrorCode } from "@calcom/lib/errorCodes"; import { ErrorWithCode } from "@calcom/lib/errors"; and throw new ErrorWithCode(ErrorCode.BadRequest, "…") (or ErrorWithCode.Factory.*).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app-store/office365video/lib/VideoApiAdapter.ts` around lines 233 -
236, Replace uses of HttpError in the new non‑tRPC error branches with
ErrorWithCode: add imports "ErrorCode" and "ErrorWithCode" from
"`@calcom/lib/errorCodes`" and "`@calcom/lib/errors`" respectively, then change
throws like the ones in VideoApiAdapter (e.g. the meeting update/delete branches
that currently throw new HttpError(...)) to throw new
ErrorWithCode(ErrorCode.BadRequest, "<message>") or use
ErrorWithCode.Factory.BadRequest("<message>") so all new error paths use
ErrorWithCode instead of HttpError.
|
Thanks for the review! This suggestion doesn't align with the existing patterns in VideoApiAdapter files — the |
1 participant
What does this PR do?
Fixes #29498
The Office 365 Video (MS Teams) adapter had two broken methods:
1.
updateMeetingcreated duplicates on every rescheduleIt was calling
POST /onlineMeetings(which creates a new meeting) instead ofPATCH /onlineMeetings/{meetingId}(which updates the existing one). Every time a user rescheduled a booking, a brand-new Teams meeting was created and the old one was left orphaned.2.
deleteMeetingwas a no-opThe implementation was
return Promise.resolve([])— it never made an HTTP request, so the remote Teams meeting stayed alive after the booking was deleted on the Cal side.Both methods now call the correct Microsoft Graph API endpoints using the meeting ID that's already stored in the booking reference.
Visual Demo (For contributors especially)
N/A — backend adapter fix.
Before reschedule: two Teams meetings visible in the organizer's Teams calendar (original + duplicate).
After: only one meeting, updated with the new time.
Before cancel: Teams meeting stays active after booking is deleted.
After: Teams meeting is removed via
DELETE /onlineMeetings/{id}.Mandatory Tasks (DO NOT REMOVE)
updateMeetingtests (they were asserting the broken POST behavior) and added a fulldeleteMeetingtest suite.How should this be tested?
Requires an Office 365 / MS Teams account connected to Cal.
Reschedule path:
Cancellation path:
Unit tests:
yarn vitest packages/app-store/office365video— all tests passChecklist
I haven't read the contributing guideMy code doesn't follow the style guidelines of this projectMy PR is too large (>500 lines or >10 files) and should be split into smaller PRs