Move the Do Not Contact list into the SMS module - #41
Draft
DorwardTech wants to merge 2 commits into
Draft
Conversation
The exclusion list binds every sender, not the birthday radar in particular: a booking reminder must skip a number that replied STOP to a birthday message, and it should not have to depend on the radar to find that out. So it moves to the module every sender already shares. SmsOptOut model, table crm_sms_opt_outs -> sms_opt_outs OptOutController + the view, /crm/opt-outs -> /sms/opt-outs the nav entry, contributed by the SMS module ## The operator sees nothing change The sidebar entry is contributed under the CRM module's navigation category, at the same order, with the same label and icon — the sidebar groups by category across modules, so Do Not Contact stays exactly where it has always been, between Replies and Settings. The operator guide needed no edit at all: it describes the screen, never the URL. Only a bookmark would notice. ## Why a rename rather than a copy This is the compliance record — every row is a customer instruction not to be contacted. Schema::rename is one atomic metadata operation with no window in which a send path could read an empty or half-populated list. Copy-then-switch has exactly that window. The migration is also reversible, unlike most here, because a rollback that stranded this table under a name nothing reads would be the worst failure it could have. ## What did NOT move, and the reason is in the schema The inbound side stays in Crm. crm_inbound_sms has twelve columns and four are birthday-party domain: matched_send_id (FK to the radar's ledger), bookeo_customer_id, parent_name, kid_name. Moving it means either restructuring live reply history or leaving a module named for a capability holding a column called kid_name — a decision to take deliberately, not to slip into a refactor. Written up in Modules/Sms/README.md. That work buys the next sender reply *attribution*. It is not needed for opt-outs, which are done: a reminders module can honour Do Not Contact today without knowing the radar exists, and there is a test that disables the CRM module entirely and manages the list anyway. ## Also ClassReferenceTest gained SmsOptOut and learned that a fully-qualified inline reference needs no import — the Blade view uses that form, since a compiled template has no namespace to import into. It was flagging that as an orphan. Verified the corrected logic still catches a planted one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
ReplyChecker was importing Modules\Crm\app\Models\SmsOptOut, which stopped existing in this branch — 24 failures, all "Class not found" inside webhook processing. The rewrite had landed correctly. What undid it was the step where I proved ClassReferenceTest still catches a planted orphan: I deleted a `use` line with sed and put the file back with `git checkout <file>`, which restores from the index — and the index held HEAD, i.e. the file as it was before any of this branch's edits. So the check I was validating reverted the fix it was meant to protect. The reason it then passed review was that I ran the two halves of the check at different moments. The import-resolution half ran before the revert and was clean; only the orphaned-reference half ran after, and an explicit import of a class that no longer exists is invisible to that one. Both halves now run in a single pass, and this exact regression was replanted afterwards to confirm the first half names the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
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
2 participants
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.
Phase 2 — or rather, the half of Phase 2 that the schema allows. I looked at the two tables before starting and they are not equally movable. That finding shaped this PR, so it's the first thing below.
The finding
crm_sms_opt_outsmobile,name,source,note,opted_out_atcrm_inbound_smsmatched_send_id(FK→crm_sms_sends),bookeo_customer_id,parent_name,kid_nameThe opt-out list is generic and moves cleanly. The inbound table is a third birthday-party domain — moving it means either restructuring live reply history or leaving a module named for a capability holding a column called
kid_name. That's a decision to take deliberately, not to slip into a refactor. See What did not move.What moved
SmsOptOut→Modules\Sms\app\Models\SmsOptOut, tablecrm_sms_opt_outs→sms_opt_outsOptOutController+ view →Modules/Sms,/crm/opt-outs→/sms/opt-outs, gatedmodule:smsPlus
SmsOptOut::isSuppressed()— the single-number check a future sender wants, so it doesn't pull the whole list to answer yes/no.The operator sees nothing change
The sidebar entry is contributed under the CRM module's navigation category, same order, same label, same icon. The sidebar groups by category across modules, so Do Not Contact stays exactly where it has always been — between Replies and Settings.
The operator guide needed no edit at all: it describes the screen, never the URL. Only a bookmark would notice.
Why a rename, not a copy
This is the compliance record — every row is a customer instruction not to be contacted.
Schema::renameis one atomic metadata operation, with no window in which a send path could read an empty or half-populated list. Copy-then-switch has exactly that window. The migration is also reversible, unlike most in this repo, because a rollback that stranded this table under a name nothing reads would be the worst failure it could possibly have.What did not move
The inbound side stays in
Modules/Crm: webhook receiver,crm_webhook_deliveries, the reply store, the Replies screen.The shape when it happens: the SMS module asks "who is this number?" through an attribution hook and the CRM module answers, so
matched_send_idand friends become a generic source reference plus whatever context the identifier supplies. A reply from a number nobody recognises is stored unattributed — already today's documented behaviour.That work buys the next sender reply attribution. It is not needed for opt-outs. A reminders module can honour Do Not Contact today without knowing the birthday radar exists — and there's a test that disables the CRM module entirely and manages the list anyway.
Written up in
Modules/Sms/README.mdso the reasoning survives without this PR description.Verification
The three sweeps from the last round, all clean: no stale table name, no stale namespace, no stale route name or view, both resolver checks green.
ClassReferenceTestgainedSmsOptOut— and immediately flagged a false positive on the Blade view, which references the model by inline FQN. A compiled template has no namespace to import into, so that form is always correct. Taught the check to strip fully-qualified references first, then re-verified it still catches a planted orphan by deleting a realusestatement and confirming it fired. The shell version I first re-typed the regex into didn't reproduce the PHP escaping, so I ran the exact test logic from a file rather than trusting the approximation.New tests: the page works with the CRM module switched off,
isSuppressed(), the table name is what the model reads, and a platform sweep never downgrades an explicit STOP.vendor/can't be installed in this environment (codeload.github.comis blocked by the proxy), so CI is the verification for the suite.Generated by Claude Code