fix(woocommerce): scope payment notice to recoverable statuses#301
Conversation
There was a problem hiding this comment.
Pull request overview
This PR narrows when the WooCommerce “update payment method” notice is generated by gating it to recoverable subscription statuses only, preventing notices on terminal or otherwise non-actionable subscriptions (e.g., expired subscriptions with stale unpaid renewal orders).
Changes:
- Added an explicit subscription-status allowlist (
on-hold,pending) for when the “needs payment” notice is actionable. - Added unit tests covering excluded statuses, the reported expired+stale-order incident, and regression guards for
on-hold/pending. - Extended WooCommerce test mocks to support the new notice code paths (
needs_payment(),get_view_order_url(), andwcs_get_subscriptions()).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| plugins/newspack-plugin/includes/plugins/woocommerce/class-woocommerce-update-payment-notice.php | Adds a recoverable-status allowlist and uses it to gate notice generation. |
| plugins/newspack-plugin/tests/unit-tests/plugins/woocommerce-update-payment-notice.php | Introduces a focused unit test suite for the allowlist/status behavior and incident regression. |
| plugins/newspack-plugin/tests/mocks/wc-mocks.php | Updates WC/WCS mocks to support wcs_get_subscriptions() and subscription methods used by the notice logic. |
|
@jason10lee before I review, should this be based against |
You're right--performing a bit of branch surgery now! |
jason10lee
force-pushed
the
hotfix/nppm-2926-payment-notice-status-allowlist
branch
from
c25f0c8 to
cc4c6e8
Compare
Surgery all done--looks to be a success. Please go ahead, @dkoo, and thanks! |
dkoo
left a comment
dkoo
left a comment
There was a problem hiding this comment.
Clean, well-scoped hotfix. Replacing the cancelled-only skip with a positive on-hold/pending allowlist is the right fix for NPPM-2926: an expired subscription carrying a stale unpaid renewal order no longer trips the “update your payment method” notice, while genuinely recoverable subscriptions still nag. The recoverable set matches the Newspack dunning lifecycle (failed payment → on-hold → expired), has_status( array ) is correct WCS usage, and the new gate composes cleanly with the downstream needs_payment() and same-product suppression checks. Test coverage genuinely exercises the production gate (flipping the allowlist would flip the assertions), the suite passes (10 tests), and host PHPCS is clean.
Verified at runtime against real WooCommerce Subscriptions in an isolated env: a reader holding an expired sub with a stale unpaid failed-renewal order (needs_payment() → true — the exact incident) no longer sees the notice, while on-hold (failed renewal) and pending (initial payment) subs still do. One optional forward-looking note below; nothing blocking.
github-actions
Bot
added
[Status] Approved
|
Hey @jason10lee, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
## newspack [6.43.3](https://github.com/Automattic/newspack-workspace/compare/newspack@6.43.2...newspack@6.43.3) (2026-06-19) ### Bug Fixes * **woocommerce:** scope payment notice to recoverable statuses ([#301](#301)) ([1f55c11](1f55c11))
All Submissions:
Changes proposed in this Pull Request:
Users continued to see the My Account / snackbar "Your '…' subscription is not active. Please update your payment method." notice following previous efforts to limit where it appeared.
This PR further limits that notice to subscriptions in a status where paying can actually restore them —
on-hold(failed renewal) andpending(awaiting initial payment).Previously we skipped only
cancelleditems and otherwise trustedneeds_payment(). An expired (or otherwise terminal) subscription that still carries a stale, unpaid failed-renewal order tripsneeds_payment(), so finished subscriptions kept firing a notice the reader had no way to resolve — telling active subscribers they were "not active."This replaces the
cancelled-only skip with an explicit recoverable-status allowlist.expired/cancelled/switched(terminal) andactive/pending-cancel(reader still has access) are intentionally excluded. A guard test pins the allowlist against the known WooCommerce Subscriptions status set so any future/custom status surfaces for a human decision rather than being silently skipped.Scope: This is a hotfix that fully resolves the reported incident. A broader preventative adjustment that will suppress the notice when an active subscription grants the same membership through a different product (the "same product was too narrow" follow-up) will ship separately on our typical release cadence and is not included here.
Closes NPPM-2926: Update Payment Notice still fires for subscribers holding both active and expired subscriptions.
How to test the changes in this Pull Request:
on-holdwith a payment due → the notice does appear with an "update your payment method" link.pending(initial payment not completed) also still shows the notice.plugins/newspack-plugin, runn test-php --group update_payment_notice→ OK (10 tests). The suite covers each excluded status (incl. the expired-with-stale-order incident), theon-hold/pendingfire paths, the not-needs-payment case, and the allowlist-vs-WCS-status-registry guard.Other information:
Cross-publisher impact was reviewed (reader-revenue / third-party-integrations surface): no Newspack repo references this class, the new constant, or the touched filter; the change only ever narrows when the notice shows, so there is no silent-breakage vector. Production diff is one new constant plus the allowlist gate; the rest is tests + test mocks.