refactor(emails): move Reset endpoint to emails-settings namespace (NPPD-1535)#158
Open
kmwilkerson wants to merge 4 commits into
Open
refactor(emails): move Reset endpoint to emails-settings namespace (NPPD-1535)#158kmwilkerson wants to merge 4 commits into
kmwilkerson wants to merge 4 commits into
Conversation
kmwilkerson
added a commit
that referenced
this pull request
May 29, 2026
Ten fixes from the PR #158 code review. Two findings deferred explicitly: - Audience_Wizard::api_reset_reader_activation_email is a sibling near- identical reset handler that the consolidation argument applies to, but it's a separate workstream outside NPPD-1535's stated scope. Tracked as a follow-up in the PR description. - Response shape mismatch between api_reset_email (raw Emails::get_emails) and api_toggle_wc_email (enriched api_get_email_settings) is real but the current shape preserves the documented "same shape as donations endpoint" compatibility promise. Switching it is a real API change that's lower priority than the correctness fixes. Fixes: 1. **JS test path** (src/wizards/.../emails.test.js:376) — `npm test` would have failed: the reset-action assertion still expected the legacy donations-namespace DELETE path. Updated to the new /wizard/newspack-settings/emails/{id} path. 2. **Duplicate Bucket E label** (tests/.../emails-section.php) — the existing file has "Bucket E — Reader Activation gating" at line 447. My new section was also labeled Bucket E. Renamed to "Bucket F — Reset endpoint (NPPD-1535)". 3. **DELETE route missing `args` block** — the sibling toggle route declares `args` with `type: integer` + `sanitize_callback: absint`. Added the same shape for the DELETE route's `id` param. Closes the defense-in-depth gap (`get_post('0123')` would have silently coerced to int 123). 4. **Error code rename asymmetry** — legacy was `newspack_reset_donation_email_reset_failed`. My port collapsed it to `newspack_reset_email_failed`, dropping the `_reset_` middle while the `_invalid_arg` suffix was preserved. Renamed to `newspack_reset_email_reset_failed` for symmetry with the `_invalid_arg` companion code. 5. **REST_BASE docblock past-tense reference to unlanded NPPD-1538** — the old wording ("even though Emails moved to the Audience wizard in NPPD-1538") read as if 1538 had already landed. Reworded to future-tense: "When NPPD-1538 later moves the Emails screen from Newspack > Settings to Audience > Configuration, this REST path MUST stay at 'newspack-settings'." 6. **Vestigial `$wizard_slug` property** — kept but annotated as vestigial in the docblock. Source of truth for the REST path is REST_BASE; the parent constructor overwrites this property from the wizard_slug arg regardless. 7. **test_reset_email_permission_check rewrite** — the original test called `api_permissions_check()` directly, which means a regression that drops `'permission_callback' => [ $this, 'api_permissions_check' ]` from the route registration would not be caught (WP would default to __return_true with a _doing_it_wrong notice and the endpoint would be open). Renamed to `test_reset_email_route_has_permission_callback` and rewritten to introspect `rest_get_server()->get_routes()` for the DELETE endpoint, asserting (a) the permission_callback is declared, (b) it's not __return_true, (c) the callback denies anonymous callers. 8. **Architectural-lock-in test cited nonexistent precedent file** — the docblock pointed to `tests/.../woocommerce-email-style-sync.php` which doesn't exist. Replaced with real precedents (`tests/.../corrections.php:53-56` for positive route assertion, `tests/.../content-gate/class-ip-access-rule.php:97-99` for route shape introspection). 9. **Architectural-lock-in test missing positive assertion** — the original test only asserted the legacy path was absent. Merged the negative assertion with a NEW positive assertion that the new path IS registered, in `test_reset_route_moved_to_emails_namespace`. Catches the case where a future refactor removes both registrations. 10. **Hardcoded `999999` sentinel ID** — replaced with `wp_insert_post` + `wp_delete_post( …, true )` so the missing-id is guaranteed rather than assumed. Also added a sanity `assertNull( get_post(...) )` on the derived ID to surface a clear failure if the deletion didn't take. 11. **wp_set_current_user(0) leak risk** — captured `$prev_user` before setting and restored after, so the test doesn't leak the anonymous- user state to subsequent tests under random or filtered ordering. PHPCS clean, full output verified (not tailed). Full PHP suite holds at 1335 tests / 0 failures / 5 pre-existing skips; assertion count went +6 (4269 → 4275) — the rewritten permission test introspects more. The JS test change is a one-line string literal swap matching the pattern used by the toggle-action assertions in the same file; `npm test` couldn't run locally due to a workspace-level pnpm dependency issue (Cannot find module 'babel-jest') unrelated to this change — CI will verify. Refs: NPPD-1535 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kmwilkerson
force-pushed
the
nppd-1525-slice-2b-email-preview
branch
from
67f0e73 to
35d4c27
Compare
…PPD-1535) The email reset endpoint historically lived in the donations wizard at `DELETE /newspack/v1/wizard/newspack-audience-donations/emails/{id}`, which is a leaky placement — the action has nothing to do with donations and the endpoint is consumed only by the unified emails wizard UI. Move it under the emails-settings namespace where it belongs. Backend: - Add `Emails_Section::REST_BASE` constant ('wizard/newspack-settings/emails') with an API-stability docblock. The hardcoded value (rather than interpolating from `$wizard_slug`) protects the REST paths from cascading rename when NPPD-1538 relocates the Emails screen from Newspack > Settings to Audience > Configuration. The 1538 refactor changes `$wizard_slug` to 'newspack-audience' but keeps the REST paths stable; this constant introduces that pattern early so future route additions know to use it. - Refactor existing GET and toggle routes to use `self::REST_BASE` for consistency — one source of truth for the path prefix. - Add new DELETE route at `self::REST_BASE . '/(?P<id>\d+)'` registered unconditionally (resetting a Newspack-managed email has no WC dependency). - Port `api_reset_donation_email` to `api_reset_email` as a static method on `Emails_Section`. Implementation is unchanged: validates the post is an email type, trashes it via `wp_trash_post`, returns the refreshed list via `Emails::get_emails(...)`. The error codes are renamed (`newspack_reset_donation_email_*` -> `newspack_reset_email_*`) to drop the donations framing. - Remove the donations-side route registration and handler from `Audience_Donations`. Imports stay as-is — `WP_Error` is still used by `check_required_plugins_installed`. Frontend: - Update `resetEmail()` path in emails.tsx from the donations namespace to the new emails namespace. - Delete the `@todo NPPD-1532` comment block — the move it called for is now done. The 1532 reference was always a typo for this work; the comment was tracked under the wrong ticket number. The endpoint contract (DELETE method, integer post id path param, response shape) is unchanged. The new path is the canonical location. Refs: NPPD-1535 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-1535) Five tests in a new "Bucket E — Reset endpoint" section of `tests/unit-tests/emails-section.php`: - `test_reset_email_successful`: happy path. Creates an email post, calls `Emails_Section::api_reset_email` with the post ID, asserts the response is not a WP_Error, the post status is now 'trash', and the response payload is an array (the refreshed list shape from `Emails::get_emails`). - `test_reset_email_invalid_post_id`: non-existent post ID -> WP_Error with code `newspack_reset_email_invalid_arg`, status 400. - `test_reset_email_wrong_post_type`: regular post (not email) ID -> same invalid_arg error. Also asserts the non-email post is NOT trashed — defense against accidental cross-type deletion if a request is crafted with a bad ID. - `test_reset_email_permission_check`: `wp_set_current_user(0)` + `(new Emails_Section())->api_permissions_check()` returns the 403 WP_Error from the inherited `Wizard_Section::api_permissions_check`. Guards against removing the `permission_callback` from the DELETE route registration. - `test_old_donations_namespace_route_no_longer_registered`: architectural-lock-in assertion. After `do_action('rest_api_init')`, the routes registered under NEWSPACK_API_NAMESPACE do NOT contain the legacy `/wizard/newspack-audience-donations/emails/(?P<id>\d+)` path. If a future change resurrects the donations-side registration (e.g. a bad merge that brings back `api_reset_donation_email`), this test fails loudly. Same pattern as `woocommerce-email-style-sync.php::test_no_customize_save_after_or_after_switch_theme_hooks`. Test landing location: extended `tests/unit-tests/emails-section.php` rather than creating a new file. Reset is now part of `Emails_Section`, and the existing file is the natural home — its existing buckets cover the schema, the response builder, and the defaults merge; this commit adds a fifth bucket for the reset endpoint. Test counts: - Isolated (filter `test_reset_email`): 4 tests, 13 assertions, OK. - Isolated (filter `test_old_donations_namespace_route_no_longer_registered`): 1 test, 1 assertion, OK. - Full suite: 1335 tests / 4269 assertions / 0 failures / 5 pre-existing env-gated skips (4 in emails-section-woocommerce.php for "Real WC not loaded" + 1 in reader-registration-endpoint.php). The pre-existing WooCommerce shim skips remain isolated — count and file locations identical to the slice 2b baseline (1330 -> 1335 = +5 new tests, no other count changes). Refs: NPPD-1535 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ten fixes from the PR #158 code review. Two findings deferred explicitly: - Audience_Wizard::api_reset_reader_activation_email is a sibling near- identical reset handler that the consolidation argument applies to, but it's a separate workstream outside NPPD-1535's stated scope. Tracked as a follow-up in the PR description. - Response shape mismatch between api_reset_email (raw Emails::get_emails) and api_toggle_wc_email (enriched api_get_email_settings) is real but the current shape preserves the documented "same shape as donations endpoint" compatibility promise. Switching it is a real API change that's lower priority than the correctness fixes. Fixes: 1. **JS test path** (src/wizards/.../emails.test.js:376) — `npm test` would have failed: the reset-action assertion still expected the legacy donations-namespace DELETE path. Updated to the new /wizard/newspack-settings/emails/{id} path. 2. **Duplicate Bucket E label** (tests/.../emails-section.php) — the existing file has "Bucket E — Reader Activation gating" at line 447. My new section was also labeled Bucket E. Renamed to "Bucket F — Reset endpoint (NPPD-1535)". 3. **DELETE route missing `args` block** — the sibling toggle route declares `args` with `type: integer` + `sanitize_callback: absint`. Added the same shape for the DELETE route's `id` param. Closes the defense-in-depth gap (`get_post('0123')` would have silently coerced to int 123). 4. **Error code rename asymmetry** — legacy was `newspack_reset_donation_email_reset_failed`. My port collapsed it to `newspack_reset_email_failed`, dropping the `_reset_` middle while the `_invalid_arg` suffix was preserved. Renamed to `newspack_reset_email_reset_failed` for symmetry with the `_invalid_arg` companion code. 5. **REST_BASE docblock past-tense reference to unlanded NPPD-1538** — the old wording ("even though Emails moved to the Audience wizard in NPPD-1538") read as if 1538 had already landed. Reworded to future-tense: "When NPPD-1538 later moves the Emails screen from Newspack > Settings to Audience > Configuration, this REST path MUST stay at 'newspack-settings'." 6. **Vestigial `$wizard_slug` property** — kept but annotated as vestigial in the docblock. Source of truth for the REST path is REST_BASE; the parent constructor overwrites this property from the wizard_slug arg regardless. 7. **test_reset_email_permission_check rewrite** — the original test called `api_permissions_check()` directly, which means a regression that drops `'permission_callback' => [ $this, 'api_permissions_check' ]` from the route registration would not be caught (WP would default to __return_true with a _doing_it_wrong notice and the endpoint would be open). Renamed to `test_reset_email_route_has_permission_callback` and rewritten to introspect `rest_get_server()->get_routes()` for the DELETE endpoint, asserting (a) the permission_callback is declared, (b) it's not __return_true, (c) the callback denies anonymous callers. 8. **Architectural-lock-in test cited nonexistent precedent file** — the docblock pointed to `tests/.../woocommerce-email-style-sync.php` which doesn't exist. Replaced with real precedents (`tests/.../corrections.php:53-56` for positive route assertion, `tests/.../content-gate/class-ip-access-rule.php:97-99` for route shape introspection). 9. **Architectural-lock-in test missing positive assertion** — the original test only asserted the legacy path was absent. Merged the negative assertion with a NEW positive assertion that the new path IS registered, in `test_reset_route_moved_to_emails_namespace`. Catches the case where a future refactor removes both registrations. 10. **Hardcoded `999999` sentinel ID** — replaced with `wp_insert_post` + `wp_delete_post( …, true )` so the missing-id is guaranteed rather than assumed. Also added a sanity `assertNull( get_post(...) )` on the derived ID to surface a clear failure if the deletion didn't take. 11. **wp_set_current_user(0) leak risk** — captured `$prev_user` before setting and restored after, so the test doesn't leak the anonymous- user state to subsequent tests under random or filtered ordering. PHPCS clean, full output verified (not tailed). Full PHP suite holds at 1335 tests / 0 failures / 5 pre-existing skips; assertion count went +6 (4269 → 4275) — the rewritten permission test introspects more. The JS test change is a one-line string literal swap matching the pattern used by the toggle-action assertions in the same file; `npm test` couldn't run locally due to a workspace-level pnpm dependency issue (Cannot find module 'babel-jest') unrelated to this change — CI will verify. Refs: NPPD-1535 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ling toggle Adds an inline comment near api_reset_email's return statement noting that the endpoint returns the raw Emails::get_emails() array, NOT the enriched api_get_email_settings() shape that api_toggle_wc_email uses. The current shape preserves the legacy donations-endpoint contract; aligning sibling endpoints in this class on a single shape is tracked separately in NPPD-1569 (which folds Audience_Wizard's api_reset_reader_activation_email into Emails_Section::api_reset_email). Captured in code rather than a parallel backlog ticket because the decision binds to the code, and the next person touching api_reset_email will see the rationale exactly where it applies. Refs: NPPD-1535, NPPD-1569 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kmwilkerson
force-pushed
the
nppd-1535-move-email-reset-endpoint
branch
from
f419394 to
af7fd60
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves the email reset REST endpoint from the donations wizard namespace into the unified Settings > Emails namespace, keeping email-management API paths stable while aligning the backend endpoint with the current frontend surface.
Changes:
- Adds
Emails_Section::REST_BASEand uses it for GET, toggle, and the new DELETE reset route. - Ports the reset handler from
Audience_DonationstoEmails_Sectionwith updated error codes and route args. - Updates frontend reset calls and tests to use
/wizard/newspack-settings/emails/{id}and adds PHP coverage for reset behavior and route migration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
plugins/newspack-plugin/includes/wizards/newspack/class-emails-section.php |
Defines the stable emails REST base, registers the DELETE reset endpoint, and implements api_reset_email(). |
plugins/newspack-plugin/includes/wizards/audience/class-audience-donations.php |
Removes the legacy donations-namespace reset route and handler. |
plugins/newspack-plugin/src/wizards/newspack/views/settings/emails/emails.tsx |
Updates the reset action to call the new emails namespace endpoint. |
plugins/newspack-plugin/src/wizards/newspack/views/settings/emails/emails.test.js |
Updates the reset action expectation to the new REST path. |
plugins/newspack-plugin/tests/unit-tests/emails-section.php |
Adds reset endpoint tests for success, invalid IDs, wrong post types, permission callback registration, and old route removal. |
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
Moves the email Reset endpoint out of the donations wizard namespace and into the unified emails-settings namespace where it belongs. Introduces a
REST_BASEconstant onEmails_Sectionthat pins the email REST paths atwizard/newspack-settings/emailsfor API stability, with a docblock noting the pin must survive NPPD-1538's later screen relocation.Resolves NPPD-1535.
Stacking
Emails_Sectionwas rebuilt with the unified GET + toggle routes — Reset slots in next to its natural neighbors there.What changes
Backend:
Emails_Section::REST_BASE = 'wizard/newspack-settings/emails'constant. Docblock explicitly notes the path is hardcoded for API stability — even though NPPD-1538 will later move the Emails screen to the Audience wizard, the REST path must remain unchanged because external callers and the frontend depend on it.self::REST_BASE(one source of truth instead of$this->wizard_sluginterpolation).self::REST_BASE . '/(?P<id>\d+)', registered unconditionally (no WC gate — Reset works on Newspack-managed emails regardless of WC presence).Emails_Section::api_reset_email()static method, ported fromAudience_Donations::api_reset_donation_email(). Error codes renamednewspack_reset_donation_email_*→newspack_reset_email_*to drop the no-longer-accurate "donation" framing.Audience_Donations: route registration and handler removed entirely. No deprecation shim — the endpoint isn't a publicly-documented API and the move ships atomically with the frontend update.Frontend:
emails.tsx::resetEmail()calls the new path:/newspack/v1/wizard/newspack-settings/emails/${postId}.@todo NPPD-1532comment block deleted — the move is done.Looking ahead — relationship to NPPD-1538
NPPD-1538 will move the Emails screen from Newspack → Settings to Audience → Configuration. The legacy #4759 handles this by renaming the wizard slug (
newspack-settings→newspack-audience) while explicitly pinning REST paths via a hardcoded constant. This PR pre-introduces that pattern under the sameREST_BASEname and docblock — so when 1538 lands, it inherits the pattern rather than introducing it. 1538's diff is smaller as a result; the API-stability discipline lands in main now rather than waiting.Testing
Five new tests in
tests/unit-tests/emails-section.phpas a fifth bucket ("Bucket E — Reset endpoint"):test_reset_email_successful— happy path: valid post ID, post moves to trash, response is the refreshed list.test_reset_email_invalid_post_id— non-existent ID returns 400 withnewspack_reset_email_invalid_arg(matches the legacy single-code pattern; both invalid-arg cases share the code).test_reset_email_wrong_post_type— non-email post returns 400 withnewspack_reset_email_invalid_arg(same code as above; the API surface treats both as "the ID you passed isn't a reset-able email").test_reset_email_permission_check— unauthenticated request blocked bypermission_callback.test_old_donations_namespace_route_no_longer_registered— architectural lock-in test: assertsrest_get_server()->get_routes()does not contain/newspack/v1/wizard/newspack-audience-donations/emails/(?P<id>\d+)for DELETE afterrest_api_initfires. Prevents accidental re-introduction of the old route in a future change.Full PHP suite: 1335 / 0 failures. Baseline against slice 2b's branch tip was 1330; the +5 tests delta matches exactly, no other test counts shifted.