diff --git a/plugins/newspack-plugin/includes/emails/class-emails.php b/plugins/newspack-plugin/includes/emails/class-emails.php index 217a29bf50..62ab51f3e4 100644 --- a/plugins/newspack-plugin/includes/emails/class-emails.php +++ b/plugins/newspack-plugin/includes/emails/class-emails.php @@ -251,57 +251,367 @@ public static function get_email_payload( $config_name, $placeholders = [] ) { /** * Send an HTML email. * - * @param string $config_name Email config name. - * @param string $to Recipient's email addesss. - * @param array $placeholders Dynamic content substitutions. + * AUTO-SEND path — triggered by site events (registration, payment, + * subscription renewal, etc.). Requires the email's underlying + * post to be in `publish` status: an inactive/draft email must + * not fire on triggered events. For the deliberate admin + * preview operation, see {@see self::send_test_email()} — + * NPPD-1547 split the two entry points because the shared + * `'publish' === status` guard incidentally blocked test-send + * for inactive emails, conflating auto-trigger gating with + * admin-deliberate-action semantics. + * + * @param string|int $config_name Email config name (string), or + * Email post ID (int) for the + * post-id path shared with + * send_test_email. + * @param string $to Recipient's email address. + * @param array $placeholders Dynamic content substitutions. + * @return bool wp_mail() result, or false if any prerequisite failed. */ public static function send_email( $config_name, $to, $placeholders = [] ) { if ( ! self::supports_emails() ) { return false; } - // Migrate to RAS-ACC email templates if migration option is not set AND there have been no manual updates to the templates. - if ( get_option( 'newspack_email_templates_migrated', '' ) !== 'v1' ) { - $migrated = true; - $templates = get_posts( - [ - 'post_type' => self::POST_TYPE, - 'posts_per_page' => -1, - 'post_status' => 'publish', - ] - ); + // Normalize numeric-string post IDs to int BEFORE the + // gettype()-based branching below. Without this, a caller + // passing a digit-only string (e.g. from $_POST, post meta, + // or a cast-naive integration) would be routed to the + // string-name branch and silently fail because + // get_email_config_by_type() looks up type names, not IDs. + // Only digit-only strings are normalized — sender-name-style + // strings stay strings. + if ( is_string( $config_name ) && '' !== $config_name && ctype_digit( $config_name ) ) { + $config_name = (int) $config_name; + } - foreach ( $templates as $template ) { - $publish_date = get_the_date( 'Y-m-d H:i:s', $template->ID ); - $last_modified_date = get_the_modified_date( 'Y-m-d H:i:s', $template->ID ); - - // Template has not been modified, so trash the post so we can trigger a template update. - if ( $publish_date === $last_modified_date ) { - if ( ! wp_trash_post( $template->ID ) ) { - // Flag the migration as failed so we can trigger another attempt later. - $migrated = false; - } + self::maybe_run_ras_acc_template_migration(); + + // Switch locale around the FULL send operation, not just + // dispatch. get_email_config_by_type's lazy-create path + // requires a template PHP file with __() calls; the locale + // active at that moment determines what gets persisted to + // the email post. Restoring symmetry via try/finally + // guarantees the locale is restored even if a wp_mail filter + // throws. + $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); + try { + if ( 'string' === gettype( $config_name ) ) { + // String path: looked up by type name. Used by every + // production auto-trigger (RAS verification, magic + // links, payment receipts, etc.). No HTML-payload check + // — the string path uses template files, not stored + // post content. + $email_config = self::get_email_config_by_type( $config_name ); + if ( ! $to || ! $email_config || 'publish' !== $email_config['status'] ) { + return false; + } + } elseif ( 'integer' === gettype( $config_name ) ) { + // Post-id path: shared with send_test_email() via + // validate_send_prerequisites(). Auto-send additionally + // requires 'publish' status; test-send does not. + $resolved = self::validate_send_prerequisites( $config_name, $to ); + if ( is_wp_error( $resolved ) ) { + return false; } + if ( 'publish' !== $resolved['config']['status'] ) { + return false; + } + $email_config = $resolved['config']; + $config_name = $resolved['name']; + } else { + return false; } - if ( $migrated ) { - update_option( 'newspack_email_templates_migrated', 'v1' ); + return self::dispatch_email( $email_config, $config_name, $to, $placeholders ); + } finally { + if ( $switched_locale ) { + \restore_previous_locale(); + } + } + } + + /** + * Run the v1 RAS-ACC email-templates migration once per request if + * it hasn't already completed. Extracted from `send_email()` so + * `send_test_email()` runs the same migration — both entry points + * are reachable from a fresh-install admin's first interaction + * with the email system, and the migration is harmless to run on + * any path (it's option-gated and idempotent). + * + * Trashing un-modified templates forces them to be regenerated + * from the current source on the next read, picking up RAS-ACC + * template updates without overwriting publisher edits. + */ + private static function maybe_run_ras_acc_template_migration() { + if ( 'v1' === get_option( 'newspack_email_templates_migrated', '' ) ) { + return; + } + $migrated = true; + $templates = get_posts( + [ + 'post_type' => self::POST_TYPE, + 'posts_per_page' => -1, + 'post_status' => 'publish', + ] + ); + + foreach ( $templates as $template ) { + $publish_date = get_the_date( 'Y-m-d H:i:s', $template->ID ); + $last_modified_date = get_the_modified_date( 'Y-m-d H:i:s', $template->ID ); + + // Template has not been modified, so trash the post so we can trigger a template update. + if ( $publish_date === $last_modified_date ) { + if ( ! wp_trash_post( $template->ID ) ) { + // Flag the migration as failed so we can trigger another attempt later. + $migrated = false; + } } } + if ( $migrated ) { + update_option( 'newspack_email_templates_migrated', 'v1' ); + } + } + + /** + * Send a test email for the given post. + * + * TEST-SEND path — the deliberate admin operation of previewing + * an email's actual rendered output. Distinct from the auto-send + * path ({@see self::send_email()}) which fires on triggered + * events and requires the email to be in `publish` status. + * + * NPPD-1547: before this split, test-send went through send_email() + * and inherited its `'publish' === status` guard, blocking + * test-send for inactive emails. That conflation was incidental + * — git blame shows the guard was introduced for the auto-send + * path with no test-send-specific design intent. Splitting the + * entry points makes the distinction explicit and means the + * test-send code path skips ONLY the draft/publish gate, + * retaining every other prerequisite plus an explicit trash + * exclusion (a trashed post is intentionally removed, not + * "inactive"). + * + * Includes its own `current_user_can( 'manage_options' )` check + * even though the only current caller (api_send_test_email) is + * already behind a REST permission_callback. send_test_email is + * `public static` and the surface invites future internal + * callers; locking the cap check inside the entry point + * guarantees no path bypasses it. + * + * @param int $post_id Email post id. + * @param string $to Recipient email address. + * @return true|WP_Error True on successful dispatch; WP_Error + * with appropriate status code on failure. + */ + public static function send_test_email( $post_id, $to ) { + if ( ! current_user_can( 'manage_options' ) ) { + return new \WP_Error( + 'newspack_emails_forbidden', + esc_html__( 'You cannot send test emails.', 'newspack-plugin' ), + [ 'status' => 403 ] + ); + } + + self::maybe_run_ras_acc_template_migration(); + $switched_locale = \switch_to_locale( \get_user_locale( \wp_get_current_user() ) ); + try { + $resolved = self::validate_send_prerequisites( $post_id, $to ); + if ( is_wp_error( $resolved ) ) { + return $resolved; + } - if ( 'string' === gettype( $config_name ) ) { - $email_config = self::get_email_config_by_type( $config_name ); - } elseif ( 'integer' === gettype( $config_name ) ) { - $email_config = self::serialize_email( null, $config_name ); - $config_name = \get_post_meta( $config_name, self::EMAIL_CONFIG_NAME_META, true ); - } else { - return false; + // Trash exclusion. The status-gate skip is for draft/pending + // (the "inactive but still being edited" case); a trashed + // post is intentionally removed from publication and + // shouldn't be sendable even by an admin's deliberate + // action. Restoring from trash is a one-click admin + // operation if the publisher really wants to test-send + // a trashed post. + if ( 'trash' === ( $resolved['config']['status'] ?? '' ) ) { + return new \WP_Error( + 'newspack_emails_post_trashed', + esc_html__( 'Cannot test-send a trashed email. Restore the email first.', 'newspack-plugin' ), + [ 'status' => 409 ] + ); + } + + $sent = self::dispatch_email( $resolved['config'], $resolved['name'], $to, [] ); + if ( ! $sent ) { + return new \WP_Error( + 'newspack_emails_test_dispatch_failed', + esc_html__( 'Test email could not be dispatched. Check your mail configuration.', 'newspack-plugin' ), + [ 'status' => 500 ] + ); + } + return true; + } finally { + if ( $switched_locale ) { + \restore_previous_locale(); + } } - if ( ! $to || ! $email_config || 'publish' !== $email_config['status'] ) { - return false; + } + + /** + * Validate the shared prerequisites for sending an email via the + * post-id path. Used by both send_email()'s post-id branch and + * send_test_email() so future shared guards (rate limiting, etc.) + * land in one place. + * + * Checks (in order, cheapest first): + * - $post_id is a positive integer + * - $to is non-empty AND a valid email per is_email() + * - Newspack Newsletters plugin is active + * - Post resolves to a config with HTML payload + * - Resolved config_name (EMAIL_CONFIG_NAME_META) is non-empty + * + * DOES NOT check post status — the caller layers its own status + * check on top when appropriate. send_email() requires + * `'publish'`; send_test_email() excludes only `'trash'`. + * + * Error codes use the `newspack_emails_*` prefix consistently so + * downstream consumers can group/filter on the namespace. + * + * @param int $post_id Email post id. + * @param string $to Recipient email address. + * @return array|WP_Error On success, [ 'config' => array, + * 'name' => string ]. On failure, WP_Error + * with `status` data field set to an + * appropriate HTTP code. + */ + private static function validate_send_prerequisites( $post_id, $to ) { + // Caller-input checks first (cheap, common failure modes). + // is_numeric() is too loose — accepts '1.5', '1e3', '1.7e308' + // which then (int)-cast to 1, 1000, or PHP_INT_MAX silently, + // potentially resolving the wrong post. Require an actual + // integer-shaped value (int type OR a digit-only string), + // then normalize to int so downstream calls operate on a + // well-typed positive integer. + $is_int_shaped = is_int( $post_id ) + || ( is_string( $post_id ) && '' !== $post_id && ctype_digit( $post_id ) ); + if ( ! $is_int_shaped || (int) $post_id <= 0 ) { + return new \WP_Error( + 'newspack_emails_invalid_post_id', + esc_html__( 'A valid email post ID is required.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); } + $post_id = (int) $post_id; + if ( empty( $to ) ) { + return new \WP_Error( + 'newspack_emails_empty_recipient', + esc_html__( 'A recipient is required.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); + } + if ( ! is_email( $to ) ) { + return new \WP_Error( + 'newspack_emails_invalid_recipient', + esc_html__( 'Recipient must be a valid email address.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); + } + // Infrastructure prereq: Newspack Newsletters provides the + // email-editor post type + meta keys. 412 (Precondition + // Failed) more accurately reflects "configuration prereq + // not satisfied" than 500 (server fault) — monitoring that + // pages on 5xx no longer fires for known-config states. + if ( ! self::supports_emails() ) { + return new \WP_Error( + 'newspack_emails_unsupported', + esc_html__( 'Email sending requires the Newspack Newsletters plugin to be active.', 'newspack-plugin' ), + [ 'status' => 412 ] + ); + } + // `serialize_email( null, $post_id )` returns false when the + // post doesn't exist OR when its EMAIL_HTML_META is missing. + // Distinguish via get_post() so the error codes (and HTTP + // status) point the publisher at the actionable cause. + if ( ! get_post( $post_id ) ) { + return new \WP_Error( + 'newspack_emails_post_missing', + esc_html__( 'Email post does not exist.', 'newspack-plugin' ), + [ 'status' => 404 ] + ); + } + // Require the Newspack email CPT explicitly. serialize_email() keys + // off post meta (EMAIL_CONFIG_NAME_META / EMAIL_HTML_META) that a + // non-email post could also carry, so gate on post_type — otherwise + // a matching-meta post of another type could reach the test-send + // dispatch path. + if ( self::POST_TYPE !== get_post_type( $post_id ) ) { + return new \WP_Error( + 'newspack_emails_wrong_post_type', + esc_html__( 'The provided post is not a Newspack email.', 'newspack-plugin' ), + [ 'status' => 400 ] + ); + } + $email_config = self::serialize_email( null, $post_id ); + if ( ! $email_config ) { + return new \WP_Error( + 'newspack_emails_html_payload_missing', + esc_html__( 'Email has no saved content. Open the email in the editor and save before sending.', 'newspack-plugin' ), + [ 'status' => 422 ] + ); + } + $config_name = (string) \get_post_meta( $post_id, self::EMAIL_CONFIG_NAME_META, true ); + // EMAIL_CONFIG_NAME_META missing/empty would propagate through + // dispatch_email → get_email_payload('') → get_email_config_by_type('') + // returning false, producing a blank-bodied email. Now reachable + // post-NPPD-1547 because send_test_email skips the publish gate + // (drafts are likelier to have incomplete meta). Fail fast + // instead of dispatching empty content. + if ( '' === $config_name ) { + return new \WP_Error( + 'newspack_emails_config_name_missing', + esc_html__( 'Email is not associated with a known config type. Cannot dispatch.', 'newspack-plugin' ), + [ 'status' => 422 ] + ); + } + // Beyond non-empty, require the config name to be a REGISTERED type. + // An unknown/stale name (e.g. a renamed provider type left on an old + // draft) would otherwise reach dispatch and re-resolve to nothing + // downstream, producing a blank-bodied email. Fail fast instead. + if ( ! isset( self::get_email_configs()[ $config_name ] ) ) { + return new \WP_Error( + 'newspack_emails_config_name_unknown', + esc_html__( 'Email is associated with an unregistered config type. Cannot dispatch.', 'newspack-plugin' ), + [ 'status' => 422 ] + ); + } + return [ + 'config' => $email_config, + 'name' => $config_name, + ]; + } + + /** + * Dispatch an email via wp_mail() — locale-switching, header + * construction, payload substitution, logging. Shared between + * send_email() and send_test_email() so the wp_mail() call + * surface is identical between auto-send and test-send. + * + * Callers are responsible for prerequisite validation before + * reaching here. + * + * @param array $email_config Serialized email config. + * @param string $config_name Resolved config name (logger context). + * @param string $to Recipient. + * @param array $placeholders Dynamic content substitutions. + * @return bool wp_mail() result. + */ + private static function dispatch_email( $email_config, $config_name, $to, $placeholders ) { + // Locale-switching is performed by the caller (send_email / + // send_test_email) around the FULL send operation, including + // config resolution — get_email_config_by_type's lazy-create + // path requires a template PHP file with __() calls, and the + // locale active at that moment determines what gets persisted + // to the email post. Moving the switch into dispatch_email + // would mean first-time-template-creation runs under site + // default locale instead of the admin's. $email_content_type = function() { return 'text/html'; @@ -314,20 +624,36 @@ public static function send_email( $config_name, $to, $placeholders = [] ) { $headers[] = sprintf( 'Reply-To: %s <%s>', $email_config['from_name'], $email_config['reply_to_email'] ); } + // try/finally so the wp_mail_content_type filter is removed + // even if wp_mail() (or a filter it triggers) throws — + // otherwise the html-content-type filter persists for every + // subsequent wp_mail() call in the same request, silently + // converting plain-text mails (password resets, WP core + // notifications) to html. add_filter( 'wp_mail_content_type', $email_content_type ); - $email_send_result = wp_mail( // phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.wp_mail_wp_mail - $to, - $email_config['subject'], - self::get_email_payload( $config_name, $placeholders ), - $headers - ); - remove_filter( 'wp_mail_content_type', $email_content_type ); - - if ( $switched_locale ) { - \restore_previous_locale(); + try { + $email_send_result = wp_mail( // phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.wp_mail_wp_mail + $to, + $email_config['subject'], + self::get_email_payload( $config_name, $placeholders ), + $headers + ); + } finally { + remove_filter( 'wp_mail_content_type', $email_content_type ); } - Logger::log( 'Sending "' . $config_name . '" email to: ' . $to ); + // Log dispatch outcome, not the attempt. The old + // unconditional "Sending..." log produced misleading + // success-shaped lines for wp_mail failures and routinely + // led incident investigation down the wrong path + // (concluding dispatch succeeded when wp_mail returned + // false). Distinguishing success/failure in the log + // matches the WP_Error contract callers see. + if ( $email_send_result ) { + Logger::log( 'Sent "' . $config_name . '" email to: ' . $to ); + } else { + Logger::log( 'Failed to send "' . $config_name . '" email to: ' . $to ); + } return $email_send_result; } @@ -581,18 +907,20 @@ public static function get_emails( $config_names = [], $get_full_configs = true * @param WP_REST_Request $request Request. */ public static function api_send_test_email( $request ) { - $was_sent = self::send_email( + // All validation (post_id, recipient empty/format, supports + // emails, post resolves, HTML payload present, config name + // present) lives in send_test_email → + // validate_send_prerequisites. The handler is a thin + // pass-through so the REST and direct-PHP entry points + // produce identical error codes for identical inputs. + $result = self::send_test_email( $request->get_param( 'post_id' ), $request->get_param( 'recipient' ) ); - if ( $was_sent ) { - return \rest_ensure_response( [] ); - } else { - return new \WP_Error( - 'newspack_test_email_not_sent', - __( 'Test email was not sent.', 'newspack-plugin' ) - ); + if ( is_wp_error( $result ) ) { + return $result; } + return \rest_ensure_response( [] ); } /** diff --git a/plugins/newspack-plugin/src/other-scripts/emails/index.js b/plugins/newspack-plugin/src/other-scripts/emails/index.js index 9219133585..a826544132 100644 --- a/plugins/newspack-plugin/src/other-scripts/emails/index.js +++ b/plugins/newspack-plugin/src/other-scripts/emails/index.js @@ -70,24 +70,34 @@ const ReaderRevenueEmailSidebar = compose( [ const sendTestEmail = async () => { setInFlight( true ); - await savePost(); - apiFetch( { - path: `/newspack/v1/newspack-emails/test`, - method: 'POST', - data: { - recipient: settings.testRecipient, - post_id: postId, - }, - } ) - .then( () => { - createNotice( 'success', __( 'Test email sent!', 'newspack-plugin' ) ); - } ) - .catch( () => { - createNotice( 'error', __( 'Test email was not sent.', 'newspack-plugin' ) ); - } ) - .finally( () => { - setInFlight( false ); + // Single try/catch around BOTH savePost() and apiFetch() with a + // shared finally cleanup. Previously only the apiFetch().finally() + // cleared the in-flight state, so a rejected savePost() (a failed + // draft save) left the sidebar stuck in "Sending…" with no notice — + // the apiFetch chain never ran. Now either failure surfaces a notice + // and always resets inFlight. + try { + await savePost(); + await apiFetch( { + path: `/newspack/v1/newspack-emails/test`, + method: 'POST', + data: { + recipient: settings.testRecipient, + post_id: postId, + }, } ); + createNotice( 'success', __( 'Test email sent!', 'newspack-plugin' ) ); + } catch ( error ) { + // Surface the server's specific message when present (NPPD-1547 + // added structured error codes / messages for each prerequisite + // failure: invalid recipient, missing HTML payload, trashed post, + // etc.). The generic is the fallback for unstructured errors + // (a savePost failure, network failures, CORS) with no `.message`. + const message = ( error && error.message ) || __( 'Test email was not sent.', 'newspack-plugin' ); + createNotice( 'error', message ); + } finally { + setInFlight( false ); + } }; return ( <> diff --git a/plugins/newspack-plugin/tests/unit-tests/emails.php b/plugins/newspack-plugin/tests/unit-tests/emails.php index fcc758dc28..693bfaaa27 100644 --- a/plugins/newspack-plugin/tests/unit-tests/emails.php +++ b/plugins/newspack-plugin/tests/unit-tests/emails.php @@ -14,6 +14,13 @@ class Newspack_Test_Emails extends WP_UnitTestCase { /** * Setup. + * + * Intentionally does NOT call `parent::set_up()` — the existing + * tests in this class depend on the absence of the parent's + * transaction lifecycle (tested empirically: adding parent::set_up + * breaks `test_emails_send_by_id`). Tests that need a user + * factory use the static accessor on `WP_UnitTestCase` rather + * than `$this->factory`. */ public function set_up() { reset_phpmailer_instance(); @@ -147,10 +154,27 @@ public function test_emails_send_by_id() { } /** - * Email post status handling. + * Email post status — auto-send vs. test-send distinction (NPPD-1547). + * + * Locks the contract that splits the two entry points: + * + * - AUTO-SEND (Emails::send_email + Emails::can_send_email) is + * blocked for draft emails. Event-triggered sends (RAS, receipts, + * renewal reminders, etc.) require the email to be in 'publish' + * status — an inactive email must not fire on triggered events. + * - TEST-SEND (Emails::send_test_email) IS allowed for draft + * emails. The admin's deliberate "Send test" operation is + * semantically distinct from auto-trigger gating; before + * NPPD-1547 the shared 'publish' === status guard incidentally + * blocked test-send for inactive emails. + * + * Both assertions in one test makes the distinction explicit and + * locked: a future refactor that re-conflates the two paths + * fails this test against both contracts simultaneously. */ public function test_emails_status() { $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); wp_update_post( [ 'ID' => $test_email['post_id'], @@ -158,12 +182,681 @@ public function test_emails_status() { ] ); - self::assertFalse( Emails::can_send_email( 'test-email-config' ), 'Email can\'t be sent – it\'s not published.' ); - $send_result = Emails::send_email( - 'test-email-config', - 'someone@example.com' + try { + // AUTO-SEND: draft is blocked. Unchanged from pre-NPPD-1547. + self::assertFalse( + Emails::can_send_email( 'test-email-config' ), + 'can_send_email() must return false for draft (auto-send gate).' + ); + $auto_send_result = Emails::send_email( 'test-email-config', 'someone@example.com' ); + self::assertFalse( $auto_send_result, 'send_email() must not dispatch for draft (auto-send gate).' ); + + // TEST-SEND: draft is allowed. The headline contract of NPPD-1547. + // send_test_email requires manage_options at its entry point; + // log in as admin for the duration of the call. + $admin = self::login_as_admin(); + try { + $test_send_result = Emails::send_test_email( $test_email['post_id'], 'someone@example.com' ); + self::assertTrue( + $test_send_result, + 'send_test_email() must return true for draft — the admin "Send test" operation is decoupled from the auto-send status gate.' + ); + } finally { + self::logout_admin( $admin ); + } + } finally { + // Restore the shared post so later tests see 'publish' + // status (this class skips parent::set_up(), so DB + // mutations leak across tests without explicit restore). + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + + /* + * ------------------------------------------------------------------ + * NPPD-1547 — Test-send entry point + recipient validation + * ------------------------------------------------------------------ + * The split between auto-send and test-send is verified at the + * status-gate level by `test_emails_status` above. The tests + * below cover the rest of the test-send contract: prerequisite + * failures surface specific newspack_emails_* WP_Error codes + * (not the previous generic "Test email was not sent." string), + * and capability + trash + recipient-format guards apply + * uniformly whether send_test_email is reached via the REST + * handler or via a direct PHP call. + */ + + /** + * Headline contract: test-send dispatches for a draft email. + */ + public function test_test_send_works_for_draft() { + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'draft', + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'send_test_email() must return true (not WP_Error) for a draft email with all other prereqs satisfied.' ); + + // Verify the email actually reached the (mocked) mailer + // with the correct recipient — guards against accidentally + // short-circuiting before dispatch_email is called. + $mailer = tests_retrieve_phpmailer_instance(); + self::assertContains( 'tester@example.com', $mailer->get_sent()->to[0] ); + } finally { + self::logout_admin( $admin ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + + /** + * Recipient format validation now lives in + * validate_send_prerequisites (the helper), not at the REST + * handler. Both the api_send_test_email REST entry AND direct + * send_test_email PHP calls reject typo input with the same + * code (`newspack_emails_invalid_recipient`) and 400 status. + */ + public function test_test_send_rejects_non_email_recipient_via_rest() { + $test_email = self::get_test_email( 'test-email-config' ); + + $admin = self::login_as_admin(); + try { + $request = new WP_REST_Request( 'POST' ); + $request->set_param( 'post_id', $test_email['post_id'] ); + $request->set_param( 'recipient', 'not-an-email' ); + + $response = Emails::api_send_test_email( $request ); + + self::assertInstanceOf( WP_Error::class, $response ); + self::assertSame( 'newspack_emails_invalid_recipient', $response->get_error_code() ); + self::assertSame( 400, $response->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Direct PHP callers of send_test_email get the same recipient + * validation as REST callers — guards against the "helper does + * one thing, handler does another" gap that would let a future + * internal caller bypass is_email(). + */ + public function test_test_send_rejects_non_email_recipient_via_direct_php() { + $test_email = self::get_test_email( 'test-email-config' ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'not-an-email' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_invalid_recipient', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Supports_emails() returning false means Newspack_Newsletters is + * not active. Acknowledged structural test-infra gap. + * + * The supports_emails() implementation uses class_exists() on Newspack_Newsletters; + * the test bootstrap loads that class and PHP doesn't allow + * un-loading. The false branch is not exercisable without either + * adding a filter to supports_emails() for test seam (a + * production change for testability) or runkit/uopz extensions. + * Accepting reduced coverage on this single guard rather than + * paying the production-code-change cost. Tracked separately as + * "make supports_emails() check mockable for testing" follow-up. + */ + public function test_test_send_blocked_when_no_newspack_newsletters() { + $this->markTestSkipped( 'supports_emails() false branch is not exercisable without modifying production code or runkit/uopz extensions. Test infra investment we are choosing not to make in this PR; tracked as a follow-up.' ); + } + + /** + * A post WITHOUT EMAIL_HTML_META cannot be dispatched. + * validate_send_prerequisites surfaces it as + * newspack_emails_html_payload_missing with 422 — distinct from + * "post doesn't exist" (now newspack_emails_post_missing/404) so + * the UI can prompt "save your draft first" vs. "the post is + * gone". + * + * Uses its own throwaway post (different config name) so the + * missing-meta state doesn't leak into other tests via the + * shared get_emails() lookup. + */ + public function test_test_send_blocked_when_missing_html_payload() { + $post_id = wp_insert_post( + [ + 'post_type' => Emails::POST_TYPE, + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 missing-html test', + 'meta_input' => [ + Emails::EMAIL_CONFIG_NAME_META => 'nppd1547-missing-html', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_html_payload_missing', $result->get_error_code() ); + self::assertSame( 422, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * Post exists with HTML payload BUT no EMAIL_CONFIG_NAME_META. + * Surfaces as newspack_emails_config_name_missing with 422. + * Without this guard, dispatch_email would call + * get_email_payload('') which returns false → array-access on + * false → blank-bodied email sent to recipient. Now reachable + * because send_test_email skips the publish gate, so drafts + * with incomplete meta can route this far. + */ + public function test_test_send_blocked_when_config_name_missing() { + $post_id = wp_insert_post( + [ + 'post_type' => Emails::POST_TYPE, + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 missing-config-name test', + 'meta_input' => [ + // Intentionally no EMAIL_CONFIG_NAME_META. + \Newspack_Newsletters::EMAIL_HTML_META => 'nppd-1547 stub', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_config_name_missing', $result->get_error_code() ); + self::assertSame( 422, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * A non-email post that happens to carry the email meta keys must not + * reach the dispatch path. serialize_email() keys off post meta, so the + * post_type guard is what keeps a matching-meta post of another type out. + */ + public function test_test_send_blocked_for_wrong_post_type() { + $post_id = wp_insert_post( + [ + 'post_type' => 'post', + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 wrong-post-type test', + 'meta_input' => [ + Emails::EMAIL_CONFIG_NAME_META => 'test-email-config', + \Newspack_Newsletters::EMAIL_HTML_META => 'stub', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_wrong_post_type', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * A non-empty but UNREGISTERED config name (e.g. a renamed provider type + * left on an old draft) is rejected before dispatch — otherwise it would + * re-resolve to nothing downstream and send a blank-bodied email. + */ + public function test_test_send_blocked_when_config_name_unknown() { + $post_id = wp_insert_post( + [ + 'post_type' => Emails::POST_TYPE, + 'post_status' => 'draft', + 'post_title' => 'NPPD-1547 unknown-config-name test', + 'meta_input' => [ + Emails::EMAIL_CONFIG_NAME_META => 'this-config-does-not-exist', + \Newspack_Newsletters::EMAIL_HTML_META => 'stub', + ], + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $post_id, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_config_name_unknown', $result->get_error_code() ); + self::assertSame( 422, $result->get_error_data()['status'] ); + } finally { + wp_delete_post( $post_id, true ); + self::logout_admin( $admin ); + } + } + + /** + * The `wp_mail_content_type` filter is request-scoped: dispatch_email + * must remove it after a SUCCESSFUL send, or subsequent plain-text mail + * in the same request silently becomes html. + */ + public function test_dispatch_removes_content_type_filter_on_success() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'Sanity: the test send should succeed.' ); + self::assertFalse( + has_filter( 'wp_mail_content_type' ), + 'wp_mail_content_type must be removed after a successful dispatch.' + ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * ...and removed after a FAILED send too — the try/finally in + * dispatch_email is what guarantees the filter never leaks past the + * wp_mail() call. wp_mail is forced to fail via `pre_wp_mail`. + */ + public function test_dispatch_removes_content_type_filter_on_failure() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + add_filter( 'pre_wp_mail', '__return_false' ); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_test_dispatch_failed', $result->get_error_code() ); + self::assertFalse( + has_filter( 'wp_mail_content_type' ), + 'wp_mail_content_type must be removed even when dispatch fails.' + ); + } finally { + remove_filter( 'pre_wp_mail', '__return_false' ); + self::logout_admin( $admin ); + } + } + + /** + * The locale switch wraps the whole send operation. After a SUCCESSFUL + * send the locale must be restored. The admin is given a distinct + * (`fr_FR`) user locale so a real switch happens and a leak would be + * observable. + */ + public function test_send_test_email_restores_locale_on_success() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + update_user_meta( $admin[0], 'locale', 'fr_FR' ); + $before = get_locale(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertTrue( $result, 'Sanity: the test send should succeed.' ); + self::assertSame( $before, get_locale(), 'Locale must be restored after a successful test-send.' ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * ...and restored on an EARLY prerequisite failure too (here an invalid + * recipient, which returns before dispatch). The switch is the + * outermost operation, so every return path must unwind it. + */ + public function test_send_test_email_restores_locale_on_early_error() { + $test_email = self::get_test_email( 'test-email-config' ); + $admin = self::login_as_admin(); + update_user_meta( $admin[0], 'locale', 'fr_FR' ); + $before = get_locale(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'not-an-email' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( $before, get_locale(), 'Locale must be restored even when an early prerequisite check fails.' ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Trashed posts cannot be test-sent. The publish-status gate + * skip in NPPD-1547 is for the "draft / inactive but being + * edited" case; a trashed post is intentionally removed and + * shouldn't be sendable. Restoring from trash is a one-click + * admin operation if the publisher really wants to test the + * trashed content. + */ + public function test_test_send_blocked_for_trashed_post() { + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'trash', + ] + ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_post_trashed', $result->get_error_code() ); + self::assertSame( 409, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + + /** + * Empty recipient surfaces newspack_emails_empty_recipient with + * 400 — same code regardless of REST vs direct entry (no more + * duplicate codes for the same state). + */ + public function test_test_send_blocked_when_recipient_empty() { + $test_email = self::get_test_email( 'test-email-config' ); + + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( $test_email['post_id'], '' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_empty_recipient', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Invalid post_id (0, negative, non-numeric) surfaces + * newspack_emails_invalid_post_id with 400 — distinct from + * "post doesn't exist" (404). 0 is the common case via + * `absint(missing_param)`; this guard returns a structurally + * correct 400 rather than the previous mis-classified 404. + */ + public function test_test_send_blocked_when_post_id_invalid() { + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( 0, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_invalid_post_id', $result->get_error_code() ); + self::assertSame( 400, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Tight post_id validation: is_numeric() is too loose — it + * accepts floats like '1.5' and scientific notation like '1e3' + * which then truncate-cast to ints (1, 1000) and resolve to + * the wrong post. Require integer-shaped input. + * + * Each value must be rejected with newspack_emails_invalid_post_id. + */ + public function test_test_send_rejects_non_integer_shaped_post_id() { + $admin = self::login_as_admin(); + try { + // Values is_numeric() accepts but ctype_digit rejects. + // Each must surface newspack_emails_invalid_post_id (400). + $non_integer_shapes = [ + '1.5', + '1e3', + '1.7e308', + '-5', + 'abc', + '12abc', + ' 5 ', + '', + null, + ]; + + foreach ( $non_integer_shapes as $bad_post_id ) { + // Friendly label for assertion-failure output. null + // and '' don't survive sprintf %s cleanly otherwise. + $label = null === $bad_post_id ? 'NULL' : ( '' === $bad_post_id ? "''" : "'{$bad_post_id}'" ); + $result = Emails::send_test_email( $bad_post_id, 'tester@example.com' ); + self::assertInstanceOf( + WP_Error::class, + $result, + "post_id {$label} must be rejected" + ); + self::assertSame( + 'newspack_emails_invalid_post_id', + $result->get_error_code(), + "post_id {$label} wrong error code" + ); + } + } finally { + self::logout_admin( $admin ); + } + } + + /** + * Digit-only string post_id normalizes to int and routes to the + * post-id branch correctly. Locks in the send_email + * gettype()-branching hardening: numeric-string callers (from + * $_POST, post meta, cast-naive integrations) no longer silently + * fall through to the string-name branch. + */ + public function test_send_test_email_accepts_numeric_string_post_id() { + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'draft', + ] ); - self::assertFalse( $send_result, 'Email has not been sent.' ); + $admin = self::login_as_admin(); + try { + // Cast post_id to a string to simulate what a $_POST + // or untyped REST param would deliver pre-absint. + $result = Emails::send_test_email( (string) $test_email['post_id'], 'tester@example.com' ); + self::assertTrue( $result, 'Numeric-string post_id must be accepted (normalized to int internally).' ); + } finally { + self::logout_admin( $admin ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + + /** + * Subscriber-level user calling the REST route is blocked by + * api_permissions_check (the route's permission_callback). + */ + public function test_test_send_blocked_when_non_admin_via_rest() { + $subscriber_id = wp_insert_user( + [ + 'user_login' => 'nppd1547_subscriber_' . uniqid(), + 'user_pass' => 'test-password', + 'user_email' => 'sub-' . uniqid() . '@example.test', + 'role' => 'subscriber', + ] + ); + $prev_user = get_current_user_id(); + wp_set_current_user( $subscriber_id ); + + try { + $result = Emails::api_permissions_check( null ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_rest_forbidden', $result->get_error_code() ); + self::assertSame( 403, $result->get_error_data()['status'] ); + } finally { + wp_set_current_user( $prev_user ); + wp_delete_user( $subscriber_id ); + } + } + + /** + * Subscriber-level user calling send_test_email DIRECTLY (not + * via REST) is blocked by the cap check at the entry point of + * send_test_email itself. Without the entry-point check, a + * future PHP caller (CLI command, plugin hook) could bypass + * the REST route's permission_callback and dispatch draft + * emails from a low-privilege context. This test locks the + * defense-in-depth contract. + */ + public function test_test_send_blocked_when_non_admin_via_direct_php() { + $test_email = self::get_test_email( 'test-email-config' ); + + $subscriber_id = wp_insert_user( + [ + 'user_login' => 'nppd1547_subscriber_direct_' . uniqid(), + 'user_pass' => 'test-password', + 'user_email' => 'sub-direct-' . uniqid() . '@example.test', + 'role' => 'subscriber', + ] + ); + $prev_user = get_current_user_id(); + wp_set_current_user( $subscriber_id ); + + try { + $result = Emails::send_test_email( $test_email['post_id'], 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_forbidden', $result->get_error_code() ); + self::assertSame( 403, $result->get_error_data()['status'] ); + } finally { + wp_set_current_user( $prev_user ); + wp_delete_user( $subscriber_id ); + } + } + + /** + * Non-existent post_id surfaces newspack_emails_post_missing + * with 404 — distinct from invalid-id-shape (400) and missing- + * content (422). The split lets the UI route by failure mode. + */ + public function test_test_send_blocked_when_post_missing() { + $admin = self::login_as_admin(); + try { + $result = Emails::send_test_email( 9999999, 'tester@example.com' ); + + self::assertInstanceOf( WP_Error::class, $result ); + self::assertSame( 'newspack_emails_post_missing', $result->get_error_code() ); + self::assertSame( 404, $result->get_error_data()['status'] ); + } finally { + self::logout_admin( $admin ); + } + } + + /** + * REGRESSION LOCK: NPPD-1547's refactor extracted the send-path + * helper but MUST preserve auto-send's status gate. Without this + * lock-in, a future refactor that accidentally routes + * send_email's post-id branch through send_test_email's + * status-less path would silently start dispatching draft + * emails on triggered events. + */ + public function test_auto_send_still_blocked_for_draft() { + $test_email = self::get_test_email( 'test-email-config' ); + $original_status = get_post_status( $test_email['post_id'] ); + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => 'draft', + ] + ); + + try { + // Post-id path of send_email — the branch that shares + // validate_send_prerequisites with send_test_email. Auto-send + // must still apply the status check on top. + $post_id_result = Emails::send_email( $test_email['post_id'], 'tester@example.com' ); + self::assertFalse( $post_id_result, 'send_email() post-id branch must remain status-gated.' ); + + // String path of send_email — the dominant auto-trigger + // surface. Status check is inline (not via the helper). + $string_result = Emails::send_email( 'test-email-config', 'tester@example.com' ); + self::assertFalse( $string_result, 'send_email() string-name branch must remain status-gated.' ); + } finally { + wp_update_post( + [ + 'ID' => $test_email['post_id'], + 'post_status' => $original_status, + ] + ); + } + } + + /** + * Login helper: create a fresh administrator user, set them + * as current, return state needed for cleanup. Tests that + * call send_test_email need to pair this with logout_admin() + * in a try/finally block. + * + * @return array{0: int, 1: int} [ admin user id, previously-current user id ] + */ + private static function login_as_admin() { + $admin_id = wp_insert_user( + [ + 'user_login' => 'nppd1547_admin_' . uniqid(), + 'user_pass' => 'test-password', + 'user_email' => 'admin-' . uniqid() . '@example.test', + 'role' => 'administrator', + ] + ); + $prev_user = get_current_user_id(); + wp_set_current_user( $admin_id ); + return [ $admin_id, $prev_user ]; + } + + /** + * Restore the previously-current user and delete the admin + * created by login_as_admin(). + * + * @param array{0: int, 1: int} $session State returned by login_as_admin. + */ + private static function logout_admin( $session ) { + [ $admin_id, $prev_user ] = $session; + wp_set_current_user( $prev_user ); + wp_delete_user( $admin_id ); } }