From b580174ad6e36b68f661ab86209aac220a5a885f Mon Sep 17 00:00:00 2001 From: Tanner Record Date: Thu, 28 Sep 2023 13:36:05 -0400 Subject: [PATCH 1/6] Refactor to remove "Mixed" status --- src/Telemetry/Opt_In/Status.php | 58 ++++------- tests/wpunit/StatusTest.php | 171 ++++++++++++++++++++++++++------ 2 files changed, 157 insertions(+), 72 deletions(-) diff --git a/src/Telemetry/Opt_In/Status.php b/src/Telemetry/Opt_In/Status.php index 4347b88..bae1d11 100644 --- a/src/Telemetry/Opt_In/Status.php +++ b/src/Telemetry/Opt_In/Status.php @@ -22,9 +22,6 @@ class Status { public const OPTION_NAME = 'stellarwp_telemetry'; public const OPTION_NAME_USER_INFO = 'stellarwp_telemetry_user_info'; - public const STATUS_ACTIVE = 1; - public const STATUS_INACTIVE = 2; - public const STATUS_MIXED = 3; /** * Gets the option name used to store the opt-in status. @@ -67,44 +64,38 @@ public function get_option() { * * @since 1.0.0 * @since 2.0.1 Correct logic so it is not subject to the order of the plugins. + * @since TBD Update to remove unnecessary "mixed" status. * - * @return integer The status value. + * @return bool The site's current opt in status. */ public function get() { - $option = $this->get_option(); - // If the status option is not an option, default to inactive. - if ( ! isset( $option['plugins'] ) ) { - return self::STATUS_INACTIVE; - } + $status = false; + $option = $this->get_option(); + $plugins = isset( $option['plugins'] ) ? $option['plugins'] : []; - $status = array_reduce( - $option['plugins'], - function( $carry, $item ) { - // First run, ignore the default STATUS_ACTIVE. - if ( empty( $carry ) ) { - return (int) $item['optin']; - } + if ( ! isset( $plugins ) || count( $plugins ) === 0 ) { + $status = false; + } - // As long as they are the same, we keep returning the same. - if ( $carry === $item['optin'] ) { - return (int) $item['optin']; - } + foreach ( $plugins as $plugin ) { - return self::STATUS_MIXED; + // If any plugins are missing an optin status or at least one is false, set status to false. + if ( ! isset( $plugin['optin'] ) || false === $plugin['optin'] ) { + $status = false; + break; } - ); - if ( 0 === $status ) { - $status = self::STATUS_INACTIVE; + $status = true; } /** * Filters the opt-in status value. * * @since 1.0.0 + * @since TBD Update to return a bool status. * - * @param integer $status The opt-in status value. + * @param bool $status If the site is currently opted in. */ return apply_filters( 'stellarwp/telemetry/' . Config::get_hook_prefix() . 'optin_status', $status ); } @@ -234,7 +225,7 @@ public function get_opted_in_plugins() { * @since 1.0.0 * @since 2.0.0 - Updated to allow defined stellar_slug. * - * @param boolean $status The status to set (Active = 1, Inactive = 2, Mixed = 3). + * @param boolean $status The status to set. * @param string $stellar_slug The stellar_slug to set the status of. * * @return boolean @@ -260,19 +251,8 @@ public function set_status( bool $status, string $stellar_slug = '' ) { * @return string */ public function get_status() { - $optin_label = ''; - switch ( $this->get() ) { - case self::STATUS_ACTIVE: - $optin_label = __( 'Active', 'stellarwp-telemetry' ); - break; - case self::STATUS_INACTIVE: - $optin_label = __( 'Inactive', 'stellarwp-telemetry' ); - break; - case self::STATUS_MIXED: - $optin_label = __( 'Mixed', 'stellarwp-telemetry' ); - break; - } + $optin_label = $this->get() ? esc_html__( 'Active', 'stellarwp-telemetry' ) : esc_html__( 'Inactive', 'stellarwp-telemetry' ); /** * Filters the opt-in status label. @@ -292,6 +272,6 @@ public function get_status() { * @return boolean */ public function is_active(): bool { - return $this->get() === self::STATUS_ACTIVE; + return $this->get(); } } diff --git a/tests/wpunit/StatusTest.php b/tests/wpunit/StatusTest.php index 06de994..830fd08 100644 --- a/tests/wpunit/StatusTest.php +++ b/tests/wpunit/StatusTest.php @@ -1,10 +1,7 @@ 'acme-commerce', - 'version' => '1.2.3' - ] - ] + 'version' => '1.2.3', + ], + ], ], 'one plugin is missing opt-in key' => [ [ @@ -96,9 +93,9 @@ public function get_opted_in_plugins_data_provider(): array { [ [ 'slug' => 'acme-commerce', - 'version' => '1.2.3' - ] - ] + 'version' => '1.2.3', + ], + ], ], 'not all plugins opt-in' => [ [ @@ -120,12 +117,12 @@ public function get_opted_in_plugins_data_provider(): array { [ [ 'slug' => 'acme-commerce', - 'version' => '1.2.3' + 'version' => '1.2.3', ], [ 'slug' => 'acme-learn', - 'version' => '5.6.7' - ] + 'version' => '5.6.7', + ], ], ], 'all plugins opt-out' => [ @@ -167,16 +164,16 @@ public function get_opted_in_plugins_data_provider(): array { [ [ 'slug' => 'acme-commerce', - 'version' => '1.2.3' + 'version' => '1.2.3', ], [ 'slug' => 'acme-tickets', - 'version' => '3.4.5' + 'version' => '3.4.5', ], [ 'slug' => 'acme-learn', - 'version' => '5.6.7' - ] + 'version' => '5.6.7', + ], ], ], ]; @@ -186,26 +183,30 @@ public function get_opted_in_plugins_data_provider(): array { * @dataProvider get_opted_in_plugins_data_provider */ public function test_get_opted_in_plugins( $option_value, $expected ): void { - $this->set_fn_return( 'get_plugin_data', static function ( string $plugin ) { - if ( strpos( $plugin, 'acme-commerce', true ) ) { - return [ - 'Name' => 'Acme Commerce', - 'Version' => '1.2.3', - ]; - } + $this->set_fn_return( + 'get_plugin_data', + static function ( string $plugin ) { + if ( strpos( $plugin, 'acme-commerce', true ) ) { + return [ + 'Name' => 'Acme Commerce', + 'Version' => '1.2.3', + ]; + } + + if ( strpos( $plugin, 'acme-tickets', true ) ) { + return [ + 'Name' => 'Acme Tickets', + 'Version' => '3.4.5', + ]; + } - if ( strpos( $plugin, 'acme-tickets', true ) ) { return [ - 'Name' => 'Acme Tickets', - 'Version' => '3.4.5', + 'Name' => 'Acme Learn', + 'Version' => '5.6.7', ]; - } - - return [ - 'Name' => 'Acme Learn', - 'Version' => '5.6.7', - ]; - }, true ); + }, + true + ); $status = new Status(); update_option( $status->get_option_name(), $option_value ); @@ -213,4 +214,108 @@ public function test_get_opted_in_plugins( $option_value, $expected ): void { $this->assertIsArray( $status->get_opted_in_plugins() ); $this->assertEquals( $expected, $status->get_opted_in_plugins() ); } + + public function get_status_data_provider(): array { + return [ + 'empty' => [ [], false ], + 'missing plugins key' => [ [ 'token' => 'foo' ], false ], + 'empty plugins' => [ [ 'plugins' => [] ], false ], + 'one plugin is missing opt-in key' => [ + [ + 'plugins' => [ + 'acme-commerce' => [ + 'wp_slug' => 'acme-commerce/acme-commerce.php', + 'optin' => true, + ], + 'acme-tickets' => [ + 'wp_slug' => 'acme-tickets/acme-tickets.php', + ], + ], + ], + false, + ], + 'not all plugins opt-in' => [ + [ + 'plugins' => [ + 'acme-commerce' => [ + 'wp_slug' => 'acme-commerce/acme-commerce.php', + 'optin' => true, + ], + 'acme-tickets' => [ + 'wp_slug' => 'acme-tickets/acme-tickets.php', + 'optin' => false, + ], + 'acme-learn' => [ + 'wp_slug' => 'acme-learn/acme-learn.php', + 'optin' => true, + ], + ], + ], + false, + ], + 'all plugins opt-out' => [ + [ + 'plugins' => [ + 'acme-commerce' => [ + 'wp_slug' => 'acme-commerce/acme-commerce.php', + 'optin' => false, + ], + 'acme-tickets' => [ + 'wp_slug' => 'acme-tickets/acme-tickets.php', + 'optin' => false, + ], + 'acme-learn' => [ + 'wp_slug' => 'acme-learn/acme-learn.php', + 'optin' => false, + ], + ], + ], + false, + ], + 'all plugins opt-in' => [ + [ + 'plugins' => [ + 'acme-commerce' => [ + 'wp_slug' => 'acme-commerce/acme-commerce.php', + 'optin' => true, + ], + 'acme-tickets' => [ + 'wp_slug' => 'acme-tickets/acme-tickets.php', + 'optin' => true, + ], + 'acme-learn' => [ + 'wp_slug' => 'acme-learn/acme-learn.php', + 'optin' => true, + ], + ], + ], + true, + ], + ]; + } + + /** + * @dataProvider get_status_data_provider + */ + public function test_get_status( $option_value, $expected ): void { + $status = new Status(); + + update_option( $status->get_option_name(), $option_value ); + + $this->assertIsBool( $status->get() ); + $this->assertEquals( $expected, $status->get() ); + } + + /** + * @dataProvider get_status_data_provider + */ + public function test_get_status_label( $option_value, $expected ): void { + $status = new Status(); + + update_option( $status->get_option_name(), $option_value ); + + $label = $expected ? 'Active' : 'Inactive'; + + $this->assertEquals( $label, $status->get_status() ); + } } From 2c938c376eaad016900381cfaa5bce8facca4245 Mon Sep 17 00:00:00 2001 From: Tanner Record Date: Thu, 28 Sep 2023 13:41:37 -0400 Subject: [PATCH 2/6] Fix phpstan issue --- src/Telemetry/Opt_In/Status.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Telemetry/Opt_In/Status.php b/src/Telemetry/Opt_In/Status.php index bae1d11..fc77cb9 100644 --- a/src/Telemetry/Opt_In/Status.php +++ b/src/Telemetry/Opt_In/Status.php @@ -74,7 +74,7 @@ public function get() { $option = $this->get_option(); $plugins = isset( $option['plugins'] ) ? $option['plugins'] : []; - if ( ! isset( $plugins ) || count( $plugins ) === 0 ) { + if ( count( $plugins ) === 0 ) { $status = false; } From 3c366ac493ecde3e18db1e59ebfc78fde0d0561b Mon Sep 17 00:00:00 2001 From: Tanner Record Date: Fri, 29 Sep 2023 14:54:55 -0400 Subject: [PATCH 3/6] Revert to use integer values for status We can't change the function signature to use a bool return type because it could break existing implementations out in the wild. --- src/Telemetry/Opt_In/Status.php | 29 +++++++++++++++++++---------- tests/wpunit/StatusTest.php | 18 +++++++++--------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/Telemetry/Opt_In/Status.php b/src/Telemetry/Opt_In/Status.php index fc77cb9..54ee5c2 100644 --- a/src/Telemetry/Opt_In/Status.php +++ b/src/Telemetry/Opt_In/Status.php @@ -22,6 +22,8 @@ class Status { public const OPTION_NAME = 'stellarwp_telemetry'; public const OPTION_NAME_USER_INFO = 'stellarwp_telemetry_user_info'; + public const STATUS_ACTIVE = 1; + public const STATUS_INACTIVE = 2; /** * Gets the option name used to store the opt-in status. @@ -60,42 +62,40 @@ public function get_option() { * The status is stored as an integer because there are multiple possible statuses: * 1 = Active * 2 = Inactive - * 3 = Mixed * * @since 1.0.0 * @since 2.0.1 Correct logic so it is not subject to the order of the plugins. * @since TBD Update to remove unnecessary "mixed" status. * - * @return bool The site's current opt in status. + * @return integer The status value. */ public function get() { - $status = false; + $status = self::STATUS_INACTIVE; $option = $this->get_option(); $plugins = isset( $option['plugins'] ) ? $option['plugins'] : []; if ( count( $plugins ) === 0 ) { - $status = false; + $status = self::STATUS_INACTIVE; } foreach ( $plugins as $plugin ) { // If any plugins are missing an optin status or at least one is false, set status to false. if ( ! isset( $plugin['optin'] ) || false === $plugin['optin'] ) { - $status = false; + $status = self::STATUS_INACTIVE; break; } - $status = true; + $status = self::STATUS_ACTIVE; } /** * Filters the opt-in status value. * * @since 1.0.0 - * @since TBD Update to return a bool status. * - * @param bool $status If the site is currently opted in. + * @param integer $status The opt-in status value. */ return apply_filters( 'stellarwp/telemetry/' . Config::get_hook_prefix() . 'optin_status', $status ); } @@ -252,7 +252,16 @@ public function set_status( bool $status, string $stellar_slug = '' ) { */ public function get_status() { - $optin_label = $this->get() ? esc_html__( 'Active', 'stellarwp-telemetry' ) : esc_html__( 'Inactive', 'stellarwp-telemetry' ); + $optin_label = ''; + + switch ( $this->get() ) { + case 1: + $optin_label = esc_html__( 'Active', 'stellarwp-telemetry' ); + break; + case 2: + $optin_label = esc_html__( 'Inactive', 'stellarwp-telemetry' ); + break; + } /** * Filters the opt-in status label. @@ -272,6 +281,6 @@ public function get_status() { * @return boolean */ public function is_active(): bool { - return $this->get(); + return $this->get() === self::STATUS_ACTIVE; } } diff --git a/tests/wpunit/StatusTest.php b/tests/wpunit/StatusTest.php index 830fd08..81774b8 100644 --- a/tests/wpunit/StatusTest.php +++ b/tests/wpunit/StatusTest.php @@ -217,9 +217,9 @@ static function ( string $plugin ) { public function get_status_data_provider(): array { return [ - 'empty' => [ [], false ], - 'missing plugins key' => [ [ 'token' => 'foo' ], false ], - 'empty plugins' => [ [ 'plugins' => [] ], false ], + 'empty' => [ [], 2 ], + 'missing plugins key' => [ [ 'token' => 'foo' ], 2 ], + 'empty plugins' => [ [ 'plugins' => [] ], 2 ], 'one plugin is missing opt-in key' => [ [ 'plugins' => [ @@ -232,7 +232,7 @@ public function get_status_data_provider(): array { ], ], ], - false, + 2, ], 'not all plugins opt-in' => [ [ @@ -251,7 +251,7 @@ public function get_status_data_provider(): array { ], ], ], - false, + 2, ], 'all plugins opt-out' => [ [ @@ -270,7 +270,7 @@ public function get_status_data_provider(): array { ], ], ], - false, + 2, ], 'all plugins opt-in' => [ [ @@ -289,7 +289,7 @@ public function get_status_data_provider(): array { ], ], ], - true, + 1, ], ]; } @@ -302,7 +302,7 @@ public function test_get_status( $option_value, $expected ): void { update_option( $status->get_option_name(), $option_value ); - $this->assertIsBool( $status->get() ); + $this->assertIsInt( $status->get() ); $this->assertEquals( $expected, $status->get() ); } @@ -314,7 +314,7 @@ public function test_get_status_label( $option_value, $expected ): void { update_option( $status->get_option_name(), $option_value ); - $label = $expected ? 'Active' : 'Inactive'; + $label = 1 === $expected ? 'Active' : 'Inactive'; $this->assertEquals( $label, $status->get_status() ); } From 4eefa6927ff3f9efb440d79445cc314c989b7eee Mon Sep 17 00:00:00 2001 From: Tanner Record Date: Fri, 29 Sep 2023 14:56:25 -0400 Subject: [PATCH 4/6] Add another test to check is_active status method --- tests/wpunit/StatusTest.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/wpunit/StatusTest.php b/tests/wpunit/StatusTest.php index 81774b8..b89aee5 100644 --- a/tests/wpunit/StatusTest.php +++ b/tests/wpunit/StatusTest.php @@ -318,4 +318,18 @@ public function test_get_status_label( $option_value, $expected ): void { $this->assertEquals( $label, $status->get_status() ); } + + /** + * @dataProvider get_status_data_provider + */ + public function test_is_active( $option_value, $expected ): void { + $status = new Status(); + + update_option( $status->get_option_name(), $option_value ); + + $is_active = 1 === $expected ? true : false; + + $this->assertIsBool( $status->is_active() ); + $this->assertEquals( $is_active, $status->is_active() ); + } } From da7c09f02fb99c679dffa8985e460e66735da815 Mon Sep 17 00:00:00 2001 From: Tanner Record Date: Mon, 2 Oct 2023 10:30:04 -0400 Subject: [PATCH 5/6] Send filtered opt_in_text with user registration --- src/Telemetry/Opt_In/Opt_In_Subscriber.php | 14 +++++++++++--- src/Telemetry/Telemetry/Telemetry.php | 8 +++++--- src/views/optin.php | 1 + 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/Telemetry/Opt_In/Opt_In_Subscriber.php b/src/Telemetry/Opt_In/Opt_In_Subscriber.php index 3d20fae..a800aa9 100644 --- a/src/Telemetry/Opt_In/Opt_In_Subscriber.php +++ b/src/Telemetry/Opt_In/Opt_In_Subscriber.php @@ -77,9 +77,15 @@ public function set_optin_status() { $stellar_slug = sanitize_text_field( $_POST['stellar_slug'] ); } + $opt_in_text = ''; + + if ( isset( $_POST['opt_in_text'] ) ) { + $opt_in_text = sanitize_text_field( $_POST['opt_in_text'] ); + } + // User agreed to opt-in to Telemetry. if ( 'true' === $_POST['optin-agreed'] ) { - $this->opt_in( $stellar_slug ); + $this->opt_in( $stellar_slug, $opt_in_text ); } // Don't show the opt-in modal again. @@ -134,17 +140,19 @@ public function initialize_optin_option() { * * @since 1.0.0 * @since 2.0.0 - Updated to allow specifying the stellar slug. + * @since TBD - Updated to add opt-in text. * * @param string $stellar_slug The slug to use when opting in. + * @param string $opt_in_text The text displayed to the user when they agreed to opt-in. * * @return void */ - public function opt_in( string $stellar_slug ) { + public function opt_in( string $stellar_slug, string $opt_in_text = '' ) { $this->container->get( Status::class )->set_status( true, $stellar_slug ); try { $this->container->get( Telemetry::class )->register_site(); - $this->container->get( Telemetry::class )->register_user( $stellar_slug ); + $this->container->get( Telemetry::class )->register_user( $stellar_slug, $opt_in_text ); } catch ( \Error $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch // We don't want to throw errors if the server cannot be reached. } diff --git a/src/Telemetry/Telemetry/Telemetry.php b/src/Telemetry/Telemetry/Telemetry.php index 8a4f9a7..34def42 100644 --- a/src/Telemetry/Telemetry/Telemetry.php +++ b/src/Telemetry/Telemetry/Telemetry.php @@ -81,15 +81,16 @@ public function register_site( bool $force = false ) { * @since 2.0.0 - Add support for setting the stellar slug. * * @param string $stellar_slug The slug to pass to the server when registering the site user. + * @param string $opt_in_text The opt-in text displayed to the user when they agreed to share their data. * * @return void */ - public function register_user( string $stellar_slug = '' ) { + public function register_user( string $stellar_slug = '', string $opt_in_text = '' ) { if ( '' === $stellar_slug ) { $stellar_slug = Config::get_stellar_slug(); } - $user_details = $this->get_user_details( $stellar_slug ); + $user_details = $this->get_user_details( $stellar_slug, $opt_in_text ); try { $this->send( $user_details, Config::get_server_url() . '/opt-in', false ); @@ -342,7 +343,7 @@ protected function get_register_site_data() { * * @return array */ - protected function get_user_details( string $stellar_slug = '' ) { + protected function get_user_details( string $stellar_slug = '', string $opt_in_text = '' ) { if ( '' == $stellar_slug ) { $stellar_slug = Config::get_stellar_slug(); } @@ -353,6 +354,7 @@ protected function get_user_details( string $stellar_slug = '' ) { 'name' => $user->display_name, 'email' => $user->user_email, 'plugin_slug' => $stellar_slug, + 'opt_in_text' => $opt_in_text, ]; /** diff --git a/src/views/optin.php b/src/views/optin.php index 2baabf8..4c9e050 100644 --- a/src/views/optin.php +++ b/src/views/optin.php @@ -56,6 +56,7 @@
+