fix(newsletters): hide abandoned auto-drafts from lists (DSGNEWS-213)

[thomasguillot]

All Submissions:

• Have you followed the Newspack Contributing guidelines?
• Does your code follow the WordPress coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

"Add Newsletter" links to post-new.php, and WordPress inserts an auto-draft as soon as that page loads, before the layout picker opens. Leaving the picker without choosing a layout left that row in the Newsletters list as a (no subject) entry, because the list queried auto-draft alongside the writable statuses.

All three admin-shell lists (Newsletters, Newsletter Ads, Layouts) now exclude auto-draft, matching core's own Posts and Pages lists. Nothing is deleted here; core's daily cron force-deletes auto-drafts after 7 days.

Any save promotes an auto-draft to draft (core-data's prePersistPostType forces the status and clears the placeholder title), so a row still sitting in auto-draft has never been saved and its stored content is always empty. There is no in-progress newsletter to lose.

Two coupled pieces move with it. The REST draft buckets re-widened auto-draft back into post_status, so changing the JS alone would have done nothing. And advertiser term counts deliberately counted auto-draft ads to match the Ads draft bucket, so they drop it too, with the recount sentinel bumped so existing sites refresh.

Closes DSGNEWS-213.

How to test the changes in this Pull Request:

1. Note the count in the "All Newsletters" heading.
2. Go to Newsletters > Add New. The layout picker overlay opens.
3. Press the browser back button without choosing a layout.
4. Go to Newsletters > All Newsletters. No new (no subject) row appears and the count is unchanged.
5. Open the Status filter and choose Draft. Saved drafts appear; no auto-drafts do.
6. Save a newsletter with no subject. It still lists as (no subject).
7. Repeat steps 2 to 4 for Newsletters > Advertising, then for Newsletters > Layouts.
8. Open Newsletters > Advertising and check the Advertiser filter. Each count matches the ads listed for that advertiser.
9. On a site with no newsletters, mis-click Add New, then return to the list. The empty state still shows.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?

• Have you written new tests for your changes, as applicable?

• Have you successfully run tests with your changes locally?

• No deletion path ships here. Existing stray rows stay in the database, invisible, until core's 7-day cron clears them.

• The advertiser recount is a one-time pass per site, triggered on the first admin_init after this deploys.

[Copilot]

Pull request overview

This PR updates the Newspack Newsletters admin-shell lists (Newsletters, Ads, Layouts) and related REST/query plumbing to exclude auto-draft rows, preventing abandoned “Add New” auto-drafts from appearing as (no subject) / (no title) entries. It also updates advertiser term counting to match the new ads list semantics and adds/adjusts tests to lock in the behavior.

Changes:

• Remove auto-draft from default status sets and “Draft” bucket/status widening logic in both JS list queries and PHP REST query helpers.
• Update ads advertiser term counting (and its recount sentinel) to exclude auto-draft rows so counts match what the UI lists.
• Expand and adjust PHPUnit/Jest coverage to ensure auto-draft does not re-enter via filters/widening.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
plugins/newspack-newsletters/tests/test-newsletters-list-rest.php	Updates REST alignment tests and adds coverage ensuring auto-draft never re-enters Draft/default status queries.
plugins/newspack-newsletters/tests/test-advertisers-list-rest.php	Updates advertiser count tests for the new “exclude auto-draft” semantics and sentinel bump.
plugins/newspack-newsletters/tests/test-ads-list-rest.php	Updates Ads REST bucketing tests to ensure Draft kind excludes auto-draft.
plugins/newspack-newsletters/src/admin-shell/screens/newsletters-list/initial-filters.test.js	Updates expected draft filter mapping for legacy post_status=auto-draft deep links.
plugins/newspack-newsletters/src/admin-shell/screens/newsletters-list/initial-filters.js	Keeps legacy auto-draft deep-link mapping but maps it to draft,pending.
plugins/newspack-newsletters/src/admin-shell/screens/newsletters-list/fields.js	Updates the Draft status filter option to draft,pending.
plugins/newspack-newsletters/src/admin-shell/screens/newsletters-list/build-query.test.js	Adds assertions that default and filtered queries never include auto-draft.
plugins/newspack-newsletters/src/admin-shell/screens/newsletters-list/build-query.js	Removes auto-draft from the default newsletters list status set.
plugins/newspack-newsletters/src/admin-shell/screens/layouts-list/use-layouts-data.js	Removes auto-draft from layouts list default status set.
plugins/newspack-newsletters/src/admin-shell/screens/ads-list/build-query.test.js	Adds assertion that ads list default status set excludes auto-draft.
plugins/newspack-newsletters/src/admin-shell/screens/ads-list/build-query.js	Removes auto-draft from the default ads list status set.
plugins/newspack-newsletters/includes/ads/class-ads.php	Updates advertiser term count query and recount sentinel to drop auto-draft.
plugins/newspack-newsletters/includes/admin/class-newsletters-list-rest.php	Updates status widening/bucketing logic to ensure Draft no longer includes auto-draft.
plugins/newspack-newsletters/includes/admin/class-ads-list-rest.php	Updates Draft-kind bucketing to exclude auto-draft in Ads list REST filtering.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[thomasguillot]

Code review

Release risk: LOW. Approve with comments.

The part I most wanted to put under test was the three-way coupling, and it held. The JS status sets, both REST draft buckets, and the advertiser count query all move together, so the claim in the description that changing the JS alone would have been a no-op is correct: align_status_filter_with_scheduled_meta re-widened auto-draft straight back into post_status.

Verified on a local site with four pre-existing abandoned auto-drafts. Before the change, the All Newsletters heading read 117 with four (no subject) rows at the top. After it, the same page held at 113 through Add Newsletter, browser back, All Newsletters, with no new row, while the auto-drafts confirmably still landed in the database. JS suite 250/250, PHP suite 734/734, PHPCS and ESLint clean.

Concerns ruled out:

• The paths that could have regressed did not. _update_term_count_on_transition_post_status keeps advertiser counts accurate when an abandoned auto-draft is later saved or cleared by core's wp_delete_auto_drafts. The newsletters and layouts duplicate and restore actions create draft or publish rows rather than auto-drafts. Legacy ?post_status=auto-draft deep links still resolve onto the Draft chip in both initial-filters.js files.
• No empty IN () or unbounded query. $post_status_set in filter_rest_query is guarded by the early return on empty $kinds, and $widened always starts from a non-empty $values.
• No cross-plugin surface. All 14 files are internal to newspack-newsletters; no shared contract paths changed and no consumer references exist elsewhere in the workspace.
• Nothing on the security side. The $wpdb->prepare() calls are correctly parameterised and the only interpolations are $wpdb table properties. No capability, nonce, escaping or i18n regression in the diff.

Seven findings, none blocking.

---

Suggestions

Severity: suggestion · plugins/newspack-newsletters/includes/admin/class-newsletters-list-rest.php:126

A status request of only auto-draft now installs no bucket filter at all. Narrowing $wants_draft to array_intersect( $values, [ 'draft', 'pending' ] ) means a request whose only status value is auto-draft leaves all four wants_* flags false. $bucket_clauses then comes out empty and install_bucket_filter returns $args untouched, so the request runs with the posts controller's post_status = ['auto-draft'] and no posts_where at all. That is the one remaining path through this endpoint that returns abandoned rows, and it now returns them completely unbucketed, where the draft clause previously applied. It is not reachable from the shipped UI, since initial-filters.js:17 rewrites the deep link to draft,pending, and it needs edit_posts, so this reads as an invariant gap rather than a leak. But auto-draft stays a valid value in the core status enum for anyone holding that cap, so a bookmarked REST URL or a future in-plugin caller walks straight through it. I think we should strip auto-draft inside parse_status_values, or treat it as an explicit no-match, which closes it without touching any other branch.

Severity: suggestion · plugins/newspack-newsletters/includes/ads/class-ads.php:427

The re-armed recount is an uncapped per-term loop whose sentinel is written last. The _v3 to _v4 bump makes every existing site re-run maybe_recount_advertiser_terms on its next admin_init. get_terms() takes no number cap, and wp_update_term_count_now() routes into update_advertiser_term_count, which issues one SELECT COUNT(*), one UPDATE, and two do_action calls per term. update_option( $option, 1 ) only runs after the whole loop completes, so nothing claims the work up front. The update_option return value is also discarded, so a failed write re-arms the full recount on every later admin request with no backoff. In practice the payload is probably empty, since assigning an advertiser term requires a save, which promotes the ad out of auto-draft, but the failure mode does not depend on a large payload to be worth closing.

Risk if not addressed: if a site with a large advertiser taxonomy takes a burst of traffic to admin-ajax.php during the upgrade window, every one of those requests runs the recount too, because admin-ajax.php fires the same hook before it checks who is calling and the recount has no capability check of its own. On such a site that means an uncapped term query plus one count and one update per advertiser, repeated per concurrent request, showing up as a database load spike right after the update rather than as wrong data.

Consider: claiming the sentinel before the loop instead of after it, and skipping the work for requests that cannot manage the taxonomy, so a burst of concurrent requests during the upgrade window does the recount once rather than once each.

Severity: suggestion · plugins/newspack-newsletters/includes/ads/class-ads.php:399

The counted status set is hand-duplicated in three places. post_status IN ( %s, %s, %s, %s, %s ) hardcodes five placeholders against five literal arguments, and that set has to stay in lockstep with the Draft bucket in Ads_List_REST::filter_rest_query and DEFAULT_STATUSES in ads-list/build-query.js. This PR is the proof of the coupling: it had to edit all three and bump a sentinel. Nothing fails loudly when one is missed. The term count quietly disagrees with the list it is meant to describe, and a publisher meets that as a wrong number in the Advertiser filter.

Consider: deriving the counted status set from a single shared constant used by the count query, the REST draft bucket, and the JavaScript default. As written, a later change to one of the three leaves the advertiser counts disagreeing with the Ads list, and because counts are only recomputed when a term is edited, that disagreement persists silently until someone bumps the sentinel again.

Nits

Severity: nit · plugins/newspack-newsletters/src/admin-shell/screens/newsletters-list/fields.js:46

Stale auto-draft title branches and comments across the three list screens. fields.js:47 still branches on 'auto-draft' === item?.status to render the (no subject) placeholder, as do ads-list/fields.js:53 and layouts-list/fields.js:121, and layouts-list/actions.js:23 still carries a comment explaining an auto-draft case. None of these can fire now that the lists never query the status. The ! raw || half of each condition still earns its keep for a saved draft with an empty subject, so the guards themselves are harmless. The comments are the problem: "New newsletters carry WordPress's 'Auto Draft' placeholder title" now describes a state the list cannot reach, and will mislead the next reader into thinking auto-drafts still land here.

Severity: nit · plugins/newspack-newsletters/tests/test-newsletters-list-rest.php:666

The query half of test_default_status_set_excludes_auto_draft cannot fail. array_merge( $args, [ 'post_status' => $defaults ] ) overwrites whatever the widening produced with the literal five-status array, which excludes auto-draft by construction, so both assertions that follow are guaranteed and would pass identically on main. The discriminating assertion is the assertNotContains on line 662: on main the widening injected auto-draft into $args['post_status'], so that line does fail on a revert. It is worth keeping, and worth a comment saying so, because on the fixed code it reads like a guard on a key the widening deliberately never writes. Driving the query from $args rather than overriding it would make the second half earn its keep too. Separately, $defaults on line 654 is a hand-copy of DEFAULT_STATUSES in build-query.js:16 with nothing tying the two together.

Severity: nit · plugins/newspack-newsletters/src/admin-shell/screens/layouts-list/use-layouts-data.js:11

Layouts is the only changed list with nothing locking the new behaviour. Newsletters and Ads each got a JS assertion that the default set does not contain auto-draft, plus PHP integration coverage. The Layouts change has neither, and the module exports only the useLayoutsData hook, so as written there is no seam to test it through. A revert of just this line would go unnoticed by CI. Exporting buildPath or the constant and asserting on it would match what the two sibling screens already do.

Severity: nit · plugins/newspack-newsletters/includes/ads/class-ads.php:426

The superseded _v3 sentinel is left orphaned in the options table. The v3 row stays behind and stays autoloaded, so every bump of the status semantics adds one more permanently autoloaded option that is read on every front-end request and never again means anything. A delete_option( 'newspack_nl_advertiser_count_recounted_v3' ) alongside the v4 write keeps the mechanism from accumulating cruft each time it is versioned.

---

Spec alignment

DSGNEWS-213 asked for the abandoned auto-draft to be discarded on abandonment, and cautioned against changing what the list displays because "other in-progress auto-drafts still need to show". This PR takes the opposite route and says why: any save promotes an auto-draft to draft via core-data's prePersistPostType, so a row still sitting in auto-draft has never been saved and holds nothing to lose. That premise is sound and the reported symptom is resolved, so the divergence looks like the better call rather than a gap. The ticket posed its direction as a question, so it is worth a nod from the author before merge. DSGNEWS-214, where one Add Newsletter click creates two auto-drafts, stays open and is untouched here.

[thomasguillot]

Final pass

All seven findings are addressed in e86b2e30 and 6eccefd5, and the code comments this PR introduced are cut back to the ones carrying a reason.

A status request of only auto-draft now resolves to the Draft bucket. align_status_filter_with_scheduled_meta rewrites the value to draft before the bucket flags are computed, so the request takes the same route as the list's own legacy deep links instead of falling through to an unfiltered post_status = auto-draft query. Covered by test_auto_draft_status_request_resolves_to_draft_bucket.

The recount claims its sentinel before doing the work, and skips requests that cannot manage the taxonomy. admin_init fires on admin-ajax.php for logged-out requests too, so the capability gate is what keeps those out; claiming first is what stops a concurrent burst running the loop once each, and stops a pass that times out re-arming itself on every later request. Two tests cover it: one asserts the sentinel is already set by the time get_terms runs, one asserts a capability-less request does nothing.

The counted status set has a single source. Ads::COUNTED_STATUSES now feeds the count query, and test_counted_statuses_match_the_list_buckets asserts it equals the union of the non-trash post_status sets that Ads_List_REST::filter_rest_query produces. If either side moves alone the test fails, which is the loud failure that was missing. The placeholders stay written out rather than generated: building them with array_fill trips PreparedSQLPlaceholders.ReplacementsWrongNumber, and PHPCS warnings block the commit hook.

The superseded _v3 option is deleted when v4 is claimed, so versioning the sentinel stops leaving an autoloaded row behind each time.

The unreachable auto-draft title branches are gone from all three list screens, leaving raw || __( '(no subject)' ), which still covers a saved draft with an empty subject. The stale rationale in layouts-list/actions.js is reworded to the reason that still holds, that ?? would keep an empty title.raw and produce "Copy of ".

Layouts has a test seam. buildPath is exported and use-layouts-data.test.js asserts the default status set, so a revert of that one line now fails CI like its two siblings.

test_default_status_set_excludes_auto_draft is discriminating end to end. It seeds post_status the way the posts controller does and drives the query from the returned $args rather than overwriting them, so both halves fail on a revert instead of only the first.

On the comments: the same "an abandoned Add new is empty by construction" note had been repeated in five files. It now appears once, next to the newsletters DEFAULT_STATUSES that defines the behaviour. What remains explains a reason that is not visible in the code: why the capability gate exists, why the sentinel is claimed early, why ?? is wrong in copyTitle, why the auto-draft key stays in the deep-link map. Comments that restated the code or a test name are gone.

Verification: JS 253/253 across 30 suites, PHP 738/738 with 3042 assertions, composer phpcs and lint:js clean.