Skip to content

feat: implement Canvas LMS integration (#31)#34

Open
baohuy1303 wants to merge 3 commits into
mainfrom
feat/issue-31-canvas-lms-adapter
Open

feat: implement Canvas LMS integration (#31)#34
baohuy1303 wants to merge 3 commits into
mainfrom
feat/issue-31-canvas-lms-adapter

Conversation

@baohuy1303

@baohuy1303 baohuy1303 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

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 LMSAdapter interface.
Closes #31


Issues

Two blocking gaps and several silent bugs in CanvasAdapter:

Problem Impact
get_course_materials() returned metadata only (content=None always) Qdrant got nothing for Canvas courses, so the agent had zero course knowledge
get_enrollments() filtered to StudentEnrollment and read a non-existent user.email No instructors/TAs synced, and every student email was null
get_course() never requested include[]=term LMSCourse.term was always None
get_assignments() sent no ordering/include params Unordered results, no submission state
get_submissions() sent include[]=assignment Full assignment object pulled every sync and discarded immediately (wasted bandwidth)

What 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.

for item in mod.get("items", []):
    materials.append(LMSCourseMaterial(
        external_id=str(item.get("id", "")),
        title=item.get("title", ""),
        material_type=item.get("type", "module_item").lower(),
        url=item.get("html_url"),
    ))            # content is always None
# ...then a second flat /files pass, also metadata-only

After: walks the modules tree and dispatches per item type, extracting text.

for module in modules:
    for item in module.get("items", []):
        item_type = item.get("type", "")
        if item_type == "SubHeader":
            continue                                   # visual divider, skip
        if item_type == "File":
            file_id = str(item.get("content_id", ""))  # stable Canvas file id
            if not file_id or file_id in seen_file_ids:
                continue                               # dedup across modules
            seen_file_ids.add(file_id)
            content, content_hash = await self._extract_file_content(course_id, file_id)
            # PDF: stream-download (Bearer auth, 20 MB cap) + pypdf text extract
        elif item_type == "Page":
            content, content_hash = await self._fetch_page_content(course_id, page_url)
            # GET /pages/:url, take the HTML `body`
        else:
            ...                                        # Assignment/Quiz/etc: metadata only

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_hash drives incremental-sync skip logic. Graceful degradation: a failed download logs a warning and stores content=None instead of aborting the whole sync.

2. get_enrollments(): full roster + emails (two-phase fetch)

Before: students only, and user.email doesn't exist on the enrollment payload.

data = await self._get_paginated(
    f"/courses/{course_id}/enrollments", {"type[]": "StudentEnrollment"},
)
return [LMSEnrollment(..., role="student",
                      email=e.get("user", {}).get("email")) ...]  # always None

After: all roles, plus a concurrent profile fetch for the real email.

data = await self._get_paginated(f"/courses/{course_id}/enrollments")
unique_user_ids = list({str(e["user_id"]) for e in enrollments_raw})
emails = await asyncio.gather(*[self._fetch_user_email(uid) for uid in unique_user_ids])
# role from the API-stable `type` field, not the customizable `role` field:
role=ROLE_MAP.get(e.get("type", ""), "student")

The nested user object on an enrollment carries only {id, name, sortable_name, short_name} (confirmed in the Canvas docs), so the email has to come from GET /users/:id/profile (primary_email). Fetches are deduped per user and throttled by the existing semaphore. Role mapping uses type (StudentEnrollment/TeacherEnrollment/TaEnrollment) because an admin can rename role to a custom string (e.g. "Lab TA"), which would otherwise misclassify a TA as a student.

3. Smaller fixes

Method Change
get_course() Send include[]=syllabus_body,term; parse term.name (was always None)
get_assignments() Send include[]=submission and order_by=due_at; guard empty submission_types
get_submissions() Drop the discarded include[]=assignment param
get_effective_due_dates() New Canvas-only helper for per-student due-date overrides
_get / _get_paginated Gated by asyncio.Semaphore(5) as a defensive concurrency cap

Files changed

File Change
src/docere/integrations/lms/canvas.py +290 / -49, all of the above
tests/unit/test_canvas_adapter.py New: 22 transport-level tests
.gitignore +1: ignore /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:

source .venv/Scripts/activate
python -m pytest tests/unit/test_canvas_adapter.py -v
# 22 passed in 0.58s

Approach: real httpx stack, faked responses

Rather than stubbing the adapter's methods, the tests inject an httpx.MockTransport into the adapter's inline httpx.AsyncClient. The real HTTP machinery still runs: URL building, the Authorization: Bearer header, query params, and Link: rel="next" pagination parsing are all exercised. Only Canvas's JSON responses are canned:

@pytest.fixture
def canvas(monkeypatch):
    adapter = CanvasAdapter(BASE_URL, TOKEN)
    transport = httpx.MockTransport(_make_handler(mock))   # routes by method + path
    real_client = httpx.AsyncClient
    def client_factory(*args, **kwargs):
        kwargs.setdefault("transport", transport)
        return real_client(*args, **kwargs)
    monkeypatch.setattr(canvas_mod.httpx, "AsyncClient", client_factory)
    return mock

PDF extraction is tested end to end with a real, hand-built single-page PDF (no reportlab/fpdf in the dep tree, so the fixture assembles valid PDF bytes with correct xref offsets). pypdf actually 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:

  • Plumbing: Bearer header on every request; include[] params on the wire; pagination follows the Link header across two enrollment pages
  • Enrollments: type-not-role mapping (custom "Lab TA" maps to ta); emails from the profile endpoint; profile-fetch dedup (3 calls for 4 rows); profile 500 degrades to email=None
  • Materials: File/Page/Assignment/SubHeader dispatch; file dedup across modules; real PDF text + hash; non-PDF stays content=None; page body extraction; PDF size guard; metadata-fetch failure degradation
  • Reads: get_course term parsing; get_assignments params and empty submission_types; get_submissions scoped vs course-wide paths; get_user_courses; get_effective_due_dates; get_grade_items group-to-category flattening
  • Writes: save_grade (posted_grade + comment body); post_announcement (is_announcement flag)
  • Errors: non-2xx propagates as HTTPStatusError

Why 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:

  1. The blast radius is tiny. One integration file. The orchestration layer is unchanged and already proven against Moodle.
  2. Every fixture shape comes from the official Canvas API docs (cited below). The tests assert against documented response shapes, so they're unlikely to diverge from a real instance.
  3. Once a Canvas sandbox exists, anyone can run the manual smoke test: point the adapter at the instance and call get_course, get_enrollments, and get_course_materials against 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)

  1. Persistent httpx.AsyncClient: share one client across calls (needs the adapters to become async context managers). Roughly 2 to 3x sync speedup.
  2. More file types: .docx/.pptx/.txt extraction (Canvas + Moodle).
  3. Webhooks / Live Events: Canvas Live Events + Moodle notifications to trigger run_lti_material_sync() on change, replacing the 2-hour poll.

Sources

API shapes and behavior were verified against the official Canvas documentation:

Topic Source
Enrollments (nested user has no email; type vs role) https://developerdocs.instructure.com/services/canvas/resources/enrollments
Modules / ModuleItem (content_id, page_url, item types) https://developerdocs.instructure.com/services/canvas/resources/modules
Files (hyphenated content-type, download url) https://developerdocs.instructure.com/services/canvas/resources/files
Pages (body field) https://developerdocs.instructure.com/services/canvas/resources/pages
Courses (include[]=term) https://developerdocs.instructure.com/services/canvas/resources/courses
Assignments (order_by, submission_types) https://developerdocs.instructure.com/services/canvas/resources/assignments
Assignment Groups (include[]=assignments) https://developerdocs.instructure.com/services/canvas/resources/assignment_groups
Submissions https://developerdocs.instructure.com/services/canvas/resources/submissions
User Profile (primary_email) https://developerdocs.instructure.com/services/canvas/resources/users
Discussion Topics (announcements) https://developerdocs.instructure.com/services/canvas/resources/discussion_topics
Pagination (Link header) https://canvas.instructure.com/doc/api/file.pagination.html
Throttling (concurrency cap rationale) https://developerdocs.instructure.com/services/canvas/basics/file.throttling
Community reference implementation https://github.com/vishalsachdev/canvas-mcp
Self-host feasibility (Docker dev setup) https://github.com/instructure/canvas-lms/blob/master/doc/docker/developing_with_docker.md
Testing tool httpx MockTransport

Note for reviewers: I keep a personal docs/huy/ folder (gitignored) with deeper working notes: a codebase overview, the full phased implementation plan, and an API audit (issues found vs research, with citations). Happy to send these over if any of the reasoning above needs more context.


Reviewer notes

Running the broader unit suite surfaces 6 failures in test_enhanced_strategy_evolution.py and test_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.

@baohuy1303 baohuy1303 requested a review from Youdahe123 June 8, 2026 05:57

@Youdahe123 Youdahe123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.email doesn't exist on the enrollment payload.
  • Using type (not role) for role mapping is the correct defense against custom role names.
  • httpx MockTransport approach is better than method stubs — real URL building, pagination parsing, and Bearer auth are exercised.
  • SHA256 content_hash enabling 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).

@Youdahe123

Copy link
Copy Markdown
Contributor

Merge status

CI: 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.

Merge order: #36#38#34

@Youdahe123

Copy link
Copy Markdown
Contributor

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 feat/issue-31-canvas-lms-adapter on main, the tiered embedding branch (#12) will be unblocked too. No other changes needed on your end — the PR itself looks good to merge.

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.
@baohuy1303 baohuy1303 force-pushed the feat/issue-31-canvas-lms-adapter branch from 0e8a0ea to e604fbf Compare June 29, 2026 04:36
@baohuy1303 baohuy1303 requested a review from Youdahe123 June 29, 2026 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Canvas LMS adapter

2 participants