Skip to content

feat(loaders): migrate Connect exports to v2 paginated JSON - #127

Merged
snopoke merged 43 commits into
dimagi-rad:mainfrom
jjackson:emdash/connect-updates-7gr
Apr 11, 2026
Merged

feat(loaders): migrate Connect exports to v2 paginated JSON#127
snopoke merged 43 commits into
dimagi-rad:mainfrom
jjackson:emdash/connect-updates-7gr

Conversation

@jjackson

@jjackson jjackson commented Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

CommCare Connect deprecated its streaming CSV export endpoints in favor of keyset-paginated JSON (Accept: application/json; version=2.0) to eliminate the 5-minute gunicorn worker timeout on large UserVisit exports. This PR migrates scout's seven Connect list-export loaders to follow the v2 contract: GET, then walk the next URL until null.

The downstream _write_connect_* writers in materializer.py are unchanged — scalars are stringified to preserve the v1 CSV-shaped TEXT-column contract, and form_json / images pass through as native dict/list for the JSONB columns. No DB migrations, no consumer-facing changes.

What changed

mcp_server/loaders/connect_base.py

  • New _paginate_export_pages(suffix, params) — yields page lists, follows the server's next URL until null, sends the versioned Accept header per call so ConnectMetadataLoader's non-versioned endpoints (/export/opp_org_program_list/, /export/opportunity/<id>/) stay untouched.
  • New stringify / stringify_record helpers and a ConnectExportError exception type.
  • Removed _get_csv and the csv / io imports.

mcp_server/loaders/connect_visits.py

  • Rewrote _normalize_visit for v2 dict input. Dropped the ast.literal_eval Python-repr fallback — form_json is now a real dict from the wire.
  • Preserved the idvisit_id rename so the existing writer schema is unchanged.
  • Falls back to the loader's opportunity_id when the serializer omits it (matches connect-labs behavior).

6 simple loaders (users, completed_works, payments, invoices, assessments, completed_modules)

  • Thin paginate-then-stringify loops. Now stream page-by-page rather than buffering the full export — free memory win for large opportunities.

Regression pin for dimagi/commcare-connect#1109

While running this work end-to-end against connect.dimagi.com we hit a server bug: the production gunicorn config defaults --forwarded-allow-ips to 127.0.0.1, so it strips X-Forwarded-Proto: https from Traefik. Django's request.build_absolute_uri() then falls back to http, and IdKeysetPagination.get_next_link() returns next URLs with the http:// scheme even on HTTPS requests. Clients following those URLs receive a 301 redirect.

#1109 is the upstream fix; until it lands, scout has to tolerate the bug. The mitigation here:

  • requests.Session.get defaults to allow_redirects=True, so we follow the edge's 301 http→https automatically.
  • requests.Session.should_strip_auth has a special case for same-host HTTP→HTTPS upgrades on default ports that preserves the Authorization header — verified empirically before adding the test.
  • An inline comment in _paginate_export_pages documents the dependency and links to #1109.
  • A new test test_follows_http_to_https_redirect_on_next_url simulates the bug end-to-end (page 1 returns next: http://..., mock 301-redirects to https, page 2 returns final results) and asserts both row aggregation and that the bearer token survives the redirect. This pins both behaviors so a future change (e.g., switching to httpx, or someone passing allow_redirects=False) cannot silently regress.

Reference implementation: connect-labs PR #55 — same migration on the labs side, where the mitigation is httpx's follow_redirects=True (httpx default is False).

Test plan

  • uv run pytest tests/test_connect_base_loader.py tests/test_connect_data_loaders.py56 passed (12 base + 13 visit + 31 simple-loader matrix)
  • uv run pytest tests/ --ignore=tests/qa797 passed, 15 skipped, 0 regressions
  • uv run ruff check + ruff format --check — clean
  • Manual verification that ConnectMetadataLoader is unaffected (the versioned Accept header is per-call, not session-global, and a base-loader test pins this)
  • After merge: end-to-end materialization run against a real opportunity to confirm the writer contract holds with native v2 JSON types

🤖 Generated with Claude Code

Jonathan Jackson and others added 30 commits March 18, 2026 06:34
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add VITE_BASE_PATH and NGINX_CONF build args to Dockerfile.frontend
so connect-labs can build with /scout/ prefix. Add nginx.prod.conf
with /scout/ prefix stripping for the production deploy.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set SOCIALACCOUNT_LOGIN_ON_GET = True so clicking an OAuth button
goes straight to the provider instead of showing an unstyled
intermediate confirmation page.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CommCare Connect's API doesn't return an email, causing allauth to
show an unstyled signup form asking for one. Set
SOCIALACCOUNT_EMAIL_REQUIRED = False so OAuth users are auto-created
without needing an email address.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Required by the ECS task definition which sets
DJANGO_SETTINGS_MODULE=config.settings.connectlabs.
Inherits production settings with ALB-specific overrides.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The API client, chat transport, public pages, and onboarding wizard
all used bare /api/ and /accounts/ paths. Under a subpath deploy
like /scout/, these 404 because nginx only proxies /scout/api/ to
the backend.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The providers API returns bare /accounts/... URLs which 404 under
a subpath deploy. Prefix both the href and the next redirect with
BASE_PATH.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Playwright smoke test for labs.connect.dimagi.com end-to-end flow
- Implementation plan for embed enhancements (reference doc)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace RegExp-based BASE_PATH stripping with startsWith/slice to
  avoid regex metacharacter bugs (e.g. dots in /scout.v2)
- Revert SOCIALACCOUNT_LOGIN_ON_GET to False to prevent Login CSRF
  (the popup OAuth flow doesn't need it)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CommCare Connect users may not have an email. The User model had
email as unique+required, so the second email-less user caused a
UniqueViolation on the empty string. Now email is nullable — empty
values are stored as NULL (PostgreSQL allows multiple NULLs in a
unique column). Includes a data migration to convert existing empty
emails.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The popup is user-initiated so Login CSRF isn't a concern. With this
set to False, users see an unstyled allauth confirmation page inside
the popup before being redirected to the provider.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cross-origin OAuth redirects (Scout → CommCare Connect → Scout) can
clear window.opener in modern browsers, preventing the popup from
auto-closing after login.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The popup OAuth URL was missing the BASE_PATH prefix (/scout),
so the popup navigated to the wrong path and fell through to
ConnectLabs instead of Scout's allauth login endpoint.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The popup was loading the full React app before the popup_close check
ran, causing a visible flash. An inline script in index.html runs
before any CSS/JS loads and closes the popup immediately.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The allauth OAuth flow drops the `next` query param, so
`popup_close=1` never reaches the final redirect URL. Instead,
detect the popup by its window name ("scout-oauth") which persists
across all navigations including cross-origin OAuth redirects.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The widget SDK now intercepts scout:auth-required and opens the OAuth
popup directly to the provider login URL with window.name="scout-oauth".
This bypasses the host app's auth overlay which was opening a popup to
the standalone Scout app instead of the OAuth endpoint. After the popup
closes, the iframe reloads to pick up the authenticated session.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Auth popup

Dockerfile: Separate dependency install from code copy so Docker layer
cache skips the slow uv pip install step when only code changes (not
pyproject.toml/uv.lock). This mirrors the frontend Dockerfile pattern.

widget.js: Handle scout:auth-required directly in the SDK by opening
the OAuth popup to the provider login URL with window.name="scout-oauth".
Previously the host app (ConnectLabs) was opening a popup to standalone
Scout instead of the OAuth endpoint, bypassing all popup close logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stop forwarding scout:auth-required to the host app (ConnectLabs).
The host was showing its own auth overlay that hid Scout's LoginForm,
then opening a popup to standalone Scout instead of the OAuth endpoint.

Now the widget SDK suppresses the event so Scout's own LoginForm stays
visible in the iframe. The LoginForm's popup OAuth flow handles auth
correctly — opening directly to the provider with window.name="scout-oauth"
and auto-closing via the inline script in index.html.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Django doesn't prepend FORCE_SCRIPT_NAME to LOGIN_REDIRECT_URL, so
after OAuth the popup was redirecting to / (ConnectLabs) instead of
/scout/ (Scout). Now it lands on Scout's index.html where the inline
script detects window.name="scout-oauth" and closes the popup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CommCare Connect's COOP headers clear window.name during cross-origin
OAuth redirects. Use a cookie instead — set before opening the popup,
checked in index.html when the popup returns to Scout after OAuth.
The cookie is cleared immediately after detection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allauth DOES preserve the next param through the OAuth flow, so the
popup lands at /scout/embed/?popup_close=1. Check both the cookie
and the query param to close the popup — the query param works when
allauth preserves next, the cookie is a fallback for when it doesn't.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Allow nullable email for OAuth users (CommCare Connect users may not have email)
- Re-enable SOCIALACCOUNT_LOGIN_ON_GET to skip allauth confirmation page
- Widget SDK handles auth internally instead of delegating to host overlay
- Fix popup OAuth URL with BASE_PATH prefix
- Set LOGIN_REDIRECT_URL to /scout/ so popup returns to Scout after OAuth
- Auto-close popup via popup_close query param and cookie fallback
- Optimize backend Dockerfile for faster code-only deploys
Revert LOGIN_ON_GET to False to prevent Login CSRF attacks (attacker
can craft a link that initiates OAuth and logs victim into attacker's
account). Instead, the popup auto-submits the allauth confirmation
form via JS — the form has a CSRF token so it's protected, and the
user doesn't see the "Continue" button.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverts LOGIN_ON_GET to False (prevents Login CSRF) and simplifies
the popup flow back to the original pattern that worked reliably:

1. Embed LoginForm opens popup to standalone Scout (/scout/)
2. User sees normal Scout login form in popup, clicks OAuth provider
3. OAuth completes, popup loads authenticated Scout
4. App.tsx detects cookie + authenticated state -> window.close()
5. Iframe polls for popup close -> fetchMe() -> authenticated

Removes: inline index.html scripts, auto-form-submit hacks,
popup_close query params, window.name detection. Uses a simple
cookie to identify popup windows instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The allauth confirmation page ("Sign In Via CommCare Connect" + Continue
button) adds no value — the user already clicked an OAuth button, and
the OAuth provider has its own authorize screen. Keeping it off caused
the popup to get stuck on an unstyled intermediate page.

Login CSRF risk is mitigated by the provider's own authorization screen
which requires explicit user consent.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ource_queries prompt (#1)

Two bugs found during artifact walkthrough testing:

1. Single-row query results caused `rows.map is not a function` because
   mergeQueryResults() converted single-row results to objects. Components
   always expect arrays. Now always returns arrays regardless of row count.

2. Agent sometimes embedded static data instead of using source_queries,
   breaking the "always-fresh data" contract. Strengthened the artifact
   prompt: CRITICAL language, removed the "< 5 rows" exception, added
   explicit "NEVER embed" instruction, updated docs to reflect arrays-only.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jjackson and others added 11 commits March 25, 2026 19:21
Sidebar links (/, /knowledge, /artifacts, etc.) didn't account for the
embed router's /embed prefix, causing React Router 404 on every click
in the ConnectLabs iframe. Detect embed mode via useEmbedParams and
prefix all navigation paths accordingly. Also add missing embed routes
(data-dictionary, settings/connections) and a catch-all redirect.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ial use

When multiple users share a workspace, run_materialization could pick up
a different user's OAuth token. Now user_id is injected server-side (same
as workspace_id) and used to filter the TenantMembership query, ensuring
each user's MCP calls use only their own credentials.

Addresses review feedback from @snopoke on PR dimagi-rad#107.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The embedded login had three compounding problems: the popup opened
Scout's root (showing the login form twice), no feedback while the
popup was open, and third-party cookie blocking prevented the session
from reaching the iframe.

Replace the cookie-based popup detection approach with the standard
embedded OAuth pattern: popup goes directly to the OAuth provider,
a lightweight callback page sends a signed one-time token via
postMessage, and the iframe exchanges it for a session with
SameSite=None cookies.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-5ow

# Conflicts:
#	apps/users/auth_views.py
#	frontend/src/components/LoginForm/LoginForm.tsx
#	frontend/src/components/Sidebar/Sidebar.tsx
#	frontend/src/pages/EmbedPage.tsx
#	mcp_server/server.py
Two fixes for the embed OAuth flow:

1. When allauth doesn't preserve the `next` URL through OAuth and the
   popup loads the full app, App.tsx now detects the same-origin opener
   and redirects to /auth/popup-complete/ automatically.

2. fetchMe() no longer sets authStatus to "loading" during re-checks
   (only on initial idle state). This prevents EmbedPage from unmounting
   LoginForm during visibilitychange, which was losing the popup spinner.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The popup approach was unnecessary — Scout and connect-labs are on the
same domain, so there are no cross-site cookie issues. Replace the
entire popup/postMessage/token-exchange mechanism with a simple
target="_top" link that navigates the full page through OAuth, then
returns to the connect-labs embed page.

This removes ~290 lines: popup-complete view, token-exchange endpoint,
postMessage handling, window.opener detection, spinner state management.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The providers API was prepending FORCE_SCRIPT_NAME to login_url, but
the frontend already prepends BASE_PATH. This produced /scout/scout/...
URLs that hit the SPA fallback instead of reaching Django's allauth.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ated JSON

CommCare Connect deprecated its streaming CSV export endpoints in favor of
keyset-paginated JSON (Accept: application/json; version=2.0) to avoid the
5-minute gunicorn worker timeout on large UserVisit exports. Scout's seven
list-export loaders now follow the v2 contract: GET, then walk the `next`
URL until null.

Changes
- ConnectBaseLoader: add `_paginate_export_pages()` (yields page lists,
  follows `next`, sends versioned Accept header per-call so the metadata
  loader's non-versioned endpoints stay untouched), plus `stringify` /
  `stringify_record` helpers and `ConnectExportError`. Drop `_get_csv` and
  the csv/io imports.
- ConnectVisitLoader: rewrite `_normalize_visit` for v2 dict input, drop
  `ast.literal_eval` Python-repr fallback (form_json is now a real dict),
  preserve the `id` → `visit_id` rename, fall back to the loader's
  opportunity_id when the serializer omits it.
- 6 simple loaders (users, completed_works, payments, invoices,
  assessments, completed_modules): thin paginate-then-stringify loops.
  Now stream page-by-page rather than buffering the full export.

The downstream `_write_connect_*` writers in materializer.py are unchanged
— scalars are stringified to preserve the v1 CSV-shaped TEXT-column
contract; `form_json` and `images` pass through as native dict/list for
the JSONB columns.

Regression pin for dimagi/commcare-connect#1109
- Production has been observed returning `next` URLs with the `http://`
  scheme even on HTTPS requests (gunicorn `--forwarded-allow-ips` defaults
  to 127.0.0.1, strips `X-Forwarded-Proto`). Until #1109 lands, scout
  relies on requests' default `allow_redirects=True` to follow the edge's
  301 → https, and on `Session.should_strip_auth`'s same-host HTTP→HTTPS
  upgrade exception to preserve the bearer token across the redirect.
- Inline comment in `_paginate_export_pages` documents the dependency.
- New test `test_follows_http_to_https_redirect_on_next_url` simulates the
  bug end-to-end and asserts both row aggregation and bearer-token
  preservation.

Tests
- 56 loader tests pass (12 base + 13 visit + 31 simple-loader matrix).
- Full repo: 797 passed, 15 skipped, 0 regressions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erializations

Symptom: "network error" in the chat UI when running run_materialization
on a ~70k-row opportunity (tenant 765). CloudWatch
/ecs/labs-jj-scout-web 2026-04-08 10:30:35 UTC:

  [error] *3796 upstream timed out (110: Operation timed out)
  while reading upstream, request: "POST /scout/api/chat/",
  upstream: "http://127.0.0.1:8000/api/chat/"

Root cause: The /scout/api/ nginx location in frontend/nginx.prod.conf
sets proxy_buffering off and chunked_transfer_encoding on (correct for
SSE) but does not set proxy_read_timeout or proxy_send_timeout, so nginx
inherits its 60s defaults. The chat SSE stream emits no bytes during
synchronous MCP tool calls, and run_pipeline's step-based progress
callback only fires between sources — the first visits load runs ~2-4
minutes silent for a 70k opp. Nginx sees >60s of upstream silence and
kills the connection at ~10:30:35, right on the default. The backend
materialization keeps running, then crashes downstream with
anyio.ClosedResourceError trying to push MCP progress notifications
through the now-closed stream.

The v2 JSON pagination migration (commit 3ed1364) fixed the downstream
leg (scout → commcare-connect bounded per page), but the upstream leg
(client → nginx → scout) still runs synchronously inside one request.
We only caught this testing the v2 migration on a large opp — the
~8k-visit opp 874 fit inside the 60s window and masked the issue.

Fix: Set proxy_read_timeout and proxy_send_timeout to 600s on the
/scout/api/ location, matching the ALB idle_timeout.timeout_seconds we
already have configured on labs-jj-alb (verified via aws elbv2
describe-load-balancer-attributes). Nginx is no longer the bottleneck
up to ~10 minutes of synchronous materialization, which handles every
realistic opp size including the 70k-row tenant 765 case.

Deliberately NOT fixed here (follow-up work):
  - Gunicorn workers still block for the full materialization duration,
    reducing backend concurrency under load.
  - The chat SSE stream still has no heartbeat during tool calls, so
    users see a stalled UI for minutes with no feedback.
  - For opps that exceed 10 minutes of materialization, the architectural
    fix is to move run_materialization to Celery (scout already has
    REDIS_URL and a get_materialization_status tool) and have the chat
    agent poll for status. Tracked as upstream work.

This is a frontend-only rebuild — NGINX_CONF is baked into the frontend
image at build time via the Dockerfile.frontend NGINX_CONF build arg.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Structural sync with dimagi-rad/scout upstream so future upstream pulls
are clean. 31 upstream commits landed; 0 merge conflicts. Our v2 Connect
loader migration (3ed1364) and nginx timeout fix (9e06ee3) are preserved
untouched.

Takeaways from upstream:
- **Bug fix, relevant to us:** d55b01e "Fix MCP query context: URL-decode
  DB password and require SSL" — mcp_server/context.py now calls
  unquote() on tenant DB username/password before passing to psycopg, and
  sets sslmode=require. Fixes silent auth failures on tenants with special
  characters in their RDS password.
- **Hardening, no-op for us:** 4c8d47f "MCP server allowed_hosts for
  scout-mcp-web" — mcp_server/server.py now passes an explicit
  TransportSecuritySettings with scout-mcp-web:* added alongside loopback
  hosts. Our ECS setup uses 127.0.0.1, so this is harmless.
- **Dockerfile:** uv pinned to 0.7.12@sha256 (reproducible builds),
  collectstatic runs at build time (avoids per-container startup cost),
  default CMD switched to uvicorn. Our ECS task def overrides the CMD
  via sh -c "migrate && uvicorn ...", so the CMD change is a no-op for
  our deployment. Verified via aws ecs describe-task-definition.
- **Kamal deployment infrastructure (14 new files):** .kamal/*,
  config/deploy*.yml, frontend/nginx.prod-kamal.conf, infra/scout-stack.yml,
  DEPLOYMENT.md, scripts/*.sh, registry_password.sh,
  .github/workflows/deploy.yml. All parallel to our existing ECS deploy
  via deploy-labs.yml. Dead files in our fork but harmless — and keeping
  them avoids rebase pain on future upstream pulls.

Verification:
- 56/56 Connect loader tests pass (pre-merge 56 → post-merge 56)
- Full suite: 798 passed, 15 skipped, 0 regressions (pre-merge 797 → 798,
  upstream added one new passing test)
- grep confirmed our _paginate_export_pages, EXPORT_ACCEPT_HEADER, and
  proxy_*_timeout 600s lines survived the merge intact.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
for page in self._paginate_export_pages("assessment/"):
if not page:
continue
stringified = [stringify_record(r) for r in page]

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.

Why is everything being converted to strings? We'll loose all the type information by doing this.

The root issue is that the connect tables all have TEXT columns. We should update these tables to use the correct data types.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fair point, you're right. I was treating this as a behavior-preserving refactor from CSV to JSON and preserved the TEXT-column contract via stringification, but that punted on the actual data-modeling fix you're describing.

Addressed in 234d043. Summary:

Loader side: deleted stringify and stringify_record from connect_base.py. The 6 simple loaders now collapse to thin pass-throughs that yield pages directly from _paginate_export_pages. connect_visits.py::_normalize_visit stops wrapping scalars in stringify() — it still does the id → visit_id rename, the opportunity_id fallback, and the defensive dict/list coercion for form_json/images, but everything else flows through untouched.

Writer side: every raw_* table's CREATE TABLE DDL in materializer.py updated to use proper types. Summary of what changed:

  • IDs → BIGINT (visit_id as PK, opportunity_id across all tables)
  • Booleans → BOOLEAN (flagged, confirmed, suspended, passed)
  • Money → NUMERIC(14, 2) (amounts, payment_accrued, saved_payment_accrued*, saved_org_payment_accrued*)
  • exchange_rateNUMERIC(14, 6) for sub-cent precision
  • Dates/datetimes → TIMESTAMPTZ (visit_date, date_created, status_modified_date, last_active, created_at, etc.)
  • raw_invoices.dateDATE (invoices are day-level)
  • Counts → INTEGER (saved_completed_count, saved_approved_count, score, passing_score, duration)

Business identifiers with ambiguous upstream shape stay TEXT (deliver_unit_id, entity_id, payment_unit_id, completed_work_id, invoice_id, invoice_number), since we don't control whether those are ints, UUIDs, or slugs. String columns stay TEXT.

Insert tuples changed from r.get("field", "") to r.get("field") on typed columns so missing keys bind to SQL NULL via psycopg instead of passing an empty string that'd fail the cast. Text columns keep the "" default for backwards compat.

No migration needed: the writers already DROP TABLE IF EXISTS ... CASCADE + CREATE TABLE on every materialization, so the next sync for any tenant recreates their schema with the new types inside the same transaction. No backfill script, no per-tenant DDL loop.

Downstream audit: grepped the whole repo for SQL that reads raw_* assuming TEXT values. Clean — no DBT assets reference these tables, no recipes, no agent prompt templates hard-code types, no Python result-processing code. The chat agent's LLM-generated SQL will actually improve since it no longer needs ::numeric casts or flagged = 'True' string comparisons.

Tests: test_flagged_bool_stringifiedtest_flagged_bool_passes_through (asserts is True). test_native_types_stringifiedtest_native_types_preserved with a per-field type round-trip check. Added two visit tests to pin specific risks: test_flagged_false_not_coerced_to_none (guards against the False or default falsy trap on nullable booleans), and test_missing_datetime_field_is_none (verifies null datetime → Python None → SQL NULL). 58 loader tests pass, full repo suite 800/800.

Thanks for the push — the query ergonomics are going to be much better this way.

jjackson and others added 2 commits April 9, 2026 12:39
…imagi-rad#127 feedback from @snopoke)

Simon's review on dimagi-rad#127:

> Why is everything being converted to strings? We'll loose all the type
> information by doing this.
>
> The root issue is that the connect tables all have TEXT columns. We
> should update these tables to use the correct data types.

He's right. The original migration (commit 3ed1364) stringified v2 JSON
scalars in the loaders to preserve the existing TEXT-column writer
contract — a scope-discipline choice that kept the blast radius small
but punted on the data-modeling fix. This commit unpunts it.

Because the materializer's writers DROP+CREATE the raw_* tables on every
materialization, there is no schema migration or data backfill: the next
sync for any tenant recreates the tables with the new types automatically
inside the same transaction. (Per user: scout is still in heavy active
development, no migration handling required.)

Changes
- mcp_server/loaders/connect_base.py: delete `stringify` and
  `stringify_record` helpers. `_paginate_export_pages` and the rest of
  the base class are unchanged.
- 6 simple loaders (users, completed_works, payments, invoices,
  assessments, completed_modules): stop calling `stringify_record`, just
  `yield page` directly. Each loader now collapses to a 10-line
  pass-through over `_paginate_export_pages`.
- mcp_server/loaders/connect_visits.py: `_normalize_visit` stops wrapping
  scalars in `stringify()`. Keeps the `id` → `visit_id` rename, the
  `opportunity_id` fallback to the loader's own int, and the defensive
  dict/list coercion for `form_json`/`images`.
- mcp_server/services/materializer.py: DDL + insert tuples updated for
  all 7 raw_* tables. Per-table type choices:

    raw_visits:
      visit_id                       TEXT     -> BIGINT PRIMARY KEY
      opportunity_id                 TEXT     -> BIGINT
      visit_date                     TEXT     -> TIMESTAMPTZ
      flagged                        TEXT     -> BOOLEAN
      status_modified_date           TEXT     -> TIMESTAMPTZ
      review_created_on              TEXT     -> TIMESTAMPTZ
      date_created                   TEXT     -> TIMESTAMPTZ
    raw_users:
      date_learn_started             TEXT     -> TIMESTAMPTZ
      payment_accrued                TEXT     -> NUMERIC(14, 2)
      suspended                      TEXT     -> BOOLEAN
      suspension_date                TEXT     -> TIMESTAMPTZ
      invited_date                   TEXT     -> TIMESTAMPTZ
      completed_learn_date           TEXT     -> TIMESTAMPTZ
      last_active                    TEXT     -> TIMESTAMPTZ
      date_claimed                   TEXT     -> TIMESTAMPTZ
    raw_completed_works:
      opportunity_id                 TEXT     -> BIGINT
      last_modified                  TEXT     -> TIMESTAMPTZ
      status_modified_date           TEXT     -> TIMESTAMPTZ
      payment_date                   TEXT     -> TIMESTAMPTZ
      date_created                   TEXT     -> TIMESTAMPTZ
      saved_completed_count          TEXT     -> INTEGER
      saved_approved_count           TEXT     -> INTEGER
      saved_payment_accrued          TEXT     -> NUMERIC(14, 2)
      saved_payment_accrued_usd      TEXT     -> NUMERIC(14, 2)
      saved_org_payment_accrued      TEXT     -> NUMERIC(14, 2)
      saved_org_payment_accrued_usd  TEXT     -> NUMERIC(14, 2)
    raw_payments:
      opportunity_id                 TEXT     -> BIGINT
      created_at                     TEXT     -> TIMESTAMPTZ
      amount, amount_usd             TEXT     -> NUMERIC(14, 2)
      date_paid, confirmation_date   TEXT     -> TIMESTAMPTZ
      confirmed                      TEXT     -> BOOLEAN
    raw_invoices:
      opportunity_id                 TEXT     -> BIGINT
      amount, amount_usd             TEXT     -> NUMERIC(14, 2)
      date                           TEXT     -> DATE (invoices are day-level)
      exchange_rate                  TEXT     -> NUMERIC(14, 6) for sub-cent precision
    raw_assessments:
      opportunity_id                 TEXT     -> BIGINT
      date                           TEXT     -> TIMESTAMPTZ
      score, passing_score           TEXT     -> INTEGER
      passed                         TEXT     -> BOOLEAN
    raw_completed_modules:
      opportunity_id                 TEXT     -> BIGINT
      date                           TEXT     -> TIMESTAMPTZ
      duration                       TEXT     -> INTEGER

  String-typed columns stay TEXT. Business IDs with ambiguous shape
  (`deliver_unit_id`, `entity_id`, `payment_unit_id`, `completed_work_id`,
  `invoice_id`, `invoice_number`, etc.) stay TEXT — we don't control
  their upstream representation and they might be UUIDs or slugs.

  Insert tuples changed from `r.get("field", "")` to `r.get("field")` for
  typed columns so missing keys bind to SQL NULL via psycopg, rather than
  passing an empty string that would fail to cast. Text columns keep the
  "" default for backwards-compat with downstream code that might rely on
  empty-string-not-null.

- tests/test_connect_data_loaders.py: the stringification assertions
  flip to native-type assertions. `test_flagged_bool_stringified` becomes
  `test_flagged_bool_passes_through` (asserts `rows[0]["flagged"] is True`).
  `test_native_types_stringified` becomes `test_native_types_preserved`
  and verifies each record value round-trips with the same type. Two new
  visit tests pin specific risks:
    - `test_flagged_false_not_coerced_to_none` guards against the
      `False or default` falsy trap when handling nullable booleans.
    - `test_missing_datetime_field_is_none` verifies that a null
      datetime in the JSON becomes Python None (not ""), so psycopg
      binds it to SQL NULL against the TIMESTAMPTZ column.

Downstream audit: grepped the full codebase for SQL that reads raw_*
assuming TEXT values. No DBT assets, no recipes, no agent prompt
templates, no Python result-processing code had TEXT assumptions. The
chat agent's LLM-generated SQL will actually improve — typed columns
mean it no longer needs to wrap amounts in ::numeric or compare
flagged = 'True' as string literals.

Verification
- 58 loader tests pass (was 56; added the two new visit tests).
- Full repo suite: 800 passed, 15 skipped, 0 regressions (was 798 pre-change).
- ruff check + format clean.
- Manual review of all 7 writer DDLs and insert tuples.
- Cannot unit-test the writer SQL without the test DB executing real
  INSERT statements, so the typed-column binding will be validated by
  the post-deploy production materialization retry on opp 874.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d043

Production materialization on tenant 765 crashed with
``psycopg.ProgrammingError: cannot adapt type 'dict' using placeholder '%s'``
(CloudWatch /ecs/labs-jj-scout-web 2026-04-09 20:18:57 UTC). The traceback
points at _write_connect_visits line 604 executemany.

Root cause: when I did the typed-columns refactor in 234d043 I only read
the UserVisit model far enough to see ``flagged`` and ``visit_date`` and
assumed the rest were plain CharFields. Wrong. A full walkthrough of
``commcare_connect/data_export/serializer.py`` and the underlying Django
models reveals 15 column type mismatches across all 7 raw_* tables. Every
one would crash or store wrong data on some tenant. My smoke test on opp
874 only worked because that opp had no flagged visits (so flag_reason
was always None and never hit psycopg's adapt check).

The 15 fixes:

raw_visits (5):
  flag_reason       TEXT -> JSONB     # Django JSONField, DRF returns dict
  deliver_unit      TEXT -> BIGINT    # ForeignKey, DRF renders PK int
  completed_work    TEXT -> BIGINT    # ForeignKey, DRF renders PK int
  completed_work_id TEXT -> BIGINT    # raw FK _id column, int
  deliver_unit_id   TEXT -> BIGINT    # raw FK _id column, int

raw_users (1):
  claim_limits      TEXT -> JSONB     # SerializerMethodField -> list[dict]

raw_completed_works (1):
  payment_unit_id   TEXT -> BIGINT    # raw FK _id column, int

raw_payments (2):
  payment_unit      TEXT -> BIGINT    # ForeignKey, int
  invoice_id        TEXT -> BIGINT    # raw FK _id column, int

raw_invoices (2):
  service_delivery  TEXT -> BOOLEAN   # actually PaymentInvoice.service_delivery
                                      # is a BooleanField, not a text label
  exchange_rate     NUMERIC -> BIGINT # actually a ForeignKey to
                                      # ExchangeRate lookup table, so it's
                                      # the FK PK not the numeric rate. My
                                      # 234d043 NUMERIC(14,6) guess was
                                      # semantically wrong too.

raw_assessments (1):
  app               TEXT -> BIGINT    # ForeignKey to CommCareApp, int

raw_completed_modules (2):
  module            TEXT -> BIGINT    # ForeignKey to LearnModule, int
  duration          INTEGER -> TEXT   # Django DurationField, DRF serializes
                                      # as string "H:MM:SS". Going to TEXT
                                      # for now — INTERVAL is the honest
                                      # type but needs more validation
                                      # against real payloads.

Writer changes
- New helper ``_json_or_none(value)`` in materializer.py for nullable
  JSONB columns. Preserves Python None → SQL NULL (json.dumps(None)
  would produce the string "null" which inserts as JSONB null, not
  SQL NULL — different semantics).
- ``_write_connect_visits`` uses ``_json_or_none`` for ``flag_reason``.
- ``_write_connect_users`` uses ``_json_or_none`` for ``claim_limits``.
- All FK/_id fields drop the ``r.get("field", "")`` empty-string default
  in favor of ``r.get("field")`` so missing keys bind to SQL NULL via
  psycopg (same pattern used for the already-typed columns in 234d043).

Test fixture updates
- ``_visit_record`` in test_connect_data_loaders.py now matches the real
  DRF output: deliver_unit/completed_work/_id fields are ints, flag_reason
  is None by default (populated in the new dict test).
- ``SIMPLE_LOADER_CASES`` matrix updated to match real serializer output
  per a walkthrough of serializer.py. payment_accrued is an IntegerField
  so it's now ``100`` not ``"100.00"``; claim_limits is now a list of
  dicts from get_claim_limits; service_delivery is a bool; exchange_rate
  is an int (FK); duration is the Django DurationField string "0:30:00";
  all FK fields across the matrix are now ints.
- Two new regression tests in TestConnectVisitLoader:
    test_flag_reason_dict_passes_through — pins the tenant 765 bug
    test_fk_ids_are_ints — pins the FK-as-int loader contract

Verification
- 60 loader tests pass (was 58; added the two visit regression pins).
- Full repo suite: 802 passed, 15 skipped, 0 regressions.
- ruff check + format clean.
- Manual walkthrough of every serializer field against the Django model
  + DRF field class to confirm the type mapping for each of the 15 fixes.

Gap I want to own: there's still no integration test that runs the
writer against a real Postgres and round-trips typed data. 234d043
shipped because the loader tests used requests_mock and never exercised
executemany. Adding a writer smoke test suite is the right follow-up so
the next bug like this fails in CI, not in production CloudWatch.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jjackson

jjackson commented Apr 9, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up: production validation and a bug fix worth naming

After 234d043 (the typed-columns change per your review, @snopoke) I shipped it to our fork's deployment and did a smoke test on a 70k+ visit opp — it crashed immediately with psycopg.ProgrammingError: cannot adapt type 'dict' using placeholder '%s' in _write_connect_visits.

Root cause: when I did the typed-columns rewrite I only read the UserVisit model far enough to see flagged and visit_date, and I assumed the rest of the scalar fields were plain CharFields. Wrong. A full walkthrough of commcare_connect/data_export/serializer.py against the underlying Django models revealed 15 column type mismatches across all 7 raw_ tables*. Every one was either a hidden JSONField, a ForeignKey that DRF's default ModelSerializer renders as the related PK int, a DurationField that serializes to a string like "0:30:00", or PaymentInvoice.service_delivery which I had misread as a label when it's actually a BooleanField. Fixed in 811a625. The specific corrections:

  • raw_visits: flag_reason TEXT → JSONB (it's a JSONField, not a CharField); deliver_unit, completed_work, deliver_unit_id, completed_work_id TEXT → BIGINT
  • raw_users: claim_limits TEXT → JSONB (it's a SerializerMethodField that returns [dict(row) for row in data])
  • raw_completed_works: payment_unit_id TEXT → BIGINT
  • raw_payments: payment_unit, invoice_id TEXT → BIGINT
  • raw_invoices: service_delivery TEXT → BOOLEAN; exchange_rate NUMERIC(14,6) → BIGINT (it's a ForeignKey(ExchangeRate), not the numeric rate — my previous NUMERIC guess was semantically wrong too)
  • raw_assessments: app TEXT → BIGINT
  • raw_completed_modules: module TEXT → BIGINT; duration INTEGER → TEXT (DRF's DurationField.to_representation returns strings like "0:30:00")

Added a _json_or_none helper for the nullable JSONB columns because json.dumps(None) produces the literal string "null" which inserts as JSONB null — not the same as SQL NULL. Worth being explicit about that distinction in the writer so nullable JSONFields round-trip correctly.

Production validation

After deploying 811a625, two back-to-back materializations on the same tenant 765 (76,512 visits, the one that originally crashed) completed cleanly:

Run Elapsed HTTP response Backend errors
1 3m 42s POST /scout/api/chat/ 200 5735 0
2 3m 50s POST /scout/api/chat/ 200 5196 0

Zero ERROR / Traceback / Pipeline failed / psycopg events in CloudWatch across both windows. Chat agent confirmed row counts populated and query-able with native types on the second run.

~77 paginated v2 JSON requests per run under the 600s nginx proxy_read_timeout I also raised in 9e06ee3 — the same bug class that made the sync-blocking-gunicorn problem go away for our Labs deployment (details on that one in the 9e06ee3 commit message; it's fork-local infrastructure config so not strictly upstream-relevant, but useful context).

Gap I want to own for the reviewer

The loader tests use requests_mock and never exercise the writer against a real Postgres. That's why 234d043 shipped with 15 wrong column types and my initial smoke test on a smaller opp didn't notice — the smaller opp had no flagged visits, so flag_reason was always None and never hit psycopg's adapt check. Adding a writer integration test suite that runs the full _write_connect_* pipeline against a throwaway schema with realistic fixtures is the right follow-up and is on my list. Flagged in the 811a625 commit message too.

Current PR head is 811a625 and fully validated against production data. Happy to take another round of review if you have cycles.

@snopoke
snopoke merged commit ebfaade into dimagi-rad:main Apr 11, 2026
6 of 7 checks passed
@jjackson
jjackson deleted the emdash/connect-updates-7gr branch April 17, 2026 08:20
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.

2 participants