feat: implement Canvas LMS integration (#31)#34
Conversation
Youdahe123
left a comment
There was a problem hiding this comment.
Thorough, well-researched PR. The Canvas adapter had two blocking gaps (no content extraction, broken enrollment emails) and several silent data bugs — all fixed here. Key observations:
What's solid:
get_course_materials()now properly dispatches by item type (File/Page/Assignment), deduplicates across modules, and gracefully degrades on download failure. Mirrors the Moodle adapter pattern correctly.- The two-phase enrollment fetch (roster → profile endpoint for email) is the right call — the Canvas API docs confirm
user.emaildoesn't exist on the enrollment payload. - Using
type(notrole) for role mapping is the correct defense against custom role names. - httpx
MockTransportapproach is better than method stubs — real URL building, pagination parsing, and Bearer auth are exercised. - SHA256
content_hashenabling incremental-sync skip is a nice touch.
Minor notes (no blockers):
- The 20 MB PDF cap is hardcoded in
_extract_file_content. Consider making it a config constant if we expect large lecture slide decks. asyncio.Semaphore(5)is a good defensive measure but the value isn't documented. A comment explaining it mirrors Canvas's burst throttle threshold would help future devs.- The 3 follow-up issues (persistent client, more file types, webhooks) are well-scoped — good to have them tracked.
CI note: Backend/frontend CI fails here because this branch predates the lint fixes in #36 and #38. These are pre-existing failures, not regressions from this PR.
✅ Approving — this is ready to land once #36 and #38 are merged first (or this branch is rebased on top of them).
Merge statusCI: All 4 checks failing — but all failures are pre-existing (this branch predates the lint fixes in #36 and #38). Feature is approved. Blocked on CI stack landing first. Once #36 and #38 are merged into main, rebase this branch on updated main. All 4 CI checks should pass after the rebase and this will be ready to merge. |
|
Hey @baohuy1303 — amazing work on all three PRs, really thorough job especially on the Canvas adapter (the enrollment two-phase fetch and the httpx MockTransport test approach are great). Quick update on the merge order: I merged the backend CI fix (#36) and frontend CI fix (#38) just now — both are on main. The Canvas adapter PR (#34) had all-red CI because it was branched before either fix landed, so it just needs a rebase on main and all four checks should go green cleanly. Once you rebase |
Post-rebase cleanup after merging onto main's backend-lint-ci baseline: reformat to match installed ruff version, add type-arg to get_effective_due_dates return type.
0e8a0ea to
e604fbf
Compare
Summary
Completes the Canvas LMS adapter so Canvas courses sync the same way Moodle courses already do. The stub could authenticate and list objects, but it pulled no file/page content (the tutoring agent had zero Canvas course knowledge) and only saw students (no teachers/TAs, no emails). This PR adds real content extraction, full-roster enrollment sync, fixes several silent data bugs, and adds a full transport-level test suite.
Everything is contained to one integration file plus its tests. Nothing in the sync service, background tasks, LTI launch, or API routes needed to change, since they already treat Canvas and Moodle through the same
LMSAdapterinterface.Closes #31
Issues
Two blocking gaps and several silent bugs in
CanvasAdapter:get_course_materials()returned metadata only (content=Nonealways)get_enrollments()filtered toStudentEnrollmentand read a non-existentuser.emailnullget_course()never requestedinclude[]=termLMSCourse.termwas alwaysNoneget_assignments()sent no ordering/include paramsget_submissions()sentinclude[]=assignmentWhat changed
All in
src/docere/integrations/lms/canvas.py.1.
get_course_materials(): real content extraction (the main work)Before: walked modules for metadata, then dumped a flat file list, never fetching any content.
After: walks the modules tree and dispatches per item type, extracting text.
Design choices (mirrors the Moodle adapter): walk modules, not the flat files API, so the module title stays as semantic context. PDFs only for now. SHA256
content_hashdrives incremental-sync skip logic. Graceful degradation: a failed download logs a warning and storescontent=Noneinstead of aborting the whole sync.2.
get_enrollments(): full roster + emails (two-phase fetch)Before: students only, and
user.emaildoesn't exist on the enrollment payload.After: all roles, plus a concurrent profile fetch for the real email.
The nested
userobject on an enrollment carries only{id, name, sortable_name, short_name}(confirmed in the Canvas docs), so the email has to come fromGET /users/:id/profile(primary_email). Fetches are deduped per user and throttled by the existing semaphore. Role mapping usestype(StudentEnrollment/TeacherEnrollment/TaEnrollment) because an admin can renameroleto a custom string (e.g. "Lab TA"), which would otherwise misclassify a TA as a student.3. Smaller fixes
get_course()include[]=syllabus_body,term; parseterm.name(was alwaysNone)get_assignments()include[]=submissionandorder_by=due_at; guard emptysubmission_typesget_submissions()include[]=assignmentparamget_effective_due_dates()_get/_get_paginatedasyncio.Semaphore(5)as a defensive concurrency capFiles changed
src/docere/integrations/lms/canvas.pytests/unit/test_canvas_adapter.py.gitignore/docs/huy(personal working notes)How this was tested
The whole suite is pure unit tests: no Docker, no Postgres/Redis/Qdrant, no network, no Canvas instance. Run it anywhere:
Approach: real httpx stack, faked responses
Rather than stubbing the adapter's methods, the tests inject an
httpx.MockTransportinto the adapter's inlinehttpx.AsyncClient. The real HTTP machinery still runs: URL building, theAuthorization: Bearerheader, query params, andLink: rel="next"pagination parsing are all exercised. Only Canvas's JSON responses are canned:PDF extraction is tested end to end with a real, hand-built single-page PDF (no
reportlab/fpdfin the dep tree, so the fixture assembles valid PDF bytes with correct xref offsets).pypdfactually parses it, and we assert the extracted text and the 64-char content hash.Coverage
Every adapter method, including the edge cases the bugs above hid:
include[]params on the wire; pagination follows theLinkheader across two enrollment pagestype-not-rolemapping (custom "Lab TA" maps tota); emails from the profile endpoint; profile-fetch dedup (3 calls for 4 rows); profile 500 degrades toemail=Nonecontent=None; page body extraction; PDF size guard; metadata-fetch failure degradationget_courseterm parsing;get_assignmentsparams and emptysubmission_types;get_submissionsscoped vs course-wide paths;get_user_courses;get_effective_due_dates;get_grade_itemsgroup-to-category flatteningsave_grade(posted_grade + comment body);post_announcement(is_announcement flag)HTTPStatusErrorWhy no live Canvas test, and why this is safe to merge
https://github.com/instructure/canvas-lms/wiki/Quick-Start The official Canvas Docs noted that:
The official Docker dev setup needs 150GB of available hard drive space, 8GB of RAM, a quad-core CPU, and a long multi-step build (web + Postgres + Redis + jobs), and ships no seed script, so test courses/files/users have to be created by hand. That's too heavy to stand up just to validate an adapter, and we don't have a hosted sandbox yet.
This is still safe to merge because:
get_course,get_enrollments, andget_course_materialsagainst a real course. The doc-grounded unit tests already cover the parsing/dispatch logic; the live run only needs to confirm connectivity and auth.Out of scope (follow-up issues)
httpx.AsyncClient: share one client across calls (needs the adapters to become async context managers). Roughly 2 to 3x sync speedup..docx/.pptx/.txtextraction (Canvas + Moodle).run_lti_material_sync()on change, replacing the 2-hour poll.Sources
API shapes and behavior were verified against the official Canvas documentation:
userhas no email;typevsrole)content_id,page_url, item types)content-type, downloadurl)bodyfield)include[]=term)order_by,submission_types)include[]=assignments)primary_email)Linkheader)Reviewer notes
Running the broader unit suite surfaces 6 failures in
test_enhanced_strategy_evolution.pyandtest_llm_stream.py. These are pre-existing and unrelated to this PR (they fail identically without this file loaded). Flagging so they aren't mistaken for regressions from this change.