Commit f28f6569 authored by Jon Mann's avatar Jon Mann Committed by Commit Bot

Messages: Re-enable feature for users that hit crbug/1131140.

This performs a one-time re-enabling of the Messages feature if the PWA
is installed but the feature is in the disabled state.  This also
prevents showing the pairing lost notification if unpaired when
re-enabled.

Bug: 1131140
Change-Id: I1114cab793c9bad4387c6edf886bcbd735fa7ca3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2432109
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Reviewed-by: default avatarAzeem Arshad <azeemarshad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811598}
parent 8fd53893
......@@ -160,6 +160,12 @@ bool AndroidSmsAppManagerImpl::HasAppBeenManuallyUninstalledByUser() {
!setup_controller_->GetPwa(url);
}
bool AndroidSmsAppManagerImpl::IsAppInstalled() {
if (GetInstalledPwaDomain())
return true;
return false;
}
bool AndroidSmsAppManagerImpl::IsAppRegistryReady() {
return pwa_delegate_->IsAppRegistryReady(profile_);
}
......
......@@ -71,6 +71,7 @@ class AndroidSmsAppManagerImpl : public AndroidSmsAppManager {
void SetUpAndLaunchAndroidSmsApp() override;
void TearDownAndroidSmsApp() override;
bool HasAppBeenManuallyUninstalledByUser() override;
bool IsAppInstalled() override;
bool IsAppRegistryReady() override;
void ExecuteOnAppRegistryReady(base::OnceClosure task) override;
......
......@@ -75,10 +75,18 @@ void PairingLostNotifier::HandleMessagesFeatureState() {
.find(multidevice_setup::mojom::Feature::kMessages)
->second;
// If Messages is currently enabled or disabled, the user has completed the
// setup process.
if (state == multidevice_setup::mojom::FeatureState::kDisabledByUser ||
state == multidevice_setup::mojom::FeatureState::kEnabledByUser) {
// If the feature is disabled we should never show any notifications or
// track the pairing state. To avoid showing the notification immediately
// if the feature is ever enabled in the future, the pref should also be
// cleared.
if (state == multidevice_setup::mojom::FeatureState::kDisabledByUser) {
pref_service_->SetBoolean(kWasPreviouslySetUpPrefName, false);
ClosePairingLostNotificationIfVisible();
return;
}
// If Messages is enabled, the user has completed the setup process.
if (state == multidevice_setup::mojom::FeatureState::kEnabledByUser) {
HandleSetUpFeatureState();
return;
}
......
......@@ -9,11 +9,19 @@
#include "chromeos/services/multidevice_setup/host_status_provider.h"
#include "chromeos/services/multidevice_setup/public/cpp/android_sms_app_helper_delegate.h"
#include "chromeos/services/multidevice_setup/public/mojom/multidevice_setup.mojom.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
namespace chromeos {
namespace multidevice_setup {
namespace {
const char kShouldAttemptReenable[] = "android_sms.should_attempt_reenable";
} // namespace
// static
AndroidSmsAppInstallingStatusObserver::Factory*
AndroidSmsAppInstallingStatusObserver::Factory::test_factory_ = nullptr;
......@@ -23,14 +31,15 @@ std::unique_ptr<AndroidSmsAppInstallingStatusObserver>
AndroidSmsAppInstallingStatusObserver::Factory::Create(
HostStatusProvider* host_status_provider,
FeatureStateManager* feature_state_manager,
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate) {
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate,
PrefService* pref_service) {
if (test_factory_) {
test_factory_->CreateInstance(host_status_provider, feature_state_manager,
std::move(android_sms_app_helper_delegate));
}
return base::WrapUnique(new AndroidSmsAppInstallingStatusObserver(
host_status_provider, feature_state_manager,
std::move(android_sms_app_helper_delegate)));
std::move(android_sms_app_helper_delegate), pref_service));
}
// static
......@@ -47,13 +56,21 @@ AndroidSmsAppInstallingStatusObserver::
feature_state_manager_->RemoveObserver(this);
}
// static
void AndroidSmsAppInstallingStatusObserver::RegisterPrefs(
PrefRegistrySimple* registry) {
registry->RegisterBooleanPref(kShouldAttemptReenable, true);
}
AndroidSmsAppInstallingStatusObserver::AndroidSmsAppInstallingStatusObserver(
HostStatusProvider* host_status_provider,
FeatureStateManager* feature_state_manager,
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate)
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate,
PrefService* pref_service)
: host_status_provider_(host_status_provider),
feature_state_manager_(feature_state_manager),
android_sms_app_helper_delegate_(android_sms_app_helper_delegate) {
android_sms_app_helper_delegate_(android_sms_app_helper_delegate),
pref_service_(pref_service) {
host_status_provider_->AddObserver(this);
feature_state_manager_->AddObserver(this);
......@@ -84,12 +101,47 @@ bool AndroidSmsAppInstallingStatusObserver::
return true;
}
void AndroidSmsAppInstallingStatusObserver::ReenableIfAppropriate() {
if (!pref_service_->GetBoolean(kShouldAttemptReenable)) {
return;
}
// This is a one-time attempt, flip the pref to prevent later tries.
pref_service_->SetBoolean(kShouldAttemptReenable, false);
if (host_status_provider_->GetHostWithStatus().host_status() !=
mojom::HostStatus::kHostVerified) {
PA_LOG(INFO) << "Can't reenable Messages, no verified host.";
return;
}
if (feature_state_manager_->GetFeatureStates()[mojom::Feature::kMessages] !=
mojom::FeatureState::kDisabledByUser) {
PA_LOG(INFO)
<< "Can't reenable Messages, feature is not in disabled state.";
return;
}
if (!android_sms_app_helper_delegate_->IsAppInstalled()) {
PA_LOG(INFO) << "Can't reenable Messages, app not installed.";
return;
}
PA_LOG(INFO) << "Performing one-time re-enable.";
feature_state_manager_->SetFeatureEnabledState(mojom::Feature::kMessages,
true);
}
void AndroidSmsAppInstallingStatusObserver::UpdatePwaInstallationState() {
if (!android_sms_app_helper_delegate_->IsAppRegistryReady()) {
PA_LOG(INFO) << "App registry is not ready.";
return;
}
// TODO(crbug/1131140): Remove in M-89. This is needed to correct a bug
// introduced in M-85 and is not permanently.
ReenableIfAppropriate();
if (!DoesFeatureStateAllowInstallation()) {
PA_LOG(INFO)
<< "Feature state does not allow installation, tearing down App.";
......
......@@ -10,6 +10,9 @@
#include "chromeos/services/multidevice_setup/feature_state_manager.h"
#include "chromeos/services/multidevice_setup/host_status_provider.h"
class PrefRegistrySimple;
class PrefService;
namespace chromeos {
namespace multidevice_setup {
......@@ -27,7 +30,8 @@ class AndroidSmsAppInstallingStatusObserver
static std::unique_ptr<AndroidSmsAppInstallingStatusObserver> Create(
HostStatusProvider* host_status_provider,
FeatureStateManager* feature_state_manager,
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate);
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate,
PrefService* pref_service);
static void SetFactoryForTesting(Factory* test_factory);
protected:
......@@ -44,11 +48,14 @@ class AndroidSmsAppInstallingStatusObserver
~AndroidSmsAppInstallingStatusObserver() override;
static void RegisterPrefs(PrefRegistrySimple* registry);
private:
AndroidSmsAppInstallingStatusObserver(
HostStatusProvider* host_status_provider,
FeatureStateManager* feature_state_manager,
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate);
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate,
PrefService* pref_service);
// HostStatusProvider::Observer:
void OnHostStatusChange(const HostStatusProvider::HostStatusWithDevice&
......@@ -60,10 +67,12 @@ class AndroidSmsAppInstallingStatusObserver
bool DoesFeatureStateAllowInstallation();
void UpdatePwaInstallationState();
void ReenableIfAppropriate();
HostStatusProvider* host_status_provider_;
FeatureStateManager* feature_state_manager_;
AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate_;
PrefService* pref_service_;
base::WeakPtrFactory<AndroidSmsAppInstallingStatusObserver> weak_ptr_factory_{
this};
......
......@@ -11,6 +11,7 @@
#include "chromeos/services/multidevice_setup/fake_host_status_provider.h"
#include "chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.h"
#include "chromeos/services/multidevice_setup/public/mojom/multidevice_setup.mojom.h"
#include "components/sync_preferences/testing_pref_service_syncable.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace chromeos {
......@@ -21,6 +22,7 @@ namespace {
const char kFakePhoneKey[] = "fake-phone-key";
const char kFakePhoneName[] = "Phony Phone";
const char kShouldAttemptReenable[] = "android_sms.should_attempt_reenable";
} // namespace
......@@ -37,16 +39,22 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
std::make_unique<FakeAndroidSmsAppHelperDelegate>();
fake_host_status_provider_ = std::make_unique<FakeHostStatusProvider>();
fake_feature_state_manager_ = std::make_unique<FakeFeatureStateManager>();
test_pref_service_ =
std::make_unique<sync_preferences::TestingPrefServiceSyncable>();
AndroidSmsAppInstallingStatusObserver::RegisterPrefs(
test_pref_service_->registry());
android_sms_app_installing_status_observer_ =
AndroidSmsAppInstallingStatusObserver::Factory::Create(
fake_host_status_provider_.get(), fake_feature_state_manager_.get(),
fake_android_sms_app_helper_delegate_.get());
fake_android_sms_app_helper_delegate_->set_is_app_registry_ready(true);
fake_android_sms_app_helper_delegate_.get(),
test_pref_service_.get());
}
void Initialize() {
fake_android_sms_app_helper_delegate_->set_is_app_registry_ready(true);
SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
fake_app_helper_delegate()->Reset();
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
}
void SetHostWithStatus(
......@@ -76,11 +84,21 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
mojom::Feature::kMessages);
}
FakeAndroidSmsAppHelperDelegate* fake_android_sms_app_helper_delegate() {
return fake_android_sms_app_helper_delegate_.get();
}
sync_preferences::TestingPrefServiceSyncable* test_pref_service() {
return test_pref_service_.get();
}
private:
std::unique_ptr<FakeHostStatusProvider> fake_host_status_provider_;
std::unique_ptr<FakeFeatureStateManager> fake_feature_state_manager_;
std::unique_ptr<FakeAndroidSmsAppHelperDelegate>
fake_android_sms_app_helper_delegate_;
std::unique_ptr<sync_preferences::TestingPrefServiceSyncable>
test_pref_service_;
std::unique_ptr<AndroidSmsAppInstallingStatusObserver>
android_sms_app_installing_status_observer_;
......@@ -91,6 +109,7 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
InstallsAfterHostPending) {
Initialize();
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
......@@ -105,6 +124,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
InstallsAfterHostVerified) {
Initialize();
SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
base::nullopt /* host_device */);
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
......@@ -115,6 +135,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotInstallsAfterHostVerifiedIfNotAllowed) {
Initialize();
SetMessagesFeatureState(mojom::FeatureState::kProhibitedByPolicy);
fake_app_helper_delegate()->Reset();
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
......@@ -129,6 +150,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotInstallAfterHostVerifiedIfUninstalledByUser) {
Initialize();
fake_app_helper_delegate()->Reset();
fake_app_helper_delegate()->set_has_app_been_manually_uninstalled(true);
......@@ -142,6 +164,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotDisableFeatureIfAppRegistryNotReady) {
Initialize();
SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
base::nullopt /* host_device */);
fake_app_helper_delegate()->Reset();
......@@ -155,6 +178,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotInstallsAfterHostVerifiedIfNotSupportedByPhone) {
Initialize();
SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByPhone);
fake_app_helper_delegate()->Reset();
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
......@@ -183,6 +207,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
InstallsWhenFeatureBecomesEnabled) {
Initialize();
SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByChromebook);
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
......@@ -191,6 +216,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
CleansUpPwaInstallationWhenDisabled) {
Initialize();
SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByChromebook);
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
......@@ -204,6 +230,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotInstallWhenFeatureIsDisabledByUser) {
Initialize();
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
SetMessagesFeatureState(mojom::FeatureState::kDisabledByUser);
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
......@@ -211,6 +238,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotInstallWhenSuiteIsDisabledByUser) {
Initialize();
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
SetMessagesFeatureState(mojom::FeatureState::kUnavailableSuiteDisabled);
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
......@@ -218,6 +246,7 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesNotInstallIfNotVerified) {
Initialize();
SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
base::nullopt /* host_device */);
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
......@@ -225,6 +254,57 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
}
// This test covers the temporary fix to re-enable users who were affected by
// crbug.com/1131140 which caused Messages to become disabled during login due
// to the app being incorrectly considered uninstalled.
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
ReenablesMessages_WhenDisabledByBug) {
// Don't call Initialize(), instead simulate the class starting disabled.
SetMessagesFeatureState(mojom::FeatureState::kDisabledByUser);
fake_app_helper_delegate()->set_has_installed_app(true);
fake_android_sms_app_helper_delegate()->set_is_app_registry_ready(true);
SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
EXPECT_EQ(mojom::FeatureState::kEnabledByUser, GetMessagesFeatureState());
EXPECT_FALSE(test_pref_service()->GetBoolean(kShouldAttemptReenable));
}
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesntReenableMessages_WhenNoHostSet) {
// Don't call Initialize(), instead simulate the class starting disabled.
SetMessagesFeatureState(mojom::FeatureState::kDisabledByUser);
fake_app_helper_delegate()->set_has_installed_app(true);
fake_android_sms_app_helper_delegate()->set_is_app_registry_ready(true);
SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
base::nullopt /* host_device */);
EXPECT_EQ(mojom::FeatureState::kDisabledByUser, GetMessagesFeatureState());
EXPECT_FALSE(test_pref_service()->GetBoolean(kShouldAttemptReenable));
}
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesntReenableMessages_WhenAppNotInstalled) {
// Don't call Initialize(), instead simulate the class starting disabled.
SetMessagesFeatureState(mojom::FeatureState::kDisabledByUser);
fake_app_helper_delegate()->set_has_installed_app(false);
fake_android_sms_app_helper_delegate()->set_is_app_registry_ready(true);
SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
EXPECT_EQ(mojom::FeatureState::kDisabledByUser, GetMessagesFeatureState());
EXPECT_FALSE(test_pref_service()->GetBoolean(kShouldAttemptReenable));
}
TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
DoesntTryReenableMessages_WhenRegistryNotReady) {
// Don't call Initialize(), instead simulate the class starting disabled.
SetMessagesFeatureState(mojom::FeatureState::kDisabledByUser);
fake_app_helper_delegate()->set_has_installed_app(true);
SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
EXPECT_EQ(mojom::FeatureState::kDisabledByUser, GetMessagesFeatureState());
EXPECT_TRUE(test_pref_service()->GetBoolean(kShouldAttemptReenable));
}
} // namespace multidevice_setup
} // namespace chromeos
......@@ -147,7 +147,8 @@ MultiDeviceSetupImpl::MultiDeviceSetupImpl(
? AndroidSmsAppInstallingStatusObserver::Factory::Create(
host_status_provider_.get(),
feature_state_manager_.get(),
android_sms_app_helper_delegate)
android_sms_app_helper_delegate,
pref_service)
: nullptr),
auth_token_validator_(auth_token_validator) {
host_status_provider_->AddObserver(this);
......
......@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "chromeos/components/multidevice/logging/logging.h"
#include "chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.h"
#include "chromeos/services/multidevice_setup/android_sms_app_installing_status_observer.h"
#include "chromeos/services/multidevice_setup/device_reenroller.h"
#include "chromeos/services/multidevice_setup/grandfathered_easy_unlock_host_disabler.h"
#include "chromeos/services/multidevice_setup/host_backend_delegate_impl.h"
......@@ -33,6 +34,7 @@ void MultiDeviceSetupService::RegisterProfilePrefs(
WifiSyncFeatureManagerImpl::RegisterPrefs(registry);
HostVerifierImpl::RegisterPrefs(registry);
GrandfatheredEasyUnlockHostDisabler::RegisterPrefs(registry);
AndroidSmsAppInstallingStatusObserver::RegisterPrefs(registry);
RegisterFeaturePrefs(registry);
}
......
......@@ -28,6 +28,8 @@ class AndroidSmsAppHelperDelegate {
// Returns true if the app was ever installed successfully since the feature
// was enabled and then been manually uninstalled by the user.
virtual bool HasAppBeenManuallyUninstalledByUser() = 0;
// Returns whether the PWA is currently installed.
virtual bool IsAppInstalled() = 0;
// Returns true when details about installed PWAs is available to query.
virtual bool IsAppRegistryReady() = 0;
// Takes a task to run when the app registry is available. If already
......
......@@ -39,6 +39,10 @@ bool FakeAndroidSmsAppHelperDelegate::HasAppBeenManuallyUninstalledByUser() {
return has_app_been_manually_uninstalled_;
}
bool FakeAndroidSmsAppHelperDelegate::IsAppInstalled() {
return has_installed_app_;
}
bool FakeAndroidSmsAppHelperDelegate::IsAppRegistryReady() {
return is_app_registry_ready_;
}
......
......@@ -19,6 +19,10 @@ class FakeAndroidSmsAppHelperDelegate
~FakeAndroidSmsAppHelperDelegate() override;
bool has_installed_app() const { return has_installed_app_; }
void set_has_installed_app(bool has_installed_app) {
has_installed_app_ = has_installed_app;
}
bool has_launched_app() const { return has_launched_app_; }
bool is_default_to_persist_cookie_set() const {
return is_default_to_persist_cookie_set_;
......@@ -42,6 +46,7 @@ class FakeAndroidSmsAppHelperDelegate
void SetUpAndLaunchAndroidSmsApp() override;
void TearDownAndroidSmsApp() override;
bool HasAppBeenManuallyUninstalledByUser() override;
bool IsAppInstalled() override;
bool IsAppRegistryReady() override;
void ExecuteOnAppRegistryReady(base::OnceClosure task) override;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment