feat(emails-rr): card expiry warning email (NPPD-1524)#155
Open
kmwilkerson wants to merge 9 commits into
Open
Conversation
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
Eight production-code fixes from the PR #155 code review: - date_i18n() -> wp_date() for *RENEWAL_DATE*. The previous call misinterpreted the GMT timestamp from get_time('next_payment') for sites whose timezone straddles the UTC date boundary, producing the wrong calendar date in the email. wp_date() handles GMT input correctly. Matches what slice 2b's Email_Preview already uses for the same placeholder's sample value. - Register newspack_deactivation -> unschedule_cron OUTSIDE the WooCommerce_Subscriptions::is_enabled() guard so the cleanup runs even if WCS or Reader Activation was disabled in between the initial schedule and the eventual Newspack deactivation. Previously an init() short-circuit on the deactivation request would leave an orphan callback-less cron event in wp_options['cron'] forever. - schedule_cron() defers first run by 24h (was time(), effectively immediate). Publishers get an opt-in window to review the template or flip the email post to draft before any seed pass or scan runs. - scan_expiring_cards() wraps each maybe_send_warning() call in try/catch + Logger::log. Previously one throwing pair (SMTP filter rejecting a malformed address, third-party WC hook throwing on save) aborted the entire daily pass and skipped every later in-window pair until tomorrow's cron. - seed_in_window_pairs() now uses PHP_INT_MAX for the discovery cap instead of get_limit_per_pass(). The per-pass cap exists to bound steady-state cron memory; the seed runs once per install and MUST cover the entire in-window set. Without this, sites with more in-window pairs than the cap would seed only the first 100, then the next cron's normal-scan branch would send to the un-seeded remainder -- exactly the Day 0 burst the seed was built to prevent. - seed_in_window_pairs() also fires Logger::newspack_log alongside Logger::log. Logger::log is gated by NEWSPACK_LOG_LEVEL (off on most production sites), but newspack_log fires the 'newspack_log' action unconditionally -- the seed is a one-time significant event and must be diagnostically discoverable for publisher support. - Discovery SQL gains ORDER BY t.token_id ASC before LIMIT for deterministic ordering across cron runs (without it, MySQL is free to return any LIMIT-sized subset; seed -> normal-scan handoffs could see disjoint windows on sites that exceed the per-pass cap). - Discovery SQL gains CHAR_LENGTH(ey.meta_value) = 4 guard so legacy 2-digit expiry_year meta values (set_expiry_year does not pad, unlike set_expiry_month) are filtered out explicitly rather than silently NULL-ing through STR_TO_DATE. CLI side: - card_expiry_warning_backfill checks Emails::can_send_email() before the confirmation prompt. Previously a publisher could confirm "send to N readers" while the email post was in draft and discover post-hoc that "Sent 0 email(s)" because send_email's status guard silently failed every send. Findings consciously deferred (need bigger design decisions): - Multi-token-per-subscription SENT_META collision (rare edge case; meta format change) - CLI cross-invocation idempotency (needs design decision on whether re-running the backfill should detect prior runs) - Soft-bounce/wp_mail-returns-true blocking retries (needs bounce tracking) - Gateway-side token rotation (Stripe Card Updater) desync - Lazy-create can_send_email failure -> deferred-seed swallow - On-hold subscription exclusion (design intent question) - Multisite per-site cron scheduling Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
…s trade-off Three test-quality fixes from the PR #155 code review sweep: - Smoke scenario 1 (cron scheduled) now calls wp_clear_scheduled_hook BEFORE schedule_cron(). Previously init() may have already scheduled the hook on plugin load, making the assertion pass even if schedule_cron() itself silently regressed (e.g. typo'd recurrence string). - Smoke cleanup snapshots wp_next_scheduled() at script entry and restores it as part of the $cleanup closure chain. Previously the trailing wp_clear_scheduled_hook call nuked the host site's pre-smoke production cron schedule unconditionally, leaving the daily scan unscheduled until the next pageload runs init. - Smoke scenario 9 (SQL LIMIT) now asserts the uncapped baseline returns >=2 pairs first, then asserts limit=1 returns EXACTLY 1. The previous `<= 1` assertion passed on a broken-discovery 0-row return, masking the regression that scenario 9 exists to catch. - Unit test tear_down keeps remove_all_filters() but adds an inline note documenting why it's safe today (no production-side callbacks register the filters yet) and what to switch to if that changes. Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f62acdb to
67f0e73
Compare
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
Eight production-code fixes from the PR #155 code review: - date_i18n() -> wp_date() for *RENEWAL_DATE*. The previous call misinterpreted the GMT timestamp from get_time('next_payment') for sites whose timezone straddles the UTC date boundary, producing the wrong calendar date in the email. wp_date() handles GMT input correctly. Matches what slice 2b's Email_Preview already uses for the same placeholder's sample value. - Register newspack_deactivation -> unschedule_cron OUTSIDE the WooCommerce_Subscriptions::is_enabled() guard so the cleanup runs even if WCS or Reader Activation was disabled in between the initial schedule and the eventual Newspack deactivation. Previously an init() short-circuit on the deactivation request would leave an orphan callback-less cron event in wp_options['cron'] forever. - schedule_cron() defers first run by 24h (was time(), effectively immediate). Publishers get an opt-in window to review the template or flip the email post to draft before any seed pass or scan runs. - scan_expiring_cards() wraps each maybe_send_warning() call in try/catch + Logger::log. Previously one throwing pair (SMTP filter rejecting a malformed address, third-party WC hook throwing on save) aborted the entire daily pass and skipped every later in-window pair until tomorrow's cron. - seed_in_window_pairs() now uses PHP_INT_MAX for the discovery cap instead of get_limit_per_pass(). The per-pass cap exists to bound steady-state cron memory; the seed runs once per install and MUST cover the entire in-window set. Without this, sites with more in-window pairs than the cap would seed only the first 100, then the next cron's normal-scan branch would send to the un-seeded remainder -- exactly the Day 0 burst the seed was built to prevent. - seed_in_window_pairs() also fires Logger::newspack_log alongside Logger::log. Logger::log is gated by NEWSPACK_LOG_LEVEL (off on most production sites), but newspack_log fires the 'newspack_log' action unconditionally -- the seed is a one-time significant event and must be diagnostically discoverable for publisher support. - Discovery SQL gains ORDER BY t.token_id ASC before LIMIT for deterministic ordering across cron runs (without it, MySQL is free to return any LIMIT-sized subset; seed -> normal-scan handoffs could see disjoint windows on sites that exceed the per-pass cap). - Discovery SQL gains CHAR_LENGTH(ey.meta_value) = 4 guard so legacy 2-digit expiry_year meta values (set_expiry_year does not pad, unlike set_expiry_month) are filtered out explicitly rather than silently NULL-ing through STR_TO_DATE. CLI side: - card_expiry_warning_backfill checks Emails::can_send_email() before the confirmation prompt. Previously a publisher could confirm "send to N readers" while the email post was in draft and discover post-hoc that "Sent 0 email(s)" because send_email's status guard silently failed every send. Findings consciously deferred (need bigger design decisions): - Multi-token-per-subscription SENT_META collision (rare edge case; meta format change) - CLI cross-invocation idempotency (needs design decision on whether re-running the backfill should detect prior runs) - Soft-bounce/wp_mail-returns-true blocking retries (needs bounce tracking) - Gateway-side token rotation (Stripe Card Updater) desync - Lazy-create can_send_email failure -> deferred-seed swallow - On-hold subscription exclusion (design intent question) - Multisite per-site cron scheduling Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5af1bd6 to
d0d16ef
Compare
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
…s trade-off Three test-quality fixes from the PR #155 code review sweep: - Smoke scenario 1 (cron scheduled) now calls wp_clear_scheduled_hook BEFORE schedule_cron(). Previously init() may have already scheduled the hook on plugin load, making the assertion pass even if schedule_cron() itself silently regressed (e.g. typo'd recurrence string). - Smoke cleanup snapshots wp_next_scheduled() at script entry and restores it as part of the $cleanup closure chain. Previously the trailing wp_clear_scheduled_hook call nuked the host site's pre-smoke production cron schedule unconditionally, leaving the daily scan unscheduled until the next pageload runs init. - Smoke scenario 9 (SQL LIMIT) now asserts the uncapped baseline returns >=2 pairs first, then asserts limit=1 returns EXACTLY 1. The previous `<= 1` assertion passed on a broken-discovery 0-row return, masking the regression that scenario 9 exists to catch. - Unit test tear_down keeps remove_all_filters() but adds an inline note documenting why it's safe today (no production-side callbacks register the filters yet) and what to switch to if that changes. Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a Newspack-managed card expiry warning email for active WooCommerce Subscriptions, with cron-based discovery, idempotency, first-deploy seeding, CLI backfill, preview sample support, and test coverage.
Changes:
- Adds
Card_Expiry_Warningintegration, email template, cron scheduling, seed behavior, and send/idempotency logic. - Adds a WP-CLI backfill command for publisher-initiated card expiry warning sends.
- Adds unit tests and manual integration smoke coverage for config, cron, seed, limit, and CLI behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
plugins/newspack-plugin/includes/plugins/woocommerce-subscriptions/class-card-expiry-warning.php |
Implements card expiry warning email discovery, seeding, sending, and idempotency. |
plugins/newspack-plugin/includes/plugins/woocommerce-subscriptions/class-woocommerce-subscriptions.php |
Loads and initializes the new card expiry warning integration. |
plugins/newspack-plugin/includes/templates/reader-revenue-emails/card-expiry-warning.php |
Adds the default Newspack email template. |
plugins/newspack-plugin/includes/cli/class-woocommerce-subscriptions.php |
Adds the card expiry warning backfill CLI command. |
plugins/newspack-plugin/includes/cli/class-initializer.php |
Registers the new WP-CLI command. |
plugins/newspack-plugin/includes/wizards/newspack/class-email-preview.php |
Adjusts preview sample renewal date for the new email context. |
plugins/newspack-plugin/tests/unit-tests/card-expiry-warning.php |
Adds PHPUnit coverage for config and core behavior contracts. |
plugins/newspack-plugin/tests/integration/README.md |
Documents manual integration smoke tests. |
plugins/newspack-plugin/tests/integration/card-expiry-warning-smoke.php |
Adds manual end-to-end smoke scenarios for WooCommerce Subscriptions environments. |
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
…(Copilot review on #155) Copilot review on #155 surfaced a real starvation bug: > Applying the per-pass `LIMIT` to token IDs before subscription/ > idempotency filtering can starve later cards indefinitely. Once the > first 100 expiring tokens are already marked with `SENT_META`, > unattached, or otherwise not sendable, each daily scan will keep > selecting the same token IDs and never reach newer unsent tokens > that are still in the window. Diagnosis: `ORDER BY t.token_id ASC LIMIT N` is deterministic — the same first-N token_ids surface every scan. Once those N are all gated (SEEDED, SENT, or not sendable for some other reason), every subsequent scan no-ops and the unprocessed remainder never gets reached. The class docblock's "remaining warnings roll into subsequent cron runs" promise was broken in exactly the scenario it was written for. Fix: stop applying the cap at SQL discovery. `scan_expiring_cards` now passes `PHP_INT_MAX` to `get_in_window_pairs` and applies the per-pass cap to ACTUAL SENDS in the foreach loop — already-processed pairs skip without consuming the cap, so new tokens get warned. Memory bound on discovery is now the window size (`newspack_card_expiry_warning_days` × subscription-density), not a SQL LIMIT. For typical newsroom-scale sites this is small; sites with very large in-window populations can shrink the window via the days filter. Trade-off captured in the class docblock's renamed "Publisher-respect — per-pass send cap" section, with explicit explanation of the starvation bug the legacy shape had — so a future maintainer who sees the cap-in-PHP shape and thinks "the SQL LIMIT was cleaner" doesn't re-introduce the bug. Touched files: - `class-card-expiry-warning.php`: - `scan_expiring_cards`: pass PHP_INT_MAX to discovery, break the foreach once `$sent >= $cap` (counts only successful sends). - Class docblock "Publisher-respect — per-pass SQL LIMIT" section renamed to "Publisher-respect — per-pass send cap" with the starvation rationale captured. - `LIMIT_PER_PASS_DEFAULT` constant docblock: updated to reference the send-cap shape. - `get_limit_per_pass` docblock: updated, plus an explicit note that the cap is NOT applied at SQL with a one-line summary of why (the Copilot finding). - `tests/integration/card-expiry-warning-smoke.php` scenario 9: rewritten to test the send-cap via scan_expiring_cards (was: tested the SQL LIMIT via get_in_window_pairs directly). New shape: clear per-token meta on both subs, set `limit_per_pass=1`, run `scan_expiring_cards`, assert exactly 1 email sent. Smoke header bullet updated to match. `get_in_window_pairs($days, $limit)` signature is unchanged — the SQL LIMIT clause still works for explicit callers (the CLI passes PHP_INT_MAX too, so nothing in production hits the limit path). Verification: - PHPCS clean (full output, not tailed) on both modified files. - Full PHP suite: 1355 tests / 4311 assertions / 0 failures / 5 pre-existing skips. Matches pre-fix baseline exactly — the unit tests for `get_limit_per_pass` test the helper's return value, not its application site, so they continue to pass. Smoke scenario 9 is integration-only and verified manually. Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
…dy-processed (Copilot review on #155) Two related Copilot findings from the latest #155 review: 1. **CLI --limit has the same starvation bug as the cron path had.** `--limit` was passed to `get_in_window_pairs($days, $limit)` as a SQL LIMIT. Once the first N tokens were marked SENT, every CLI invocation would no-op against the same N and never reach later unsent pairs. 2. **CLI --dry-run reported already-sent pairs as "Would send".** Dry-run iterated every discovered pair without checking the idempotency gate, so it produced false-positive previews — pairs that wouldn't fire (SENT meta blocks even with bypass=true) still showed up as "Would send to ...". Both fixes ride the same change: discovery passes PHP_INT_MAX (no SQL LIMIT), then a single `array_filter` against `Card_Expiry_Warning::is_already_processed( …, $bypass=true )` filters out pairs the gate would block. The filtered list is what the confirm prompt counts, what --dry-run prints, and what the real sends iterate over (capped by --limit at the foreach level). Also: **the inline filter docblock at `get_limit_per_pass()` was stale** (Copilot finding 3). The outer function docblock was updated in the prior fix but the inline `apply_filters` docblock still said "discovery query." Updated to reflect the send-cap semantic. `is_already_processed()` was promoted from `private static` to `public static` so the CLI dry-run can call it for accurate previews. Marked `@internal` in the docblock — the CLI is the only legitimate caller, the surface is not part of the stable public API. Touched files: - `includes/plugins/woocommerce-subscriptions/class-card-expiry-warning.php`: - Promote `is_already_processed` to public (added @internal note). - Fix the inline filter docblock at `get_limit_per_pass()` to reflect the send-cap semantic (Copilot finding 3). - `includes/cli/class-woocommerce-subscriptions.php`: - Pass `PHP_INT_MAX` to `get_in_window_pairs`. - Filter the result with `Card_Expiry_Warning::is_already_processed( …, true )` to drop pairs that wouldn't actually send. - Apply `--limit` as a send-loop break inside the foreach (matches the cron's shape from the earlier fix). - Confirm prompt now uses `min($count, $limit)` so the prompt's number matches the post-run total. - "No pairs" message updated to note the filter. Verification: - PHPCS clean (full output, not tailed) on both files. - Full PHP suite: 1355 / 4311 / 0 failures / 5 pre-existing skips — matches baseline exactly. The Reflection-based unit tests for `is_already_processed` continue to pass even with the method now public (Reflection works on public methods too). Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sends a warning to a reader whose saved credit card is about to
expire on an active WC Subscription. Steady-state behavior: a daily
wp-cron scan finds CC tokens whose last-valid-day falls within the
next 14 days (filterable via `newspack_card_expiry_warning_days`),
looks up active subscriptions on each token via WCS_Payment_Tokens,
and sends one warning per (subscription, token+expiry) pair.
Idempotency lives in a per-subscription meta key
(`_newspack_card_expiry_warning_sent`) whose value encodes
`token_id:MM/YYYY`. Re-running the cron is a no-op for already-sent
pairs. When a publisher's reader updates their payment method, the
`woocommerce_subscription_payment_method_updated` action clears the
meta so a future new-card expiry can re-trigger.
First-deploy behavior (Day 0 burst protection) is deliberately NOT
addressed in this commit — that's commit 2, which adds the seed
mechanism + WP-CLI backfill. Without those, this commit's behavior
on first deploy is to send warnings to every reader currently in
the 14-day window the next time cron fires. Don't merge this
commit alone — it stacks with the seed work that follows.
Unified-schema integration:
- Registers via `newspack_email_configs` (slice 1's modern
schema). The four UI-metadata fields are declared explicitly in
`add_email_config`:
* `chip => 'reader-revenue'` (matches category; could be
derived via apply_config_defaults but explicit declaration
keeps the UI surface visible at the registration site)
* `recipient => 'reader'`
* `recommended => true` (this email is a defense against
involuntary churn — recommended-by-default is correct)
* `trigger_description => '...'`
- No change to `class-emails-section.php` — the legacy registry
pattern that file used to host was removed in slice 1's refactor.
The same metadata that legacy added to the registry entry now
lives directly on the config above.
- Sample-substitution map already covers all four card-specific
tokens (`*CARD_LAST_4*`, `*EXPIRY_DATE*`, `*RENEWAL_DATE*`,
`*UPDATE_PAYMENT_URL*`) — they were added proactively as part of
slice 2b's preview surface. The only sample tweak here is
`*RENEWAL_DATE*` from `+1 year` to `+30 days`, matching the
realistic context for a card-expiry preview.
Template: standard Newspack reader-revenue block template, using
the established token set.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… for card expiry (NPPD-1524) The publisher-respect machinery commit-1 deliberately deferred. First-deploy seed mechanism On the very first scheduled scan after install (detected via the absence of a new `newspack_card_expiry_warning_seeded` option), Card_Expiry_Warning runs a SEED pass instead of a normal scan: iterates the same in-window (subscription, token) pairs the normal scan would have sent to, marks each as already-warned via the existing SENT_META, and writes the seeded flag — all WITHOUT sending. On subsequent scheduled runs, the normal scan proceeds. Rationale: a publisher who has just installed Newspack hasn't seen this email's wizard surface yet and hasn't had a chance to consent to the mass-email behavior. Sending real emails to real readers on Day 0 before that consent is a trust violation. The reader-outcome harm (one cycle of card-failure for in-window readers) is bounded and recoverable via the WP-CLI backfill below; the trust harm is not. Logs the seed result via Newspack\Logger and explicitly references the WP-CLI backfill as the publisher-controlled escape hatch. Per-pass SQL LIMIT (default 100) The discovery query (`get_expiring_cc_tokens`) now carries a SQL- level `LIMIT %d` filterable via `newspack_card_expiry_warning_limit_per_pass`. Applied AT THE SQL LEVEL, not after fetching — so a migration day or unusual burst can't load unbounded rows into PHP memory. A site exceeding the cap on a given day will see remaining warnings roll into subsequent cron runs; sustained load is unaffected. Docblock on `get_limit_per_pass()` makes this trade-off explicit. `$bypass_idempotency` arg on maybe_send_warning() Defaults `false` (cron path unchanged). The WP-CLI backfill passes `true` so the seeded SENT_META doesn't block its explicit publisher-initiated sends. Method is now public + returns bool so the CLI can count sends. WP-CLI: `wp newspack card-expiry-warning-backfill` Lives on the existing `Newspack\CLI\WooCommerce_Subscriptions` class (next to `migrate_expired_subscriptions`), registered with the same flat-hyphenated naming convention. Flags: [--dry-run] Print would-send list without sending. No confirmation prompt. [--limit=N] Cap sends per invocation. Default uncapped (publisher-initiated explicit action). [--days=N] Window override. Default: Card_Expiry_Warning::get_days_before_expiry(). [--yes] Auto-handled by WP_CLI::confirm($prompt, $assoc_args). Confirmation gate prompts with the discovered count for honesty before sending. Output uses past-tense for real action ("Sent to ..."), "Would send to ..." for dry-run. Also new in card-expiry-warning class: - `SEEDED_OPTION` constant (stored with autoload=false). - `LIMIT_PER_PASS_DEFAULT` constant (100). - `get_limit_per_pass()` filter-aware helper. - `seed_in_window_pairs()` — the seed pass. - `get_in_window_pairs( $days, $limit )` — public discovery helper that both seed/scan and CLI use, returning an array of `['subscription' => WC_Subscription, 'token' => WC_Payment_Token_CC]` pairs. Cap applied via the SQL-level LIMIT. Class-level docblock documents the publisher-respect rationale + the backfill relationship so a future reader understands the seed's intent without having to dig through git history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…respect machinery Adds 20 PHPUnit tests in tests/unit-tests/card-expiry-warning.php covering config shape, the days-before-expiry filter, the limit-per-pass filter, the first-deploy seed flag-setting (via observable SEEDED_OPTION side effects), and the maybe_send_warning() $bypass_idempotency arg signature. Adds a 12-scenario manual integration smoke script (tests/integration/card-expiry-warning-smoke.php) that exercises cron scheduling, happy-path send, idempotency, payment-method-update flag clear, new-card re-send, unattached-card no-op, first-deploy seed, the $bypass_idempotency escape hatch, SQL-LIMIT cap, and CLI dry-run + normal-run behaviors against a real WC + WC Subscriptions environment. Test docblocks reference the "Publisher-respect — first-deploy seed" and "Publisher-respect — per-pass SQL LIMIT" sections of the class docblock so the design intent → docblock → test lock-in chain is explicit. Preflight adjustments on the production class (no behavior change): - Bump the seed Logger::log() type from 'info' to 'warning'. The seed is a significant one-time event; a publisher debugging "card-expiry warnings didn't fire on day 1" needs the entry to stand out under grep, and at 'warning' the entry carries a [WARNING]: prefix in error_log output. - Add @internal to the maybe_send_warning() docblock to flag that the method is public only because the WP-CLI backfill needs to pass the bypass arg; it is not part of the stable public API. Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight production-code fixes from the PR #155 code review: - date_i18n() -> wp_date() for *RENEWAL_DATE*. The previous call misinterpreted the GMT timestamp from get_time('next_payment') for sites whose timezone straddles the UTC date boundary, producing the wrong calendar date in the email. wp_date() handles GMT input correctly. Matches what slice 2b's Email_Preview already uses for the same placeholder's sample value. - Register newspack_deactivation -> unschedule_cron OUTSIDE the WooCommerce_Subscriptions::is_enabled() guard so the cleanup runs even if WCS or Reader Activation was disabled in between the initial schedule and the eventual Newspack deactivation. Previously an init() short-circuit on the deactivation request would leave an orphan callback-less cron event in wp_options['cron'] forever. - schedule_cron() defers first run by 24h (was time(), effectively immediate). Publishers get an opt-in window to review the template or flip the email post to draft before any seed pass or scan runs. - scan_expiring_cards() wraps each maybe_send_warning() call in try/catch + Logger::log. Previously one throwing pair (SMTP filter rejecting a malformed address, third-party WC hook throwing on save) aborted the entire daily pass and skipped every later in-window pair until tomorrow's cron. - seed_in_window_pairs() now uses PHP_INT_MAX for the discovery cap instead of get_limit_per_pass(). The per-pass cap exists to bound steady-state cron memory; the seed runs once per install and MUST cover the entire in-window set. Without this, sites with more in-window pairs than the cap would seed only the first 100, then the next cron's normal-scan branch would send to the un-seeded remainder -- exactly the Day 0 burst the seed was built to prevent. - seed_in_window_pairs() also fires Logger::newspack_log alongside Logger::log. Logger::log is gated by NEWSPACK_LOG_LEVEL (off on most production sites), but newspack_log fires the 'newspack_log' action unconditionally -- the seed is a one-time significant event and must be diagnostically discoverable for publisher support. - Discovery SQL gains ORDER BY t.token_id ASC before LIMIT for deterministic ordering across cron runs (without it, MySQL is free to return any LIMIT-sized subset; seed -> normal-scan handoffs could see disjoint windows on sites that exceed the per-pass cap). - Discovery SQL gains CHAR_LENGTH(ey.meta_value) = 4 guard so legacy 2-digit expiry_year meta values (set_expiry_year does not pad, unlike set_expiry_month) are filtered out explicitly rather than silently NULL-ing through STR_TO_DATE. CLI side: - card_expiry_warning_backfill checks Emails::can_send_email() before the confirmation prompt. Previously a publisher could confirm "send to N readers" while the email post was in draft and discover post-hoc that "Sent 0 email(s)" because send_email's status guard silently failed every send. Findings consciously deferred (need bigger design decisions): - Multi-token-per-subscription SENT_META collision (rare edge case; meta format change) - CLI cross-invocation idempotency (needs design decision on whether re-running the backfill should detect prior runs) - Soft-bounce/wp_mail-returns-true blocking retries (needs bounce tracking) - Gateway-side token rotation (Stripe Card Updater) desync - Lazy-create can_send_email failure -> deferred-seed swallow - On-hold subscription exclusion (design intent question) - Multisite per-site cron scheduling Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s trade-off Three test-quality fixes from the PR #155 code review sweep: - Smoke scenario 1 (cron scheduled) now calls wp_clear_scheduled_hook BEFORE schedule_cron(). Previously init() may have already scheduled the hook on plugin load, making the assertion pass even if schedule_cron() itself silently regressed (e.g. typo'd recurrence string). - Smoke cleanup snapshots wp_next_scheduled() at script entry and restores it as part of the $cleanup closure chain. Previously the trailing wp_clear_scheduled_hook call nuked the host site's pre-smoke production cron schedule unconditionally, leaving the daily scan unscheduled until the next pageload runs init. - Smoke scenario 9 (SQL LIMIT) now asserts the uncapped baseline returns >=2 pairs first, then asserts limit=1 returns EXACTLY 1. The previous `<= 1` assertion passed on a broken-discovery 0-row return, masking the regression that scenario 9 exists to catch. - Unit test tear_down keeps remove_all_filters() but adds an inline note documenting why it's safe today (no production-side callbacks register the filters yet) and what to switch to if that changes. Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix-1 + Fix-2) Addresses two confirmed bugs from the NPPD-1524 code review (tracked in NPPD-1568) with a single structural fix: **Fix-1 (multi-token SENT_META collision):** the single per-subscription SENT_META key collapsed multi-token state — when one subscription had multiple in-window CC tokens, each iteration overwrote the prior token's idempotency marker. Result: suppression for all but the last- iterated token was lost. **Fix-2 (CLI cross-invocation idempotency):** with one shared key and the bypass flag skipping the read entirely, a second invocation of the WP-CLI backfill on the same window re-sent every email. Bypass was read-only; the post-send write didn't distinguish "actually sent" from "seeded suppression marker." Both bugs share a root cause: the old single-meta-key conflated "would have warned but didn't" with "actually sent." Splitting into two per-token meta prefixes resolves both atomically. ## Schema - Dropped: `const SENT_META = '_newspack_card_expiry_warning_sent'` - Added: `const SEEDED_META_PREFIX = '_newspack_card_expiry_warning_seeded_'` - Added: `const SENT_META_PREFIX = '_newspack_card_expiry_warning_sent_'` - Full meta key per token: `PREFIX . $token->get_id()` - Meta value: `$expiry_key` (`token_id:MM/YYYY`) — preserves the legacy value-match semantics that auto-invalidate the marker when a token's expiry changes in place (e.g., Stripe Card Account Updater webhook reissues the same `token_id` with a new expiry). ## New gating helper `is_already_processed( $sub, $token_id, $expiry_key, $bypass )` — the single source of truth for "skip this pair?": - SENT meta matches `$expiry_key` → always blocks (even with bypass) - SEEDED meta matches `$expiry_key` → blocks unless `$bypass=true` - Otherwise → proceed Docblock explicitly warns against simplifying to `metadata_exists()`: the Card-Updater scenario depends on value-match to auto-invalidate the stale marker. ## Write sites - `seed_in_window_pairs()`: writes SEEDED meta per token (was SENT). - `maybe_send_warning()`: replaces inline idempotency check with the new helper. After a successful send, deletes the SEEDED entry AND writes the SENT entry — preserves the "at most one of {SEEDED, SENT}" invariant per (subscription, token). - `clear_sent_flag()`: iterates `$subscription->get_meta_data()` and deletes every entry with either prefix. `$changed` guard avoids firing `save()` (and its hook chain) when no match. WC CRUD pattern matches surrounding code; no LIKE-query against `wp_postmeta`. ## Smoke script updates Five sites in `tests/integration/card-expiry-warning-smoke.php` updated to read per-token keys: scenarios 2/3/5 read SENT meta after sends; scenario 4 verifies clear; scenario 7 reads SEEDED meta after the seed pass; scenarios 8 + 11 read SEEDED + verify the promote invariant (SEEDED deleted, SENT written) on bypass-driven sends. Scenario 10 uses `clear_sent_flag` directly to reset state. Header docblock + scenario labels updated to reference the new terminology ("SEEDED meta" vs "SENT meta") instead of the legacy "SENT_META" shorthand. ## Verification - PHPCS clean (full output, not tailed) — exit 0 on all 3 modified files. - Full PHP suite: 1350 tests / 4297 assertions / 0 failures / 5 skips (pre-existing env-gated). Matches the pre-commit baseline exactly — no test count change, no assertion delta. Schema refactor threaded cleanly through the existing test paths. Tests covering the *new* invariants (multi-token independence, CLI cross-invocation idempotency, SEEDED → SENT promote, clear_sent_flag clears both prefixes) land in commit 2. Refs: NPPD-1568, NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ariants (NPPD-1568) Five new tests in a "Two-meta-keys-per-token schema" bucket of `tests/unit-tests/card-expiry-warning.php`, each locking in an invariant the NPPD-1568 schema change establishes. Test approach: the gating helper `is_already_processed()` is the design center for SEEDED + SENT gating, and the schema's correctness hinges on its per-token behavior — so the tests drive it directly via Reflection rather than going through `maybe_send_warning()` end-to-end (which would require mocking the full subscription → user → WC URL → wp_mail chain). End-to-end behavior — including the SEEDED → SENT promote step inside `maybe_send_warning` — is covered by scenarios 7, 8, and 11 of the integration smoke script with real WC; the unit tests lock in the structural invariants at the helper layer. A test-local subscription stub (anonymous class) implements the WC_Data subset the schema needs: `get_meta`, `update_meta_data`, `delete_meta_data`, `save`, `get_meta_data` (array-of-objects form). Test-local rather than extending tests/mocks/wc-mocks.php so the mock surface stays scoped to this file. Tests: - `test_multi_token_subscription_seeds_independently` — Fix-1 lock-in. SEEDED meta for token 100 must NOT gate token 200 on the same subscription. The legacy single-key shape would have collapsed both tokens onto one value. - `test_multi_token_subscription_scans_independently_post_seed` — Fix-1 lock-in (scan path). After both tokens are seeded, a normal-scan call (no bypass) must skip BOTH. Without per-token keys, only the last-iterated token's seed would survive. - `test_cli_backfill_idempotent_across_invocations` — Fix-2 lock-in. SENT meta must block even with `bypass_idempotency=true` — otherwise a second operator invocation of the CLI on the same window re-sends every email. - `test_clear_sent_flag_clears_all_per_token_meta` — exhaustive clear contract. Set up a sub with 4 meta entries (2 tokens × 2 prefixes), call clear_sent_flag, assert all 4 entries gone via `get_meta_data()`. The "set both for one token" state isn't naturally reachable (the invariant is at-most-one-of-{SEEDED,SENT}), but it's the exhaustive shape that proves the clear handles every prefix. - `test_seeded_meta_deleted_when_sent_promotes` — decision-#2 invariant. Asserts the structural property that SENT (post-promote) blocks both normal-scan AND bypass paths, AND that the absence of SEEDED in the post-promote state is observable. The full delete-SEEDED → write-SENT sequence inside maybe_send_warning runs end-to-end in smoke scenario 8. Verification: - PHPCS clean (full output, not tailed) — exit 0. - Card-expiry class in isolation: 25 tests / 56 assertions (was 20/42); +5 tests / +14 assertions exactly matches the new bucket. - Full PHP suite: 1355 tests / 4311 assertions / 0 failures / 5 pre-existing env-gated skips (baseline was 1350/4297/5). +5 tests +14 assertions delta matches exactly — no other test counts shifted, confirming the WC shim from slice 2a stays isolated (the 4 `Real WC not loaded` skips in emails-section-woocommerce.php unchanged). Refs: NPPD-1568, NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(Copilot review on #155) Copilot review on #155 surfaced a real starvation bug: > Applying the per-pass `LIMIT` to token IDs before subscription/ > idempotency filtering can starve later cards indefinitely. Once the > first 100 expiring tokens are already marked with `SENT_META`, > unattached, or otherwise not sendable, each daily scan will keep > selecting the same token IDs and never reach newer unsent tokens > that are still in the window. Diagnosis: `ORDER BY t.token_id ASC LIMIT N` is deterministic — the same first-N token_ids surface every scan. Once those N are all gated (SEEDED, SENT, or not sendable for some other reason), every subsequent scan no-ops and the unprocessed remainder never gets reached. The class docblock's "remaining warnings roll into subsequent cron runs" promise was broken in exactly the scenario it was written for. Fix: stop applying the cap at SQL discovery. `scan_expiring_cards` now passes `PHP_INT_MAX` to `get_in_window_pairs` and applies the per-pass cap to ACTUAL SENDS in the foreach loop — already-processed pairs skip without consuming the cap, so new tokens get warned. Memory bound on discovery is now the window size (`newspack_card_expiry_warning_days` × subscription-density), not a SQL LIMIT. For typical newsroom-scale sites this is small; sites with very large in-window populations can shrink the window via the days filter. Trade-off captured in the class docblock's renamed "Publisher-respect — per-pass send cap" section, with explicit explanation of the starvation bug the legacy shape had — so a future maintainer who sees the cap-in-PHP shape and thinks "the SQL LIMIT was cleaner" doesn't re-introduce the bug. Touched files: - `class-card-expiry-warning.php`: - `scan_expiring_cards`: pass PHP_INT_MAX to discovery, break the foreach once `$sent >= $cap` (counts only successful sends). - Class docblock "Publisher-respect — per-pass SQL LIMIT" section renamed to "Publisher-respect — per-pass send cap" with the starvation rationale captured. - `LIMIT_PER_PASS_DEFAULT` constant docblock: updated to reference the send-cap shape. - `get_limit_per_pass` docblock: updated, plus an explicit note that the cap is NOT applied at SQL with a one-line summary of why (the Copilot finding). - `tests/integration/card-expiry-warning-smoke.php` scenario 9: rewritten to test the send-cap via scan_expiring_cards (was: tested the SQL LIMIT via get_in_window_pairs directly). New shape: clear per-token meta on both subs, set `limit_per_pass=1`, run `scan_expiring_cards`, assert exactly 1 email sent. Smoke header bullet updated to match. `get_in_window_pairs($days, $limit)` signature is unchanged — the SQL LIMIT clause still works for explicit callers (the CLI passes PHP_INT_MAX too, so nothing in production hits the limit path). Verification: - PHPCS clean (full output, not tailed) on both modified files. - Full PHP suite: 1355 tests / 4311 assertions / 0 failures / 5 pre-existing skips. Matches pre-fix baseline exactly — the unit tests for `get_limit_per_pass` test the helper's return value, not its application site, so they continue to pass. Smoke scenario 9 is integration-only and verified manually. Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dy-processed (Copilot review on #155) Two related Copilot findings from the latest #155 review: 1. **CLI --limit has the same starvation bug as the cron path had.** `--limit` was passed to `get_in_window_pairs($days, $limit)` as a SQL LIMIT. Once the first N tokens were marked SENT, every CLI invocation would no-op against the same N and never reach later unsent pairs. 2. **CLI --dry-run reported already-sent pairs as "Would send".** Dry-run iterated every discovered pair without checking the idempotency gate, so it produced false-positive previews — pairs that wouldn't fire (SENT meta blocks even with bypass=true) still showed up as "Would send to ...". Both fixes ride the same change: discovery passes PHP_INT_MAX (no SQL LIMIT), then a single `array_filter` against `Card_Expiry_Warning::is_already_processed( …, $bypass=true )` filters out pairs the gate would block. The filtered list is what the confirm prompt counts, what --dry-run prints, and what the real sends iterate over (capped by --limit at the foreach level). Also: **the inline filter docblock at `get_limit_per_pass()` was stale** (Copilot finding 3). The outer function docblock was updated in the prior fix but the inline `apply_filters` docblock still said "discovery query." Updated to reflect the send-cap semantic. `is_already_processed()` was promoted from `private static` to `public static` so the CLI dry-run can call it for accurate previews. Marked `@internal` in the docblock — the CLI is the only legitimate caller, the surface is not part of the stable public API. Touched files: - `includes/plugins/woocommerce-subscriptions/class-card-expiry-warning.php`: - Promote `is_already_processed` to public (added @internal note). - Fix the inline filter docblock at `get_limit_per_pass()` to reflect the send-cap semantic (Copilot finding 3). - `includes/cli/class-woocommerce-subscriptions.php`: - Pass `PHP_INT_MAX` to `get_in_window_pairs`. - Filter the result with `Card_Expiry_Warning::is_already_processed( …, true )` to drop pairs that wouldn't actually send. - Apply `--limit` as a send-loop break inside the foreach (matches the cron's shape from the earlier fix). - Confirm prompt now uses `min($count, $limit)` so the prompt's number matches the post-run total. - "No pairs" message updated to note the filter. Verification: - PHPCS clean (full output, not tailed) on both files. - Full PHP suite: 1355 / 4311 / 0 failures / 5 pre-existing skips — matches baseline exactly. The Reflection-based unit tests for `is_already_processed` continue to pass even with the method now public (Reflection works on public methods too). Refs: NPPD-1524 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
67f0e73 to
35d4c27
Compare
a345109 to
7803892
Compare
3 tasks
kmwilkerson
added a commit
that referenced
this pull request
Jun 1, 2026
…mp (NPPD-1566) Three Copilot findings on PR #162 + one user-reported UX issue from smoketest. All small polish; bundled into one commit. COPILOT #1 — api_get_settings default diverges from Emails::get_from_email() A previous review-fix commit guarded the empty-host edge in api_get_settings: when wp_parse_url(network_home_url()) returns no host, return '' for sender_email_address default instead of the visibly-broken 'no-reply@'. The intent was avoiding a publisher- facing broken placeholder. Copilot caught the divergence: `Emails::get_from_email()` (the actual send path) does NOT guard the same case — it still returns the literal 'no-reply@'. Result: on a misconfigured site, the modal placeholder would show '' while outbound mail would send from 'no-reply@'. Silent UX gap: publishers see one default and get another. Aligned the response to match the send path's literal 'no-reply@'. The broken-looking placeholder is honest signal that the site's home URL doesn't resolve; hiding it didn't make the underlying issue go away. Fixing the misconfigured-host fallback at the send path is a separate follow-up. Updated the test: `test_get_settings_default_sender_email_has_non_empty_host` (which asserted no bare `no-reply@`) renamed to `test_get_settings_default_sender_email_matches_send_path`, now asserts the contract directly: `Emails::get_from_email()` === defaults.sender_email_address. Added `test_get_settings_defaults_match_send_path_helpers` for the parallel contract on sender_name and contact_email_address. COPILOT #2 — awkward help text "no-reply@ your site domain" Reads like a formatting mistake. Rewrote to natural prose: "Leave blank to use a no-reply address at your site's domain." Removes the literal `@ ` break that read as broken. COPILOT #3 — stale GET-route inline comment Said the handler "Reads through `Reader_Activation::get_setting()`", but the Option-A handler intentionally does NOT call get_setting() (that would collapse value+default into one resolved string, which is the conflation the modal's placeholder UX exists to avoid). Rewrote the comment to describe the actual behavior + explicitly forbid the regression: future refactors must preserve the value-vs-placeholder split. USER — description clamp truncating new emails at 2 lines PR #155's "Card expiry warning" and slice 2a's "Renewal reminder" have single-sentence descriptions that exceed the 2-line clamp at 12px in the narrow grid card width. The clamp was set in slice 1 when descriptions were shorter; new descriptions outgrew the budget. Bumped 2 → 3 lines: fits sentence-length descriptions cleanly while still bounding card height enough to preserve grid uniformity. Verification: PHPCS exit 0, ESLint exit 0. PHP 1343 → 1344 (+1 test / +2 assertions; one test renamed/rescoped + one new test). JS 217/217 unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Adds a Newspack-managed Card expiry warning email — sent ~14 days before a saved payment method expires on an active WooCommerce subscription, prompting the reader to update their card before renewal fails. Lives in the Reader revenue chip; requires WooCommerce Subscriptions.
Resolves NPPD-1524.
Stacking
main. Review order: #137 → #144 → #146 → this. Stacking on 2b means the new email is previewable from day one via 2b's preview infrastructure.What's in the slice
Backend:
Card_Expiry_Warningclass registers a new Newspack-managed Pattern A email vianewspack_email_configswithchip='reader-revenue',recipient='reader',recommended=true, and an explicittrigger_description.wp_schedule_eventcron scans active subscriptions for saved payment methods within N days of expiry (default 14, filterable vianewspack_card_expiry_warning_days).SENT_METAkeyed ontoken_id:expiry_month/year. A reader who updates their card clears the meta via thewoocommerce_subscription_payment_method_updatedhook, so the new card's eventual expiry re-triggers normally.includes/templates/reader-revenue-emails/card-expiry-warning.php, using the standard Newspack substitution tokens (*BILLING_FIRST_NAME*,*CARD_LAST_4*,*EXPIRY_DATE*,*RENEWAL_DATE*,*UPDATE_PAYMENT_URL*, etc.).Discoverability tweak: small one-line change to slice 2b's
Email_Preview::get_sample_substitutions()—*RENEWAL_DATE*sample shifts from+1 yearto+30 daysfor a more realistic card-expiry preview context. No 2b behavior change.Publisher-respect machinery
This slice introduces three mechanisms not present in legacy #4756, addressing concerns that would have shipped as destructive defaults.
1. First-deploy seed (prevents Day-1 burst)
The concern: without protection, the first cron run after deploy would discover every subscription with a card expiring in the next 14 days and send the warning email to all of them at once. On a site with 200 active subs and 30 in the window, that's a burst of 30 unexpected emails to readers before the publisher has seen the feature in their UI, reviewed the template, or had a chance to consent.
The mechanism: on the first scheduled scan after install (detected via an absent
SEEDED_OPTION), a seed pass runs — it marksSENT_METAon every in-window(subscription, token)pair without sending and sets the seeded option. Subsequent runs execute the normal scan logic. The result: existing in-window readers don't get a Day-1 surprise; new readers entering the window after deploy are warned normally.The seed event is logged at
warninglevel viaNewspack\Loggerso it's diagnostically discoverable if a publisher reports "I expected warnings to fire on day 1 but they didn't."Trade-off: the in-window readers at the moment of deploy don't get the warning for that card cycle. They do get warned normally for the next card they add (since the
payment_method_updatedhook clearsSENT_META). Publishers who explicitly want to send the deferred warnings have a WP-CLI backfill — see below.2. Per-pass SQL LIMIT (bounds memory and burst size)
The discovery query carries a
LIMIT %dplaceholder, defaulting to 100 and filterable vianewspack_card_expiry_warning_limit_per_pass. Bounded at query time (not after fetch), so even an extreme migration day can't load thousands of rows into memory. A site whose in-window count exceeds the cap on a given day sees remaining warnings on subsequent cron runs — sustained load is fine; this only affects bursts.3. WP-CLI backfill command
For publishers who do want to send the deferred warnings the seed pass suppressed, or for any one-off catch-up:
--dry-runprints what would be sent without sending. Output reads "Would send to X (sub #N, card …4242, expires 12/2026)".--limit=<n>caps sends per invocation (defaults uncapped since this is explicit publisher action).--days=<n>overrides the 14-day window (or whatevernewspack_card_expiry_warning_daysresolves to).--yesis passed.The command lives in
Newspack\CLI\WooCommerce_Subscriptionsalongside the existingmigrate-expired-subscriptionsbackfill (same flat-hyphenated naming convention). It invokesmaybe_send_warning( $sub, $token, $bypass_idempotency = true )— the bypass arg means the backfill sends through the seed's existingSENT_METAmarkers. The$bypass_idempotencyparameter defaults tofalse, so the cron path's behavior is unchanged; this is a locked-in invariant via a Reflection test.Implementation notes
maybe_send_warning()ispublicbecause the WP-CLI command in a separate class needs to call it. The docblock carries@internalto mark it as public-by-necessity-not-by-design — future maintainers should treat it as Newspack-internal API.get_in_window_pairs(int $days, int $limit): arrayis a public helper that's the single source of truth for the discovery logic. The cron scan, the seed pass, and the WP-CLI command all call it. Centralizing it means any future refinement to the query lands once.plugin_dependency: 'woocommerce-subscriptions'gates the email config to only show in the wizard when WC Subs is active.Testing
tests/unit-tests/card-expiry-warning.php): config shape and required fields,get_days_before_expiry()(default/filterable/min-clamp),get_limit_per_pass()(default/filterable/min-clamp), seed-then-scan branching (flag flips on first scan, holds on second, stays unset whencan_send_emailguards out — defense-in-depth), and a Reflection-based signature lock on$bypass_idempotency's default.tests/integration/card-expiry-warning-smoke.php): legacy 1-6 (cron schedule, happy-path, idempotency, flag clear, new-card resend, unattached no-op) pre-setSEEDED_OPTION='1'to exercise the post-seed normal-scan path; new 7-11 cover the first-deploy seed (marksSENT_METAwithout sending),$bypass_idempotency=trueescape hatch, SQL-LIMIT cap, and the CLI command's dry-run and normal-run behavior.Test docblocks reference the class's "Publisher-respect — first-deploy seed" and "Publisher-respect — per-pass SQL LIMIT" sections, so the design intent → docblock → test lock-in chain is explicit.