feat: Zoom Meeting support alongside Webinars - #17
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
a0c9518 to
04f5eb8
Compare
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
- Added 'send_zoom_registration_email' and 'column_break_narx' fields to the Zoom Meeting JSON configuration. - Updated field definitions for better clarity and structure. - Modified permissions to include 'Zoom Manager' role. - Adjusted 'duration' field to allow None values in the Python class.
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Zoom Webinar Registration and Zoom Webinar Attendance Record now hold rows for Zoom Meetings as well as Zoom Webinars, so their names no longer match what they store. Rename both to the "session" vocabulary already used by the resource-parameterized helpers in api.py: Zoom Webinar Registration -> Zoom Session Registration Zoom Webinar Attendance Record -> Zoom Session Attendance Record The pre_model_sync patch does the move with frappe.rename_doc, which renames the tables and repoints Link options, property setters, report ref_doctype, attachments and dynamic links. It has to run before the model sync, otherwise migrate creates the new names as empty tables first and the rename collides. Both controllers also gain a validate() guard requiring exactly one of webinar/meeting. Dropping reqd on `webinar` for the meeting support left it possible to insert a record linked to neither, and the attendance controller had no validation at all. BREAKING CHANGE: /api/resource/Zoom%20Webinar%20Registration and /api/resource/Zoom%20Webinar%20Attendance%20Record are gone; use the Zoom Session paths. Python imports of the old module paths break. Server Scripts, Client Scripts and Notification conditions containing the old doctype names are not rewritten by the patch and must be updated by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Zoom Meeting fell back to hash naming, so the desk URL and every link carried an opaque ID with no relation to the meeting on Zoom. Name it after zoom_meeting_id instead: before_insert stores the ID from the create call, and Frappe runs before_insert ahead of set_new_name, so the value is there when the name is assigned. The names are only unique because the Zoom IDs are, which broke the tests: IntegrationTestCase rolls back per class, not per test, so repeated inserts of the same mocked create response collided on the primary key. Tests now take their meeting ID from create_meeting_response and pass a distinct one each. Also cover additional_params reaching Zoom, which had no test, before the child doctype behind it gets renamed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Zoom Webinar Additional Param was the last doctype left on the webinar vocabulary. It is the additional_params child table of Zoom Session Registration, which now holds meeting rows too, so the name no longer matches what it stores. Add it to the existing pre_model_sync rename patch. frappe.rename_doc handles the istable case: update_options_for_fieldtype repoints the parent's Table field, and the child rows' parent/parenttype still name Zoom Session Registration, which did not change. Also drop the Consolidated Webinar Attendance report. It was scaffolding that never got written -- get_columns returns "Column 1"/"Column 2" and get_data returns two literal rows -- with no workspace link and no caller. Its ref_doctype was updated by the earlier rename while its own name was not, so deleting it removes the inconsistency rather than renaming dead code. Migrate's orphan-report sweep removes the stale Report doc, so the patch needs no extra step. BREAKING CHANGE: /api/resource/Zoom%20Webinar%20Additional%20Param is gone; use the Zoom Session path. Python imports of the old module path break. The Consolidated Webinar Attendance report is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Naming Zoom Meeting by zoom_meeting_id only applied to new docs, so meetings created earlier kept their hash name while zoom_meeting_id held the real Zoom ID. Anything building a Zoom URL from the docname was wrong for those. rename_doc repoints Buzz Event.zoom_meeting and the dynamic reference_name on the session registration and attendance records. Zoom Meeting also gains the Connections it never had, matching Zoom Webinar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Zoom Session Registration and Zoom Session Attendance Record each carried a
`webinar` Link and a `meeting` Link, exactly one of which was meant to be set.
Nothing in the schema enforced that, every read had to branch on which one was
populated, and adding a third session type would have meant a third column.
Both now carry reference_doctype + reference_name instead. Selecting the
registration's session collapses from
frappe.get_cached_doc(
"Zoom Meeting" if self.meeting else "Zoom Webinar", self.meeting or self.webinar
)
to a plain get_cached_doc(self.reference_doctype, self.reference_name), and
`reqd` on both fields replaces the hand-written "exactly one of" guard.
reference_doctype carries link_filters so the desk only offers the two session
doctypes, but those apply client-side, so validate_session_reference keeps the
check on the server where it counts.
The backfill fills empty references only. The legacy columns are left for
`bench trim-tables` and are not kept in step afterwards, so overwriting a
populated reference would undo a later rename.
Connections on Zoom Webinar now match on reference_name. DocType Link has no
dynamic filter field, so the panel no longer restricts by session type; both
doctypes are named from the same Zoom ID space, so a collision needs two
sessions sharing an ID.
Test IDs are drawn fresh per call rather than fixed: sessions are named by
Zoom ID and registrant_id is unique, and rows outlive a run, so fixed IDs
collided with their own leftovers on a rerun.
BREAKING CHANGE: the `webinar` and `meeting` fields are gone from Zoom Session
Registration and Zoom Session Attendance Record; read reference_doctype and
reference_name. API clients filtering on the old fieldnames must be updated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Both failures only showed up in CI, because the dev bench has buzz installed and real Zoom credentials configured. Zoom Webinar posts through `requests` and fetches its own token rather than going via the api.py helpers, so patching `requests` alone left the token call live: it hit zoom.us on every run and threw "Zoom Settings Incomplete" wherever credentials were absent. mock_zoom_post patches both. The meeting rename test also queried Buzz Event, which this app does not depend on and CI does not install. The remaining assertion, that every meeting is named after its zoom_meeting_id, covers the patch on its own. Verified by making authenticate() raise and confirming the suite still passes, so nothing reaches live auth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
The change guard compared a datetime.date to str(date), which is never equal, so it never fired and every save sent a PATCH to Zoom. Editing a Buzz Event cascades into a meeting save, so ordinary edits were hitting the API. Both controllers carried it -- the meeting one was copied from the webinar one. getdate() on both sides fixes each. The tests missed it because they saved the doc returned by insert(), which still holds `date` as the string it was given, so the comparison matched by accident. The new tests reload from the database first, which is what the desk and the Buzz Event hook do. Also drops a backfill assertion that compared a literal to itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The registration lookup filtered on email alone, so it returned any registration for that address. Once one table started holding meetings as well as webinars, a meeting registration could be attached to a webinar attendance record. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
create_request_log stores request_headers verbatim on the Integration Request, so every logged call persisted its Authorization bearer token. Integration Request is readable by any System Manager. Frappe has no redaction helper and nothing in frappe or the payments app passes request_headers at all -- Stripe, Razorpay, PayPal, Braintree, GoCardless, Paytm, Mpesa and Paymob all omit it. Our headers only ever hold the token and a constant content-type, so dropping the argument loses no debugging value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 135-entry list Zoom accepts was duplicated byte for byte in both session form scripts, 138 of the meeting script's 163 lines. It now lives in public/js and loads once via app_include_js. A test asserts neither form inlines it again, since nothing else would catch the two copies drifting apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_paginate took a url_builder callback and a page_size nobody overrode, so callers wrapped a one-line f-string in a lambda. They now pass the URL. Pagination had no multi-page test at all -- both existing ones used single-page fixtures, leaving the loop unexercised. The new test covers three pages, which is the point: with two pages an appending implementation still produces one token and looks correct. Verified by writing the appending version and watching the third request carry two tokens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
pre-commit was never installed in this clone, so prettier and eslint had not run on any commit here. The shared list is the first JS this app has added in a while, which is what surfaced it. zoom_integration is a namespace frappe.provide creates at runtime, so eslint needs it declaring alongside frappe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
What
Adds first-class Zoom Meeting support next to the existing Webinar support, reusing shared API helpers instead of duplicating the Webinar controller — then generalises the shared doctypes so they describe what they actually hold.
Changes
api.py— resource-parameterized HTTP helpers (resource="meetings"|"webinars"): create / update / delete / add-registrant / paginated registrants & participants.Zoom Meetingdoctype + controller — create (type 2,approval_type:0so registration works), update, delete,add_registrant, andsync_attendance(viapast_meetings/{uuid}). Named byzoom_meeting_id, so the docname is the Zoom ID.Zoom Webinar Registration→Zoom Session Registration,Zoom Webinar Attendance Record→Zoom Session Attendance Record,Zoom Webinar Additional Param→Zoom Session Additional Param. They hold meeting rows as well as webinar rows now, soWebinarno longer belonged in the name.webinarandmeetinglinks forreference_doctype+reference_name. Picking the session collapses from a branch on which link is set toget_cached_doc(self.reference_doctype, self.reference_name), andreqdon both fields replaces the hand-written "exactly one of" guard.Consolidated Webinar Attendancereport (scaffolding that still returnedColumn 1/Row 1).public/js/zoom_timezones.js, loaded once viaapp_include_js.Migrations
rename_webinar_doctypes_to_sessionpre_model_syncbackfill_session_referencepost_model_syncreference_doctype/reference_namefrom the legacywebinar/meetingcolumns. Fills empty references only — the legacy columns are not kept in step afterwards, so overwriting a set reference would undo a later rename.rename_meetings_to_zoom_idpost_model_syncrename_docrepointsBuzz Event.zoom_meetingand the dynamicreference_name.Legacy
webinar/meetingcolumns are left behind forbench trim-tables.Fixes found while reviewing this branch
Three defects the tests were not catching, each now covered:
datetime.datetostr(date), never equal, so every save hit the API. Present in the webinar controller too, which the meeting one was copied from.create_request_logstoresrequest_headersverbatim, so every logged call persisted its bearer token. Nothing in frappe or thepaymentsapp passes that argument — dropping it matches the convention.Gotchas handled
//UUIDs.settings.approval_type:0on a scheduled (type 2) meeting — set in the create body and pinned by assertions.id; cast to string on store (Data field + URL paths).link_filtersonreference_doctypenarrows the desk picker to the two session doctypes, but applies client-side only —validate_session_referenceenforces it on the server.reference_name.DocType Linkhas no dynamic filter field, so the panel no longer restricts by session type; both doctypes name from the same Zoom ID space, so a collision needs two sessions sharing an ID. Zoom Meeting gains the Connections it never had.Required Zoom app scopes (Server-to-Server OAuth)
The app authenticates with
grant_type=account_credentials, so these are account-level (:admin) granular scopes. Add them to the Zoom Marketplace app before merging — every call below 4711s without its scope. Webinar scopes already on the app are unchanged; these are purely additive.meeting:write:meeting:adminPOST /users/me/meetingsZoom Meetinginsertmeeting:update:meeting:adminPATCH /meetings/{meetingId}meeting:delete:meeting:adminDELETE /meetings/{meetingId}on_trashmeeting:write:registrant:adminPOST /meetings/{meetingId}/registrantsZoom Session Registrationis submittedmeeting:read:list_past_participants:adminGET /past_meetings/{meetingUUID}/participantssync_attendance— needs a Pro plan or highermeeting:read:list_registrants:adminGET /meetings/{meetingId}/registrantsget_zoom_registrantshelper — no caller yetreport:read:list_meeting_participants:adminGET /report/meetings/{meetingId}/participantspast_meetings, which is gated bymeeting:read:list_past_participants:admininsteadmeeting:read:meeting:adminGET /meetings/{meetingId}Tests
All mocked — no real Zoom calls, and no live token fetch:
Zoom Webinarcallsrequestsand the token endpoint directly rather than going throughapi.py, so both are patched. Fixtures intests/zoom_fixtures.pymirror live Zoom v2 JSON. 36 passing, ruff and pre-commit clean.Session and registrant IDs are drawn fresh per call rather than fixed: sessions are named by Zoom ID and
registrant_idis unique, andIntegrationTestCaserolls back per class rather than per test, so rows outlive a run and fixed IDs collided with their own leftovers on a rerun.Three of these tests passed on their first run and had to be rewritten before they meant anything:
insert(), which still holdsdateas the string it was given, so the broken comparison matched by accident. It now reloads from the database, which is what the desk does.frappe.db.get_valueordering, which is undefined — it would have passed or failed at random. Rewritten so the correct answer isNone.Breaking changes
Renamed DocTypes
Zoom Webinar RegistrationZoom Session RegistrationZoom Webinar Attendance RecordZoom Session Attendance RecordZoom Webinar Additional ParamZoom Session Additional ParamRenamed fields
On both
Zoom Session RegistrationandZoom Session Attendance Record, the two links collapse into one dynamic pair:webinar(Link → Zoom Webinar)reference_doctype="Zoom Webinar",reference_name= the old valuemeeting(Link → Zoom Meeting)reference_doctype="Zoom Meeting",reference_name= the old valueAlso changed
zoom_meeting_idnow. Existing hash-named meetings are renamed by patch, so any docname you stored elsewhere is stale.Consolidated Webinar Attendancereport.What the patches fix for you
frappe.rename_docrepoints Link field options, Custom Field options, Property Setters, Dynamic Links,Report.ref_doctype,Client Script.dtandNotification.document_type. The backfill fillsreference_doctype/reference_namefrom the old columns. The old columns stay until you runbench trim-tables, so nothing is lost if you need to check.What you must fix by hand
Nothing rewrites code or expression bodies. Grep your custom app, Server Scripts, Client Scripts, Notification conditions, Print Formats and Jinja templates for the old names:
Then apply:
frappe.get_all("Zoom Webinar Registration", ...)frappe.get_all("Zoom Session Registration", ...)filters={"webinar": w}filters={"reference_doctype": "Zoom Webinar", "reference_name": w}filters={"meeting": m}filters={"reference_doctype": "Zoom Meeting", "reference_name": m}doc.webinar/doc.meetingdoc.reference_name(checkdoc.reference_doctypefor which)from ...doctype.zoom_webinar_registration.zoom_webinar_registration import ZoomWebinarRegistrationfrom ...doctype.zoom_session_registration.zoom_session_registration import ZoomSessionRegistration/api/resource/Zoom%20Webinar%20Registration/api/resource/Zoom%20Session%20RegistrationController classes follow the same rename:
ZoomWebinarAttendanceRecord→ZoomSessionAttendanceRecord,ZoomWebinarAdditionalParam→ZoomSessionAdditionalParam.A row can only ever have one session, so
webinar or meetingbecomes justreference_name.Pairs with bwhtech/buzz#288 — ship together, this app migrates first.
🤖 Generated with Claude Code