Skip to content

feat(emails-rr): card expiry warning email (NPPD-1524)#155

Open
kmwilkerson wants to merge 9 commits into
nppd-1525-slice-2b-email-previewfrom
nppd-1524-card-expiry-warning-newspack-managed
Open

feat(emails-rr): card expiry warning email (NPPD-1524)#155
kmwilkerson wants to merge 9 commits into
nppd-1525-slice-2b-email-previewfrom
nppd-1524-card-expiry-warning-newspack-managed

Conversation

@kmwilkerson
Copy link
Copy Markdown

@kmwilkerson kmwilkerson commented May 29, 2026

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.

image

Stacking

⚠️ This PR is stacked on slice 2b (#146) and targets its branch, not 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_Warning class registers a new Newspack-managed Pattern A email via newspack_email_configs with chip='reader-revenue', recipient='reader', recommended=true, and an explicit trigger_description.
  • Daily wp_schedule_event cron scans active subscriptions for saved payment methods within N days of expiry (default 14, filterable via newspack_card_expiry_warning_days).
  • Per-subscription idempotency via SENT_META keyed on token_id:expiry_month/year. A reader who updates their card clears the meta via the woocommerce_subscription_payment_method_updated hook, so the new card's eventual expiry re-triggers normally.
  • Block-template email at 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.).
  • Group subscriptions: warning recipient resolves to the customer on the subscription (the group sub owner for institutional access).

Discoverability tweak: small one-line change to slice 2b's Email_Preview::get_sample_substitutions()*RENEWAL_DATE* sample shifts from +1 year to +30 days for 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 marks SENT_META on 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 warning level via Newspack\Logger so 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_updated hook clears SENT_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 %d placeholder, defaulting to 100 and filterable via newspack_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:

wp newspack card-expiry-warning-backfill [--dry-run] [--limit=] [--days=] [--yes]
  • --dry-run prints 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 whatever newspack_card_expiry_warning_days resolves to).
  • Confirmation prompt before sending unless --yes is passed.

The command lives in Newspack\CLI\WooCommerce_Subscriptions alongside the existing migrate-expired-subscriptions backfill (same flat-hyphenated naming convention). It invokes maybe_send_warning( $sub, $token, $bypass_idempotency = true ) — the bypass arg means the backfill sends through the seed's existing SENT_META markers. The $bypass_idempotency parameter defaults to false, so the cron path's behavior is unchanged; this is a locked-in invariant via a Reflection test.

Implementation notes

  • maybe_send_warning() is public because the WP-CLI command in a separate class needs to call it. The docblock carries @internal to 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): array is 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

  • Unit tests (20 tests, 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 when can_send_email guards out — defense-in-depth), and a Reflection-based signature lock on $bypass_idempotency's default.
  • Integration smoke (12 scenarios, tests/integration/card-expiry-warning-smoke.php): legacy 1-6 (cron schedule, happy-path, idempotency, flag clear, new-card resend, unattached no-op) pre-set SEEDED_OPTION='1' to exercise the post-seed normal-scan path; new 7-11 cover the first-deploy seed (marks SENT_META without sending), $bypass_idempotency=true escape hatch, SQL-LIMIT cap, and the CLI command's dry-run and normal-run behavior.
  • Full PHP suite: 1350 tests / 0 failures.

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.

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>
@kmwilkerson kmwilkerson force-pushed the nppd-1525-slice-2b-email-preview branch from f62acdb to 67f0e73 Compare May 29, 2026 04:25
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 kmwilkerson force-pushed the nppd-1524-card-expiry-warning-newspack-managed branch from 5af1bd6 to d0d16ef Compare May 29, 2026 04:28
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>
@kmwilkerson kmwilkerson marked this pull request as ready for review May 29, 2026 13:05
Copilot AI review requested due to automatic review settings May 29, 2026 13:05
@kmwilkerson kmwilkerson requested a review from a team as a code owner May 29, 2026 13:05
@kmwilkerson kmwilkerson marked this pull request as draft May 29, 2026 13:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_Warning integration, 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 kmwilkerson requested a review from Copilot May 29, 2026 17:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment thread plugins/newspack-plugin/includes/cli/class-woocommerce-subscriptions.php Outdated
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>
kmwilkerson and others added 9 commits May 29, 2026 12:49
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>
@kmwilkerson kmwilkerson force-pushed the nppd-1525-slice-2b-email-preview branch from 67f0e73 to 35d4c27 Compare May 29, 2026 17:49
@kmwilkerson kmwilkerson force-pushed the nppd-1524-card-expiry-warning-newspack-managed branch from a345109 to 7803892 Compare May 29, 2026 17:49
@kmwilkerson kmwilkerson marked this pull request as ready for review May 29, 2026 17:51
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants