fix(self-scheduling): stop reporting a committed booking as a failure - #1059
Merged
Conversation
Found by the manual smoke on the testes site. Two defects, both pre-existing and unrelated to the captcha epic — the smoke only exposed them. The visitor saw an error and the appointment was created anyway. The cause was a TypeError: `$appointment['id']` arrives from `$wpdb` as a string — the MySQL driver returns every column as one — and `AppointmentReceiptHandler:: get_receipt_url()` declares `int` under strict_types. It fires inside the booking-confirmation email, which runs *after* the transaction commits. It surfaced as HTTP 500 rather than a handled error because the handler caught `\Exception`, and a TypeError is an `\Error`: `\Throwable`, but not `\Exception`. So it escaped as an uncaught fatal — no JSON body, leaving the client with nothing to render but its generic "an error occurred", for an appointment that was already in the database. The natural response, booking again, either duplicated it or hit the duplicate guard. The suite never caught it because `makeAppointment()` built `id` as an int. The test was green because it exercised a world that does not exist; the fixture now returns a string, like the database does. Beyond the cast, the post-commit region is now structurally safe. Each step — the after-create hook, the notification emails, the receipt link — is isolated with its own catch and a log naming the stage, and the AJAX handler catches `\Throwable`. Once the row is committed the response is success, carrying `degraded: true` and `email_sent: false` rather than claiming a confirmation that never went out. Answering failure there is worse than answering a degraded truth, because it sends the visitor to book a second time. The error log also revealed a second, independent bug. `on_appointment_created()` passes `$appointment_id` as the fourth argument of `ActivityLog::log()`, which is `$user_id` — the sibling `on_appointment_cancelled()` passes `$cancelled_by` there, confirming the intent. With `fk_ffc_activity_log_user` present MySQL rejects the insert, so every appointment_created row is lost; without the FK the row is written against whichever user holds that id. Both regression tests were verified failing against the previous code: the email one reproduces the production TypeError verbatim, and the attribution ones fail with `777 is not identical to 42`. Closes #1058 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
rpgmem
marked this pull request as ready for review
September 5, 2026 05:03
rpgmem
enabled auto-merge (squash)
September 5, 2026 05:04
Coverage Report for CI Build 33946135004Warning No base build found for commit Coverage: 89.932%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
84 tasks
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.
Fecha #1058. Encontrados no smoke manual do ambiente de testes; ambos pré-existentes e sem relação com o épico do captcha (#1053).
Bug 1 — a reserva era feita e o visitante ouvia que falhou
$appointment['id']chega do$wpdbcomo string — o driver MySQL devolve toda coluna como string — e o método declaraintsobstrict_types. Dispara dentro do e-mail de confirmação, que roda depois do commit da transação.Virou HTTP 500 em vez de erro tratado porque o handler capturava
\Exception, eTypeErroré um\Error:\Throwable, mas não\Exception. Escapava como fatal não capturado — sem corpo JSON, deixando ao cliente apenas sua mensagem genérica, para um agendamento que já estava no banco. E a reação natural do usuário, reservar de novo, duplicava ou colidia com o guard de duplicidade.Por que a suíte não pegou:
makeAppointment()construíaidcomo int. O teste era verde porque exercitava um mundo que não existe. A fixture agora devolve string, como o banco.Bug 2 — todo activity log de agendamento criado era perdido
on_appointment_created()passa$appointment_idno 4º argumento deActivityLog::log(), que é$user_id. Repare no payload: o contexto dizuser_id: 1, a coluna recebeu7— o id do agendamento. O irmãoon_appointment_cancelled()passa$cancelled_byali, o que confirma a intenção do parâmetro.Com a FK presente, o insert é rejeitado e a linha se perde. Sem a FK, é gravada atribuída ao usuário errado.
O que mudou além dos dois casts
A região pós-commit ficou estruturalmente segura: cada etapa — hook de after-create, e-mails, link de recibo — isolada com catch próprio e log que nomeia a etapa; o handler AJAX captura
\Throwable.E o principal: uma reserva commitada nunca mais é reportada como falha. A resposta vira sucesso com
degraded: trueeemail_sent: false, em vez de afirmar uma confirmação que não saiu. Responder falha ali é pior do que responder uma verdade degradada, porque manda o visitante reservar de novo.Type of change
Test plan
composer test— 7510 testes, 21651 asserções, verdevendor/bin/phpcs --standard=phpcs.xml.dist includes/— limpovendor/bin/phpcs --standard=phpcs-tests.xml.dist tests/— limpocomposer lint(PHPStan) — não executado (pacote dist-only, proxy deste ambiente dá 403 na API do GitHub). Fica para o CI.Os dois testes de regressão foram verificados falhando contra o código antigo:
test_booking_confirmation_survives_a_string_id_from_the_databasereproduz o TypeError de produção literalmenteActivityLogAppointmentAttributionTest(2 casos) falha com777 is not identical to 42e777 is not identical to 0—777sendo o id do agendamento vazando para o campo de usuárioChecklist
CHANGELOG.mdatualizado sob[Unreleased] / FixedNota sobre o alias mock
ActivityLogAppointmentAttributionTestfica em classe própria porque alias-mockarActivityLogé global ao processo, e as outras asserções doActivityLogSubscriberTestdependem do buffering real. Ele também precisa desetConstantsMap— um alias mock não carrega constantes de classe, e o subscriber lêLEVEL_INFO.O que este PR não faz
Não audita os demais parâmetros tipados que recebem colunas do
$wpdb. O Bug 1 é de uma classe — string do banco em parâmetrointestrito — e nada garante que seja a única ocorrência. Uma varredura é trabalho próprio; se quiser, abro issue.Closes #1058
🤖 Generated with Claude Code
https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
Generated by Claude Code