Conversation
…on and multi-action flows
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Deploying care-preview with
|
| Latest commit: |
b7f4ee9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4cf0e9d6.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://bodhi-questionnaire-actions-71q9.care-preview-a7w.pages.dev |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CARE Review — E2E coverage for the appointment action configuration builder
Test-only change: three specs over /admin/actions, exercising the six context operators, multi-condition AND/OR, multi-instruction, and multi-action delete-and-persist. No .tsx touched, so the UI/UX lens doesn't apply.
What it does, and it reads well. The intent is legible throughout — step names say what each block proves, and the comments explain non-obvious mechanics (why the value control is a textbox for context values, why collapsed cards leave the a11y tree) rather than restating the code. Reading each assertion against the base branch, the compiled-expression expectations are right: CONTEXT_OPERATORS in actionVariables.ts is exactly the six comparisons, compileCondition joins with and / or, the untyped context input coerces a numeric string via NUMERIC_TEXT, and the "6 actions" / "2 actions" row text matches action_count_other. The toHaveCount(1) collapse waits are the right fix for the one-card-open-at-a-time controlled openIndex.
Four things worth a look, all about assertions being weaker than the comments next to them:
- Hidden single-instruction coupling — dropping the explicit "Add instruction" makes all three tests depend on the registry having exactly one instruction.
addActiononly preseeds atlength === 1; a second registered instruction turns every message fill into an opaque timeout. Worth one guard inappointmentInstruction(). Delete.nth(2)is positional over an unscoped button set — correct today only because/newhas no config-level Delete. AtoHaveCount(3)pre-assertion makes the coupling explicit.- "Exactly six operators" isn't asserted — six visibility checks plus one
Containsabsence lets a seventh operator through.expect(page.getByRole("option")).toHaveCount(6)is shorter and actually closed. - The step removal is never asserted — add-then-remove leaves no check that the right step went.
None of these are blocking; they're places where a future change breaks the test in a way that won't say why. Approach and scope are proportionate — the helpers earn themselves across three tests, and reusing appointmentInstruction() / fetchConfiguration() rather than hardcoding slugs is right.
One note on merge order: this stacks on bodhi/questionnaire-actions (#16618), so the base needs to land first.
Not approving only because of finding 1 — a registry change silently breaking three tests is the kind of thing worth deciding on before merge, not after.
Generated by CARE PR Reviewer for #16825 · opus50 · 419.8 AIC · ⌖ 2.83 AIC · ⊞ 18.4K
| // Adding the action preseeded the one registered instruction | ||
| // ("Log a message"), already selected — only its message needs filling. | ||
| await page | ||
| .getByRole("textbox", { name: paramLabel }) | ||
| .fill(`Elderly patient booked ${stamp}`); |
There was a problem hiding this comment.
Dropping the explicit "Add instruction" + instruction-select couples all three tests to the registry holding exactly one instruction globally. ActionListEditor.addAction only preseeds when registry.instructions?.length === 1; with two registered it pushes { slug: "", params: {} }, no param inputs render, and every fillMessage / paramLabel fill in this file times out with an opaque "textbox not found" rather than a readable failure.
That contradicts what the file already does one helper up — appointmentInstruction() deliberately filters a multi-instruction list by context === "Appointment", so the spec anticipates more than one instruction while the flow assumes one.
Cheapest guard: assert the assumption once where it's made, e.g. in appointmentInstruction() throw when data.instructions.length !== 1 ("preseed assumes a single registered instruction"), or select the instruction explicitly when there's more than one. Either way the next person adding a backend instruction gets a sentence instead of a timeout.
| // Drop the third rule (no steps or config-level delete exist yet, so | ||
| // the only "Delete" controls are the condition rows). | ||
| await page | ||
| .getByRole("button", { name: "Delete", exact: true }) | ||
| .nth(2) | ||
| .click(); | ||
| await expect( | ||
| page.getByRole("combobox", { name: "Condition 3 Field" }), | ||
| ).toHaveCount(0); |
There was a problem hiding this comment.
getByRole("button", { name: "Delete", exact: true }).nth(2) is positional over a set that isn't scoped to the open action card. The comment says "the only Delete controls are the condition rows" — that holds only because this is a /new page (no configuration-level Delete) with a single expanded action. Add a delete on an action-level control, or expand a second card, and .nth(2) silently clicks a different button; the follow-up toHaveCount(0) on Condition 3 would still pass if it removed row 3 for an unrelated reason.
The condition row's aria-label is a bare t("delete") in ActionConditionEditor, so the row can't be addressed by name — but the assertion can be made non-positional by asserting the pre-state: await expect(page.getByRole("button", { name: "Delete", exact: true })).toHaveCount(3) before the click. That turns a future mismatch into a failure that names the cause.
(Worth a companion note upstream: aria-label={t("delete")} on every condition row gives three identically-named buttons to a screen reader too — ${rowLabel} ${t("delete")} would fix both the a11y and the selector.)
| // The first action proves the numeric field offers exactly the six | ||
| // comparison operators — and none of the choice-only ones. | ||
| if (index === 0) { | ||
| for (const candidate of NUMERIC_OPERATORS) { | ||
| await expect( | ||
| page.getByRole("option", { name: candidate.label, exact: true }), | ||
| ).toBeVisible(); | ||
| } | ||
| await expect( | ||
| page.getByRole("option", { name: "Contains", exact: true }), | ||
| ).toHaveCount(0); | ||
| } |
There was a problem hiding this comment.
The comment claims this proves the field offers exactly the six comparison operators, but the assertions are six toBeVisible() plus one Contains absence — a seventh operator (or Does not contain) would sail through. operatorsFor(undefined) returns CONTEXT_OPERATORS, so the real invariant is a closed list; assert it as one:
await expect(page.getByRole("option")).toHaveCount(NUMERIC_OPERATORS.length);That's one line instead of seven and actually enforces "exactly", including the not in case the current check misses.
| await fillMessage(page, paramLabel, `Keep ${stamp}`, 0); | ||
| await page.getByRole("button", { name: "Add instruction" }).click(); | ||
| await fillMessage(page, paramLabel, `Drop ${stamp}`, 1); | ||
| await page.getByRole("button", { name: "Remove step" }).nth(1).click(); |
There was a problem hiding this comment.
The add-then-remove of a second step is never asserted — Remove step is clicked and the step block moves straight on to action 2. If the remove button were wired to the wrong index and dropped step 1 instead, the saved instructions would still have length 1 and the later params[paramName] check would be the only thing catching it, from a different test step.
A await expect(page.getByRole("button", { name: "Remove step" })).toHaveCount(1) right after the click localises that failure to where it happens. Same shape as the Condition 3 Field count check ten lines up, which does this correctly.
There was a problem hiding this comment.
CARE Review — E2E coverage for appointment action configuration (re-review)
Delta since my last review (0b05717 → cf1ed15) is one commit: a third test in tests/facility/patient/encounter/fill/fillActions.spec.ts that authors four rules over a single integer answer via the API and checks which fire at submit. actionConfiguration.spec.ts is unchanged, so my four open threads there still stand as written — I re-fetched the file at the head SHA to confirm rather than assuming.
The new test is the right addition. Layer 1 proved conditions compile to the expected Python; nothing proved they evaluate. Doing it through the questionnaire path — where the action outcome is observable as a toast — rather than waiting on appointment slot fixtures is the pragmatic call, and the PR body says so instead of overclaiming. Authoring through POST /api/v1/questionnaire/ rather than the UI is correct for a fill-time test, and it reuses firstInstruction() / createQuestionnaireWithActions() from the two existing tests instead of growing new scaffolding.
Two findings on the new test, both about the assertions being narrower than the words around them:
- "pins every comparison" isn't true — the four rules exercise
>=,<,<=;==and!=never run at fill time anywhere in this file. Fix the comment or add a rule. - The negative assertions are the load-bearing half and are sampled, not held —
toHaveCount(0)resolves on the first check, and sonner caps at 3 visible toasts while this submission produces exactly 3. An overflowed toast and an un-fired action look identical to these two lines. Asserting the total count of band toasts (toHaveCount(2)over a regex) closes that.
Still open from the previous round, unchanged in the code:
- Hidden single-instruction coupling in
appointmentInstruction()(actionConfiguration.spec.ts:155) - Positional
Delete.nth(2)(:322) - "Exactly six operators" not actually asserted (
:249) - Step-removal never asserted (
:329)
None of the six are blocking correctness of the feature — they're all places where a future change breaks a test in a way that won't say why. Commenting rather than approving because finding 2 can produce a false pass on the suite's only negative case, which is worth a decision before merge.
Merge order note still applies: this stacks on bodhi/questionnaire-actions (#16618).
Generated by CARE PR Reviewer for #16825 · opus50 · 219 AIC · ⌖ 3.25 AIC · ⊞ 18.4K
| }, | ||
| ], | ||
| // Four rules over the same answer; at age 70 exactly two hold, which | ||
| // pins every comparison and both connectors in one submission. |
There was a problem hiding this comment.
The comment (and the PR body) says these four rules pin every comparison operator, but only >=, < and <= appear here. == and != are never evaluated at fill time by any test in this file, and > is covered only incidentally by the earlier q_temp > 38 test. Either drop the claim to what the rules actually cover, or add an == / != rule — a reader trusting the comment will believe equality is covered and skip it when the next change touches the evaluator.
This is the same shape as the open thread on actionConfiguration.spec.ts:249: the prose asserts a closed set the assertions don't.
| await expectToast(page, `senior-${stamp}`); | ||
| await expectToast(page, `dependent-${stamp}`); | ||
| const toaster = page.locator(".toaster.group"); | ||
| await expect(toaster.getByText(`adult-${stamp}`)).toHaveCount(0); |
There was a problem hiding this comment.
The two negative assertions are the load-bearing part of this test (they're what distinguishes "the engine evaluated" from "the engine fired everything"), and toHaveCount(0) resolves immediately — it passes at the instant the DOM is checked, which is racy in both directions.
The risk here isn't a late toast so much as an early check against a toaster that hasn't rendered yet: by line 211 expectToast has awaited senior, so the region exists, but sonner's stack is capped at 3 visible toasts and this submission produces 3 ("submitted successfully" + senior + dependent). If ordering ever puts adult/working in the overflow rather than un-fired, these assertions pass for the wrong reason — a false green on the only negative case in the suite.
Preferably assert the whole set at once, so overflow can't masquerade as non-firing:
await expect(toaster.getByText(new RegExp(`(senior|adult|working|dependent)-${stamp}`)))
.toHaveCount(2);At minimum, await expect(...).toHaveCount(0) with an explicit short timeout so the absence is held rather than sampled once.
…+ AND/OR + negative)" This reverts commit cf1ed15.
There was a problem hiding this comment.
CARE Review — E2E coverage for appointment action configuration
Test-only PR: two specs, no product code. Lens 3 (UI/UX) skipped — no .tsx changed.
What it does: adds two admin specs proving the condition builder compiles each operator and both connectors to the right Python expression (asserted against the stored configuration, not the UI), plus one fill spec proving the engine evaluates those conditions at submit. Intent is legible throughout — the step names read as sentences and the comments explain the non-obvious UI behaviour (collapsing action cards, preseeded instructions) rather than restating the code. Assertions go through the API for storage and through the UI for behaviour, which is the right split.
Findings
Broken
- Negative toast assertions may be checking a capped list. The fill spec fires four toasts in one submission (two action toasts + the submit-success toast) against sonner's default
visibleToasts: 3. Toasts past the cap stay in the DOM withdata-visible="false", sotoHaveCount(0)still works, but thetoBeVisible()assertions above it can fail on a toast that did fire. Inline comment has the detail.
Convention
- Instruction-picker coverage is removed, not relocated. The diff deletes the only place any spec opens the
Instructioncombobox, replacing it with a comment asserting the single registered instruction is preselected. That is true today and breaks the day a second instruction is registered. .nth(2)on an ambiguousDeletebutton. Relies on the/newpage having no otherDeletecontrol.Delete action 2, used a few lines later, is the pattern to follow.
Polish
- The instruction/param lookup preamble is copy-pasted across all three tests;
appointmentInstruction()could return it.
Not flagged
The expectation that Add action collapses the previous card (toHaveCount(1) on Add a condition) is a real wait, not a sleep — good. The boundary values in the fill spec check out: 70 satisfies >= 60 and < 13 or >= 60, and fails < 60 and 18..65, so the four-rule design does pin every operator and both connectors in one submission as claimed.
Nothing here blocks a merge — commenting rather than approving because of the toast-cap issue, which will show up as an intermittent failure rather than a clean one.
Generated by CARE PR Reviewer for #16825 · opus50 · 378.7 AIC · ⌖ 2.66 AIC · ⊞ 18.4K
| .click(); | ||
| await page.getByRole("textbox", { name: "Condition 1 Value" }).fill("60"); | ||
|
|
||
| await page.getByRole("button", { name: "Add instruction" }).click(); | ||
| await page.getByRole("combobox", { name: "Instruction" }).click(); | ||
| await page | ||
| .getByRole("option") | ||
| .filter({ hasText: /Log a message|Show a message/ }) | ||
| .first() | ||
| .click(); | ||
| // Adding the action preseeded the one registered instruction |
There was a problem hiding this comment.
This deletes the last place the suite touches the instruction picker. Before this change the test clicked Add instruction, opened the Instruction combobox and chose an option; now no spec in tests/ opens that combobox at all (Add instruction survives only in the new multi-rule test, and it is never followed by a selection). Choosing an instruction is the one authoring step that decides what an action actually does, so it is a poor thing to drop.
The replacement also encodes an assumption that is only true while the registry has exactly one entry: the comment says the instruction is preseeded and already selected. The moment a second instruction is registered, the preselection may change or disappear, and this test will fail on a paramLabel textbox that is not there — with no hint that the cause was the picker.
Suggest keeping one explicit selection through the combobox here (the registry lookup at the top already gives you instruction.slug, so you can pick by name rather than by .first()).
| const toaster = page.locator(".toaster.group"); | ||
| await expect(toaster.getByText(`adult-${stamp}`)).toHaveCount(0); | ||
| await expect(toaster.getByText(`working-${stamp}`)).toHaveCount(0); |
There was a problem hiding this comment.
These two negative assertions can pass for the wrong reason. Sonner renders at most visibleToasts (default 3) toasts; toasts past that are still in the DOM but get data-visible="false" and opacity:0. This submission fires four toasts at once — the two action toasts plus the questionnaire submitted successfully toast from submitAndExpectSuccess, so you are already at or over the cap.
toHaveCount(0) is a DOM-presence check, so it would still catch a wrongly-fired adult toast. But the inverse is the real risk: if the engine regressed and adult/working fired while senior/dependent did not, the expectToast calls above (which assert toBeVisible()) could fail on a toast that did fire but was pushed out of the visible window — a confusing failure that reads like a condition bug.
Simplest fix: assert the four outcomes without stacking four simultaneous toasts — e.g. assert senior/dependent visible, then the negatives, with visibleToasts raised, or split the four rules across two submissions. At minimum, note the cap so the next person debugging a flake does not chase the condition engine.
| // the only "Delete" controls are the condition rows). | ||
| await page | ||
| .getByRole("button", { name: "Delete", exact: true }) | ||
| .nth(2) | ||
| .click(); |
There was a problem hiding this comment.
getByRole("button", { name: "Delete", exact: true }).nth(2) targets the third condition row only by positional luck, and the comment admits it: "no steps or config-level delete exist yet, so the only Delete controls are the condition rows." That premise is fragile — this is the /new page, and the moment the editor grows any other Delete-labelled control (a header action, a per-step delete), .nth(2) silently removes the wrong thing and the test fails somewhere later with a confusing compiled-condition mismatch.
The file already shows the robust idiom two steps down: Delete action 2 is an accessible name that identifies its target. If the condition rows carry a similar name (Delete condition 3), use it; if they do not, that itself is an a11y gap worth naming, since a screen-reader user hits the same ambiguity — three buttons all announced as "Delete".
| const instruction = await appointmentInstruction(); | ||
| const paramName = Object.keys(instruction.input_schema.properties ?? {})[0]; | ||
| const paramLabel = | ||
| instruction.input_schema.properties?.[paramName]?.title ?? paramName; | ||
| const stamp = Date.now(); | ||
| const name = `Every operator ${stamp}`; | ||
| let configurationId = ""; |
There was a problem hiding this comment.
This four-line instruction/param lookup is now duplicated verbatim in all three tests. It is small, but it is the kind of duplication that drifts: appointmentInstruction() already exists as a helper, so folding the paramName/paramLabel derivation into it (returning { instruction, paramName, paramLabel }) removes twelve lines and gives one place to fix when the registry shape changes.
Polish only — not a reason to hold the PR.
This branch has not been deployed
1 participant
https://entire.io/gh/ohcnetwork/care_fe/trails/13
Summary
E2E coverage for the appointment action configuration feature, stacked on #16618. Two complementary layers:
1. Configuration —
tests/admin/actions/actionConfiguration.spec.tsAuthoring/storage through the admin UI (
/admin/actions):==,!=,>,>=,<,<=), and that the field offers exactly those six (no choice-only operators).and/orexpression.performable) toggle; Appointments-only "Runs for" scope.2. Execution —
tests/facility/patient/encounter/fill/fillActions.spec.tsVerifies the shared condition engine actually evaluates at submit, via the observable encounter/questionnaire path (action outcome → toast). Adds a test with four rules over one answer (
>= 60,< 60,AND 18..65,OR <13/>=60); at age 70 exactly the two matching actions fire and the other two do not — pinning every operator and both connectors, including the negative (no-fire) case. Complements layer 1 (which proves compilation/storage) by proving runtime evaluation.Verification
3 passedagainst a live backend.3 passedagainst a live backend (fresh encounter fixture).Note: appointment-booking execution isn't asserted directly — it needs schedulable-resource/slot fixtures and the only registered instruction (
logging) surfaces nothing on the appointment UI; the shared evaluation engine is instead verified through the encounter path above. Base is #16618 (bodhi/questionnaire-actions).