Add appointment-reminders extension - #419
Draft
meerao-cm wants to merge 24 commits into
Draft
Conversation
Appointment reminders and confirmations over SMS (Twilio) and email (SendGrid), with per-business-line branding and routing and a two-way Y/N confirm flow. Canvas's native appointment reminders are a single global setting: one cadence, one wording, no per-visit-type variation, and a confirm loop that cannot be extended. This plugin replaces them with campaigns configured per visit type and per business line, and turns patient replies into state: Y confirms the appointment on the schedule, N opens a follow-up Task. Five campaigns (confirmation, reminder, cancellation, no-show, telehealth join), all shipping disabled so an install sends nothing until a campaign is switched on. A fail-closed TESTING_MODE allowlist restricts sending to named patients and recipients during setup, and LOCK_MESSAGE_TEMPLATES freezes approved copy so a manual send cannot reword it. Author: Meera Rao <meera.rao@canvasmedical.com>
meerao-cm
marked this pull request as draft
July 25, 2026 22:03
The patient bell panel lets a staff member send against a standalone note
as well as an appointment, and "Telehealth Join" was the only real campaign
offered for a note. That path hand-built a 9-key variables dict instead of
using the shared renderer, so it omitted telehealth_link,
business_line_attribution, location_phone and the org variables. Unmatched
placeholders survive render_template, so the patient received literal
"{{telehealth_link}}" text. The same branch never converted to local time
and dropped the zone label, rendering a 3:40 PM ET visit as "07:40 PM"
while the picker directly above it showed 3:40 PM.
- Route notes through a shared _build_variables() so notes and appointments
expose the same placeholder set in the same timezone, falling back to the
provider's personal meeting room for telehealth_link since a note has no
appointment-level link. Tolerates a missing datetime_of_service.
- Match the appointment branch's prefetches on the note query, since the
shared renderer reads provider roles and the location's address/telecom.
- Refuse a manual send whose rendered body still contains {{placeholder}},
on the channels actually being sent, so template syntax cannot reach a
patient. Covers client-supplied copy too, which an unlocked panel posts
straight through.
- Drop the dead "Custom" dropdown entry. The campaign was removed from the
product and the send endpoint rejects it while copy is locked.
- Stop zero-padding rendered dates and times: "August 5, 2026" and
"3:40 PM" rather than "August 05, 2026" and "03:40 PM", matching the
wording Canvas core uses.
10 new tests, 245 passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`canvas install` (CLI 0.182.0) now fails the plugin on services/twilio_inbound.py:19 with [bytearray-blocked]: bytearray is not available in the plugin sandbox. This blocked deploying v0.7.6 to plugin-testing even though v0.7.5 was already running there, so the validator appears to have tightened since that install. Accumulate the decoded bytes in a list[int] and wrap with bytes() before decoding. Behavior is identical; all 245 tests pass unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_form_params sourced params from the SDK's Request.form_data(), which builds
its MultiDict from parse_qsl(body) without keep_blank_values and so drops any
key whose value is empty.
Twilio signs url + "".join(key + value for key in sorted(params)), including
key + "" for every empty-valued param. Reconstructing the signing string from
form_data() therefore omitted those pairs, the recomputed HMAC differed, and
_signature_ok failed closed with 401. Carriers routinely omit the geo fields,
which Twilio then sends blank (FromCity, FromState, FromZip, ToCity, ToZip),
so this rejected essentially every genuine inbound SMS -- the two-way Y/N
confirm flow had never worked against live Twilio.
Parse the raw body instead. The sibling helper parse_form_body already
preserves blanks for exactly this reason ("Blank values are kept so signature
computation matches Twilio's exactly"), so the fallback path was correct all
along and only the preferred path was wrong. form_data() is now consulted only
when the raw body is unavailable.
Confirmed against the deployed endpoint: 20 non-empty params verified 200,
the same payload plus one empty-valued param signed over all of them returned
401, and signing over only the non-empty subset returned 200 -- proving the
handler was reconstructing without the blanks. Reproduced locally: the old
path kept 8 of 13 params and failed verification, the new path keeps all 13
and passes.
Every existing inbound test built a fully-populated payload, which is the case
that already passed. Added an end-to-end test driving inbound() with a
realistic 13-param payload carrying five blank geo fields, unit coverage on
parse_form_body and valid_twilio_signature for blanks, and inverted the two
_form_params tests whose asserted source precedence this reverses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
STOP writes back to the chart. Twilio blocks an opted-out number on its own side and forwards the keyword to the webhook, but nothing carried that decision into Canvas. The native incoming-SMS handler used to record it; pointing the number at this plugin for two-way confirm retires that, so delivery.py kept seeing has_consent=True and later sends would fail with Twilio 21610. The stale claim in _get_patient_contacts' docstring -- that the webhook sets opted_out -- has been corrected accordingly. services/consent.py now clears has_consent on the contact point the message came from, and START/UNSTOP/YES restore it. Only has_consent is writable: the Patient effect's contact-point payload has no opted_out field. That is the better field regardless, since it gates SMS alone where opted_out would also suppress email, and STOP is an SMS carrier keyword. Twilio's keyword sets overlap this plugin's, so consent is classified on its own axis in services/twilio_inbound.py: CANCEL both opts out and opens the decline Task, YES both opts in and confirms the appointment. Consent writes resend every live contact point, because Canvas documents addresses updates as replace-based, says nothing about contact_points, and the payload carries no row ids -- correct under either reading, and flagged in the README for UAT confirmation. Admin console gated by role. StaffSessionAuthMixin proves only that the caller is some logged-in staff member -- its docstring says it "only cares that they are a staff, with no regard to roles" -- so any staff account could POST /admin/config and rewrite every campaign template or switch live sending on. NotificationAPI.authenticate now also requires a role listed in the new ADMIN_ROLE_NAMES secret for /admin* routes, matched against each StaffRole's name and internal_code. Fails closed: unset denies everyone. The per-patient chart panel routes stay open to any logged-in staff member. The provider-menu item itself cannot be hidden per user -- visible() is defined only on EmbeddedApplication, and ProviderMenuConfiguration explicitly does not apply to plugin-provided menu items -- so NotifyAdminApp shows a refusal page instead. The API check is the real boundary, since the URLs are reachable without ever opening the menu. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'secrets' is deprecated in the manifest schema in favour of 'variables', whose entries carry a per-name sensitive flag; canvas validate had been warning about it on every run. Migrating also lets each value be classified rather than treating all fourteen the same. Sensitive values are write-only -- masked in the Admin UI and reported by canvas config list only as [set] / [not set]. Every credential is marked sensitive, as are TESTING_MODE_PATIENTS and TESTING_MODE_RECIPIENTS: those are not configuration but patient identifiers and contact details, and there is no reason for them to be readable. The rest stay readable on purpose. twilio-phone-number, the inbound webhook URL, sendgrid-from-email and ADMIN_ROLE_NAMES are the values an operator has to check when something is misrouted, and a masked variable can only be confirmed as [set] -- which would make diagnosing a signature mismatch or a wrong sender number needlessly hard. No code change: self.secrets[...] still resolves variables regardless of how they are declared. Scaffold tests now assert the classification rather than just the presence of namespace_read_write_access_key, so a credential added later without a sensitive flag fails the suite instead of shipping readable. The helper reads both manifest keys so the invariants hold either side of the migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
meerao-cm
force-pushed
the
add-appointment-reminders
branch
from
August 22, 2026 23:59
6e26975 to
9801ba7
Compare
_resolve_patient filtered contact points on the last 4 digits with __contains and then sliced [:50] off an unordered queryset. Two compounding faults: __contains matched those digits anywhere in the value rather than at the end, and Patient declares no Meta.ordering, so the slice was LIMIT 50 with no ORDER BY and kept arbitrary rows. At production scale that returned roughly 200 candidates per lookup and threw away three quarters of them before the exact-match loop ran, so most inbound replies never resolved. Not flaky either: the slice is arbitrary but stable, so a given patient tended to fail consistently rather than intermittently. And it failed silently -- a miss returns 200 with no audit row, which ops reads as patient non-response. Match the last 10 digits as a suffix instead, uncapped. Both sides are already canonical: the caller normalizes Twilio's From to E.164 via _normalize_phone, and stored contact-point values are bare digits. Verified that normalization yields the same 10-digit suffix for every input format, E.164 and punctuated alike. So the 4-digit prefilter was defending against variation that occurs on neither side, and the precision it gave up was the bug. Left uncapped deliberately: after a 10-digit suffix match, more than one hit means genuinely duplicated patient records, which the caller should see rather than have silently truncated. The Python exact-match pass stays, so a suffix collision cannot resolve wrongly. Tests: the cap regression is the one that matters -- it places the target at index 150 of 201 candidates and fails against the old implementation while passing against the new. Existing fixtures never caught this because each uses a phone number unique in its dataset. Also pins the query's lookup kwargs, the absence of a slice, and the raised length guard. Known gaps records that __endswith compiles to LIKE '%...' and cannot use a B-tree index, so this is a sequential scan -- acceptable for a webhook, and cheaper than what it replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….8.2)
Two changes to inbound reply handling.
Audit replies from unmatched numbers.
A verified reply whose sender resolved to no patient returned 200 and wrote
nothing. The appointment stayed unconfirmed, which is indistinguishable from
the patient never replying -- so a misconfigured number, a patient texting from
a phone not on their chart, and a genuine wrong number all looked like silence.
Now recorded as a NotificationDelivery row with campaign_type=inbound_response
and status=unresolved_sender, and logged at warning rather than info.
NotificationDelivery.patient becomes nullable, for this case only. No migration:
Canvas does not create `not null` constraints on CustomModels ("Unsupported
constraints: not null"), nor database-level `references`, so the column has
always been nullable in Postgres and the declaration only aligns the ORM with
what the table already allows. Confirmed the model still loads in the sandbox.
Added GET /admin/unresolved-senders to read them back. Not scope creep: the only
existing history query filters on patient__id, so a patient-less row would have
been unreadable by anything, and a write-only audit row would not have met the
goal. Placed under /admin because these rows belong to no chart, which also
means they inherit the ADMIN_ROLE_NAMES gate.
The write sits after the signature check and after the MessageSid replay guard,
so unverified requests cannot inject arbitrary numbers and bodies into the log
and a replay cannot inflate it. Both are pinned by tests, along with the two
writers being mutually exclusive for any single reply.
The rows retain a phone number and message body belonging to someone with no
patient record. That is required to follow up, but it is a new category of
retained data, so the README calls it out and asks that the endpoint be treated
as a patient-data surface. There is no admin-UI surface yet; Known gaps says so.
Remove CANCEL from the decline set.
Twilio publishes CANCEL as an unsubscribe synonym alongside STOP, so a patient
texting it means "stop texting me", not "cancel my appointment". Consent still
clears -- that branch fires from _OPT_OUT and is untouched -- but the reschedule
Task no longer opens, because inferring an intent about the visit from an
unsubscribe keyword attributes something the patient never said.
YES remains in both sets, so the two-axis classification still earns its keep:
it is genuinely an opt-in and a confirm, and both halves are actioned.
Also covers log_inbound_response, which had no direct tests -- history.py goes
from 84% to 100%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Testing mode was three plugin variables: TESTING_MODE, TESTING_MODE_PATIENTS and TESTING_MODE_RECIPIENTS. It is now part of CampaignConfig, edited under "Testing mode" in the admin app. Plugin config takes instance-level access to change, which is the right bar for credentials and for who may administer the plugin, and the wrong bar for the person running a test send. It also bought no safety: anyone who can enable a campaign in the admin app can already cause live sending, so the setting that restrains that sending belongs beside the switch that arms it. Plugin config is now credentials and staff permissions only, an invariant test_scaffold pins. Defaults to ON with both allowlists empty, which means nothing sends at all. Deliberate in both directions. A fresh install cannot message anyone until someone opens it up, matching the "do the first run behind testing mode" guidance the README already gave. And an instance upgrading from 0.8.x lands closed rather than silently broadcasting to every patient the moment the old secret stops being read -- the failure mode that would otherwise be a TCPA incident rather than an inconvenience. CampaignConfig.from_dict applies the default to config rows written before the field existed, so the upgrade path is covered rather than just the fresh-install one. Allowlists do not carry over from the secrets; the README says so. deliver_to_patient takes the config rather than reading it. All four call sites already had it loaded outside their loops, so this adds no queries -- omitting it would have meant a config read per patient inside the reminder cron. The fallback load is still there for safety and is itself fail-closed. Two tests pin both halves of that contract. manual_send had no config in scope; it was calling load_config() inline for the business-line from-number. It now loads once and uses it for both. The admin card mirrors the server's fail-closed rule: it warns when the gate is on with an empty list, since "silently sending nothing" is the failure mode the banner exists to prevent. Testing mode also stops being reported through /admin/integration-status as the browser's source of truth -- the checkbox is live and that response would go stale the moment it is toggled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The toggle's visible control was <span class="toggle-slider">, but the CSS class is `slider` -- toggle-slider has no rule anywhere in the file. Since .toggle input is opacity:0 and the styled span *is* the control, an undefined class rendered nothing: the switch was invisible and unclickable, so an install had no way to leave testing mode from the UI. The card body also used `campaign-body` where the defined class is `nt-card-body`. Both are the same failure mode. The admin page is one large HTML string, so a class name no rule defines fails silently rather than erroring, and the existing tests only asserted the page returns 200. Three tests now cover it, all of which fail against the markup as shipped: every `class="toggle"` label must contain <span class="slider">; every class on the testing-mode card must have a CSS rule behind it; and the testing-mode controls must be present and wired to both updateTestingModeUI and the save payload. The class audit is scoped to the card rather than the whole page -- a page-wide sweep picks up class names assembled inside JS template strings and drowns in false positives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both settings here are set once and rarely revisited, so neither belongs at the top of the Campaigns tab. They now live on their own Settings tab as collapsed cards that open on click, the same pattern the campaign cards use. Testing mode moves there unchanged. Because the card starts collapsed, the header carries an ON badge while the gate is closed -- a gate that silently suppresses every send should not be invisible from the tab. Task assignment is new. The follow-up task raised when a patient declines by SMS was always created unassigned, so it landed in no team's queue and someone had to go looking for it. A new decline_task_team_id picks the receiving team from those configured on the instance, read through a new GET /admin/teams. Unassigned remains the default, matching the previous behavior for an existing install. The team id is verified before use. A team chosen here can be deleted in Canvas long afterwards, and handing AddTask a dangling id risks the effect failing -- which would lose the task, the one artifact telling staff this patient wants to reschedule. A missing team logs a warning and the task is created unassigned instead. The dropdown likewise flags a configured team it cannot find, so a stale setting does not silently read as "Unassigned" and get dropped on the next save. The config and team lookups are lazy, on the decline branch only, so they stay off the path of every other inbound reply -- one test pins that. The markup tests added in 0.9.1 earned their keep immediately: they caught `badge` on the new header, which is only defined under .card-header in the patient-view page's CSS and so would have rendered unstyled on the admin page. The class audit now covers both cards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oading note bodies (v0.10.1)
Three findings from a database-performance pass. The fourth and largest -- the
reminder cron hydrating a three-day appointment window every five minutes to act
on a seven-minute band -- is recorded in Known gaps and left for its own change.
Cron no longer writes the config.
execute() called save_config() on every tick to "refresh the config TTL". That
was real when config lived in the cache; it moved to a CampaignConfigRecord row
with no expiry, so the call rewrote the whole config blob -- every campaign's
SMS and email templates, every per-visit-type and per-business-line override,
and now the testing-mode allowlists -- 288 times a day to change nothing. It
also opened a window, narrow but pointless, where a tick could clobber an
admin's concurrent save with its own stale copy. Removed, along with the import,
so a future caller has to reach for it deliberately.
Indexed the unresolved-sender query.
get_unresolved_senders filters campaign_type + status and orders by
-created_at, but NotificationDelivery only had (patient, -created_at) and
(-created_at). The query therefore walked the log in created_at order filtering
as it went, and the worst case was the healthy one: no unresolved senders means
scanning every row to return an empty list, degrading as the log grows. Added
the composite index.
Stopped loading note bodies on chart open.
get_patient_appointments pulled full Note rows -- including `body` and
`related_data`, both JSONFields holding the clinical content -- for up to 20
notes, while the serializer reads only id, title, datetime_of_service and two
scalars off related rows. `location` was also joined on both the appointments
and the notes queryset and read by neither.
Measured against real field resolution rather than assumed: the notes query goes
from 109 selected columns and 4 joins to 88 and 3, with both JSONFields gone.
Tests assert on generated SQL rather than mocked querysets. A mock never
resolves a field name, so a reintroduced join or a wrong column would pass
against mocks and only fail against a live database -- the same blind spot that
lets Count("id") through on a custom-data model.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…send times (v0.11.0) Two changes to the reminder cron, sized against the numbers on a test instance rather than the estimate in the earlier review. Skip the scan when no interval can fire. A reminder interval of a day or more is date-relative: it fires at a configured send time on the target date, so it can only fire inside one GRACE_MINUTES window per day. A reminder interval under a day, and every telehealth interval whatever its size, is time-relative and can fire on any tick. execute() now asks which of those is configured before querying anything, and returns early when only day-out intervals exist and no send window is open. Measured against the live config there (reminder_intervals=[1440], send_time 00:50 America/New_York, telehealth off): 2 of 288 daily ticks now query, 286 return immediately. Row hydrations drop from 11,232/day to 78. The instance numbers also corrected the earlier plan. The window holds 39 appointments, not the ~3.4k a code comment cited from a different instance, so the per-scan cost was never the problem -- the wasted scans were. And all 10 overrides there turned out to be pure on/off records with no intervals or send times, so the band-union complexity that made the fuller fix look risky does not exist on this config. Narrowing the surviving scan's window is left open and recorded in Known gaps. Interval collection now keeps reminder and telehealth intervals apart, because classifying by size alone would treat a day-sized telehealth interval as date-relative and gate away scans it needs. Send-time sources are the global setting plus per-visit-type overrides; business-line overrides provably cannot set one, so that pair is complete. A missed source would silently stop a visit type's reminders, so both are tested. Fix send times in the last GRACE_MINUTES of the local day. _is_day_out_window anchored its scheduled instant to *now*'s local date and then required now.date() == target_date. With send_time 23:58 the only ticks inside the grace window fall on the following date, fail that check, and the reminder is never sent -- not late, lost. Anchoring the instant to the target date instead lets the window span midnight and makes the date check redundant, since being within grace of one specific instant already implies the date. Verified against the old implementation: it returns False at every tick for 23:58, where the new one fires once and is still bounded by grace. Send-time parsing also no longer raises. "9" with no colon was an IndexError inside the per-appointment loop, which would have taken down the whole scan and every patient's reminders over one bad config string; it now falls back to 09:00 with a warning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Post-deploy verification on a test instance showed notificationdelivery carrying a single index, on (patient_id, created_at DESC). The other two declared in Meta.indexes -- (-created_at), which predates this work, and the (campaign_type, status, -created_at) added in 0.10.1 for get_unresolved_senders -- did not exist, despite schema_version recording a completed sync. Cause is identifier truncation. Auto-generated names are built from schema plus table and then cut to Postgres's 63-byte limit. "canvas__appointment_reminders_notificationdelivery_" is 51 bytes, leaving 12 for the discriminator, which the literal "notificatio_" fills exactly. All three names truncated to one identical identifier and only the first was created. The sibling canvas__patient_comms schema is 8 bytes shorter, had room for "_created_" and "_patient_" to differ, and has both of its indexes -- which is what confirmed the mechanism rather than a sync failure. So the 0.10.1 index fix was a no-op on a real instance while passing every test, because the tests asserted on the declaration rather than on what the database ended up with. Each index now carries an explicit short name. A test asserts every index has one, that Django's 30-character cap is respected, and that the names stay distinct after being prefixed and truncated to 63 bytes -- so the same collision cannot return silently. Whether the existing auto-named index is dropped or left orphaned alongside the new one is up to Canvas's schema sync; a duplicate on the same columns would be wasteful but harmless. Re-verify against pg_indexes after deploying. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…1.2) end_window sized the scan by treating every reminder interval as a duration: now + max(all_intervals) + GRACE. A day-out interval does not fire on a duration. It fires at send_time on (appointment's local date - interval_days), and the appointment can sit anywhere within that date, so its real lead time runs from just over interval_days days to a full day more. Everything past interval_minutes + GRACE never reached the loop -- no log line, no error. Reproduced on a test instance at the 09:00 ET window on 2026-08-25 with reminder_intervals [1440]. Three appointments on 2026-08-26, all date-eligible for the 1-day interval: dbid 16728, 01:30 ET, 16.5h out -> 2 delivery rows, SMS and email dbid 16727, 14:00 ET, 29.0h out -> zero rows dbid 16729, 20:00 ET, 35.0h out -> zero rows The old horizon was 24.1h, which admits only the first. Note the lead times are 16.5/29/35 hours, not the 28/41/47 in the report -- measured against the actual 09:00 ET window. The mechanism and the conclusion are unaffected. Day-out reminder intervals are now padded to cover their whole target date, so 1440 gives a 49h horizon and 4320 gives 97h. Telehealth intervals are never padded whatever their size, because that branch is genuinely time-relative and ignores send_time -- the same distinction the scan gate draws. Sub-day reminder intervals are left alone for the same reason. Two deliberate deviations from the specced fix, both flagged rather than quiet: The padding adds an hour beyond the whole-day bound. A DST fall-back day is 25 hours long locally, which would otherwise put the last hour of the target date out of reach once a year. Over-inclusion costs nothing here, since _is_day_out_window still makes the exact per-appointment decision. The classification runs over reminder_intervals rather than all_intervals, so a day-sized telehealth interval is not mistaken for date-relative. Padding it would have been harmless but wrong in kind. max_interval_minutes is untouched and still feeds the three dedup cache TTLs, which want the raw interval. Those compute max_interval_minutes + 1440, which equals the new horizon for a whole-day interval -- suggesting the padding was the original intent and the window sizing is where it was lost. The scan gate is untouched: it decides whether to scan, this decides how far the scan reaches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lookup (v0.11.3) _patient_allowlisted compared only against id and dbid, so an MRN pasted into the allowlist matched nothing. The failure was silent: every send was skipped with skipped:testing_mode, which is indistinguishable from the gate working as configured. MRN is the identifier staff actually see and quote, so it is the likeliest thing someone puts there. Verified against the real identifiers on a test instance -- patient 00000000000000000000000000000000, dbid 100001, MRN 900000001. Before the change the id and dbid matched and the MRN did not; now all three do, and an unrelated number still does not. Also removed the `key` lookup. Patient.id is declared CharField(db_column="key"), so the attribute is `id` and `.key` never existed -- that branch has been dead since it was written. The test that covered it passed only because a bare MagicMock answers any getattr, so p.key returned a value the real model cannot produce. Allowlist test doubles now use spec= to constrain their attributes, and one test asserts a `key` attribute is specifically not consulted, so the same false positive cannot come back. Help text in the admin app and the README named "id, key, or dbid", which was wrong in both directions. Both now name MRN and the chart-URL id. The README also notes that a non-matching entry fails silently, so a test send should be confirmed to arrive rather than inferred from the absence of errors. Not addressed: validating allowlist entries on save so a wrong or typo'd identifier is reported rather than silently inert. That is the underlying problem this fix only narrows, and it is a bigger change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Customer feedback: the decline task routes to a team correctly, but with no due
date it sorts nowhere and gets lost in a large task queue. They asked for a due
date of the current date.
New Settings -> Task assignment toggle, "Give the task a due date", off by
default so an existing install keeps creating undated tasks. Stored as
decline_task_due_end_of_day.
Two choices in how the date is computed, both load-bearing.
End of day, not the reply moment. AddTask.due is a timestamp; setting it to
"now" would render the task as already overdue the instant it appears, which is
a different signal from "handle this today".
Anchored to the instance's own timezone via the environment's
INSTALLATION_TIME_ZONE, which the SDK documents for exactly this ("scheduling
work in their local day"). Because due is a timestamp and not a date, computing
end-of-day in UTC renders as the *previous* date for any instance behind UTC --
midnight UTC is the evening before in Eastern. Neither Organization nor
PracticeLocation carries a timezone, and patient last_known_timezone is empty
across this instance, so the environment is the only correct source. Falls back
to UTC with a warning if absent, which keeps the date right for US instances and
merely shifts the hour, rather than silently dropping a setting the admin
switched on.
The decline branch now takes one config read shared between the team lookup and
the due date, so _decline_task_team_id receives the config instead of loading its
own. Every other reply path still reads no config at all, which a test pins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0.12.0 shipped a plain on/off flag that hardcoded "end of the day the patient replied". The rule is now set in the admin app: an offset in business days from the reply day (same day, +1, +2, +3, +5) and a time of day, defaulting to 23:59. decline_task_due_end_of_day is replaced by decline_task_due_days (None meaning no due date) and decline_task_due_time. from_dict migrates the old boolean -- True meant 0 days at 23:59 -- and an explicit new-style value wins over it. A malformed offset resolves to None rather than 0, so a bad string cannot silently start dating every task; a malformed time falls back to 23:59. Business days, per the product decision. Weekends are skipped, so a Friday reply with +1 is due Monday. A reply arriving on a weekend rolls forward before counting, so "same day" on a Saturday is Monday rather than a day nobody is working -- an offset of 0 is not a no-op. Public holidays are NOT skipped: Canvas exposes no holiday calendar a plugin can read, so guessing at one would be worse than the gap. Called out in the README and in the admin help text rather than left to be discovered. The help text under the checkbox now spells the rule out as it is edited, because "+1 business day" reads as "tomorrow" and is wrong when the patient replies on a Friday. Still anchored to INSTALLATION_TIME_ZONE for the reason 0.12.0 established: due is a timestamp, so computing the day in UTC renders as the previous date for any instance behind it. Verified America/New_York on a test instance. parse_hhmm moves into services/config so the reminder send time and the task due time share one defensive parser instead of two. Three due-date tests asserted "the due date is today's local date", which broke the moment weekend rolling landed -- today is a Sunday. They now compare against the same business-day computation, so they no longer pass or fail depending on the day of the week they run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….14.0) The Integration Status panel reported "Twilio SMS: Configured" on the strength of credentials alone. On a test instance the number's inbound webhook had been removed: outbound reminders kept sending perfectly and every Y, N and STOP was discarded for five days with no error, no log line and no audit row, because the request never reached the plugin at all. The panel said Configured throughout. services/twilio_routing.py now asks Twilio where inbound messages actually go, and the panel reports three states rather than a boolean: Configured credentials + routed here Outbound only - patient replies are dropped credentials, nothing routed here Not configured no credentials Folding routing into the existing boolean would have been wrong in the other direction: outbound-only is a legitimate install, since two-way confirm is optional, and calling that "Not configured" would be false. Hence the middle state instead. The check is biased against crying wolf. NOT_ROUTED is returned only when neither the number's own sms_url nor any Messaging Service's inbound_request_url points here -- the latter being the arrangement the README recommends, and it also honours use_inbound_webhook_on_number, which makes the number's webhook win over the service's. Anything Twilio cannot settle reports UNKNOWN and renders as "Configured (inbound routing unverified)": an unreachable API, a 401, a number absent from the account (hosted numbers and short codes do not appear, so absence is not evidence), or a TwiML app on the number, which makes Twilio ignore every sms_*_url. A false "replies are being dropped" would teach people to ignore the warning, which costs more than the warning is worth. Cached for five minutes. The SDK's HTTP client enforces a fixed 30s timeout with no per-request override and this panel loads on every admin page view, so the result is cached and keyed on the webhook URL plus the from-number, meaning re-pointing a webhook re-checks rather than serving the old verdict. The panel already loads asynchronously, so a slow Twilio call delays that box and not the page. Skipped entirely when credentials are absent. Known limit, in the README: only the global twilio-phone-number is checked, not per-business-line from-numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The warning worked -- it caught the removed webhook on a test instance the moment
it shipped -- but it was written for whoever wrote the plugin. It led with
"Inbound replies are not reaching this plugin", quoted a raw endpoint path, and
explained itself by saying the signature is computed over that string. None of
that helps the person who has to go and fix it.
Now it leads with what is happening to patients ("Patient replies aren't
reaching Canvas"), says outbound is unaffected before anything else so it does
not read as total failure, describes the symptom in terms of what a patient does
rather than which tokens the parser recognises, and gives the fix as an
instruction about Twilio's own UI.
It also shows the exact address to paste, read back from
twilio-inbound-webhook-url, rather than describing it. That setting is declared
non-sensitive precisely so it can be read back, and "must match exactly" is the
entire failure mode -- a described address invites the transcription error the
warning exists to prevent. Rendered through escapeHtml since it goes into
innerHTML. When no address is configured at all, the message says so and points
at whoever holds plugin config, since the admin cannot set it themselves.
"the signature is computed over that string" becomes "it has to match this
address exactly, character for character" -- same constraint, no HMAC required.
A test asserts the banner prose stays free of that jargon.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…(v0.15.0) Both fixes come from one incident. A confirmation SMS was logged as "delivered" and never arrived: Twilio accepted the API call, returned a MessageSid, and the message died at the carrier -- almost certainly A2P registration on a newly provisioned number. The plugin captured that SID and discarded it, so answering "did it actually send?" meant hunting Twilio's console by timestamp, and the audit row asserted a delivery that had not happened. NotificationDelivery gains a message_id column, populated from the DeliveryResult the senders already return -- Twilio's MessageSid, SendGrid's X-Message-Id. The patient panel shows it as "Carrier reference" so a message can be looked up directly. Django applies column defaults, not Postgres, so rows written before this column read back NULL and are coalesced on read. A successful send is now recorded as "accepted" rather than "delivered", in both the audit row and the appointment metadata. The plugin consumes no status callback, so it never learns what the carrier did; "delivered" asserted knowledge it does not have. The activity log labels both the new value and legacy "delivered" rows as "Sent" -- the old value never meant more than accepted either -- via a server-side status_label, so wording lives in one place rather than being spelled out in JavaScript. The badge carries a tooltip saying Canvas is not told whether the message was finally delivered. Two mistakes worth recording, both the same shape as earlier ones in this plugin. The renderer changes belong to the patient-view page, not the admin page -- separate HTML strings with separate stylesheets -- so tests asserting against the admin page passed vacuously until retargeted. And .status-accepted was first added to only the admin stylesheet, which would have rendered the badge unstyled in the page that actually shows it, exactly the undefined-class failure that hid the invisible toggle in 0.9.1. Both are now asserted against the page that renders them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things visible in the patient panel. "Call 8005550199 to reschedule" is what patients were actually receiving. location_phone and organization_phone come straight off the contact point, which stores bare digits, and nothing formatted them on the way into the message. North American numbers now render as (800) 555-0199. Anything else passes through untouched -- an unformatted international number beats a mangled one. The panel showed "Inbound_response" as a campaign name, because the JS capitalized the raw key. Labels now come from the server alongside status_label, so wording lives in one place, and they match the admin app's campaign cards: a patient reply reads "Patient reply", not the storage key. Unrecognised types are tidied rather than shown with underscores. Both were on the known-limits list from UAT rather than newly discovered; the screenshot just made them concrete. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….16.0) Two reported bugs. Both are real in the code; the severity of each differs from the report on the affected instances, noted below. Bug 1 -- the reminder scan crashed on patient-less appointments. Appointment.patient is null=True. Admin blocks, provider availability blocks and imported calendar holds are real rows with patient_id NULL, they carry a real note_type so they resolve to the global config and come back enabled, and the send path then reads patient.first_name. There was no guard and no try/except anywhere in the scheduler, so one such row raised AttributeError and killed the whole tick, losing every appointment after it in iteration order. The sibling patient_communications plugins all carry the guard; it was dropped when this plugin was split out of that one. Restored the guard, and added per-appointment isolation, which is the more valuable half: the guard closes the one hole, the try/except closes the class. A day-out interval gets one grace window per day, so losing the rest of a tick costs a whole day of reminders. This is a deliberate departure from CLAUDE.md's "don't wrap handler logic in a bare except" -- right for a request handler, wrong for a batch loop where the alternative is losing every subsequent row. log.exception preserves the traceback, so nothing is hidden from Sentry. Verified the crash directly rather than by reading: get_template_variables(None, ...) raises AttributeError: 'NoneType' object has no attribute 'first_name'. The three new tests fail against the unguarded code, confirmed by reverting only the scheduler and re-running them. The one that matters puts the bad row *before* a valid one, since a test that only asserted "does not raise" would pass against broken code once a guard existed. Severity correction: latent on these instances, not active. a live instance has zero patient-less appointments across all 63,802 rows, ever, and its day-out interval has fired (delivery leads 1050-2871 minutes, consistent with the 2880 interval). The report's 370 patient-less rows and "day-out never fired in 26 days" are from a different deployment. The fix is still right -- a nullable FK dereferenced without a guard is a defect whether or not today's data trips it. Bug 2 -- the admin app could not persist "inherit". The resolver is three-state and correct: absent and true both mean inherit, only an explicit false opts a visit type out. The save path always wrote a hard true or false, so inherit was a state the UI could display and never store, and because Save gathers this tab whichever tab you were on, it rewrote records nobody had opened. Now only false is persisted; an absent key means inherit. Backward compatible with no migration, since existing explicit true values already resolve as inherit. Severity correction: the mass-silencing path the report describes cannot occur on this copy. The global-off branch was already guarded -- when a campaign is globally off the per-type toggles are forced off visually, and the code already preserved the stored value rather than reading the DOM. a live instance bears that out: every visit type carries explicit true, none false, so pinning is confirmed and harm is not. What remained was illegible stored state, which is what this fixes. The save logic is JavaScript inside a Python string, so the Python suite could only assert its shape -- which is precisely how this got through, since the shape was right and the values were wrong. tests/js/test_note_type_save.mjs extracts the decision from the shipped source and executes it under node, including that an untouched visit type round-trips byte-identically and that no input writes a hard true. Wired into pytest via tests/test_admin_js.py, skipped when node is absent. Not addressed: the gtm-extensions copy needs the same patch, and CampaignConfigRecord still has no audit trail -- both recorded in the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported: appointment acknowledgements showed one timezone for every patient rather than the patient's own, while the customer's prior reminders had been patient-local. The resolver already walked patient -> clinic default -> Eastern, and every message path used it, so acknowledgements and reminders behaved identically. What failed was step one. Patient.last_known_timezone is the only patient timezone column in the schema and nothing in routine charting writes it; it is populated by the FHIR Patient tz-code extension. Across the production fleet it holds a value on five patient rows in total and is NULL everywhere else, so every message fell through to the single configured zone. services/timezones.py resolves a US address to an IANA zone: a state table plus three-digit ZIP prefixes for the states split across two zones (Florida panhandle, northwest and southwest Indiana, western Kentucky, east Tennessee, El Paso, western Dakotas, Malheur County, northern Idaho). Arizona resolves to America/Phoenix rather than a Denver that is an hour off for half the year. Prefixes are keyed by state, so a mistyped ZIP degrades to the state's zone instead of jumping to whichever state owns that prefix. A non-US or unrecognized address resolves to nothing and falls through, rather than being placed on a US zone. resolve_timezone_name now goes explicit -> address -> configured default -> Eastern, lazily, so the address lookup is skipped when a zone is already set. Candidates are type-checked before ZoneInfo sees them, since last_known_timezone is free text and a non-string raises a TypeError the existing guard did not catch. Acknowledgement, reminders, telehealth, cancellation, no-show, manual send and preview all pick this up; the four patient queries prefetch addresses. Day-out reminders now fire at their send time in the patient's zone, so 09:00 means 09:00 where the patient is. The pre-query gate therefore has to admit any tick that is a send time in any resolvable zone, about nine US zones instead of one: a single daily reminder opens roughly nine scans a day rather than one, with the rest still returning without touching the appointment table. The configured zone keeps its old name and becomes the fallback for patients whose own zone is unknown; both admin hints say so. Verified against live data, not only tests. Replaying every distinct address shape behind one instance's next 30 days of appointments, all resolve, spread across six zones where there was one: Eastern 414, Phoenix 146, Pacific 143, Central 125, Denver 64, Boise 1. That sweep caught a gap in the first table, TN prefix 374 (Chattanooga), now covered. The three scheduler tests were confirmed to fail against the old behavior before passing against the new. Separately, and unrelated to the feature: this repo is public and several test and comment sites carried values copied off a live instance. A customer name in six places, a Twilio sender, a clinic's toll-free line, and a test patient's id, MRN and phone number. None of it is PHI, the patient being a test record and the phone matching no patient on the instance, but none of it belongs in a public reference plugin either. All replaced with synthetic values in the reserved 555-01xx range and a generic instance reference. The customer name also appears in nine earlier commit messages on this branch, which this commit does not change. Not addressed: address resolution is a state and ZIP lookup rather than a geocode, so a patient in a county a prefix straddles can be an hour off. The per-patient remedy is writing the real zone to last_known_timezone, which outranks the table. Recorded in the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
meerao-cm
force-pushed
the
add-appointment-reminders
branch
from
September 3, 2026 22:41
c3e1e1f to
daed1f9
Compare
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.
Appointment reminders and confirmations over SMS (Twilio) and email (SendGrid), with per-business-line branding and routing and a two-way Y/N confirm flow.
What it does
Canvas's native appointment reminders are a single global setting: one cadence, one wording, no per-visit-type variation, and a confirm loop that cannot be extended. This extension replaces them with campaigns you configure per visit type and per business line, and turns patient replies into real state —
Yconfirms the appointment on the schedule,Nopens a follow-up Task for staff.Five campaigns: confirmation, reminder, cancellation, no-show, and telehealth join. Reminders support multiple intervals (default 3 days plus 45 minutes), each with its own SMS and email template and channel selection.
Who it's for
Practices that need more than one reminder policy: multiple business lines each needing their own sender number and patient-facing name, telehealth visits needing a join link separate from the day-before reminder, compliance-approved copy that must not be reworded by hand, or two-way confirmation that updates the schedule rather than landing in an inbox.
Practices needing one reminder at one interval with one wording should use Canvas's native setting instead; the README says so explicitly.
Safety defaults
TESTING_MODEis a fail-closed allowlist requiring both the patient and the destination address to match before anything is delivered.LOCK_MESSAGE_TEMPLATESfreezes approved copy. Enforcement is server-side: the send endpoint re-renders from the stored template and discards any client-supplied body.MessageSidreplay protection.The README opens with a warning that this replaces Canvas's native reminders and cannot detect them, plus a pre-install checklist, since leaving the native setting enabled would double-message patients.
Components
7 handlers: an appointment event handler, a reminder cron, an admin/history SimpleAPI, a signature-gated Twilio inbound webhook, a timeline filter, and two applications (provider-menu admin, patient chart panel).
Test plan
uv run pytest tests/)mypyclean across all source filescanvas validate)Y/N, bad-signature rejection, replay dedupKnown gaps
Documented in the README: no runtime detection of native reminders (a deployment-checklist step), no send retry queue, email uses implicit opt-out consent with no built-in unsubscribe, and no consolidated confirmation-status roll-up.
Author: Meera Rao meera.rao@canvasmedical.com