Split Bookeo and 5c SMS into modules of their own - #40
Merged
DorwardTech merged 4 commits intoAug 3, 2026
Merged
Conversation
A run sheet needs Bookeo. Booking reminders need SMS. Neither has anything
to do with the birthday radar, and both clients lived inside it — so this
extracts them before a second consumer arrives rather than after.
Modules/Bookeo BookeoClient, config/bookeo.php, bookeo:check, the API
notes, the credential-leak tests
Modules/Sms SmsClient, config/sms.php, MobileNumber, GsmMessage, the
API notes, the client tests
Modules/Crm everything else, now declaring dependencies() on both
Integration modules, so neither has navigation, settings or an operator
guide — there is nothing there for whoever runs the venue. They still
appear on /admin/modules, which is the point: when Bookeo credentials are
the problem, the module list is where somebody will look.
Singletons on purpose, and there is a test for it. BookeoClient paces
itself against Bookeo's rate limit through a shared cache key; two
instances would each think they had the whole budget.
## Behaviour-neutral by construction
No table moved, no app_settings key was renamed, and every env var name is
unchanged — only which config file reads it. Nothing about a deployment
moves with this. `crm:bookeo-check` became `bookeo:check`, which is the
one operator-visible change, and the guide and README moved with it.
## The deploy trap
ModuleRegistry::sync() inserts a newly discovered module DISABLED, and
ModuleSeeder only auto-enables a fixed list. So the first deploy after
this would have left CRM enabled while both modules it now depends on were
off. Nothing would visibly break — providers register their bindings
regardless of enabled state, which is deliberate so a disabled module can
still be diagnosed from a shell — but the next person to toggle CRM off
and on would hit "depends on [bookeo], which is not enabled" with no idea
why. A migration enables them where CRM is already on; ModuleSeeder covers
fresh installs.
## What did not move, and why
The whole inbound side stays in CRM: the webhook receiver, the deliveries
table, the opt-out list, the reply store and both screens. ReplyChecker
matches an inbound message back to crm_sms_sends to work out whose it is,
so moving it as-is would make Sms depend on Crm, which depends on Sms.
Breaking that needs an attribution hook — Sms asks "who is this number?"
and Crm answers — which is worth doing when a second sender exists, and is
noted in Modules/Sms/README.md along with the consequence in the meantime:
anything new that sends SMS has to check CRM's opt-out list itself.
bookeo:check lost its write-back advisory for the same reason. It now
reports the customer-write permission factually, without naming who wants
it; crm:bookeo-log-backfill --dry-run already says what its absence means
for the SMS mirror.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
212 failures, and the rule causing them is the one this PR adds: enable() refuses a module whose dependencies are not already on, and nine test files enable 'crm' directly. Fixed in tests/Pest.php rather than nine times over, and by reading dependencies() off the manifest rather than hardcoding ['bookeo','sms'] — the same list would otherwise need updating by hand the day it changes. Production solves the same problem in ModuleSeeder by ordering its list. IntegrationModuleTest keeps calling the registry directly: it is asserting the dependency rule itself, so going through a helper that satisfies dependencies for it would test nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
The 92 remaining failures were eight files that a repo-wide grep called clean, because none of them ever named the class in full. SmsClient and BookeoClient lived in Modules\Crm\app\Services alongside RadarSender, RadarBatchBuilder, OwnerNotifier, ReplyChecker, BookeoSmsLog and DashboardSummary. A sibling in the same namespace resolves with no `use` statement — so when the two clients moved out, there was no old FQN anywhere to find and rewrite, and every one of those services broke with "Class not found" at runtime. CoworkStateParser was the same story with MobileNumber. That is the failure mode of a namespace move, and searching for the thing you moved cannot see it. So there is now a test for it, with two halves that catch opposite mistakes: one resolves every first-party `use` to a file that must exist, the other finds a watched class used with no import and no class of that name in the file's own namespace. The second is the one that would have caught this. Both were run against the tree before being written into a test, and the second was confirmed to fail on the real breakage rather than merely return empty — the fix script had reported "ADDED" for CoworkStateParser without checking, and its `use` block never landed because that file had no existing imports for the regex to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
Last failure: the module went to 2.1.0 and this test asserted '2.0.0'. Rewritten to assert what the test is actually for — that the manifest, the rendered page and GUIDE.md all state the same version. The literal made every release edit this test while catching nothing it claims to: a bump that updated the manifest and forgot the guide would still have passed, so long as somebody changed the literal too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014E51bA27LnFMK7v4YigZY1
DorwardTech
marked this pull request as ready for review
August 3, 2026 01:46
DorwardTech
merged commit Aug 3, 2026
bc9f01b
into
claude/zone3-darwin-internal-tool-YQKKN
2 checks passed
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
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 1 of the plan. A run sheet needs Bookeo; booking reminders need SMS. Neither has anything to do with the birthday radar, and both clients lived inside it — so this extracts them before the second consumer arrives rather than after.
Modules/BookeoBookeoClient,config/bookeo.php,bookeo:check, the API notes, the credential-leak testsModules/SmsSmsClient,config/sms.php,MobileNumber,GsmMessage, the API notes, the client testsModules/Crmdependencies() => ['bookeo', 'sms']Both are integration modules — no navigation, no settings page, no operator guide, because there's nothing there for whoever runs the venue. They still appear on
/admin/modules, which is the point: when Bookeo credentials are the problem, the module list is where somebody will look.Singletons on purpose, with a test pinning it.
BookeoClientpaces itself against Bookeo's rate limit through a shared cache key — two instances would each think they had the whole budget.Behaviour-neutral by construction
app_settingskey renamed.BOOKEO_API_KEYand friends are untouched, so nothing about a deployment moves with this.The one operator-visible change is
crm:bookeo-check→bookeo:check. The guide and both READMEs moved with it, and there are no stale references left.The deploy trap this had to handle
ModuleRegistry::sync()inserts a newly discovered module disabled, andModuleSeederonly auto-enables a fixed list. Without intervention, the first deploy would leave CRM enabled while both modules it now depends on were off.Nothing would visibly break — service providers register their bindings regardless of enabled state, which is deliberate so a disabled module can still be diagnosed from a shell — but the next person to toggle CRM off and on again would hit
Module [crm] depends on [bookeo], which is not enabledwith no idea why.So: a migration enables them where CRM is already on, and
ModuleSeederlists them first for fresh installs (enable()refuses a module whose dependencies aren't already up).What deliberately did not move
The whole inbound side stays in CRM — webhook receiver, deliveries table, opt-out list, reply store, both screens.
ReplyCheckermatches an inbound message back tocrm_sms_sendsto work out which customer and child it belongs to. Moving it as-is would makeSmsdepend onCrm, which depends onSms.Breaking that needs an attribution hook — Sms asks "who is this number?", Crm answers, an unrecognised number stays unattributed (already the documented behaviour). Worth doing when a second sender exists, not before. It's written up in
Modules/Sms/README.mdalong with the consequence in the meantime, stated plainly:bookeo:checklost its write-back advisory for the same reason — it was reading acrm.*setting. It now reports the customer-write permission factually without naming who wants it;crm:bookeo-log-backfill --dry-runalready says what its absence means for the SMS mirror.Verification
Mechanical and wide: 19 files had an import rewritten, 5 had a config read repointed. I checked each of these came back clean —
Modules\Crmreference anywhere in the two new modulesconfig('crm.sms.*')/config('crm.bookeo.*')readcomposer validatepasses and the lockfile needs no change — the rootModules\ → Modules/PSR-4 mapping already covers the new namespaces10 new tests in
tests/Feature/Modules/IntegrationModuleTest.phpcover the wiring that fails silently: both modules discovered, each client shared not transient, bindings available while the module is disabled, CRM's declared dependencies, enable refused before dependencies are up, disable refused while CRM uses them, and credentials read from the config files that moved.vendor/can't be installed in this environment (codeload.github.comis blocked by the proxy), so CI is the verification for the suite itself.Next
Phase 2 — opt-out list and webhook receiver into
Modules/Sms, behind the attribution hook — when you want it. That one touches the webhook URL and live settings rows, so it deserves its own PR.Generated by Claude Code