feat(access-control): auto-signup on renewal only for already subscribed lists

[dkoo]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• 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:

#4589 added functionality to auto-signup users to restricted lists when they meet access requirements for those lists. However, if a reader purchases a subscription that includes access to restricted lists, then subsequently unsubscribe from a restricted list outside of the site (such as by clicking on the "unsubscribe from this newsletter" link in the delivered email), we don't want to automatically re-add them to this list, which could result in frustration on the user's part.

This PR adds a check when a renewal order is created to cache the user's subscribed lists as user meta. When an access check for the user occurs, if that user meta exists, auto-signup will only occur for lists the user was already subscribed to when the most recent renewal began. The user meta cache is cleared after any access check is completed, so it doesn't persist beyond a single pending check.

This way, auto-signup will only occur after a new subscription is created, a subscription is switched to a different product within the same group/parent, or if the subscription is activated through means other than a renewal (such as an admin action, CLI script, or a subscription status change via manual user action in My Account).

Since the access check queue is only processed once per hour, there's a small chance a reader could unsubscribe from a restricted list after a renewal starts but before their scheduled access check occurs. In this case they'll be added back to the list when the queue is processed. But the chance that an intentional unsubscribe happens in this interim period is pretty low.

Also, caches the premium newsletter gates when processing the queue so we don't perform more post queries than we need to for a large queue of users.

How to test the changes in this Pull Request:

1. In Newsletters > Premium make sure Auto-signup is enabled in Advanced Settings. Set up and activate at least one premium newsletter gate targeting more than one restricted list.
2. As a reader, purchase the required subscription.
3. Trigger the initial auto-signup by running the scheduled queue processing: wp cron event run newspack_premium_newsletters_access_check
4. After the cron job finishes, confirm that the reader is automatically added to the restricted lists.
5. Remove the reader from one of the restricted lists (you can do this directly in the ESP, or by clicking on the unsubscribe link in an email delivered to the reader).
6. As an admin, trigger a renewal for the reader's subscription.
7. After the renewal completes, trigger the cron job to process the queue again.
8. After the cron job finishes, confirm that the reader was NOT automatically re-added to the list they were unsubscribed from at the time the renewal started.
9. As an admin, manually update the reader's subscription to "on hold" status and then back to "active" status.
10. Run the cron job again and confirm that the reader is automatically re-added to all restricted lists.

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 ran tests with your changes locally?

[Copilot]

Pull request overview

This PR refines Premium Newsletters’ auto-signup behavior during subscription renewals by snapshotting the reader’s current newsletter subscriptions at renewal start and using that snapshot to limit which restricted lists can be auto-added during the subsequent access check.

Changes:

• Cache the reader’s subscribed newsletter lists to user meta when a renewal attempt begins.
• Gate auto-signup during access checks so only lists present in that cached snapshot can be re-added, then clear the snapshot meta.
• Add a small in-request cache for premium newsletter gates (get_gates()).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[adekbadek]

Nice, tightly scoped change. The renewal-snapshot approach fits the stated problem cleanly and the new tests cover the happy paths well. I want to flag one correctness concern that I think undermines the fix on retries (see inline on the delete_user_meta call) plus a design question about unrelated events inheriting the snapshot. Everything else is suggestion/nit.

One out-of-scope observation while I was here: process_access_check_queue() doesn't wrap the per-user check_access() in a try/catch, so a single bad user (e.g. a deleted list post referenced from the restriction rules) throws out of the whole pass and aborts the batch. Pre-existing, but combined with the blocker below it makes the retry contract fragile — worth a follow-up to catch per user and log.

[adekbadek]

Blocker: this delete_user_meta() runs unconditionally before add_and_remove_lists() on line 250. The class docblock on process_access_check_queue() (:302-304) explicitly relies on queue-persists-on-error so failed entries are retried on the next cron tick. But if add_and_remove_lists() fails (AC 5xx, network timeout, auth error, partial response), the snapshot is already gone — the retry then sees "no meta = add all" and re-adds the list the reader unsubscribed from, which is exactly the regression this PR is trying to prevent.

Suggest moving the delete after a successful add_and_remove_lists(), e.g. guarded by a success flag or a try { ... } finally {} that only deletes on the success branch. Worth a companion test that injects a throwing provider and asserts the meta still exists afterwards.

[dkoo]

Good catch—added the try / catch block in 014ffce.

[adekbadek]

Design question: between set_subscribed_lists() firing on renewal and the next cron run, any other event that enqueues the same user (reader_verified, product_subscription_changed, domain whitelist verification, etc.) will also read this snapshot and silently suppress auto-signup of lists the user dropped — changing the semantics of those unrelated flows. E.g. test_reader_verified_grants_list_access_for_whitelisted_domain would behave differently for any user who had a recent renewal.

Worth considering tagging queue entries with the triggering event, or adding an expiry to the meta, so the snapshot only governs the renewal-triggered check. Not a blocker, but the coupling feels load-bearing enough to raise.

[dkoo]

Good call — went with the tagging approach in c615c89. Each queue entry is now [ 'user_id', 'source' ], with SOURCE_RENEWAL reserved for entries enqueued by set_subscribed_lists() itself (made it self-enqueue rather than rely on product_subscription_changed firing alongside). Per-event wrappers handle_product_subscription_changed, handle_donation_subscription_changed, handle_reader_verified tag their respective sources.

check_access() only reads/clears the snapshot when source === SOURCE_RENEWAL. So reader_verified (or any other event) hitting a user with a stale snapshot now does normal auto-signup and leaves the snapshot intact for any subsequent renewal-source check.

Dedup logic in add_user_to_queue() prefers renewal: a renewal-source enqueue overrides any non-renewal entry for the same user, but a non-renewal source never downgrades an existing renewal entry. That keeps the renewal semantics intact when subscription_renewal_attempt and product_subscription_changed both fire in the same window.

Backward compat: queue entries already in flight at deploy are bare ints; a normalize_queue_entry() helper reads both shapes so the rollover is safe.

New test test_non_renewal_event_does_not_consult_snapshot plants a stale snapshot, fires handle_reader_verified, and asserts (a) the user gets auto-subscribed, and (b) the snapshot survives.

[adekbadek]

Nit: the stored payload is the full contact-list set from get_contact_lists(), not just the restricted lists. Behavior is still correct because check_access only compares against restricted lists, but the PR title and class docblock read as if this is a restricted-only snapshot. Either filter to restricted public IDs at snapshot time, or update the docblock/meta-key name to say "the contact's full ESP list membership at renewal time."

[dkoo]

Addressed in 014ffce.

[adekbadek]

Minor perf: self::get_public_id( $list_id ) instantiates a Subscription_List per restricted list per user per cron run. Given the PR's secondary goal of reducing repeated queries, consider hoisting a $restricted_list_public_ids map once before the foreach.

Also worth noting: get_public_id() can return null, and in_array( null, $subscribed_lists, true ) silently drops the list from lists_to_add. A comment (or an explicit guard + log) would make that behavior intentional rather than incidental.

[dkoo]

Addressed in 014ffce.

[adekbadek]

Test gap suggestions:

1. Failure retry: add a test that stubs a throwing/erroring add_and_remove_lists (e.g. via a mock provider) and asserts SUBSCRIBED_LISTS_META_KEY is still present after check_access — this is the test that would have caught the blocker I flagged on class-premium-newsletters.php:248.
2. auto_signup = 0 path: set_subscribed_lists() early-returns when the option is off. A one-liner test that confirms nothing is written in that case prevents regressions if the guard is touched later.
3. test_renewal_readds_user_who_remained_subscribed zeroes out $contact_lists mid-test to avoid the dedup filter. The inline comment explains why, but the scenario it simulates ("ESP drops the user between renewal and cron") is narrower than the name suggests — consider renaming or splitting.

[dkoo]

Addressed in 014ffce.

[dkoo]

Thanks, @adekbadek—addressed your feedback!

[dkoo]

Re: the out-of-scope observation about process_access_check_queue() aborting the batch on a single bad user — folded the fix into c615c89 alongside the source-tagging work since the two interact (per-user try/catch + log means a transient ESP failure clears the queue entry but the renewal snapshot survives, so the next renewal-source enqueue still respects the unsubscribe).

Each entry now runs in its own try { … } catch ( \Throwable $e ) { Logger::log( … ); }. Queue is still cleared after the loop, so a permanently-bad entry (e.g. deleted list post in the restriction rules) won't loop forever — it logs once and moves on.

Covered by test_process_queue_continues_after_per_user_exception: enqueues two users, makes the first call into the contacts API throw, asserts both users were attempted and the queue is empty afterwards.