feat(reader-activation): replace RAS auto-enable with manual toggle#196
feat(reader-activation): replace RAS auto-enable with manual toggle#196miguelpeixe wants to merge 12 commits into
Conversation
…onal state updates for better state management
miguelpeixe
force-pushed
the
nppd-1066-allow-ras-to-be-active-with-less-requisites
branch
from
15fa83a to
48cadff
Compare
chickenn00dle
left a comment
chickenn00dle
left a comment
There was a problem hiding this comment.
Some review notes from a pass over the diff — all non-blocking (no blockers found). 👍
The skip-subsystem / auto-enable removal looks clean (no leftover callers anywhere in the repo), the REST permission/validation/sanitization checks are intact, and CI is green. The inline comments below are 4 suggestions + a couple of nits — edge-case hardening and polish.
One non-code note: the PR description lists NEWSPACK_INTEGRATIONS_SETTINGS_ENABLED / Audience_Integrations::is_enabled() as "new", but they already exist on main — this PR wires up their consumption, so a one-line description tweak might save a future reader some confusion.
(Still doing a manual test pass — will follow up after.)
| /> | ||
| <Route path="/content-gating" render={ () => <ContentGating { ...props } /> } /> | ||
| <Route path="/payment" render={ () => <Payment { ...props } /> } /> | ||
| <Route path="/campaign" render={ () => <Campaign { ...props } /> } /> |
There was a problem hiding this comment.
💡 Suggestion — With auto-enable gone, api_activate_reader_activation no longer re-asserts enabled = true, and these routes (/campaign, /complete) render unconditionally — they don't check config.enabled/chooserOpen. So a site with Audience Management disabled that lands directly on #/campaign or #/complete could publish a campaign while RAS stays disabled. Very unlikely in the normal flow, but the old code was defensively idempotent here. Might be worth gating these routes on config.enabled (or re-asserting enabled on activate).
| export const PLATFORM_PLUGINS = { | ||
| [ NEWSPACK ]: [ 'woocommerce', 'woocommerce-subscriptions', 'newspack-blocks' ], | ||
| [ NRH ]: [ 'newspack-blocks' ], | ||
| [ OTHER ]: [], |
There was a problem hiding this comment.
💡 Suggestion — Selecting Other auto-installs nothing here, but Reader_Activation::get_reader_revenue_required_plugins() still always lists newspack-blocks as required. So an "Other" site with newspack-blocks inactive would see a "recommended plugin" on the config page that the chooser never offered to install. Impact is basically nil (newspack-blocks is effectively always active), but the chooser and Setup disagree slightly — either add newspack-blocks to the OTHER set, or a quick comment noting Setup's recommendations are intentionally broader than the chooser's auto-install set.
| @@ -0,0 +1,17 @@ | |||
| import { OPTIONS, PLATFORM_PLUGINS } from './'; | |||
|
|
|||
| describe( 'PlatformSelection mapping', () => { | |||
There was a problem hiding this comment.
💡 Suggestion — This only asserts the static OPTIONS/PLATFORM_PLUGINS mappings — none of the component's actual behavior: the save-then-advance flow, the "don't advance if the save failed" guard, the non-blocking installer "Continue" path, or the disable-confirmation modal. For a ~190-line component that's the centerpiece of this feature, a render/interaction test (mocking the save + PluginInstaller.onStatus) would protect the riskiest new logic. Not blocking — the PHP side is nicely covered by contrast. 🙂
| $params = $request->get_params(); | ||
|
|
||
| Donations::set_platform_slug( $params['platform'] ); | ||
| if ( isset( $params['platform'] ) ) { |
There was a problem hiding this comment.
💡 Suggestion — Nice defensive guard. One thing to consider: platform isn't marked 'required' => true on this route, so a POST without it now silently returns the current payment data with a 200 and no change. That's fine for the NRH-settings-only path, but a dropped platform param becomes an invisible no-op — might be worth 'required' => true (or splitting the NRH-only update into its own concern).
🔸 Minor, while you're here: the method's docblock still reads @return WP_REST_Response Boolean success, but it actually returns the payment-data array via rest_ensure_response().
| * @return bool | ||
| */ | ||
| public static function is_platform_selected() { | ||
| return null !== get_option( self::NEWSPACK_READER_REVENUE_PLATFORM, null ); |
There was a problem hiding this comment.
🔸 Nit — This is the load-bearing first-run detection, and it's coupled to a side effect: get_platform_slug() writes this option during the legacy stripe → wc migration, so just reading the platform on a legacy site flips is_platform_selected() from false to true. It's documented and intentional (migrated sites are treated as "Newspack selected"), so no change needed — just flagging that a read-with-side-effect feeding first-run detection is a touch fragile if get_platform_slug() ever moves earlier in the request lifecycle.
| @@ -693,7 +642,7 @@ public function api_update_salesforce_settings( $request ) { | |||
| * @return bool | |||
| */ | |||
| public function api_validate_platform( $value ) { | |||
There was a problem hiding this comment.
🔸 Nit — You tightened the validation here nicely (strict in_array). Newer methods in this class use typed signatures/returns (e.g. api_update_checkout_configuration( WP_REST_Request $request ): WP_REST_Response), whereas this and Donations::is_platform_selected() stay untyped. Given the 8.3 target, api_validate_platform( mixed $value ): bool and is_platform_selected(): bool would read a touch more consistently. Totally optional.
There was a problem hiding this comment.
Looks great @miguelpeixe!
This is good to ship as is but I left some inline nits from my Claude review session.
One other non-blocking thought is it feels strange to render a warning for optional Audience Management settings:
Maybe we can make this a simple heading instead. Either that or reintroduce the skip button so the publisher can indicate they intentionally plan to ignore that setting so we can remove the warning once all settings are touched. WDYT?
github-actions
Bot
added
[Status] Approved
2 participants
All Submissions:
Changes proposed in this Pull Request:
Closes NPPD-1066.
Reworks Audience Management (RAS) setup so it can be enabled with far fewer prerequisites, and reorganizes the wizard around an explicit reader‑revenue platform selection. RAS no longer auto‑enables based on a fully‑configured plugin stack; an administrator picks a platform, and Audience Management is enabled as part of that choice, regardless of which optional plugins are installed.
Platform selection (first‑run step)
Enable / disable
Tabs
Configuration page cleanup
Plugin independence
Reader_Activation::skip()/is_skipped()/is_ras_ready_to_configure(), the/audience-management/skipREST route, theSKIP_CAMPAIGN_SETUP_OPTIONconstant, and theis_skipped_campaign_setuplocalized field.Reader_Activation::is_reader_revenue_ready()helper (only consumed by the removed Reader Revenue prerequisite) and other dead code (an unuseddonation_settingsread inDonations::remove_donations_from_cart(), the never‑setis_unavailableprerequisite branch) are removed.Reader_Activation::setup_nav_menu()no longer requires WooCommerce for the anonymous Sign In link.Integrations Settings flag (separate concern, included here)
NEWSPACK_INTEGRATIONS_SETTINGS_ENABLEDconstant is defined and truthy (the upcoming dedicated Integrations admin area), the Sync contacts to ESP ActionCard on the Audience configuration page is hidden — including its per‑ESP settings (Mailchimp / ActiveCampaign / Constant Contact), the "Sync user account deletion" checkbox, and the metadata fields. The save‑time validation that alerts on empty audience / master‑list IDs is suppressed in that mode so a previously‑savedsync_esp = truecan't trap users on an alert for a section they can no longer see. The "ESP Advanced Settings" section header and the "Newsletter subscription text on registration" control above remain visible because they aren't about ESP sync.Migration
enabled = true. The legacynewspack_reader_activation_*_skippedoptions are left in the database, intentionally unread.'stripe'platform to'wc'is preserved inDonations::get_platform_slug(). Sites that previously stored'stripe'will surface as platform‑selected (wc) on first run under the new chooser — they will not see the chooser unless Audience Management is explicitly disabled.How to test the changes in this Pull Request:
Other information: