Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ras): add flag to setup CLI command to skip default campaign setup #3132

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

feat(ras): add flag to setup CLI command to skip default campaign setup #3132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0561344
feat(ras): add flag to setup CLI command to skip default campaign setp
dkoo May 23, 2024
25a1d4d
fix: install plugins even if skipping Campaigns setup
dkoo May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions includes/cli/class-initializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,31 @@ public static function register_comands() {
// Utility commands for managing RAS data via WP CLI.
WP_CLI::add_command(
'newspack ras setup',
[ 'Newspack\CLI\RAS', 'cli_setup_ras' ]
[ 'Newspack\CLI\RAS', 'cli_setup_ras' ],
[
'shortdesc' => 'Enable Reader Activation features.',
'synopsis' => [
[
'type' => 'flag',
'name' => 'skip-campaigns',
'default' => false,
'description' => 'Skip the creation of default campaign prompts and segments.',
'optional' => true,
],
],
]
);

WP_CLI::add_command(
'newspack verify-reader',
[ 'Newspack\CLI\RAS', 'cli_verify_reader' ],
[
'shortdesc' => 'Verify a reader account . ',
'shortdesc' => 'Verify a reader account.',
'synopsis' => [
[
'type' => 'positional',
'name' => 'user',
'description' => 'ID or email of the user account . ',
'description' => 'ID or email of the user account.',
'optional' => false,
'repeating' => false,
],
Expand Down
33 changes: 23 additions & 10 deletions includes/cli/class-ras.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
namespace Newspack\CLI;

use WP_CLI;
use Newspack\Engagement_Wizard;
use Newspack\Plugin_Manager;
use Newspack\Reader_Activation;

defined( 'ABSPATH' ) || exit;
Expand All @@ -17,34 +19,45 @@
*/
class RAS {
/**
* Verify the given user, bypassing the need to complete the email-based verification flow.
* Enable Reader Activation features without having to complete the onboarding wizard.
*
* @param array $args Positional args.
* @param array $assoc_args Associative args.
*/
public static function cli_setup_ras() {
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' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ( ! class_exists( '\Newspack_Popups_Presets' ) ) {
WP_CLI::error( __( 'Newspack Campaigns plugin not found.', 'newspack-plugin' ) );
WP_CLI::warning( __( 'Newspack Campaigns plugin not found. Activating...', 'newspack-plugin' ) );
Plugin_Manager::install( 'newspack-popups' ); // Must be installed before being activated to avoid a fatal.
Plugin_Manager::activate( 'newspack-popups' );
}


if ( ! class_exists( '\Newspack_Newsletters_Subscription' ) ) {
WP_CLI::error( __( 'Newspack Newsletters plugin not found.', 'newspack-plugin' ) );
WP_CLI::warning( __( 'Newspack Newsletters plugin not found. Activating...', 'newspack-plugin' ) );
Plugin_Manager::activate( 'newspack-newsletters' );
}

if ( \is_wp_error( \Newspack_Newsletters_Subscription::get_lists() ) ) {
if ( ! $skip_campaigns && \is_wp_error( \Newspack_Newsletters_Subscription::get_lists() ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should $skip_campaigns factor in to this check? AFAIK Newsletters should always be set up properly for RAS to fully work.

WP_CLI::error( __( 'Newspack Newsletters provider not set.', 'newspack-plugin' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

$result = \Newspack_Popups_Presets::activate_ras_presets();

$result = $skip_campaigns ? Reader_Activation::update_setting( 'enabled', true ) : \Newspack_Popups_Presets::activate_ras_presets();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CLI command should always enable RAS, regardless of the skip-campaigns flag.

image

if ( ! $result ) {
WP_CLI::error( __( 'Something went wrong. Please check for required plugins and try again.', 'newspack-plugin' ) );
exit;
}

WP_CLI::success( __( 'RAS enabled with default prompts.', 'newspack-plugin' ) );
WP_CLI::success(
sprintf(
// Translators: 'with' or 'without' default prompts.
__( 'RAS enabled %s default prompts.', 'newspack-plugin' ),
$skip_campaigns ? 'without' : 'with'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"with" and "without" are should be translatable:

Suggested change
$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.

)
);
}

/**
Expand Down