From a8169880ce21bdfc916963578f16d65f6c9cb550 Mon Sep 17 00:00:00 2001
From: Doug <douglase@element.io>
Date: Fri, 1 Jul 2022 10:44:43 +0100
Subject: [PATCH] Self review for PR.

---
 .../Authentication/LoginScreen/LoginCoordinator.swift    | 9 +++++----
 .../ServerSelection/ServerSelectionCoordinator.swift     | 4 ++--
 .../Services/Authentication/AuthenticationService.swift  | 3 +++
 .../Authentication/MockAuthenticationService.swift       | 3 +++
 .../Services/UserSessionStore/UserSessionStore.swift     | 2 --
 changelog.d/pr-126.change                                | 1 +
 6 files changed, 14 insertions(+), 8 deletions(-)
 create mode 100644 changelog.d/pr-126.change

diff --git a/ElementX/Sources/Screens/Authentication/LoginScreen/LoginCoordinator.swift b/ElementX/Sources/Screens/Authentication/LoginScreen/LoginCoordinator.swift
index e96e93d07c..3f02d40184 100644
--- a/ElementX/Sources/Screens/Authentication/LoginScreen/LoginCoordinator.swift
+++ b/ElementX/Sources/Screens/Authentication/LoginScreen/LoginCoordinator.swift
@@ -127,14 +127,14 @@ final class LoginCoordinator: Coordinator, Presentable {
     }
     
     /// Processes an error to either update the flow or display it to the user.
-    private func handleError(_ error: Error) {
+    private func handleError(_ error: AuthenticationServiceError) {
         switch error {
-        case AuthenticationServiceError.invalidCredentials:
+        case .invalidCredentials:
             loginViewModel.displayError(.alert(ElementL10n.authInvalidLoginParam))
-        case AuthenticationServiceError.accountDeactivated:
+        case .accountDeactivated:
             loginViewModel.displayError(.alert(ElementL10n.authInvalidLoginDeactivatedAccount))
         default:
-            loginViewModel.displayError(.alert(error.localizedDescription))
+            loginViewModel.displayError(.alert(ElementL10n.unknownError))
         }
     }
     
@@ -146,6 +146,7 @@ final class LoginCoordinator: Coordinator, Presentable {
             switch await authenticationService.login(username: username, password: password) {
             case .success(let userSession):
                 callback?(.signedIn(userSession))
+                stopLoading()
             case .failure(let error):
                 stopLoading()
                 handleError(error)
diff --git a/ElementX/Sources/Screens/Authentication/ServerSelection/ServerSelectionCoordinator.swift b/ElementX/Sources/Screens/Authentication/ServerSelection/ServerSelectionCoordinator.swift
index 1fb157cd0e..f891a4021e 100644
--- a/ElementX/Sources/Screens/Authentication/ServerSelection/ServerSelectionCoordinator.swift
+++ b/ElementX/Sources/Screens/Authentication/ServerSelection/ServerSelectionCoordinator.swift
@@ -116,9 +116,9 @@ final class ServerSelectionCoordinator: Coordinator, Presentable {
     }
     
     /// Processes an error to either update the flow or display it to the user.
-    private func handleError(_ error: Error) {
+    private func handleError(_ error: AuthenticationServiceError) {
         switch error {
-        case AuthenticationServiceError.invalidServer:
+        case .invalidServer:
             serverSelectionViewModel.displayError(.footerMessage(ElementL10n.loginErrorHomeserverNotFound))
         default:
             serverSelectionViewModel.displayError(.footerMessage(ElementL10n.unknownError))
diff --git a/ElementX/Sources/Services/Authentication/AuthenticationService.swift b/ElementX/Sources/Services/Authentication/AuthenticationService.swift
index 126038b3cb..abcd4497ba 100644
--- a/ElementX/Sources/Services/Authentication/AuthenticationService.swift
+++ b/ElementX/Sources/Services/Authentication/AuthenticationService.swift
@@ -57,8 +57,11 @@ class AuthenticationService: AuthenticationServiceProtocol {
         
         switch await loginTask.result {
         case .success(let client):
+            Benchmark.endTrackingForIdentifier("Login", message: "Finished login")
             return await userSession(for: client)
         case .failure(let error):
+            Benchmark.endTrackingForIdentifier("Login", message: "Login failed")
+            
             MXLog.error("Failed logging in with error: \(error)")
             guard let error = error as? ClientError else { return .failure(.failedLoggingIn) }
             
diff --git a/ElementX/Sources/Services/Authentication/MockAuthenticationService.swift b/ElementX/Sources/Services/Authentication/MockAuthenticationService.swift
index 67839a73ba..bac0ff901c 100644
--- a/ElementX/Sources/Services/Authentication/MockAuthenticationService.swift
+++ b/ElementX/Sources/Services/Authentication/MockAuthenticationService.swift
@@ -20,6 +20,7 @@ class MockAuthenticationService: AuthenticationServiceProtocol {
     }
     
     func startLogin(for homeserverAddress: String) async -> Result<Void, AuthenticationServiceError> {
+        // Map the address to the mock homeservers
         if LoginHomeserver.mockMatrixDotOrg.address.contains(homeserverAddress) {
             homeserver = .mockMatrixDotOrg
             return .success(())
@@ -34,10 +35,12 @@ class MockAuthenticationService: AuthenticationServiceProtocol {
             return .success(())
         }
         
+        // Otherwise fail with an invalid server.
         return .failure(.invalidServer)
     }
     
     func login(username: String, password: String) async -> Result<UserSessionProtocol, AuthenticationServiceError> {
+        // Login only succeeds if the username and password match the valid credentials property
         guard username == validCredentials.username, password == validCredentials.password else {
             return .failure(.invalidCredentials)
         }
diff --git a/ElementX/Sources/Services/UserSessionStore/UserSessionStore.swift b/ElementX/Sources/Services/UserSessionStore/UserSessionStore.swift
index ad4354615d..b3f1f3e1fc 100644
--- a/ElementX/Sources/Services/UserSessionStore/UserSessionStore.swift
+++ b/ElementX/Sources/Services/UserSessionStore/UserSessionStore.swift
@@ -98,8 +98,6 @@ class UserSessionStore: UserSessionStoreProtocol {
     }
     
     private func setupProxyForClient(_ client: Client) async -> Result<ClientProxyProtocol, UserSessionStoreError> {
-        Benchmark.endTrackingForIdentifier("Login", message: "Finished login")
-        
         do {
             let accessToken = try client.restoreToken()
             let userId = try client.userId()
diff --git a/changelog.d/pr-126.change b/changelog.d/pr-126.change
new file mode 100644
index 0000000000..c362ea0573
--- /dev/null
+++ b/changelog.d/pr-126.change
@@ -0,0 +1 @@
+Add AuthenticationService and missing UI tests on the flow.