Skip to content

SMS Customer Outreach 2.0.0 — fix the age parser, add batch editing, retry the Bookeo log, ship an operator guide - #34

Merged
DorwardTech merged 5 commits into
claude/zone3-darwin-internal-tool-YQKKNfrom
CRM
Jul 25, 2026
Merged

SMS Customer Outreach 2.0.0 — fix the age parser, add batch editing, retry the Bookeo log, ship an operator guide#34
DorwardTech merged 5 commits into
claude/zone3-darwin-internal-tool-YQKKNfrom
CRM

Conversation

@DorwardTech

@DorwardTech DorwardTech commented Jul 25, 2026

Copy link
Copy Markdown
Owner

A verification pass against the stated requirements found three things that were not as believed. All three are fixed here, plus the guide, the developer documentation the module folder was missing, and a clean-up for the credentials still sitting in the database.

Verification result

Claim Verdict
1st of month, pull ~11 months back ✅ target month = next month, bookings read from that month minus a year
Parent name, child name, mobile
Age they are turning a date of birth produced the wrong age
Bookings are definitely "Parties" ✅ matched on the product name, not the booking title
A message hasn't already been sent ✅ 75-day window + unique(mobile, batch_month)
Number isn't on the opt-out list ✅ local table is authoritative
Batch waits for approval
"I have the edit tools I need" there were none — approve or cancel, nothing else
All SMS sent and received written to Bookeo ⚠️ wired both ways, but nothing ever retried a failed write

1. A date of birth produced the wrong age

The field is open text. The old extractor was one line — preg_match('/\d{1,2}/') plus one — which takes the first one or two digits anywhere in the string:

Answer Message said Should be
7 turning 8
2018-03-12 no age safe by luck (grabs "20", out of range)
12/03/2018 turning 13 turning 8
12 March 2018 turning 13 turning 8
12/03/18 turning 13 turning 8

It was reading the day of the month. The Australian format is the common one, so this was live and customer-facing.

New KidAge parser. The two kinds of answer count differently and that is the whole job: a stated age is the age at last year's party so it gains one, while a date of birth already fixes the year and must not. Anything ambiguous returns null and the message omits the age — 12/03 with no year is refused rather than guessed at, and a mobile typed into the wrong field yields nothing.

Also widened the label match to date of birth / dob / birth date, since a form asking for a date of birth won't say "how old".

2. There were no edit tools

Every pending row now has a Fix disclosure: edit that message, or drop that recipient. No more cancel-and-re-prepare to correct one row.

Two things deliberately withheld:

  • The mobile is not editable anywhere. Numbers only come from a Bookeo pull, so a wrong one is dropped, never retyped. That is what makes a mistyped recipient structurally impossible, and an edit box on that column would have thrown it away. A test asserts the rendered page contains no input for it.
  • An edited message clears the same compliance bar as a generated one, through the same assertCompliant() and the same configured phrases. Otherwise this box is the single route to sending an SMS with no opt-out notice. Over one segment is allowed but repriced.

Both actions recount recipient_count and estimated_credits — stale values would be worse than cosmetic, since recipient_count is what canBeApproved() tests and what the confirm dialog quotes. Guarded three ways: batch awaiting approval, row still pending, and row belongs to that batch (nested bindings resolve independently, so without the ownership check a crafted URL could reach into a sent batch through a pending one).

3. The Bookeo log had no retry

Both mirror paths mark the row on success and skip marked rows — which reads exactly like a retry. There wasn't one. The send mirror ran once per batch, the reply mirror once per message, so any Bookeo failure lost that line permanently.

Not hypothetical: the expired key made every mirror write fail for a day. Messages went out, replies arrived, none of it reached a customer file, and there was no way to catch up.

Now fixed twice over — crm:bookeo-log-backfill for "do it now", and up to 20 rows retried on every hourly reply-checker pass so a backlog self-heals. That retry is last in the job and cannot fail it: notifying you about a real reply is the point of the run.

It gives up after three consecutive failures rather than grinding the whole backlog against a dead key, and points at crm:bookeo-check. Two cases it does not treat as a backlog, because retrying can't fix either: a send whose booking carried no customerId, and a reply from a number never in the send ledger.

Documentation: two audiences, two files

Modules/Crm/GUIDE.md — the operator guide, written for whoever is running the venue. No Laravel, no terminal. Monthly rhythm, what gets filtered and why, reading and fixing a batch, replies and STOPs, Do Not Contact, every setting, costs, symptom-keyed troubleshooting, known limitations. Commands in an appendix.

A User guide button on /admin/modules renders it at /admin/modules/{slug}/guide. Generic mechanism (HasUserGuide returns a path and a heading), so any future module gets it; the button only appears when the file exists, so an unwritten guide degrades to no link rather than a 404.

Markdown on disk rather than a Blade page on purpose: reviewable in a diff, greppable, and readable on GitHub by someone who can't get into the admin — one of the situations a guide is for.

Modules/Crm/README.md — the developer document, which the folder was missing. Every other module keeps its technical documentation at that path, and the CRM module had an operator guide there instead, so a new developer had nothing. It covers the architecture, the schema and both status flows, the two invariants the design exists to protect (no send without a human approve; recipient numbers only ever joined from source data), the vendor quirks that have each cost a bug — Bookeo authenticating by query parameter, 5c giving every endpoint its own success envelope, 5c having no endpoint to add an opt-out — and the traps that are invisible from the code: Blade's two PHP-island forms not being mixable, Laravel splitting a rule string on | inside a regex, and Eloquent writing a Carbon's wall clock without converting it.

2.0.0, major not minor: reply timestamps were 9.5 h out, the age parser reported a day-of-month as an age, and the webhook matched no real delivery. Data written by 1.x can't be assumed correct.

Clearing the credentials already in the database

The forward fix for the Bookeo leak landed in #32 — the client builds its own error message, never chains the Guzzle exception, and redacts in all three encodings. But rows written before that fix still hold the api key and secret key, and crm_radar_batches.error_message is rendered on the batch page, so opening an old failed batch still prints both onto an admin screen.

2026_07_25_140000_scrub_leaked_bookeo_credentials.php redacts them. Two choices worth stating:

  • By parameter name, not by comparing against the configured key. The strings in those rows are the credentials as they were before rotation; config no longer knows them.
  • chunkById, not chunk. Each update makes the row stop matching the filter it was selected by, so offset paging would step over the rows that shuffle down into pages already read and silently skip roughly half. There is a test with more rows than one chunk for exactly that.

The rest of each message survives — the HTTP status, the path, and the Bookeo errorId are still readable, because blanking the row would lose the audit trail.

Credentials also remain in storage/logs/laravel.log on any host that ran a failing request before the fix. A migration has no business rewriting log files, so that one is noted in the README and needs doing by hand.

Things I got wrong on the way, since they're in the diff

  • Claimed in a comment that Str::markdown() escapes raw HTML. It doesn't — CommonMark passes it through. Now html_input => 'strip' explicitly, and the comment says what's true.
  • First version of the guide-heading assertion pinned CommonMark's exact element markup, which would break on a library upgrade. Softened to "rendered, not dumped".
  • The ownership test built two batches for the same month with the same recipient and hit unique(mobile, batch_month) — the dedupe rule doing its job. The fixture was wrong, not the constraint; two batches only ever coexist across months.
  • The README first described the degradation ladder as dropping the greeting as its last rung. It doesn't: variants() nests brand → hook → ask, so the order is ask → hook → brand, and the greeting is omitted only when there is no usable parent name.

Tests

~66 added across six files: 43 age assertions (both answer kinds, the four formats that were wrong, every refusal, a moving target year); 16 batch-edit tests; 15 backfill tests including that a second run writes nothing; 11 guide tests including one per topic the guide must cover, so a rewrite that drops the approval rule fails; 6 for the credential scrub, covering idempotency and the chunking trap.

vendor/ can't be installed here (codeload.github.com is blocked by the proxy, so composer can't fetch dists), so everything was pre-flighted through standalone runners — 43 age assertions, every inspect() case and every scrub pattern case executed directly, plus the regression scripts green. CI is the real verification, and it caught a production timezone bug earlier in this branch that no local check would have.


Generated by Claude Code

claude added 5 commits July 25, 2026 10:30
Two of the three gaps the verification pass turned up.

**A date of birth produced a wrong age in a customer's text.** The age field
on the booking form is open text, so parents type an age or a date of birth in
whatever format they like. The old extractor was one line — take the first one
or two digits found anywhere, add one. For the Australian-format
"12/03/2018" that is the DAY: the parent of a child turning 8 was told "Ivy
turning 13 already?". "12 March 2018" and "12/03/18" failed the same way. The
ISO form escaped only by luck (it grabs "20", which lands out of range and so
says nothing).

Parsing now lives in KidAge, because the two kinds of answer have to be
counted differently and that is the whole job: a stated AGE is the age at LAST
year's party, so it gains one, while a DATE OF BIRTH already fixes the year and
must not. Ambiguity returns null and the message omits the age — a wrong number
in a customer's text is worse than no number, so "12/03" with no year is
refused rather than guessed at, and a mobile typed into the wrong field yields
nothing.

Also widened the label match to "date of birth" / "dob" / "birth date", since a
form asking for a date of birth won't say "how old".

**A wrong batch meant cancel-the-lot.** The approval screen had exactly two
controls, so one bad row cost a full re-prepare: re-reading all of Bookeo and
re-checking every number to fix one message. Each row awaiting approval now has
a Fix disclosure holding an edit box and a Drop button.

Two properties matter more than the convenience, and both are tested:

- The MOBILE is not editable anywhere. Recipients can only come from rows a
  Bookeo pull created, so a wrong number is dropped, never retyped. That is
  what makes a mistyped recipient structurally impossible, and an edit box on
  that column would have quietly thrown it away. One test asserts the rendered
  page contains no input for it at all.
- An edited message clears the same compliance bar as a generated one, through
  the same assertCompliant() and the same configured required phrases.
  Otherwise this box is the single route to sending an SMS with no opt-out
  notice. Over one segment is allowed but repriced, because that is a cost
  decision rather than a compliance failure — and the batch totals follow, or
  the approval gate would understate the bill.

Dropping and editing both recount recipient_count and estimated_credits.
Leaving those stale would be worse than cosmetic: recipient_count is what
canBeApproved() tests, what the confirm dialog quotes, and what the balance
warning compares against. Drop the last recipient and the approve button
correctly disappears.

Guarded three ways: only while the batch is awaiting approval, only on a row
still pending, and only on a row that belongs to that batch — nested route
bindings resolve independently, so without the ownership check a crafted URL
could reach into a sent batch through a pending one.

Tests: 43 age assertions covering both answer kinds, the four formats that were
actively wrong, every refusal case, and a moving target year; 16 batch-edit
tests covering the drop, the recount, the compliance refusals, the guards and
the absent mobile field.
The third gap from the verification pass, and the one already causing damage.

Every send and every reply is meant to be mirrored onto the customer's Bookeo
file. Both paths mark the row on success — sms_sends.bookeo_logged_at and
crm_inbound_sms.mirrored_at — and both filter on that marker being null, which
reads exactly like a retry. Nothing retried. The send mirror ran once per batch
and the reply mirror once per message, so any Bookeo failure lost that line
permanently.

Not hypothetical: the expired Bookeo key made every mirror write fail for a
day. The messages went out and the replies arrived, but none of it reached a
customer file and there was no way to catch up.

Two ways to fix it, both wired:

  php artisan crm:bookeo-log-backfill --dry-run   # how much is waiting
  php artisan crm:bookeo-log-backfill             # write it now

and the reply checker now retries up to 20 rows on every hourly pass, so a
backlog clears itself without anyone remembering. Deliberately last in that
job and unable to fail it: notifying Robert about a real reply is the point of
the run, and staff visibility on a customer file must never be the reason that
didn't happen.

The markers are what make retrying safe — a row already mirrored has a
timestamp and is never revisited, so this cannot double-write a customer's
file. That property has its own test, because a backfill nobody dares re-run is
not much of a backfill.

It gives up after three consecutive failures rather than working through the
whole backlog. A backlog almost always fails for one reason, identically, on
every row — usually the credentials — so grinding two hundred rows against a
dead key wastes the API budget and buries the real cause in noise. The command
says so and points at crm:bookeo-check.

Two things it deliberately does NOT treat as a backlog, because neither is
fixable by retrying: a send whose Bookeo booking carried no customerId (nothing
to attach a line to), and a reply from a number that was never in the send
ledger (no customer to attach it to). Both have tests so they can't start being
counted as failures.

Fifteen tests: the mirror lands and is marked, a second run writes nothing, off
means off, the two unattachable cases are ignored, a failed send is not
mirrored, the early stop happens at three not ten, a limit is honoured so the
hourly retry can't burst, STOP gets the opt-out wording, and the command's dry
run writes nothing.
The bus test: if I'm unavailable, whoever picks this up should be able to run
it. A markdown file in the repo only satisfies that if somebody thinks to look
in the repo, so the app renders it too.

`Modules/Crm/README.md` is written for whoever is running the venue, not for a
developer — no Laravel, no terminal. It covers the monthly rhythm, what gets
filtered out of a batch and why, reading and fixing a batch, approving, replies
and STOPs, Do Not Contact, every setting, what it costs, a troubleshooting
section keyed to symptoms rather than causes, and the known limitations. Console
commands are relegated to an appendix, because needing one is the exception.

The two things it states most plainly are the two that matter: nothing is texted
without a person clicking Approve, and a wrong phone number gets the row dropped
rather than the number retyped.

A **User guide** button now sits on /admin/modules next to Settings, rendering
the markdown at /admin/modules/{slug}/guide. The mechanism is generic — a
manifest implementing HasUserGuide returns a path and a heading — so any future
module gets the same treatment. The button only shows when the file actually
exists, so a manifest pointing at something unwritten degrades to no link
rather than a link to a 404.

Markdown on disk rather than a Blade page, deliberately: it stays reviewable in
a diff, greppable, and readable on GitHub by someone who cannot get into the
admin at all — which is one of the situations a guide exists for.

Version 2.0.0, major rather than minor. Reply timestamps were 9.5 hours out,
the age parser reported a date of birth's day-of-month as an age, and the
webhook matched no real delivery. Data written by 1.x cannot be assumed correct,
which is exactly what a major bump is for.

Also set html_input to strip on the markdown renderer. CommonMark passes raw
HTML through by default — the comment I first wrote claimed the opposite. The
guide is repo-controlled prose so it was never an injection route, but a file
that grows an HTML block later should not silently gain the ability to inject
markup into the admin.

Eleven tests, including one per topic the guide has to cover, so a rewrite that
quietly drops the approval rule or the opt-out keywords fails. The heading
assertion checks that markdown was rendered rather than dumped without pinning
CommonMark's exact attribute set, which would break on a library upgrade.
crm_sms_sends is uniquely indexed on (mobile, batch_month) so the same
parent can't be texted twice in one campaign. The ownership test built
two batches for the same month with the same recipient and hit that
index, so it failed before it could exercise what it was testing.

The fixture takes a month now and the second batch uses a different one,
which is also the realistic shape: two batches only ever coexist across
months. The constraint stays as it is — it was doing its job.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
Two things the 2.0.0 pass left unfinished.

The module had an operator guide sitting where every other module keeps
its technical documentation. GUIDE.md now holds the operator guide (the
manifest points the in-app button at it) and README.md is the developer
document the folder was missing: architecture, schema, the two invariants
the design exists to protect, the vendor quirks that each cost a bug, and
the Blade/validation/timezone traps that are not visible from the code.

BookeoClient stopped leaking the api key and secret key into
crm_radar_batches.error_message, but the rows written before that fix
still hold the pair — and the batch page still renders them, so opening
an old failed batch prints both onto an admin screen. The migration
redacts by parameter name rather than by comparing against the configured
key, because the strings in those rows are the credentials as they were
before rotation. The rest of each message survives, so the audit trail
and the Bookeo errorId are still there to read.

chunkById rather than chunk: the update makes each row stop matching the
filter it was selected by, and offset paging would then step over the
rows that shuffle down into pages already read. Tested with more rows
than one chunk for exactly that reason.

Credentials also remain in storage/logs/laravel.log on any host that ran
a failing request before the fix. A migration has no business rewriting
log files, so that one is noted in the README instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
@DorwardTech
DorwardTech marked this pull request as ready for review July 25, 2026 11:06
@DorwardTech
DorwardTech merged commit 2e6cde8 into claude/zone3-darwin-internal-tool-YQKKN Jul 25, 2026
2 checks passed
@DorwardTech
DorwardTech deleted the CRM branch August 3, 2026 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants