feat: E2E testing infrastructure with Playwright#34
Merged
Conversation
Add Playwright-based E2E test for the audit_with_ai_review workflow template, exercising the full pipeline: create workflow, create run, select visits, trigger async audit via Celery, poll completion, and verify sessions. Infrastructure: - conftest.py: Django server + Celery worker subprocesses, OAuth session injection, queue purge for stale tasks - views_test_auth.py: DEBUG-only view to inject CLI token into session - LEARNINGS.md: hard-won lessons from Windows/Playwright/Celery Bug fixes discovered during E2E development: - Fix type mismatch in fetch_raw_visits: visit IDs from CharField (strings) vs CSV parsing (ints) caused silent filter miss -> 0 images - Fix SSE BaseSSEStreamView.get() missing **kwargs - Add include_images parameter to pipeline/backend for image extraction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis pull request establishes an end-to-end testing infrastructure for workflow templates using Playwright against a Django runserver with real OAuth tokens and a Celery worker, along with comprehensive design and planning documentation for KMC templates, MCP server integration, and workflow documentation refactoring. Changes
Sequence DiagramsequenceDiagram
participant Pytest as Pytest Runner
participant Conftest as E2E Conftest
participant Django as Django Server<br/>(port 8001)
participant Celery as Celery Worker
participant Browser as Playwright<br/>Browser
participant TestAuth as /labs/test-auth<br/>View
participant Test as E2E Test<br/>Class
Pytest->>Conftest: fixture: live_server_url (session)
Conftest->>Django: check port 8001 available
Conftest->>Django: start runserver
Conftest->>Django: wait for readiness
Pytest->>Conftest: fixture: celery_worker (session)
Conftest->>Celery: purge stale tasks
Conftest->>Celery: start worker (solo pool)
Conftest->>Celery: wait for readiness
Pytest->>Conftest: fixture: authenticated_context (session)
Conftest->>Browser: create browser context
Conftest->>TestAuth: navigate to /labs/test-auth/
TestAuth->>TestAuth: inject OAuth session (TokenManager)
TestAuth->>Conftest: preserve session cookies
Pytest->>Conftest: fixture: auth_page
Conftest->>Browser: create fresh page from<br/>authenticated context
Pytest->>Test: run test_full_audit_workflow()
Test->>Browser: navigate to workflow UI
Test->>Browser: validate React rendering
Test->>Browser: configure audit settings
Test->>Django: trigger audit creation (API)
Test->>Django: await audit completion
Test->>Browser: validate results
Test->>Django: cleanup run (API)
Test-->>Pytest: test complete
Pytest->>Conftest: teardown celery_worker
Conftest->>Celery: stop worker
Pytest->>Conftest: teardown live_server_url
Conftest->>Django: stop server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commcare_connect/labs/analysis/backends/sql/backend.py (1)
117-125:⚠️ Potential issue | 🟠 MajorDon’t reuse an image-less raw cache for
include_images=True.
has_valid_raw_cache()is still count-based only, so a prior slim/raw fetch can populateRawVisitCachewithoutimagesand this branch will still return a cache hit for the later image-extraction path. In the audit flow that meansfetch_visits_for_ids()can still see emptyimageseven after opting in here.Safe fallback until cache validity distinguishes image-bearing rows
- if not force_refresh and expected_visit_count: + use_cache = not include_images + if use_cache and not force_refresh and expected_visit_count: if cache_manager.has_valid_raw_cache(expected_visit_count, tolerance_pct=tolerance_pct): logger.info(f"[SQL] Raw cache HIT for opp {opportunity_id} (tolerance={tolerance_pct}%)") return self._load_from_cache(cache_manager, skip_form_json, filter_visit_ids)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/labs/analysis/backends/sql/backend.py` around lines 117 - 125, The current logic treats any valid raw cache as a hit even when include_images=True, causing image-less cached rows to be returned; update the cache check so when include_images is True you only accept a cache hit if the cached RawVisitCache actually contains images (e.g. add/use a flag or method like cache_manager.has_images_in_raw_cache or extend has_valid_raw_cache(require_images=True)); if that check fails fall through to the API fetch path and then _load_from_cache can be used only when the cache meets the image requirement. Ensure references: has_valid_raw_cache, cache_manager, include_images, _load_from_cache, _fetch_from_api, and RawVisitCache.
🧹 Nitpick comments (4)
commcare_connect/data_export/serializer.py (1)
124-134: Clean implementation of conditional image serialization.The approach of checking for
_prefetched_imagesand falling back toobj.imagesis a good pattern for supporting both prefetched batch operations and direct access.Consider the static analysis suggestion to use list unpacking for a slightly more idiomatic approach:
♻️ Optional: Use list unpacking instead of concatenation
class Meta(UserVisitDataSerializer.Meta): - fields = UserVisitDataSerializer.Meta.fields + ["images"] + fields = [*UserVisitDataSerializer.Meta.fields, "images"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/data_export/serializer.py` around lines 124 - 134, The Meta.fields definition currently concatenates lists; change it to use list unpacking for clarity and immutability: replace fields = UserVisitDataSerializer.Meta.fields + ["images"] with fields = [*UserVisitDataSerializer.Meta.fields, "images"] inside the UserVisitDataWithImagesSerializer.Meta; keep get_images and the _prefetched_images fallback as-is.commcare_connect/data_export/tests/test_user_visit_export.py (1)
63-71: Minor: Prefix unused variable with underscore.The
rowsvariable is unpacked but never used. Prefix it with_to signal intent and silence the linter.♻️ Fix unused variable
- rows, fieldnames = _parse_csv_response(response) + _, fieldnames = _parse_csv_response(response) assert "images" not in fieldnames🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/data_export/tests/test_user_visit_export.py` around lines 63 - 71, In test_images_false_excludes_images_column, the unpacked variable rows from the call to _parse_csv_response is unused; rename it to _rows (or prefix with an underscore) to indicate it is intentionally unused and silence the linter—i.e., update the unpacking in test_images_false_excludes_images_column where response = api_client.get(self._get_url(opportunity.id, images="false")) and rows, fieldnames = _parse_csv_response(response) to use _rows, fieldnames = _parse_csv_response(response).commcare_connect/templates/workflow/run.html (2)
19-19: Move stylesheet link to head block for proper document structure.The
<link>stylesheet tag is placed inside the{% block content %}which renders in<body>. For valid HTML and optimal loading, stylesheets should be in<head>.♻️ Proposed fix
Move the Leaflet CSS into the
{% block javascript %}(or add a dedicated{% block css %}if base.html supports it):{% block javascript %} {{ block.super }} {% if has_context and render_code and not error %} <script src="{% static 'bundles/js/workflow-runner-bundle.js' %}?v=3" defer></script> +<!-- Leaflet CSS for map visualizations --> +<link rel="stylesheet" href="https://unpkg.com/leaflet@1.9.4/dist/leaflet.css" crossorigin=""/> <!-- Chart.js for workflow visualizations -->{% block content %} -<link rel="stylesheet" href="https://unpkg.com/leaflet@1.9.4/dist/leaflet.css" crossorigin=""/> <div class="max-w-full mx-auto px-4 py-6" style="overflow-x: clip">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/templates/workflow/run.html` at line 19, The Leaflet stylesheet link tag (<link rel="stylesheet" href="https://unpkg.com/leaflet@1.9.4/dist/leaflet.css" crossorigin=""/> ) is currently inside the `{% block content %}`; move this stylesheet tag into a head-level block so it renders inside `<head>` instead of `<body>`—either place it in `{% block javascript %}` if that block is rendered in the head or add/use a dedicated `{% block css %}` in the template (and update run.html to insert the link there) so the stylesheet is loaded from the head.
10-14: Add Subresource Integrity (SRI) hashes to external CDN scripts for improved security.Loading scripts without SRI hashes means a compromised CDN could serve malicious JavaScript. Adding
integrityattributes provides cryptographic verification that the scripts haven't been tampered with.Suggested fix with SRI hashes
<!-- Chart.js for workflow visualizations --> -<script src="https://cdn.jsdelivr.net/npm/chart.js@4.4.0/dist/chart.umd.min.js"></script> -<script src="https://cdn.jsdelivr.net/npm/chartjs-adapter-date-fns@3.0.0/dist/chartjs-adapter-date-fns.bundle.min.js"></script> +<script src="https://cdn.jsdelivr.net/npm/chart.js@4.4.0/dist/chart.umd.min.js" integrity="sha384-e6nUZLBkQ86NJ6TVVKAeSaK8jWa3NhkYWZFomE39AvDbQWeie9PlQqM3pmYW5d1g" crossorigin="anonymous"></script> +<script src="https://cdn.jsdelivr.net/npm/chartjs-adapter-date-fns@3.0.0/dist/chartjs-adapter-date-fns.bundle.min.js" integrity="sha384-cVMg8E3QFwTvGCDuK+ET4PD341jF3W8nO1auiXfuZNQkzbUUiBGLsIQUE+b1mxws" crossorigin="anonymous"></script> <!-- Leaflet for map visualizations --> -<script src="https://unpkg.com/leaflet@1.9.4/dist/leaflet.js" crossorigin=""></script> +<script src="https://unpkg.com/leaflet@1.9.4/dist/leaflet.js" integrity="sha384-cxOPjt7s7Iz04uaHJceBmS+qpjv2JkIHNVcuOrM+YHwZOmJGBXI00mdUXEq65HTH" crossorigin="anonymous"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commcare_connect/templates/workflow/run.html` around lines 10 - 14, The external script tags for Chart.js ("https://cdn.jsdelivr.net/npm/chart.js@4.4.0/dist/chart.umd.min.js"), chartjs-adapter-date-fns ("https://cdn.jsdelivr.net/npm/chartjs-adapter-date-fns@3.0.0/dist/chartjs-adapter-date-fns.bundle.min.js") and Leaflet ("https://unpkg.com/leaflet@1.9.4/dist/leaflet.js") need Subresource Integrity (SRI) attributes and consistent CORS to prevent tampered CDN content; update each <script> element (the three script tags in run.html) to include a correct integrity="sha384-..." value for the exact file version and add crossorigin="anonymous" (or ensure existing crossorigin is "anonymous") so the browser can verify the SRI hash at load time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commcare_connect/flags/README.md`:
- Line 36: Update the README sentence about creating new switches to tighten
wording: change "prior to deploy" to "before deployment" and change "followup"
to "follow-up" in the sentence describing WAFFLE_CREATE_MISSING_SWITCHES
auto-creating switches (also apply the same fixes to the other occurrence
mentioned); ensure the phrasing reads smoothly (e.g., "for new switches, they
can be created before deployment via Django Admin or added via migration with
the release... Description should be added as a follow-up if created
automatically") while keeping the reference to WAFFLE_CREATE_MISSING_SWITCHES
intact.
In `@commcare_connect/form_receiver/processor.py`:
- Around line 73-74: _guard _get_matching_blocks against matches that lack the
"@xmlns" key by only selecting match.value entries that are mapping-like and
contain "@xmlns" before indexing; update the comprehension in
_get_matching_blocks (uses variables jsonpath, xform and constant
CCC_LEARN_XMLNS) to filter out non-dict or missing-key matches so you don't
access match.value["@xmlns"] unguarded and thus avoid KeyError escaping the
JSONPathError handling in process_learn_form.
- Line 176: The Celery task is being dispatched inline via
notify_user_for_scored_assessment.delay(assessment.pk), which can race with the
DB transaction; wrap the publication in transaction.on_commit(...) so the task
is only queued after the Assessment row is committed (same approach used for the
attachment task near line 368). Ensure you import transaction from django.db if
not already present and call transaction.on_commit(lambda:
notify_user_for_scored_assessment.delay(assessment.pk)) referencing
notify_user_for_scored_assessment and assessment.pk.
In `@commcare_connect/labs/urls.py`:
- Around line 21-22: The route mounting path("test-auth/",
views_test_auth.test_auth_view, name="test_auth") is currently registered
unconditionally and needs to be wrapped so it is only added when settings.DEBUG
is True; modify commcare_connect/labs/urls.py to check settings.DEBUG before
calling urlpatterns.append(...) or include the path inside an if settings.DEBUG:
block, and also ensure the corresponding entry in the middleware/public_paths
list (the public_paths configuration that currently allows the test-auth
pattern) is added only when settings.DEBUG is True so the public/unauthenticated
middleware rule is gated the same way as the view.
In `@commcare_connect/opportunity/tasks.py`:
- Around line 714-724: The notification payload currently hardcodes English for
the "title" and "body" fields when constructing Message (in the block that sets
data with keys "action", "key", "opportunity_status", "opportunity_id",
"opportunity_uuid", "title", "body"); change it to use the same localization
helper used elsewhere in this module (e.g., the module's _()/gettext/_lazy
function or a build_notification_text helper) so both title and body are
translated, and interpolate opportunity.name and any variables via the
localization call rather than f-strings, preserving the existing data keys and
values like "opportunity_id" and "opportunity_uuid".
In `@commcare_connect/organization/decorators.py`:
- Line 88: The decorator currently calls view_func(request, org_slug=org_slug,
opp_id=opp_id, *args, **kwargs) which can raise TypeError when the wrapped view
accepts extra positional args; to fix, inject org_slug and opp_id into the
keyword args before calling the view: update kwargs with {'org_slug': org_slug,
'opp_id': opp_id} and then call view_func(request, *args, **kwargs). Locate the
call in decorators.py (the view_func invocation) and replace the direct keyword
placement with a pre-call kwargs.update(...) then call view_func using *args and
**kwargs so functions like edit_payment_unit, payment_delete, fetch_attachment,
user_visit_details, etc. no longer get conflicting arguments.
In `@commcare_connect/templates/email/base.html`:
- Around line 1-2: The HTML lang attribute is hardcoded to "en"; change it to
use Django's current language instead—add the template tag {%
get_current_language as LANGUAGE_CODE %} (or use the LANGUAGE_CODE context
variable if already provided) and replace lang="en" with lang="{{ LANGUAGE_CODE
}}" on the <html> element so the email language is correct for translated
content and assistive technologies.
In `@commcare_connect/templates/email/base.txt`:
- Around line 1-6: The plain-text footer in the email base template is hardcoded
in English; update the footer to use Django template translation (e.g., use {%
trans %} or {% blocktrans %}) instead of literal English so it matches the
localized HTML footer and avoids mixed-language multipart emails; modify the
footer text around the existing {% block content %} in templates/email/base.txt
to wrap "Thank you, Connect" and "This inbox is not monitored. Please do not
respond to this email." with the appropriate translation tags.
In `@commcare_connect/workflow/templates/kmc_longitudinal.py`:
- Around line 1247-1248: When switching the selected child you must reset the
selected visit index to avoid an out-of-range selection; add a React effect that
calls setSelectedVisitIdx(0) whenever selectedChildId changes (e.g., place a
React.useEffect(() => setSelectedVisitIdx(0), [selectedChildId]) inside the
component that defines selectedChildId and setSelectedVisitIdx). Ensure this is
added alongside existing navigation handling (in addition to handleBackToList)
so both the “All Children” tab flow and direct child-selection clicks (the code
that calls setSelectedChildId(...) and setCurrentView('timeline')) will reset
the visit index; apply the same change in the second similar location described
(the other child-selection block).
- Around line 1023-1029: The clickable visit item currently implemented as a div
with onClick and className (using setSelectedVisitIdx and comparing
selectedVisitIdx) must be made keyboard-operable: replace the interactive divs
with semantic interactive elements (e.g., a <button> or <a>) or move the onClick
into a focusable control inside the row; ensure the control uses the same
setSelectedVisitIdx handler and visual selected state (selectedVisitIdx) and
keep styling consistent, and apply the same change pattern to similar spots (the
other visit items and KPI cards and table header/row actions referenced around
the other ranges). Also ensure table header/row actions are implemented on
focusable controls inside th/tr (not on the th/tr themselves) and preserve
existing aria attributes/labels for screen readers.
- Around line 981-983: The code currently prefixes weight changes with a
hard-coded '+' causing outputs like "+-50g" for negative values; update the
rendering logic around the weight display (where child.weightGain and
weightGainPct are used) to derive the sign from the numeric value instead of
hard-coding '+'. Concretely, change the string construction in the template that
references child.weightGain and weightGainPct to prepend a computed sign (e.g.,
'' for zero/positive or '-' for negative, or '+' only when value > 0) and apply
the same sign logic to weightGainPct; ensure you update both occurrences (the
block using child.weightGain and the similar code at the other occurrence around
lines 1242-1243) so weight loss is displayed correctly.
- Around line 17-20: The Active bucket's predicates currently only exclude
"discharged" so entries with status "lost_to_followup" are still counted as
Active; update every active predicate/condition that currently filters out
"discharged" to also exclude "lost_to_followup" (i.e., add "lost_to_followup"
alongside "discharged" in the exclusion checks), referencing the "statuses" list
and all occurrences of the Active predicates (the blocks that build the Active
KPI/active list) so the "lost_to_followup" status is not counted as Active.
In `@commcare_connect/workflow/tests/e2e/conftest.py`:
- Around line 67-70: The current e2e fixture reuses whatever is bound to
E2E_PORT when the port check returns result == 0; change this so the fixture
fails fast instead: in the function that performs the port check (the e2e server
fixture in conftest.py around the result == 0 branch) raise an explicit
exception (e.g., RuntimeError) explaining that port is in use and tests will not
proceed, unless an explicit opt-in is set (introduce and check an env var like
REUSE_EXISTING_E2E_SERVER or similar); if the opt-in is set, allow the existing
behavior of yielding f"http://{E2E_HOST}:{E2E_PORT}", otherwise raise with a
clear error message and include which host/port were occupied so CI/devs can
resolve it.
In `@commcare_connect/workflow/tests/e2e/test_audit_workflow.py`:
- Around line 152-164: Capture the run_id as soon as it is discovered (e.g.,
where run_id_match is computed) and move the deletion logic into a finally block
or a pytest fixture finalizer so it always executes even on timeouts/assertion
failures; in that finally ensure you retrieve the CSRF token (from page.evaluate
or stored data), call page.request.post to DELETE
f"{live_server_url}/labs/workflow/api/run/{run_id}/delete/?opportunity_id={opportunity_id}"
using headers={"X-CSRFToken": csrf_token}, and assert the response indicates
success (check response.status or similar) so failures are surfaced.
- Around line 72-74: The test currently clicks a generic Create Run link via
create_run_link = page.get_by_role("link", name="Create Run").first which may
target an older workflow; instead capture the created workflow's ID/URL from the
POST response that creates the audit workflow and navigate directly to that
specific record. Replace the .first selection and click with code that reads the
create-workflow POST response (where the API call is made in your test), extract
the returned workflow ID or URL, then use page.goto(created_workflow_url) or
page.get_by_role("link", name="Create
Run").filter(has=page.get_by_attribute("href", created_workflow_url)) to
navigate to the exact workflow; update references to create_run_link and any
subsequent assertions to use the explicitly-targeted URL/ID.
- Around line 142-150: The test currently skips verification when no sessions
are present because of the conditional around sessions_header; change it to fail
if the sessions view doesn't appear by replacing the conditional with an
explicit assertion that sessions_header is visible (use
expect(sessions_header).to_be_visible(timeout=5_000) or assert
sessions_header.is_visible(timeout=5_000)), then continue to locate
sessions_text and review_links and assert review_links.count() > 0;
alternatively, if zero sessions are valid for this dataset, pin the test data or
add an explicit assertion that zero sessions are expected instead of silently
passing.
---
Outside diff comments:
In `@commcare_connect/labs/analysis/backends/sql/backend.py`:
- Around line 117-125: The current logic treats any valid raw cache as a hit
even when include_images=True, causing image-less cached rows to be returned;
update the cache check so when include_images is True you only accept a cache
hit if the cached RawVisitCache actually contains images (e.g. add/use a flag or
method like cache_manager.has_images_in_raw_cache or extend
has_valid_raw_cache(require_images=True)); if that check fails fall through to
the API fetch path and then _load_from_cache can be used only when the cache
meets the image requirement. Ensure references: has_valid_raw_cache,
cache_manager, include_images, _load_from_cache, _fetch_from_api, and
RawVisitCache.
---
Nitpick comments:
In `@commcare_connect/data_export/serializer.py`:
- Around line 124-134: The Meta.fields definition currently concatenates lists;
change it to use list unpacking for clarity and immutability: replace fields =
UserVisitDataSerializer.Meta.fields + ["images"] with fields =
[*UserVisitDataSerializer.Meta.fields, "images"] inside the
UserVisitDataWithImagesSerializer.Meta; keep get_images and the
_prefetched_images fallback as-is.
In `@commcare_connect/data_export/tests/test_user_visit_export.py`:
- Around line 63-71: In test_images_false_excludes_images_column, the unpacked
variable rows from the call to _parse_csv_response is unused; rename it to _rows
(or prefix with an underscore) to indicate it is intentionally unused and
silence the linter—i.e., update the unpacking in
test_images_false_excludes_images_column where response =
api_client.get(self._get_url(opportunity.id, images="false")) and rows,
fieldnames = _parse_csv_response(response) to use _rows, fieldnames =
_parse_csv_response(response).
In `@commcare_connect/templates/workflow/run.html`:
- Line 19: The Leaflet stylesheet link tag (<link rel="stylesheet"
href="https://unpkg.com/leaflet@1.9.4/dist/leaflet.css" crossorigin=""/> ) is
currently inside the `{% block content %}`; move this stylesheet tag into a
head-level block so it renders inside `<head>` instead of `<body>`—either place
it in `{% block javascript %}` if that block is rendered in the head or add/use
a dedicated `{% block css %}` in the template (and update run.html to insert the
link there) so the stylesheet is loaded from the head.
- Around line 10-14: The external script tags for Chart.js
("https://cdn.jsdelivr.net/npm/chart.js@4.4.0/dist/chart.umd.min.js"),
chartjs-adapter-date-fns
("https://cdn.jsdelivr.net/npm/chartjs-adapter-date-fns@3.0.0/dist/chartjs-adapter-date-fns.bundle.min.js")
and Leaflet ("https://unpkg.com/leaflet@1.9.4/dist/leaflet.js") need Subresource
Integrity (SRI) attributes and consistent CORS to prevent tampered CDN content;
update each <script> element (the three script tags in run.html) to include a
correct integrity="sha384-..." value for the exact file version and add
crossorigin="anonymous" (or ensure existing crossorigin is "anonymous") so the
browser can verify the SRI hash at load time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3812974e-44cf-45a9-8a53-f90fead8ce51
📒 Files selected for processing (53)
commcare_connect/audit/data_access.pycommcare_connect/data_export/serializer.pycommcare_connect/data_export/tests/__init__.pycommcare_connect/data_export/tests/test_user_visit_export.pycommcare_connect/data_export/views.pycommcare_connect/flags/README.mdcommcare_connect/form_receiver/processor.pycommcare_connect/form_receiver/tests/test_process_xform.pycommcare_connect/labs/analysis/backends/sql/backend.pycommcare_connect/labs/analysis/pipeline.pycommcare_connect/labs/analysis/sse_streaming.pycommcare_connect/labs/middleware.pycommcare_connect/labs/urls.pycommcare_connect/labs/views_test_auth.pycommcare_connect/microplanning/const.pycommcare_connect/microplanning/urls.pycommcare_connect/microplanning/views.pycommcare_connect/opportunity/tasks.pycommcare_connect/opportunity/tests/test_tasks.pycommcare_connect/organization/decorators.pycommcare_connect/organization/tasks.pycommcare_connect/templates/email/base.htmlcommcare_connect/templates/email/base.txtcommcare_connect/templates/microplanning/home.htmlcommcare_connect/templates/microplanning/map_handler.htmlcommcare_connect/templates/opportunity/email/automated_invoice_created.htmlcommcare_connect/templates/opportunity/email/automated_invoice_created.txtcommcare_connect/templates/opportunity/email/invoice_paid.htmlcommcare_connect/templates/opportunity/email/invoice_paid.txtcommcare_connect/templates/program/email/monthly_delivery_reminder.htmlcommcare_connect/templates/program/email/monthly_delivery_reminder.txtcommcare_connect/templates/program/email/opportunity_created.htmlcommcare_connect/templates/program/email/opportunity_created.txtcommcare_connect/templates/program/email/program_invite_applied.htmlcommcare_connect/templates/program/email/program_invite_applied.txtcommcare_connect/templates/program/email/program_invite_notification.htmlcommcare_connect/templates/program/email/program_invite_notification.txtcommcare_connect/templates/workflow/run.htmlcommcare_connect/workflow/templates/__init__.pycommcare_connect/workflow/templates/kmc_longitudinal.pycommcare_connect/workflow/tests/e2e/LEARNINGS.mdcommcare_connect/workflow/tests/e2e/__init__.pycommcare_connect/workflow/tests/e2e/conftest.pycommcare_connect/workflow/tests/e2e/test_audit_workflow.pyconfig/settings/base.pydocker-compose.ymldocs/plans/2026-03-06-kmc-longitudinal-implementation.mddocs/plans/2026-03-06-kmc-longitudinal-workflow-design.mdpyproject.tomlrequirements/base.inrequirements/base.txtrequirements/dev.intailwind/safelists.txt
|
|
||
| - switch names are added in the file `switch_names.py` | ||
| - use `switch_is_active` to check if switch is enabled | ||
| - for new switches, they can be created prior to deploy via Django Admin or added via migration with the release. However, `WAFFLE_CREATE_MISSING_SWITCHES` is set to automatically add new switches to database when they are encountered in the codebase. Description should be added as a followup if created automatically. |
There was a problem hiding this comment.
Tighten the deploy wording and fix followup.
These lines read a bit awkwardly in docs. I'd change prior to deploy to before deploy/before deployment, and followup to follow-up.
✏️ Suggested doc tweak
-- for new switches, they can be created prior to deploy via Django Admin or added via migration with the release. However, `WAFFLE_CREATE_MISSING_SWITCHES` is set to automatically add new switches to database when they are encountered in the codebase. Description should be added as a followup if created automatically.
+- for new switches, they can be created before deployment via Django Admin or added via migration with the release. However, `WAFFLE_CREATE_MISSING_SWITCHES` is set to automatically add new switches to the database when they are encountered in the codebase. A description should be added as a follow-up if created automatically.
…
-- for new flags, they **should** be created on the relevant environment prior to deploy via Django Admin or added via a migration with the release
+- for new flags, they **should** be created on the relevant environment before deployment via Django Admin or added via a migration with the releaseAlso applies to: 44-44
🧰 Tools
🪛 LanguageTool
[style] ~36-~36: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...- for new switches, they can be created prior to deploy via Django Admin or added via mi...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~36-~36: Ensure spelling is correct
Context: ...ebase. Description should be added as a followup if created automatically. ### Flags C...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/flags/README.md` at line 36, Update the README sentence
about creating new switches to tighten wording: change "prior to deploy" to
"before deployment" and change "followup" to "follow-up" in the sentence
describing WAFFLE_CREATE_MISSING_SWITCHES auto-creating switches (also apply the
same fixes to the other occurrence mentioned); ensure the phrasing reads
smoothly (e.g., "for new switches, they can be created before deployment via
Django Admin or added via migration with the release... Description should be
added as a follow-up if created automatically") while keeping the reference to
WAFFLE_CREATE_MISSING_SWITCHES intact.
Comment on lines
+73
to
+74
| def _get_matching_blocks(jsonpath, xform): | ||
| return [match.value for match in jsonpath.find(xform.form) if match.value["@xmlns"] == CCC_LEARN_XMLNS] |
There was a problem hiding this comment.
Guard _get_matching_blocks() against matches without @xmlns.
This indexes match.value["@xmlns"] directly. If the JSONPath returns a block without that key, this raises KeyError, bypasses the JSONPathError handling in process_learn_form, and aborts the whole submission.
Defensive filter for mixed JSONPath matches
def _get_matching_blocks(jsonpath, xform):
- return [match.value for match in jsonpath.find(xform.form) if match.value["@xmlns"] == CCC_LEARN_XMLNS]
+ return [
+ match.value
+ for match in jsonpath.find(xform.form)
+ if isinstance(match.value, dict) and match.value.get("@xmlns") == CCC_LEARN_XMLNS
+ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/form_receiver/processor.py` around lines 73 - 74, _guard
_get_matching_blocks against matches that lack the "@xmlns" key by only
selecting match.value entries that are mapping-like and contain "@xmlns" before
indexing; update the comprehension in _get_matching_blocks (uses variables
jsonpath, xform and constant CCC_LEARN_XMLNS) to filter out non-dict or
missing-key matches so you don't access match.value["@xmlns"] unguarded and thus
avoid KeyError escaping the JSONPathError handling in process_learn_form.
| if not created: | ||
| return ProcessingError("Learn Assessment is already completed") | ||
|
|
||
| notify_user_for_scored_assessment.delay(assessment.pk) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n commcare_connect/form_receiver/processor.py | sed -n '170,185p'Repository: jjackson/commcare-connect
Length of output: 743
🏁 Script executed:
grep -n "transaction.on_commit" commcare_connect/form_receiver/processor.pyRepository: jjackson/commcare-connect
Length of output: 161
🏁 Script executed:
head -40 commcare_connect/form_receiver/processor.pyRepository: jjackson/commcare-connect
Length of output: 1409
Queue the assessment notification after commit.
Dispatching the Celery job inline can race the worker against the Assessment insert or fire even if a surrounding transaction rolls back. Use transaction.on_commit(...) here, like the attachment task at line 368.
Delay task publication until the row is committed
- notify_user_for_scored_assessment.delay(assessment.pk)
+ transaction.on_commit(partial(notify_user_for_scored_assessment.delay, assessment.pk))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| notify_user_for_scored_assessment.delay(assessment.pk) | |
| transaction.on_commit(partial(notify_user_for_scored_assessment.delay, assessment.pk)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/form_receiver/processor.py` at line 176, The Celery task is
being dispatched inline via
notify_user_for_scored_assessment.delay(assessment.pk), which can race with the
DB transaction; wrap the publication in transaction.on_commit(...) so the task
is only queued after the Assessment row is committed (same approach used for the
attachment task near line 368). Ensure you import transaction from django.db if
not already present and call transaction.on_commit(lambda:
notify_user_for_scored_assessment.delay(assessment.pk)) referencing
notify_user_for_scored_assessment and assessment.pk.
Comment on lines
+21
to
+22
| # E2E test auth (DEBUG only) | ||
| path("test-auth/", views_test_auth.test_auth_view, name="test_auth"), |
There was a problem hiding this comment.
Make /labs/test-auth/ truly DEBUG-only.
test_auth_view checks settings.DEBUG, but Line 22 still mounts the route in every labs deploy, and the middleware change makes it public before the handler runs. That means non-debug environments get a new unauthenticated code path instead of preserving the existing auth/redirect behavior. Please register this URL only when settings.DEBUG is true, and gate the matching public_paths entry the same way.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/labs/urls.py` around lines 21 - 22, The route mounting
path("test-auth/", views_test_auth.test_auth_view, name="test_auth") is
currently registered unconditionally and needs to be wrapped so it is only added
when settings.DEBUG is True; modify commcare_connect/labs/urls.py to check
settings.DEBUG before calling urlpatterns.append(...) or include the path inside
an if settings.DEBUG: block, and also ensure the corresponding entry in the
middleware/public_paths list (the public_paths configuration that currently
allows the test-auth pattern) is added only when settings.DEBUG is True so the
public/unauthenticated middleware rule is gated the same way as the view.
Comment on lines
+714
to
+724
| message = Message( | ||
| usernames=[user.username], | ||
| data={ | ||
| "action": "ccc_generic_opportunity", | ||
| "key": "scored_assessment", | ||
| "opportunity_status": "learn", | ||
| "opportunity_id": str(opportunity.id), # added for backward compatibility | ||
| "opportunity_uuid": str(opportunity.opportunity_id), | ||
| "title": "Update on your Assessment", | ||
| "body": f"Assessment for opportunity '{opportunity.name}' scored, check your status", | ||
| }, |
There was a problem hiding this comment.
Localize this notification payload like the others in this module.
This is user-facing copy, but unlike the surrounding notification builders it is hardcoded English. That will create a mixed-language experience for translated users.
Suggested fix
assessment = Assessment.objects.get(pk=assessment_pk)
user = assessment.user
opportunity = assessment.opportunity
+ title = gettext("Update on your Assessment")
+ body = gettext("Assessment for opportunity '%(opportunity)s' scored, check your status") % {
+ "opportunity": opportunity.name,
+ }
message = Message(
usernames=[user.username],
data={
"action": "ccc_generic_opportunity",
"key": "scored_assessment",
"opportunity_status": "learn",
"opportunity_id": str(opportunity.id), # added for backward compatibility
"opportunity_uuid": str(opportunity.opportunity_id),
- "title": "Update on your Assessment",
- "body": f"Assessment for opportunity '{opportunity.name}' scored, check your status",
+ "title": title,
+ "body": body,
},
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/opportunity/tasks.py` around lines 714 - 724, The
notification payload currently hardcodes English for the "title" and "body"
fields when constructing Message (in the block that sets data with keys
"action", "key", "opportunity_status", "opportunity_id", "opportunity_uuid",
"title", "body"); change it to use the same localization helper used elsewhere
in this module (e.g., the module's _()/gettext/_lazy function or a
build_notification_text helper) so both title and body are translated, and
interpolate opportunity.name and any variables via the localization call rather
than f-strings, preserving the existing data keys and values like
"opportunity_id" and "opportunity_uuid".
Comment on lines
+1247
to
+1248
| onClick={() => { setSelectedChildId(child.beneficiary_case_id); setCurrentView('timeline'); }} | ||
| className="hover:bg-gray-50 cursor-pointer border-b border-gray-100" |
There was a problem hiding this comment.
Reset the selected visit when opening a different child.
selectedVisitIdx only resets via handleBackToList(). If a user leaves a timeline through the “All Children” tab and opens another child, the old index can be out of range and the new timeline loads with no visit selected.
💡 Minimal fix
React.useEffect(() => {
setSelectedVisitIdx(0);
}, [selectedChildId]);Also applies to: 1293-1295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/workflow/templates/kmc_longitudinal.py` around lines 1247 -
1248, When switching the selected child you must reset the selected visit index
to avoid an out-of-range selection; add a React effect that calls
setSelectedVisitIdx(0) whenever selectedChildId changes (e.g., place a
React.useEffect(() => setSelectedVisitIdx(0), [selectedChildId]) inside the
component that defines selectedChildId and setSelectedVisitIdx). Ensure this is
added alongside existing navigation handling (in addition to handleBackToList)
so both the “All Children” tab flow and direct child-selection clicks (the code
that calls setSelectedChildId(...) and setCurrentView('timeline')) will reset
the visit index; apply the same change in the second similar location described
(the other child-selection block).
Comment on lines
+67
to
+70
| if result == 0: | ||
| # Port already in use — assume dev server is running, reuse it | ||
| yield f"http://{E2E_HOST}:{E2E_PORT}" | ||
| return |
There was a problem hiding this comment.
Fail fast when port 8001 is already occupied.
These lines currently reuse whatever process is bound there, so the suite can run against stale code or a server started without the env vars this fixture sets. That makes the E2E environment uncontrolled and the results hard to trust. Prefer raising here, or require an explicit opt-in to reuse an existing server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/workflow/tests/e2e/conftest.py` around lines 67 - 70, The
current e2e fixture reuses whatever is bound to E2E_PORT when the port check
returns result == 0; change this so the fixture fails fast instead: in the
function that performs the port check (the e2e server fixture in conftest.py
around the result == 0 branch) raise an explicit exception (e.g., RuntimeError)
explaining that port is in use and tests will not proceed, unless an explicit
opt-in is set (introduce and check an env var like REUSE_EXISTING_E2E_SERVER or
similar); if the opt-in is set, allow the existing behavior of yielding
f"http://{E2E_HOST}:{E2E_PORT}", otherwise raise with a clear error message and
include which host/port were occupied so CI/devs can resolve it.
Comment on lines
+72
to
+74
| create_run_link = page.get_by_role("link", name="Create Run").first | ||
| expect(create_run_link).to_be_visible(timeout=10_000) | ||
| create_run_link.click() |
There was a problem hiding this comment.
Target the workflow you just created, not .first.
After the create call, Line 72 can open any existing audit workflow card if older E2E data is still present. That makes the rest of the test run against stale records instead of the workflow created in this test. Capture the created workflow ID/URL from the POST response and navigate to that record explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/workflow/tests/e2e/test_audit_workflow.py` around lines 72 -
74, The test currently clicks a generic Create Run link via create_run_link =
page.get_by_role("link", name="Create Run").first which may target an older
workflow; instead capture the created workflow's ID/URL from the POST response
that creates the audit workflow and navigate directly to that specific record.
Replace the .first selection and click with code that reads the create-workflow
POST response (where the API call is made in your test), extract the returned
workflow ID or URL, then use page.goto(created_workflow_url) or
page.get_by_role("link", name="Create
Run").filter(has=page.get_by_attribute("href", created_workflow_url)) to
navigate to the exact workflow; update references to create_run_link and any
subsequent assertions to use the explicitly-targeted URL/ID.
Comment on lines
+142
to
+150
| # Check if sessions were created (depends on opportunity having image data) | ||
| sessions_header = page.get_by_text("Audit Sessions Created") | ||
| if sessions_header.is_visible(timeout=5_000): | ||
| # Sessions exist — verify the full results | ||
| sessions_text = page.get_by_text(re.compile(r"\d+ sessions? linked to this workflow run")) | ||
| expect(sessions_text).to_be_visible(timeout=5_000) | ||
|
|
||
| review_links = page.get_by_role("link", name="Review") | ||
| assert review_links.count() > 0, "Expected at least one audit session with a Review link" |
There was a problem hiding this comment.
Don't let a zero-session audit count as success.
This branch only verifies the results when sessions happen to exist, so the test still passes if the regression returns 0 visits/images and no sessions are created. For this happy-path E2E, assert that the sessions view appears, or pin the test to data where zero sessions is explicitly expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/workflow/tests/e2e/test_audit_workflow.py` around lines 142
- 150, The test currently skips verification when no sessions are present
because of the conditional around sessions_header; change it to fail if the
sessions view doesn't appear by replacing the conditional with an explicit
assertion that sessions_header is visible (use
expect(sessions_header).to_be_visible(timeout=5_000) or assert
sessions_header.is_visible(timeout=5_000)), then continue to locate
sessions_text and review_links and assert review_links.count() > 0;
alternatively, if zero sessions are valid for this dataset, pin the test data or
add an explicit assertion that zero sessions are expected instead of silently
passing.
Comment on lines
+152
to
+164
| # --- Step 8: Cleanup --- | ||
| # Delete the workflow run to avoid polluting production labs records | ||
| current_url = page.url | ||
| run_id_match = re.search(r"run_id=(\d+)", current_url) | ||
| if run_id_match: | ||
| run_id = run_id_match.group(1) | ||
| # Get CSRF token from the page | ||
| csrf_token = page.evaluate("document.querySelector('#workflow-root')?.dataset?.csrfToken || ''") | ||
| if csrf_token: | ||
| page.request.post( | ||
| f"{live_server_url}/labs/workflow/api/run/{run_id}/delete/?opportunity_id={opportunity_id}", | ||
| headers={"X-CSRFToken": csrf_token}, | ||
| ) |
There was a problem hiding this comment.
Run cleanup from a finally block or fixture finalizer.
Any timeout or assertion above these lines skips the delete call, leaving workflow/run records behind in the shared labs backend. That state pollution is exactly what makes the later .first selection flaky. Capture run_id as soon as it is known, clean up in finally, and assert that the delete request succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commcare_connect/workflow/tests/e2e/test_audit_workflow.py` around lines 152
- 164, Capture the run_id as soon as it is discovered (e.g., where run_id_match
is computed) and move the deletion logic into a finally block or a pytest
fixture finalizer so it always executes even on timeouts/assertion failures; in
that finally ensure you retrieve the CSRF token (from page.evaluate or stored
data), call page.request.post to DELETE
f"{live_server_url}/labs/workflow/api/run/{run_id}/delete/?opportunity_id={opportunity_id}"
using headers={"X-CSRFToken": csrf_token}, and assert the response indicates
success (check response.status or similar) so failures are surfaced.
- Fix JSONPathError import path in form_receiver - Add error logging to LabsRecordAPIClient create method - Handle missing WeasyPrint gracefully on Windows - Handle missing resource module on Windows (mbw_monitoring) - Solicitations: add program context checks, remove LLO entity fields, simplify response submission, update templates - Update package-lock.json Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflict in conftest.py: keep --noreload flag for E2E server. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
audit_with_ai_reviewworkflow template that exercises the full pipeline: create workflow → create run → select visits → trigger async Celery audit → poll completion → verify sessionsfetch_raw_visitswhereCharFieldvisit IDs (strings) were compared to CSV-parsed IDs (ints), silently filtering out all visits and producing 0 imagesBaseSSEStreamView.get()missing**kwargscausing 500 errors on SSE endpointsinclude_imagesparameter plumbing through pipeline/backend for image extractionInfrastructure
conftest.py: Django server + Celery worker subprocesses, OAuth session injection via CLI token, Redis queue purge for stale tasks,--pool=solofor Windowsviews_test_auth.py: DEBUG-only view at/labs/test-auth/to inject CLI token into Django sessionLEARNINGS.md: comprehensive reference for Windows/Playwright/Celery gotchasTest plan
DEBUG=True🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements