M19.1: Notification coverage audit#70
Merged
Merged
Conversation
Confirms 14 Mail classes (no app/Notifications/, no raw-mailer bypass) and lists the two outbound callsites — DeliverInvoiceMail (canonical delivery-log-driven path) and NotificationSettingsController (owner branding preview, not delivery-log driven). Two drift flags for §3 to address: - Spec §4.3 and §4.4 title overpay/underpay alerts as "(Client)" only, but code has paired client/issuer classes and §5.4 / §5.12.2 imply the pair. Small inline spec tweak indicated. - Partial-warning Mail classes are wired into the type map but no service queues them; matches spec §4.4.3 / §5.12.3 (deprecated alert family, legacy display only). Will surface as `stubbed` in the §2 matrix.
…Client) Code has had paired client/issuer Mailables for both alerts since they were introduced, and §5.4 / §5.12.2 of NOTIFICATIONS.md already imply the pair. §4.3 and §4.4 still titled them as "(Client)" only, leaving the spec narrative internally inconsistent. Retitled both as "(Owner + Client)" — matching §4.2 Past Due — and added one owner-alert sub-bullet to each, derived from the actual issuer mail bodies: - §4.3 owner: overpayment percentage + disposition prompt (tip / credit / manual adjustment / refund) with link back into app. - §4.4 owner: outstanding balance + link back for follow-up or manual adjustment. Existing client-alert language and the fragmented-partial-payment guidance are unchanged. Pulled this work forward from §3 of the M19.1 strategy doc; the §1 drift flag is annotated as resolved with a breadcrumb to here.
14-row coverage matrix replaces the MS17 placeholder. One row per outbound Mailable in `app/Mail/`. Status: 12 `live`, 2 `stubbed` (InvoicePartialWarning client/issuer — Mail classes wired into DeliverInvoiceMail's type map but no service queues new rows; matches spec §4.4.3 deprecation guidance and §5.12.3 legacy-display rule). Each row links to its feature test(s) by relative path; delivery log types reference the canonical map in DeliverInvoiceMail::handle plus `send` (manual default) and `—` for NotificationBrandingPreviewMail which bypasses the delivery log per spec §5.14.4.1. `(MS17 deliverable)` suffix dropped from the heading. Strategy doc §2 checkoffs all marked, with a brief §2 results note.
n8bar
added
documentation
Walked each of the 14 matrix rows. Read each Mailable's Blade view and its trigger callsite (InvoiceAlertService for alerts, DeliverInvoiceMail for sends, the two controllers for manual send/receipt and the branding preview, and the past-due console command). Aligned (11 rows): InvoiceReady (modulo F1 question), payment-ack pair, InvoicePaidReceipt, InvoiceIssuerPaidNotice, past-due pair, overpay pair (post-tweak), underpay-issuer, partial-warning pair (stubbed by design, matches §4.4.3 / §5.12.3), NotificationBrandingPreview. Three findings raised for §4: - F1: Spec §3.1.2 says manual send "issues the invoice out of `draft`" but no code path transitions Invoice::status on send. Either spec over-specifies or code is missing the transition. Decision needed. - F2: Spec §5.12.2 / §5.12.3 examples use "(owner)" but shipped labels use "(issuer)" after the MS17 sweep. Small spec tweak. - F3: Client underpay alert view shows USD only; spec §4.4.2 calls for USD/BTC. Either add BTC line to the view or drop "/BTC" from spec. Decision needed (small either way). Strategy doc §3 checkoffs marked; findings recorded inline.
Three findings from §3 resolved in-phase. No backlog deferrals. All
Phase 1 exit criteria satisfied. Strategy doc renamed with `x` prefix
per the project's completed-doc convention; Phase 1 rollup checked off
in the M19 milestone doc.
F1 — Manual send draft transition (Option C):
- Spec §3.1.2 reworded: explicit transition rule with no-regression
guarantee for already-sent/partial/pending/paid/void invoices.
- InvoiceDeliveryController::store: after a successful queue, draft
invoices transition to `sent`; non-draft invoices keep their
current status.
- Two new feature tests in InvoiceDeliveryTest covering both the
transition and the no-regression cases.
F2 — Owner → Issuer in spec display labels:
- Spec §5.12.2 example: `Underpayment alert (owner)` → `(issuer)`.
- Spec §5.12.3 example: `Partial payment warning (client|owner)` →
`(client|issuer)`.
- Catches the spec up to the MS17 owner→issuer terminology sweep
that already shipped in InvoiceDelivery::typeLabel().
F3 — Client underpay alert USD/BTC:
- Spec §4.4.2 narrowed to "outstanding USD amount". Linked invoice
page surfaces full BTC; email body stays simple/USD-focused.
- No code change — current `invoice-underpayment-client.blade.php`
is already aligned with the narrower spec.
Test suite: 336 passing (was 334; +2 new tests for F1).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d075c620c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+72
to
+74
| if ($invoice->status === 'draft') { | ||
| $updates['status'] = 'sent'; | ||
| } |
There was a problem hiding this comment.
When a draft already has detected payment activity, such as a public draft link paid before the owner manually sends it or a partially paid invoice reset to draft and resent, this always changes the invoice to sent without running the existing ledger/status logic. Invoice::refreshPaymentState() deliberately keeps underpaid draft invoices as draft, but once this action issues the invoice out of draft it should immediately become pending/partial based on those existing payments; otherwise it can remain incorrectly sent until some later watcher or correction event happens to refresh it.
Useful? React with 👍 / 👎.
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.
Summary
M19.1 — Notification Coverage Audit, Phase 1 of MS19. All four strategy doc sections complete, all three drift findings resolved in-phase, all exit criteria met.
NOTIFICATIONS.md(12live, 2stubbed).Strategy doc renamed
19.1 → x19.1per the project's completed-doc convention; Phase 1 rollup checked off indocs/milestones/19_RC_HARDENING_OPS.md.Findings & resolutions
draft" but no code path transitionedInvoice::status. Drafts that got sent stayeddraft, andrefreshPaymentStateskipped them entirely → paid invoices could remain stuck atdraft.InvoiceDeliveryController::storeupdated to transitiondraft→senton successful queue. Two new tests inInvoiceDeliveryTest.(owner)but shipped labels use(issuer)after the MS17 sweep.(issuer)to matchInvoiceDelivery::typeLabel().Plus an earlier in-phase tweak (committed before §3 scan): NOTIFICATIONS spec §4.3 / §4.4 retitled
(Client)→(Owner + Client)to match the paired client/issuer Mailable classes that have always existed in code, and an owner-alert sub-bullet added to each, derived from the actual issuer mail bodies.What changed
Spec (
docs/specs/NOTIFICATIONS.md):(Owner + Client), owner-alert sub-bullet added.(issuer).## Coverage & Statussection: 14-row matrix replacing the MS17 placeholder.Code:
app/Http/Controllers/InvoiceDeliveryController.php—store()transitionsdraft→sentafter a successful queue.Tests:
tests/Feature/InvoiceDeliveryTest.php—test_sending_draft_invoice_transitions_status_to_sent(transition path) andtest_sending_does_not_regress_status_for_non_draft_invoice(no-regression forpaid).Docs:
docs/strategies/19.1_NOTIFICATION_COVERAGE_AUDIT.md→x19.1_...(completed-doc rename); §1–§4 results fully populated; exit criteria all checked off.docs/milestones/19_RC_HARDENING_OPS.md— Phase 1 rollup checked off with date and link to the renamed strategy doc.Verification
./vendor/bin/sail artisan test→ 336 passing (1640 assertions). Was 334 before this PR; +2 from the F1 transition tests.tests/Feature/**paths (relative).Test plan
NOTIFICATIONS.mdrenders cleanly in the GitHub web view.Process notes
This phase was the second pass of the M19 GitHub Issues trial. Audit-style phase work — driven by a strategy doc rather than a single bug — didn't have a parent Issue. The PR description is doing the work of a milestone-progress note. Worth a look at the M20 retro: should phases like this still have an umbrella Issue for visibility, or is the strategy doc + PR sufficient?