feat(ras): add flag to setup CLI command to skip default campaign setup #3132
Conversation
dkoo
added
[Status] Needs Review
| WP_CLI::error( __( 'Newspack Newsletters provider not set.', 'newspack-plugin' ) ); | ||
| } | ||
|
|
||
| $result = \Newspack_Popups_Presets::activate_ras_presets(); | ||
|
|
||
| $result = $skip_campaigns ? Reader_Activation::update_setting( 'enabled', true ) : \Newspack_Popups_Presets::activate_ras_presets(); |
There was a problem hiding this comment.
This CLI command should always enable RAS, regardless of the skip-campaigns flag.
| sprintf( | ||
| // Translators: 'with' or 'without' default prompts. | ||
| __( 'RAS enabled %s default prompts.', 'newspack-plugin' ), | ||
| $skip_campaigns ? 'without' : 'with' |
There was a problem hiding this comment.
"with" and "without" are should be translatable:
| $skip_campaigns ? 'without' : 'with' | |
| $skip_campaigns ? __('without', 'newspack-plugin') : __('with', 'newspack-plugin') |
However, this might be making too many assumptions about language structure. I think a safer bet would be to have two separate no-placeholder strings instead of a conditional placeholder value.
| if ( Reader_Activation::is_ras_campaign_configured() ) { | ||
| public static function cli_setup_ras( $args, $assoc_args ) { | ||
| $skip_campaigns = ! empty( $assoc_args['skip-campaigns'] ); | ||
| if ( ! $skip_campaigns && Reader_Activation::is_ras_campaign_configured() ) { | ||
| WP_CLI::error( __( 'RAS is already configured for this site.', 'newspack-plugin' ) ); |
There was a problem hiding this comment.
This should not error out, but only warn about the campaigns being already configured. See comment below – this CLI command should always allow to enable RAS.
| } | ||
|
|
||
| if ( \is_wp_error( \Newspack_Newsletters_Subscription::get_lists() ) ) { | ||
| if ( ! $skip_campaigns && \is_wp_error( \Newspack_Newsletters_Subscription::get_lists() ) ) { | ||
| WP_CLI::error( __( 'Newspack Newsletters provider not set.', 'newspack-plugin' ) ); |
There was a problem hiding this comment.
The error message should rather inform about the inability to get lists, instead of making an assumption about how get_lists relates to a provider being set or not.
| } | ||
|
|
||
| if ( \is_wp_error( \Newspack_Newsletters_Subscription::get_lists() ) ) { | ||
| if ( ! $skip_campaigns && \is_wp_error( \Newspack_Newsletters_Subscription::get_lists() ) ) { |
There was a problem hiding this comment.
Why should $skip_campaigns factor in to this check? AFAIK Newsletters should always be set up properly for RAS to fully work.
adekbadek
added
[Status] Needs changes or feedback
All Submissions:
Changes proposed in this Pull Request:
#3051 added the option to skip Campaigns default prompt + segment setup during RAS onboarding. This adds the same option to the
wp newspack ras setupCLI command. It also tweaks that command so that if a required plugin isn't installed, the script will just install it instead of failing with an error.How to test the changes in this Pull Request:
wp newspack setupto complete the initial site setup.wp newspack ras setup --skip-campaigns. After it completes, visit Newspack > Engagement > Reader Activation in the admin and confirm that it's enabled and that you can view/edit advanced settings despite skipping the prerequisites.Other information: