Repository navigation
Conversation
Adds v2 alongside v1 and makes it the default for new Teachable sources. Under a v2 pin, users, courses, course_enrollments, course_progress and lectures read the v2 wire; transactions, pricing_plans, quizzes and quiz_responses stay on v1 because their v2 endpoints are marked beta. v1 pins keep their existing request path unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4462ff72-5db5-460e-b686-69deec4edba9
Gilbert09
added
the
stamphog
Talyn App
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Risk: No findings This increment freezes the Teachable config dataclasses and makes the v2 credential check scope-aware: Sentinel reviewed |
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/autumn/source.py:102 |
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/source.py:103 |
18 | 83 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
⚠️ Comment density — 5% of added code lines are comments (22 of 420)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/tests/test_teachable.py |
8 | 195 |
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/settings.py |
6 | 24 |
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/teachable.py |
6 | 185 |
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/source.py |
2 | 9 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 99.0% of changed backend lines covered — 1 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ████████████████████ 99.0% (144 / 145)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/teachable.py |
99.0% | 190 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 58506993099661 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
warehouse_sources |
███████████████████░ 96.5% |
489,610 / 507,158 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
There was a problem hiding this comment.
Reviewed the PR description, diff, existing discussion, and relevant source, pagination, version-resolution, and setup code at d6a3036. Compared v2 paths and response shapes with Teachable's API documentation, and checked scope and overengineering. No tests, builds, linters, or installs were run, as requested.
P1
P1: Keep v1 as the default until v2 access is generally available, or provide an explicit opt-in during setup. Teachable's authentication docs say v2 access must be requested from Support. With this default, a school with a valid v1 API key but no v2 grant now fails credential validation against /v2/products/courses. The create flow validates credentials before creating the source and always stamps source.default_version, so users cannot create a source and then select v1 to recover. This blocks previously supported new connections even though v1 remains available; the improved 403 message does not preserve that setup path.
No P0 or P2 issues found.
Inline comments
products/warehouse_sources/backend/temporal/data_imports/sources/teachable/source.py:48— P1: Keep v1 as the default until v2 access is generally available, or provide an explicit opt-in during setup. Teachable's authentication docs say v2 access must be requested from Support. With this default, a school with a valid v1 API key but no v2 grant now fails credential validation against /v2/products/courses. The create flow validates credentials before creating the source and always stamps source.default_version, so users cannot create a source and then select v1 to recover. This blocks previously supported new connections even though v1 remains available; the improved 403 message does not preserve that setup path.
— Robo Bill v2 (gpt-6.1-sol, high reasoning)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4462ff72-5db5-460e-b686-69deec4edba9
![posthog[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed d6a3036: 1 Must fix, 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| # v2 moves users, courses, enrollments, course progress and lectures onto the v2 wire. The | ||
| # other tables stay on v1 under a v2 pin, so the table set is the same for both versions. | ||
| supported_versions = (TEACHABLE_API_VERSION_V1, TEACHABLE_API_VERSION_V2) | ||
| default_version = TEACHABLE_API_VERSION_V2 |
There was a problem hiding this comment.
P1 · Keep the default usable for schools without v2 beta access
Schools without Teachable's opt-in v2 access now fail credential validation despite having working v1 keys. Creation always uses default_version, and api_version is read-only, so users cannot select v1. The source guidance requires an accessible stable version; retain v1 as the default until v2 is generally available.
There was a problem hiding this comment.
Valid concern, and it is the open decision this PR flags in its warning. The task that produced this PR asked for v2 as the default for new sources, so the code keeps it. A person on the owning team should decide whether the flip waits until Teachable makes v2 generally available. Reverting it is a one-line change to default_version. Leaving this thread unresolved for that decision.
HostHog preview —
|
![stamphog[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Owning-team authorship and STRONG familiarity provide assurance, but the current code leaves substantive reviewer concerns unresolved. Address the default-access, scoped-key and unpinned-source concerns with @PostHog/team-warehouse-sources, with Daniel RC as a suggested reviewer, before re-requesting.
- Author wrote 96% of the modified lines and has 41 merged PRs in these paths (familiarity STRONG).
- PostHog Review (Standard) reviewed the current head: no findings.
- @bill-ph and @PostHog, products/warehouse_sources/backend/temporal/data_imports/sources/teachable/source.py: the default remains v2, while source_setup.py validates credentials without a selectable version and creates sources with the default pin. Schools whose keys cannot access v2 still have no v1 setup fallback. Retain a usable v1 default or implement explicit version selection before re-requesting.
- @PostHog, products/warehouse_sources/backend/temporal/data_imports/sources/teachable/teachable.py: v2 credential validation still always probes courses; source.py ignores schema_name when validating. A users-only key rejected by that probe cannot connect despite being usable for its selected table. Make validation respect the requested resources and address this thread before re-requesting.
- @parameterai[bot], discussion comment: common/base.py resolves missing pins through the default, and the current tests explicitly confirm unpinned Teachable sources now use v2. There is no pin-preservation migration in this diff, so existing unpinned connections can switch to an inaccessible API. Preserve their v1 behavior or obtain explicit acceptance of this concern from a non-author reviewer.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 244L, 4F substantive, 431L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (431L, 6F, two-areas, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ b5d8239 · reviewed head b5d8239 |
fuziontech
left a comment
fuziontech
left a comment
There was a problem hiding this comment.
Nice work on adding Teachable v2 support! Clean separation of v2 endpoints while leaving the beta ones (transactions, quizzes, pricing_plans) on v1, and keeping existing v1 pins fully backwards compatible. Good test coverage across versions as well.
Left a couple minor non-blocking notes inline.
…tial check v2 keys carry per-resource scopes, so the credential check now probes the endpoint the validated table reads, and at creation accepts a key that can read either the course or the user list. Also declares frozen dataclasses for the Teachable endpoint and resume configs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4462ff72-5db5-460e-b686-69deec4edba9
|
On the unpinned-source concern (Sentinel and stamphog): a Teachable row should not have a NULL pin. The API create path stamps |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Owning-team authorship and STRONG familiarity provide assurance for the ingestion changes, but the substantive setup-access concern remains unresolved in the current code. Address @PostHog's and @bill-ph's concern with @PostHog/team-warehouse-sources before re-requesting; Daniel RC is a suggested reviewer.
- Author wrote 97% of the modified lines and has 41 merged PRs in these paths (familiarity STRONG).
- PostHog Review (Standard) reviewed the current head: no findings.
- @PostHog's inline concern and @bill-ph's older review concern in products/warehouse_sources/backend/temporal/data_imports/sources/teachable/source.py remain unaddressed: the default is still v2, and credential validation fails when both v2 probes return 403. source_setup.py validates before creation, exposes the version as read-only, and stamps the default, leaving no v1 setup choice for schools without v2 access. Retain a usable v1 default or provide an explicit setup choice, then ask @PostHog/team-warehouse-sources to resolve the concern before re-requesting.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 272L, 4F substantive, 496L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (496L, 6F, two-areas, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ 7ca2196 · reviewed head 7ca2196 |
Problem
Warning
Teachable labels v2 a beta that Teachable Support turns on for each school. A new source fails its credential check for a school without v2 access. Decide whether the default flip waits.
Changes
v2. Existingv1pins send the same requests as before.v2, users, courses, enrollments, course progress and lectures read v2 endpoints. The other tables stay on v1, because their v2 endpoints are marked beta.sale_id.How did you test this code?
Test rationale: New v2 cases cover paging, per-version paths, row shaping and resume.
Not checked: a live Teachable account. The v2 shapes come from the docs.
Release status
Docs update
🤖 Agent context
Autonomy: Fully autonomous
Agent: Claude Code, claude-opus-5-5
Skills:
/warehouse-source-new-version,/writing-tests,/writing-user-facing-copy,/writing-pr-descriptionsMore context for AI agents
user/saleids that the fan-out helper cannot flatten.Created with PostHog Desktop from a PostHog Desktop task
🤖 Generated with Claude Code