feat(emails): allow test-sends for inactive Newspack-managed emails (NPPD-1547)#187
Open
kmwilkerson wants to merge 4 commits into
Open
feat(emails): allow test-sends for inactive Newspack-managed emails (NPPD-1547)#187kmwilkerson wants to merge 4 commits into
kmwilkerson wants to merge 4 commits into
Conversation
A publisher couldn't test-send a Newspack-managed transactional email when the email was in draft/inactive status. The cause was an incidental conflation: `Emails::send_email()` AND'd a `'publish' === post_status` guard that was designed for the auto-send code path (event-triggered RAS verification, payment receipts, etc.) but also blocked the test-send REST handler that went through the same function. Git blame on the guard (c121e5c3ce, d2937017d1) confirms no test-send-specific design intent — the gate was shared incidentally. Recon also confirmed the generous-scope fix is safe: NO Newspack-managed email config registers its own per-email test-send guard. All eight reader-activation emails + the three reader-revenue emails + group-subscription-invite + card-expiry- warning route through `Emails::send_email()` with no custom gating. WC-managed emails have a separate test-send path entirely (via WC's settings UI) — unaffected by this change. REFACTOR — semantic split - Extracted `validate_send_prerequisites( $post_id, $to )`: shared post-id-path prereqs (Newspack_Newsletters active, recipient non-empty, post resolves, HTML payload saved via EMAIL_HTML_META). Returns the resolved config + config name, or a WP_Error identifying which prereq failed. Future shared guards (rate limiting, etc.) land here. - Extracted `dispatch_email( $email_config, $config_name, $to, $placeholders )`: shared wp_mail() dispatch — locale switching, header construction, payload substitution, logging. Both send_email() and send_test_email() route through it so the wp_mail surface stays identical between auto-send and test-send. - Refactored `send_email()`: string path keeps its inline checks (used by every production auto-trigger); post-id branch now routes through validate_send_prerequisites + adds its own status check on top + dispatches via dispatch_email. - Added `send_test_email( $post_id, $to )`: new public entry point. Calls validate_send_prerequisites, intentionally skips the status check, dispatches via dispatch_email. Returns true|WP_Error so callers can surface specific failure causes. - Refactored `api_send_test_email()`: calls send_test_email instead of send_email. Returns the WP_Error from send_test_email directly when prereqs fail — replaces the previous generic "Test email was not sent." message with specific error codes (newspack_emails_unsupported, newspack_emails_empty_recipient, newspack_emails_post_not_resolvable, newspack_test_email_dispatch_failed). RECIPIENT VALIDATION Added `is_email( $recipient )` validation at api_send_test_email() before entering the dispatch path. Returns 400 + newspack_invalid_test_recipient on failure. Same anti-pattern as NPPD-1566's sanitize_email-vs-is_email validation gap: the args' sanitize_text_field callback strips tags but doesn't validate email format, so a typo like 'not-an-email' would silently reach wp_mail() and fail with no clue why. Cheap to add while we're here. OBSERVABLE BEHAVIOR - Auto-send (every production caller): unchanged. send_email() with a string config name still requires post_status='publish'. - Test-send (admin Send button on the email-edit screen): now works for inactive emails. Returns specific WP_Error codes on prereq failures. - Existing 4 emails tests still pass — the test_emails_status test that asserts auto-send blocks drafts is unchanged in behavior (it goes through send_email's post-id branch which retains the status check via the helper-then-status flow). DEVIATION NOTE User offered a fallback to the bool-parameter approach if the helper-extraction proved harder than expected. The extraction went cleanly because the post-id branch of send_email is exclusively used by the test-send path (only one production non-test caller passes a post_id — and that's api_send_test_email itself). Helper approach shipped as planned. Tests for the new behavior land in commit 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrites `test_emails_status` to lock the auto-send-vs-test-send
distinction explicitly (both paths asserted in one test). Adds 8
new tests covering the test-send entry point, recipient
validation, prerequisite failures, permission gating, and a
regression lock against accidentally relaxing the auto-send
status gate during future refactors.
NEW TESTS
1. test_test_send_works_for_draft — headline contract. Sets the
email post to draft, calls send_test_email, asserts true return.
Verifies the mailer received the recipient (guards against
short-circuit before dispatch_email runs).
2. test_test_send_rejects_non_email_recipient — typo recipient
('not-an-email') routed through api_send_test_email returns
400 + newspack_invalid_test_recipient, NOT silently reaching
wp_mail and failing with the generic dispatch error. Locks
the is_email() validation added in commit 1.
3. test_test_send_blocked_when_no_newspack_newsletters — SKIPPED
with explicit reason. supports_emails() uses class_exists(),
which can't be made false in the test bootstrap without
runkit/uopz or a production-code change (adding a filter to
supports_emails) — coverage gap documented rather than
silently omitted.
4. test_test_send_blocked_when_missing_html_payload — fresh
throwaway post (not the shared test-email-config — avoids
leaking the missing-meta state into later tests in the same
class, since this file deliberately doesn't call
parent::set_up). Asserts WP_Error newspack_emails_post_not_resolvable
with 404 status.
5. test_test_send_blocked_when_recipient_empty — empty recipient
passed directly to send_test_email surfaces
newspack_emails_empty_recipient with 400. Tests the helper's
guard at a level below the api handler's is_email() check.
6. test_test_send_blocked_when_non_admin — subscriber user hits
api_permissions_check, asserts 403 + newspack_rest_forbidden.
Uses wp_insert_user directly (not $this->factory) because
this class deliberately doesn't call parent::set_up() — the
factory dependency would force the parent call, which breaks
the existing test_emails_send_by_id test (empirically
verified).
7. test_test_send_blocked_when_post_missing — post_id=9999999
surfaces newspack_emails_post_not_resolvable with 404. Same
error code as missing-HTML-payload but distinct test name
communicates the scenario.
8. test_auto_send_still_blocked_for_draft — REGRESSION LOCK.
Sets the post to draft, calls send_email() via BOTH the
string-name branch AND the post-id branch, asserts both
return false. Without this lock, a future refactor that
accidentally routes the post-id branch through
send_test_email's status-less path would silently start
dispatching draft emails on triggered events — a much worse
failure mode than the original bug (silent over-firing vs.
silent under-firing).
REWRITTEN test_emails_status
The original test asserted only that can_send_email() and
send_email() return false for draft. The rewrite keeps those
assertions AND adds the inverse for the test-send path:
send_test_email() returns true for the same draft post. Both
contracts in one test makes the distinction explicit — a future
refactor that re-conflates the two paths fails both assertions
simultaneously, producing a clear "the split has been re-broken"
signal rather than mysterious behavior changes.
VERIFICATION
- PHPCS exit 0 (full output verified)
- Emails class tests in isolation: 12 / 34 / 1 skipped
- Full PHP suite: 1278 / 3850 / 2 skipped — delta from this
commit: +8 tests / +20 assertions / +1 skipped (the
intentional one). No other test counts shifted; no WC shim
interactions introduced.
NOTES ON STATE ISOLATION
This test class deliberately doesn't call parent::set_up() —
empirically, adding parent::set_up breaks test_emails_send_by_id
(the parent's transaction lifecycle interacts with the file's
state-mutation expectations). Without the parent's transaction
rollback, DB changes survive across tests in this file. The new
tests avoid leaking state by:
- test #4 (missing-html-payload): uses a fresh throwaway
post + wp_delete_post in the same test
- test #6 (non-admin): uses wp_insert_user + wp_delete_user
- other new tests: read-only or use ephemeral post IDs
Filed forward as a potential cleanup follow-up: investigate why
parent::set_up breaks the existing test, and migrate to standard
WP_UnitTestCase transaction isolation if feasible.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses 13 of 15 findings from the code review on PR #187. Two deferred as follow-ups (#1 dispatch_email re-fetching via get_email_payload — pre-existing, touches auto-send hot path; #13 string-branch bypasses helper — larger refactor, helper is post-id-only by design). CORRECTNESS - **Empty EMAIL_CONFIG_NAME_META blank-body** (#2). Without a guard, send_test_email on a draft with HTML payload but no config-name meta dispatched a blank-bodied email via get_email_payload('') → get_email_config_by_type('') returning false → array-access on false. Added newspack_emails_config_name_missing (422) guard in validate_send_prerequisites. Reachable post-NPPD-1547 because the publish gate no longer blocks drafts with incomplete meta. - **Frontend swallows new error codes** (#3). The sendTestEmail handler in src/other-scripts/emails/index.js previously caught ALL errors as 'Test email was not sent.' — collapsing every specific WP_Error the PR added back to a single generic string. Now surfaces error.message when present, falling back to the generic only for unstructured errors (network/CORS failures). - **send_test_email public + no permission check** (#4). Defense- in-depth: added current_user_can( 'manage_options' ) at the entry point. Only safe before because the sole caller was behind the REST permission_callback; future internal callers (CLI commands, plugin hooks) would have bypassed. New test test_test_send_blocked_when_non_admin_via_direct_php locks the contract. - **is_email() validation moved into helper** (#5). Previously lived at api_send_test_email — direct PHP callers of send_test_email skipped the format check. Now in validate_send_prerequisites alongside the empty-recipient check; REST and direct entry surface identical error codes for identical inputs. BEHAVIORAL ALIGNMENT (preserve pre-PR semantics) - **RAS-ACC migration moved to a helper** (#6) called by both send_email AND send_test_email. Pre-PR test-send went through send_email and triggered the migration; the original NPPD-1547 refactor dropped that side effect for test-send. Now reinstated. - **switch_to_locale moved back around the FULL send operation** (#7), not just dispatch_email. Pre-PR the locale switch wrapped config resolution, so get_email_config_by_type's lazy-create path used the admin's locale for load_email_template's __() calls. The original NPPD-1547 refactor put the switch inside dispatch_email only, meaning first-time post creation persisted HTML in site default locale instead. Wrapped both send_email and send_test_email with try/finally for locale restore. - **Trash exclusion** (#8) in send_test_email. The publish-gate skip was for the "draft / inactive" case; trashed posts are intentionally removed and shouldn't be sendable. New code newspack_emails_post_trashed (409) + test test_test_send_blocked_for_trashed_post. TEST INFRASTRUCTURE - **Test state restoration** (#9). Every test that mutates the shared test-email-config post now uses try/finally to restore the original status. Previously these tests left the post in draft, silently failing any future test that depends on 'publish' status. - Added login_as_admin() / logout_admin() helpers — every test that calls send_test_email now logs in as admin (required by the new entry-point cap check) and restores the previously- current user in a finally block. ERROR CODE SPLITS + NAMING - **Split conflated error codes** (#10). newspack_emails_post_not_resolvable (404 for all three of post-id-shape, post-missing, HTML-missing) is now three distinct codes: - newspack_emails_invalid_post_id (400) — post_id is 0, negative, or non-numeric (the absint(missing-param) case) - newspack_emails_post_missing (404) — post_id is valid but no post exists - newspack_emails_html_payload_missing (422) — post exists but EMAIL_HTML_META is missing/empty This lets the UI route by failure mode ("save your draft first" vs "the post is gone" vs "fix the form"). - **Validation order reordered** (#11). Cheapest/most-common failure modes first: post_id shape → recipient empty → recipient format → infrastructure prereq → post lookup → HTML payload → config name. - **Eliminated duplicate empty-recipient codes** (#12). The old api handler's newspack_invalid_test_recipient + helper's newspack_emails_empty_recipient surfaced two codes for the same state depending on entry point. Now a single code path through the helper: empty → newspack_emails_empty_recipient (400); invalid format → newspack_emails_invalid_recipient (400). - **Naming consistency** (#14). All emails-related error codes now use the newspack_emails_* prefix uniformly. Consumers can group/filter on the namespace without knowing both old conventions. Renamed: - newspack_invalid_test_recipient → newspack_emails_invalid_recipient - newspack_test_email_dispatch_failed → newspack_emails_test_dispatch_failed - **supports_emails() returns 412 instead of 500**. Configuration prereq not satisfied is not a server fault — 412 (Precondition Failed) more accurately reflects the state and reduces noise in uptime monitoring. LOGGER FIDELITY - **Logger::log fires conditionally** on dispatch outcome (#15). Pre-fix: "Sending X to Y" fired unconditionally after wp_mail regardless of return value, producing misleading success-shaped log lines for actual failures. Post-fix: separate "Sent" / "Failed to send" lines that match the WP_Error contract callers observe. TEST DELTA PHP suite: 1278 → 1283 tests / 3850 → 3865 assertions / 2 → 2 skipped. +5 tests / +15 assertions / 0 skipped change. No regressions in non-emails tests. JS suite: 185/185 unchanged (no JS test coverage for the sidebar handler exists; the frontend update is in plumbing code that has no test file). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR separates “auto-send” (event-triggered) from “test-send” (admin-initiated) in the Newspack emails system so admins can send test emails even when the underlying email post is inactive (e.g., draft), while preserving the publish-status gate for automated sends.
Changes:
- Refactors
Emails::send_email()and introducesEmails::send_test_email()plus shared helpers for prerequisite validation and mail dispatch. - Updates the editor sidebar test-send UX to display specific server error messages when available.
- Adds/expands PHPUnit coverage to lock the auto-send vs test-send contract and validate new error paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
plugins/newspack-plugin/includes/emails/class-emails.php |
Adds send_test_email() and shared helpers; refactors send_email() to keep auto-send publish gating while enabling draft test sends. |
plugins/newspack-plugin/src/other-scripts/emails/index.js |
Improves error reporting for test-send failures by surfacing server-provided messages. |
plugins/newspack-plugin/tests/unit-tests/emails.php |
Adds regression/contract tests for draft test sends, recipient validation, capability checks, and failure-mode error codes. |
Three findings from Copilot's review of PR #187, all confirmed and fixed. COPILOT #1 — dispatch_email's wp_mail filter not in try/finally `add_filter( 'wp_mail_content_type', ... )` was paired with a bare `remove_filter()` AFTER the wp_mail() call. If wp_mail() (or any filter it triggers) threw, the html-content-type filter would persist for every subsequent wp_mail() call in the same request — silently converting plain-text mails (password resets, WP core notifications, etc.) to html. Wrapped the wp_mail() invocation in try/finally so remove_filter() always runs. COPILOT #2 — is_numeric() too loose for post_id validation validate_send_prerequisites used is_numeric() which accepts: - floats: '1.5' (int)('1.5') = 1 - scientific: '1e3' (int)('1e3') = 1000 - exponents: '1.7e308' (int)((float)'1.7e308') = PHP_INT_MAX Any of these would slip through the validate gate and silently resolve the wrong post (or trigger PHP_INT_MAX-shaped overflow on the subsequent query). Tightened the integer-shape check: is_int( $post_id ) || ( is_string( $post_id ) && '' !== $post_id && ctype_digit( $post_id ) ) ctype_digit accepts only digit-only strings (no '.', no 'e', no sign), then the value is cast to int once and the rest of the function operates on a well-typed positive integer. COPILOT #3 — gettype()-based branching can't see numeric strings send_email's `'string' === gettype($config_name)` vs `'integer' === gettype($config_name)` branching silently routed digit-only strings to the string-name branch (which then failed because get_email_config_by_type looks up type names, not post IDs). Pre-existing — production callers all pass either string type names or absint'd integers — but a future caller passing a numeric string from $_POST, post meta, or an untyped integration would silently fail. Normalized digit-only strings to int at the top of send_email so the post-id branch is reached correctly. NEW TESTS - test_test_send_rejects_non_integer_shaped_post_id loops nine values that is_numeric accepts but ctype_digit rejects ('1.5', '1e3', '1.7e308', '-5', 'abc', '12abc', ' 5 ', '', null) and asserts each is rejected with newspack_emails_invalid_post_id. - test_send_test_email_accepts_numeric_string_post_id locks in the hardening for the digit-only-string-survives-and-routes- correctly contract. Test delta: +2 tests / +19 assertions. PHP suite 1283 → 1285 / 3865 → 3884 / 2 skipped (unchanged). PHPCS exit 0. 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
Allows publishers to send test emails for Newspack-managed emails that are currently inactive (draft post status). Currently, all test sends through the Newspack email sidebar fail silently when the underlying post isn't published, even though the publisher's intent on clicking "Send" is clearly a one-off authenticated test — not a request to fire automated event-driven sends.
Resolves NPPD-1547.
The conceptual fix
Today,
Emails::send_email()gates on'publish' !== $email_config['status']to prevent automated sends from firing during an email's draft/editing state. That's a legitimate guard for auto-triggered sends (the receipt cron, the card-expiry scan, etc.) — publishers shouldn't have draft emails accidentally go out to real readers.But the same guard also blocks test sends initiated by an authenticated admin from the email's edit screen. Those are two conceptually different operations:
Conflating them under a shared
send_email()guard was an implementation accident, not a deliberate design choice. Verified viagit blameon the gate-checking lines — neither originating commit (PRs #1929, #1938 from August/September 2022) mentions test sends, drafts, or activation-as-test-prerequisite in the commit message or PR discussion. The gate is shared because both paths happened to share a method, not because the design required it.Implementation
Emails::validate_send_prerequisites( $post_id, $to )helper — new private static. Centralizes the prerequisites both auto-send and test-send need to satisfy:supports_emails()(Newspack Newsletters active), recipient non-empty, post exists withEMAIL_HTML_METApopulated. Returns either[ $email_config, $config_name ]or aWP_Errorwith a specific error code per failure mode.Emails::dispatch_email( $email_config, $config_name, $to, $placeholders )helper — new private static. Centralizes thewp_mailhandoff: locale switching, header construction, payload, logging. Both auto-send and test-send route through it so the wire-level send surface stays identical.Emails::send_email()— refactored to use both helpers. The status check stays in the auto-send path; behavior is observably unchanged.Emails::send_test_email( $post_id, $to )— new public method. Callsvalidate_send_prerequisites, intentionally skips the status check, dispatches viadispatch_email. ReturnstrueorWP_Error.Emails::api_send_test_email()— updated to callsend_test_emailand surface its specific WP_Error codes. Previously returned a generic "Test email was not sent." message; now returns actionable errors (newspack_emails_unsupported,newspack_emails_empty_recipient,newspack_emails_post_not_resolvable,newspack_test_email_dispatch_failed).Also adds
is_email()validation on the recipient at the handler (returns 400 +newspack_invalid_test_recipienton typo input). Same anti-pattern as NPPD-1566 —sanitize_text_fieldalone letsnot-an-emailthrough to a silentwp_mailfailure. Cheap fix while touching the file.Scope
The fix applies to all Newspack-managed emails, not just the Newspack Receipt referenced in the ticket title. Verified during recon that:
Emails::and it correctly applies uniformly.wp-admin/admin.php?page=wc-settings&tab=emailthat doesn't go throughEmails::send_email(). Out of scope for this PR.The ticket's "Newspack Receipt" framing was descriptive of the symptom reported, not prescriptive about scope. Fixing this Receipt-only would have left the same footgun on every other Newspack-managed email type.
Guards preserved (NOT bypassed for test sends)
The fix bypasses only the publish-status gate. All other guards remain in effect for test sends:
current_user_can( 'manage_options' )permission checkNewspack_Newslettersplugin must be active and support emailsEMAIL_HTML_META(an unsaved template has nothing to send)dispatch_emailwp_mail call still subject to the site's normal mail-send infrastructureTesting
8 new tests (
tests/unit-tests/emails.php):test_test_send_works_for_draft— headline contract: draft email + non-empty HTML payload + valid recipient → successtest_test_send_rejects_non_email_recipient— typo recipient → 400 +newspack_invalid_test_recipienttest_test_send_blocked_when_no_newspack_newsletters— skipped, see belowtest_test_send_blocked_when_missing_html_payload— emptyEMAIL_HTML_META→newspack_emails_post_not_resolvabletest_test_send_blocked_when_recipient_empty→newspack_emails_empty_recipienttest_test_send_blocked_when_non_admin— subscriber-level user via the REST endpoint → 403test_test_send_blocked_when_post_missing— bogus post_id →newspack_emails_post_not_resolvabletest_auto_send_still_blocked_for_draft— architectural-lock-in test. Asserts the auto-send path (send_emailwith both string and post-id call shapes) still returns false for draft emails, preventing the refactor from accidentally relaxing the auto-send guard.Existing
test_emails_statusrewritten to assert both contracts in one test:send_email()returns false for drafts (auto-send still gated) ANDsend_test_email()returns true for drafts (new test-send contract). Failure of either assertion signals the split has been re-broken.PHP suite delta: +8 tests / +20 assertions / +1 skipped. No other test counts shifted.
Test infrastructure notes flagged
Two pre-existing concerns surfaced during test development. Both filed as follow-up tickets, neither blocks this PR.
supports_emails()guard not mockable.test_test_send_blocked_when_no_newspack_newslettersis skipped becauseclass_exists( 'Newspack_Newsletters' )can't be falsified withoutrunkit/uopzor a production-side filter. We're accepting reduced coverage on this specific guard, relying on code review to catch regressions. A future cleanup could add a filter or wrapper to make the check mockable.parent::set_up()breaks an existing test in this file. The new tests sidestep this by using throwaway posts and direct user CRUD instead of the shared test-email-config post for mutating operations. The underlying issue — this test class isn't usingWP_UnitTestCase's transaction-per-test isolation — is a fragility that any future test added to this file inherits. Worth investigating and migrating to standard isolation.Things deliberately out of scope
newspack_test_email_dispatch_failedcatch-all for wp_mail failures is still moderately generic — worth a future ticket to surface the underlying mailer reason where available.api_send_test_emailcurrently doesn't pass a$placeholdersarray, so tokens like*AMOUNT*render literally in test emails. Pre-existing behavior, but the publisher's test result doesn't reflect what readers will see.