[iOS] Move add account logging into user sign-in flow.

Moves all logging into the user sign-in workflow and improves existing
tests for add account.

Bug: 971989
Change-Id: Id6804607c81eed23ade30863d9741f4302341052
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2116177
Commit-Queue: Nohemi Fernandez <fernandex@chromium.org>
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754959}

ios/chrome/browser/ui/authentication/signin/add_account_signin/add_account_signin_coordinator.mm

@@ -4,7 +4,6 @@
 
 #import "ios/chrome/browser/ui/authentication/signin/add_account_signin/add_account_signin_coordinator.h"
 
-#import "components/signin/public/base/signin_metrics.h"
 #import "components/signin/public/identity_manager/identity_manager.h"
 #import "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #import "ios/chrome/browser/main/browser.h"
@@ -115,9 +114,7 @@ using signin_metrics::PromoAction;
                                              ->GetPrefs()
                          identityManager:identityManager];
   self.mediator.delegate = self;
-  [self.mediator handleSigninWithIntent:self.signinIntent
-                            accessPoint:self.accessPoint
-                            promoAction:self.promoAction];
+  [self.mediator handleSigninWithIntent:self.signinIntent];
 }
 
 - (void)stop {

ios/chrome/browser/ui/authentication/signin/add_account_signin/add_account_signin_mediator.h

@@ -5,7 +5,6 @@
 #ifndef IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGNIN_ADD_ACCOUNT_SIGNIN_ADD_ACCOUNT_SIGNIN_MEDIATOR_H_
 #define IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGNIN_ADD_ACCOUNT_SIGNIN_ADD_ACCOUNT_SIGNIN_MEDIATOR_H_
 
-#import "components/signin/public/base/signin_metrics.h"
 #import "ios/chrome/browser/ui/authentication/signin/add_account_signin/add_account_signin_enums.h"
 #import "ios/chrome/browser/ui/authentication/signin/signin_constants.h"
 
@@ -51,11 +50,7 @@ class IdentityManager;
 
 // Handles the sign-in operation.
 // |signinIntent| is the add account sign-in flow intent.
-// |accessPoint| is the view where the sign-in button was displayed.
-// |promoAction| is promo button used to trigger the sign-in.
-- (void)handleSigninWithIntent:(AddAccountSigninIntent)addAccountSigninIntent
-                   accessPoint:(signin_metrics::AccessPoint)accessPoint
-                   promoAction:(signin_metrics::PromoAction)promoAction;
+- (void)handleSigninWithIntent:(AddAccountSigninIntent)addAccountSigninIntent;
 
 @end
 

ios/chrome/browser/ui/authentication/signin/add_account_signin/add_account_signin_mediator.mm

@@ -15,9 +15,6 @@
 #error "This file requires ARC support."
 #endif
 
-using signin_metrics::AccessPoint;
-using signin_metrics::PromoAction;
-
 @interface AddAccountSigninMediator ()
 
 // The coordinator's manager that handles interactions to add identities.
@@ -49,30 +46,22 @@ using signin_metrics::PromoAction;
   return self;
 }
 
-- (void)handleSigninWithIntent:(AddAccountSigninIntent)signinIntent
-                   accessPoint:(AccessPoint)accessPoint
-                   promoAction:(PromoAction)promoAction {
+- (void)handleSigninWithIntent:(AddAccountSigninIntent)signinIntent {
   switch (signinIntent) {
     case AddAccountSigninIntentAddSecondaryAccount: {
       [self addAccount];
       break;
     }
     case AddAccountSigninIntentReauthPrimaryAccount: {
-      signin_metrics::LogSigninAccessPointStarted(accessPoint, promoAction);
       [self reauthenticate];
       break;
     }
-    default: {
-      NOTREACHED();
-    }
   }
 }
 
 #pragma mark - Private
 
-// Completes the add account flow including handling any errors that have not
-// been handled internally by ChromeIdentity. Will return the sign-in status of
-// the user.
+// Returns the sign-in result status of the user.
 // |identity| is the identity of the added account.
 // |error| is an error reported by the SSOAuth following adding an account.
 - (SigninCoordinatorResult)signinStateWithIdentity:(ChromeIdentity*)identity
@@ -90,7 +79,7 @@ using signin_metrics::PromoAction;
   return SigninCoordinatorResultSuccess;
 }
 
-// Handles the add account operation for a user. User account will be
+// Handles the reauth operation for a user. User account will be
 // automatically populated using the primary user account. In the case of a
 // sign-in error will display a modal alert and abort adding an account.
 - (void)reauthenticate {
@@ -122,8 +111,8 @@ using signin_metrics::PromoAction;
                     }];
 }
 
-// Handles the reauthentication or add account operation if the flow has not
-// been interrupted by a sign-in error.
+// Handles the reauthentication or add account operation or displays an alert
+// if the flow is interrupted by a sign-in error.
 - (void)operationCompletedWithIdentity:(ChromeIdentity*)identity
                                  error:(NSError*)error {
   SigninCoordinatorResult signinResult = [self signinStateWithIdentity:identity

ios/chrome/browser/ui/authentication/signin/add_account_signin/add_account_signin_mediator_unittest.mm

@@ -95,26 +95,30 @@ class AddAccountSigninMediatorTest : public PlatformTest {
 
  protected:
   void SetUp() override {
+    PlatformTest::SetUp();
     mediator_ = [[AddAccountSigninMediator alloc]
         initWithIdentityInteractionManager:identity_interaction_manager_
                                prefService:GetPrefService()
                            identityManager:GetIdentityManager()];
     mediator_delegate_ =
-        OCMProtocolMock(@protocol(AddAccountSigninMediatorDelegate));
+        OCMStrictProtocolMock(@protocol(AddAccountSigninMediatorDelegate));
     mediator_.delegate = mediator_delegate_;
   }
 
-  void TearDown() override { EXPECT_OCMOCK_VERIFY(mediator_delegate_); }
+  void TearDown() override {
+    EXPECT_OCMOCK_VERIFY((id)mediator_delegate_);
+    PlatformTest::TearDown();
+  }
 
   // Needed for test browser state created by TestChromeBrowserState().
   base::test::TaskEnvironment environment_;
   std::unique_ptr<TestChromeBrowserState> browser_state_;
 
   AddAccountSigninMediator* mediator_ = nil;
-  id mediator_delegate_ = nil;
+  id<AddAccountSigninMediatorDelegate> mediator_delegate_ = nil;
   id<ChromeIdentityInteractionManagerDelegate> manager_delegate_ = nil;
 
-  ios::FakeChromeIdentityService* identity_service_ = nil;
+  ios::FakeChromeIdentityService* identity_service_ = nullptr;
   FakeChromeIdentityInteractionManager* identity_interaction_manager_ = nil;
   FakeChromeIdentity* fake_identity_ = nil;
 };
@@ -124,16 +128,12 @@ class AddAccountSigninMediatorTest : public PlatformTest {
 //   - Completion callback is called with success state
 TEST_F(AddAccountSigninMediatorTest, AddAccountIntent) {
   // Verify that completion was called with success state.
-  [[mediator_delegate_ expect]
+  OCMExpect([mediator_delegate_
       addAccountSigninMediatorFinishedWithSigninResult:
           SigninCoordinatorResultSuccess
-                                              identity:fake_identity_];
+                                              identity:fake_identity_]);
 
-  [mediator_
-      handleSigninWithIntent:AddAccountSigninIntentAddSecondaryAccount
-                 accessPoint:signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS
-                 promoAction:signin_metrics::PromoAction::
-                                 PROMO_ACTION_WITH_DEFAULT];
+  [mediator_ handleSigninWithIntent:AddAccountSigninIntentAddSecondaryAccount];
   [identity_interaction_manager_ addAccountViewControllerDidTapSignIn];
 
   // Account added.
@@ -145,18 +145,12 @@ TEST_F(AddAccountSigninMediatorTest, AddAccountIntent) {
 //   - Completion callback is called with user cancel state
 TEST_F(AddAccountSigninMediatorTest, AddAccountIntentWithUserCancel) {
   // Verify that completion was called with canceled result state.
-  [[mediator_delegate_ expect]
+  OCMExpect([mediator_delegate_
       addAccountSigninMediatorFinishedWithSigninResult:
           SigninCoordinatorResultCanceledByUser
-                                              identity:nil];
-  [[mediator_delegate_ reject]
-      addAccountSigninMediatorFailedWithError:[OCMArg any]];
-
-  [mediator_
-      handleSigninWithIntent:AddAccountSigninIntentAddSecondaryAccount
-                 accessPoint:signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS
-                 promoAction:signin_metrics::PromoAction::
-                                 PROMO_ACTION_WITH_DEFAULT];
+                                              identity:nil]);
+
+  [mediator_ handleSigninWithIntent:AddAccountSigninIntentAddSecondaryAccount];
   [identity_interaction_manager_ addAccountViewControllerDidTapCancel];
 
   // No account is present.
@@ -171,14 +165,10 @@ TEST_F(AddAccountSigninMediatorTest,
        AddAccountIntentWithErrorHandledByMediator) {
   // Verify that completion was called with canceled result state and an error
   // is shown.
-  [[mediator_delegate_ expect]
-      addAccountSigninMediatorFailedWithError:[OCMArg any]];
-
-  [mediator_
-      handleSigninWithIntent:AddAccountSigninIntentAddSecondaryAccount
-                 accessPoint:signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS
-                 promoAction:signin_metrics::PromoAction::
-                                 PROMO_ACTION_WITH_DEFAULT];
+  OCMExpect([mediator_delegate_
+      addAccountSigninMediatorFailedWithError:[OCMArg any]]);
+
+  [mediator_ handleSigninWithIntent:AddAccountSigninIntentAddSecondaryAccount];
   [identity_interaction_manager_
       addAccountViewControllerDidThrowUnhandledError];
 
@@ -186,28 +176,33 @@ TEST_F(AddAccountSigninMediatorTest,
   EXPECT_FALSE(identity_service_->HasIdentities());
 }
 
-// TODO(crbug.com/971989): Add the ReauthIntent test.
 // Verifies the following state in the successful reauth flow:
 //   - Account is added to the identity service
 //   - Completion callback is called with success state
+TEST_F(AddAccountSigninMediatorTest, ReauthIntentWithSuccess) {
+  // Verify that completion was called with canceled result state.
+  OCMExpect([mediator_delegate_
+      addAccountSigninMediatorFinishedWithSigninResult:
+          SigninCoordinatorResultSuccess
+                                              identity:fake_identity_]);
+
+  [mediator_ handleSigninWithIntent:AddAccountSigninIntentReauthPrimaryAccount];
+  [identity_interaction_manager_ addAccountViewControllerDidTapSignIn];
+
+  EXPECT_TRUE(identity_service_->HasIdentities());
+}
 
 // Verifies the following state in the reauth flow with a user cancel:
 //   - Account is not added to the identity service
 //   - Completion callback is called with user cancel state
 TEST_F(AddAccountSigninMediatorTest, ReauthIntentWithUserCancel) {
   // Verify that completion was called with canceled result state.
-  [[mediator_delegate_ expect]
+  OCMExpect([mediator_delegate_
       addAccountSigninMediatorFinishedWithSigninResult:
           SigninCoordinatorResultCanceledByUser
-                                              identity:nil];
-  [[mediator_delegate_ reject]
-      addAccountSigninMediatorFailedWithError:[OCMArg any]];
-
-  [mediator_
-      handleSigninWithIntent:AddAccountSigninIntentReauthPrimaryAccount
-                 accessPoint:signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS
-                 promoAction:signin_metrics::PromoAction::
-                                 PROMO_ACTION_WITH_DEFAULT];
+                                              identity:nil]);
+
+  [mediator_ handleSigninWithIntent:AddAccountSigninIntentReauthPrimaryAccount];
   [identity_interaction_manager_ addAccountViewControllerDidTapCancel];
 
   // No account is present.
@@ -221,14 +216,10 @@ TEST_F(AddAccountSigninMediatorTest, ReauthIntentWithUserCancel) {
 TEST_F(AddAccountSigninMediatorTest, ReauthIntentWithErrorHandledByMediator) {
   // Verify that completion was called with canceled result state and an error
   // is shown.
-  [[mediator_delegate_ expect]
-      addAccountSigninMediatorFailedWithError:[OCMArg any]];
-
-  [mediator_
-      handleSigninWithIntent:AddAccountSigninIntentReauthPrimaryAccount
-                 accessPoint:signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS
-                 promoAction:signin_metrics::PromoAction::
-                                 PROMO_ACTION_WITH_DEFAULT];
+  OCMExpect([mediator_delegate_
+      addAccountSigninMediatorFailedWithError:[OCMArg any]]);
+
+  [mediator_ handleSigninWithIntent:AddAccountSigninIntentReauthPrimaryAccount];
   [identity_interaction_manager_
       addAccountViewControllerDidThrowUnhandledError];
 

ios/chrome/browser/ui/authentication/signin/signin_coordinator.mm

@@ -56,7 +56,7 @@ using signin_metrics::PromoAction;
     upgradeSigninPromoCoordinatorWithBaseViewController:
         (UIViewController*)viewController
                                                 browser:(Browser*)browser {
-  UpgradeSigninLogger* logger = [[UpgradeSigninLogger alloc]
+  UserSigninLogger* logger = [[UpgradeSigninLogger alloc]
       initWithAccessPoint:AccessPoint::ACCESS_POINT_SIGNIN_PROMO
               promoAction:PromoAction::PROMO_ACTION_NO_SIGNIN_PROMO];
   return [[UserSigninCoordinator alloc]

ios/chrome/browser/ui/authentication/signin/user_signin/logging/BUILD.gn

@@ -16,7 +16,6 @@ source_set("logging") {
   ]
   deps = [
     "//components/signin/public/base",
-    "//ios/chrome/browser/ui/settings/sync/utils",
     "//ios/public/provider/chrome/browser",
     "//ios/public/provider/chrome/browser/signin",
   ]

ios/chrome/browser/ui/authentication/signin/user_signin/logging/user_signin_logger.h

@@ -7,7 +7,6 @@
 
 #import "components/signin/public/base/signin_metrics.h"
 #import "ios/chrome/browser/ui/authentication/signin/signin_constants.h"
-#import "ios/chrome/browser/ui/settings/sync/utils/sync_presenter.h"
 
 // Logs metrics for user sign-in operations.
 @interface UserSigninLogger : NSObject