Rework FirestoreAccountStorage implementation (#25)

# Rework FirestoreAccountStorage implementation

## ♻️ Current situation & Problem
This PR fixes several issues identified within #24. As it turns out, we
completely forgot to add testing for this component.
This follow-up PR addresses some of the issues and adds proper UI
testing.

## ⚙️ Release Notes 
* Fixes several issues with the `FirestoreAccountStorage` module


## 📚 Documentation
--


## ✅ Testing
We added testing for the respective components.

## 📝 Code of Conduct & Contributing Guidelines 

By submitting creating this pull request, you agree to follow our [Code
of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md):
- [x] I agree to follow the [Code of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md).

Package.swift

@@ -27,7 +27,7 @@ let package = Package(
     dependencies: [
         .package(url: "https://github.com/StanfordSpezi/Spezi", .upToNextMinor(from: "0.8.0")),
         .package(url: "https://github.com/StanfordSpezi/SpeziViews.git", .upToNextMinor(from: "0.6.1")),
-        .package(url: "https://github.com/StanfordSpezi/SpeziAccount", .upToNextMinor(from: "0.8.0")),
+        .package(url: "https://github.com/StanfordSpezi/SpeziAccount", .upToNextMinor(from: "0.8.1")),
         .package(url: "https://github.com/StanfordSpezi/SpeziStorage", .upToNextMinor(from: "0.5.0")),
         .package(url: "https://github.com/firebase/firebase-ios-sdk", from: "10.13.0")
     ],

Sources/SpeziFirebaseAccount/Account Services/FirebaseAccountService.swift

@@ -92,8 +92,6 @@ extension FirebaseAccountService {
             throw FirebaseAccountError.notSignedIn
         }
 
-        var changes = false
-
         do {
             // if we modify sensitive credentials and require a recent login
             if modifications.modifiedDetails.storage[UserIdKey.self] != nil || modifications.modifiedDetails.password != nil {
@@ -107,7 +105,6 @@ extension FirebaseAccountService {
             if let userId = modifications.modifiedDetails.storage[UserIdKey.self] {
                 Self.logger.debug("updateEmail(to:) for user.")
                 try await currentUser.updateEmail(to: userId)
-                changes = true
             }
 
             if let password = modifications.modifiedDetails.password {
@@ -120,14 +117,10 @@ extension FirebaseAccountService {
                 let changeRequest = currentUser.createProfileChangeRequest()
                 changeRequest.displayName = name.formatted(.name(style: .long))
                 try await changeRequest.commitChanges()
-
-                changes = true
             }
 
-            if changes {
-                // non of the above request will trigger our state change listener, therefore just call it manually.
-                try await context.notifyUserSignIn(user: currentUser, for: self)
-            }
+            // None of the above requests will trigger our state change listener, therefore, we just call it manually.
+            try await context.notifyUserSignIn(user: currentUser, for: self)
         } catch let error as NSError {
             Self.logger.error("Received NSError on firebase dispatch: \(error)")
             throw FirebaseAccountError(authErrorCode: AuthErrorCode(_nsError: error))

Sources/SpeziFirebaseAccountStorage/FirestoreAccountStorage.swift

@@ -72,15 +72,16 @@ import SpeziFirestore
 public actor FirestoreAccountStorage: Module, AccountStorageStandard {
     @Dependency private var firestore: SpeziFirestore.Firestore // ensure firestore is configured
 
-    private let collection: CollectionReference
+    private let collection: () -> CollectionReference
 
 
-    public init(storeIn collection: CollectionReference) {
+    public init(storeIn collection: @Sendable @autoclosure @escaping () -> CollectionReference) {
         self.collection = collection
     }
 
+
     private func userDocument(for accountId: String) -> DocumentReference {
-        collection.document(accountId)
+        collection().document(accountId)
     }
 
     public func create(_ identifier: AdditionalRecordId, _ details: SignupDetails) async throws {

Sources/SpeziFirebaseAccountStorage/Visitor/FirestoreEncodeVisitor.swift

@@ -8,22 +8,40 @@
 
 
 import FirebaseFirestore
+import OSLog
 import SpeziAccount
 
 
+private struct SingleKeyContainer<Value: Codable>: Codable {
+    let value: Value
+}
+
+
 class FirestoreEncodeVisitor: AccountValueVisitor {
     typealias Data = [String: Any]
 
+    private let logger = Logger(subsystem: "edu.stanford.spezi.firebase", category: "FirestoreEncode")
+
     private var values: Data = [:]
     private var errors: [String: Error] = [:]
 
     init() {}
 
     func visit<Key: AccountKey>(_ key: Key.Type, _ value: Key.Value) {
         let encoder = Firestore.Encoder()
+
+        // the firestore encode method expects a container type!
+        let container = SingleKeyContainer(value: value)
+
         do {
-            values["\(Key.self)"] = try encoder.encode(value)
+            let result = try encoder.encode(container)
+            guard let encoded = result["value"] else {
+                preconditionFailure("Layout of SingleKeyContainer changed. Does not contain value anymore: \(result)")
+            }
+
+            values["\(Key.self)"] = encoded
         } catch {
+            logger.error("Failed to encode \("\(value)") for key \(key): \(error)")
             errors["\(Key.self)"] = error
         }
     }

Tests/UITests/TestApp/FirebaseAccountStorage/AccountStorageTestStandard.swift

@@ -0,0 +1,42 @@
+//
+// This source file is part of the Stanford Spezi open-source project
+//
+// SPDX-FileCopyrightText: 2022 Stanford University and the project authors (see CONTRIBUTORS.md)
+//
+// SPDX-License-Identifier: MIT
+//
+
+import FirebaseFirestore
+import Spezi
+import SpeziAccount
+import SpeziFirebaseAccountStorage
+
+
+actor AccountStorageTestStandard: Standard, AccountStorageStandard {
+    static var collection: CollectionReference {
+        Firestore.firestore().collection("users")
+    }
+
+    @Dependency var storage = FirestoreAccountStorage(storeIn: collection)
+
+
+    func create(_ identifier: AdditionalRecordId, _ details: SignupDetails) async throws {
+        try await storage.create(identifier, details)
+    }
+
+    func load(_ identifier: AdditionalRecordId, _ keys: [any AccountKey.Type]) async throws -> PartialAccountDetails {
+        try await storage.load(identifier, keys)
+    }
+
+    func modify(_ identifier: AdditionalRecordId, _ modifications: SpeziAccount.AccountModifications) async throws {
+        try await storage.modify(identifier, modifications)
+    }
+
+    func clear(_ identifier: AdditionalRecordId) async {
+        await storage.clear(identifier)
+    }
+
+    func delete(_ identifier: AdditionalRecordId) async throws {
+        try await storage.delete(identifier)
+    }
+}

Tests/UITests/TestApp/FirebaseAccountStorage/BiographyKey.swift

@@ -0,0 +1,53 @@
+//
+// This source file is part of the Spezi open-source project
+//
+// SPDX-FileCopyrightText: 2023 Stanford University and the project authors (see CONTRIBUTORS.md)
+//
+// SPDX-License-Identifier: MIT
+//
+
+import SpeziAccount
+import SpeziValidation
+import SwiftUI
+
+
+struct BiographyKey: AccountKey {
+    typealias Value = String
+
+
+    static let name: LocalizedStringResource = "Biography" // we don't bother to translate
+    static let category: AccountKeyCategory = .personalDetails
+}
+
+
+extension AccountKeys {
+    var biography: BiographyKey.Type {
+        BiographyKey.self
+    }
+}
+
+
+extension AccountValues {
+    var biography: String? {
+        storage[BiographyKey.self]
+    }
+}
+
+
+extension BiographyKey {
+    public struct DataEntry: DataEntryView {
+        public typealias Key = BiographyKey
+
+
+        @Binding private var biography: Value
+
+        public init(_ value: Binding<Value>) {
+            self._biography = value
+        }
+
+        public var body: some View {
+            VerifiableTextField(Key.name, text: $biography)
+                .autocorrectionDisabled()
+        }
+    }
+}

Tests/UITests/TestApp/Shared/FeatureFlags.swift

@@ -0,0 +1,14 @@
+//
+// This source file is part of the Stanford Spezi open-source project
+//
+// SPDX-FileCopyrightText: 2023 Stanford University and the project authors (see CONTRIBUTORS.md)
+//
+// SPDX-License-Identifier: MIT
+//
+
+import Foundation
+
+
+enum FeatureFlags {
+    static let accountStorageTests = ProcessInfo.processInfo.arguments.contains("--account-storage")
+}

Tests/UITests/TestApp/Shared/TestAppDelegate.swift

@@ -16,17 +16,37 @@ import SwiftUI
 
 class TestAppDelegate: SpeziAppDelegate {
     override var configuration: Configuration {
-        Configuration {
+        if FeatureFlags.accountStorageTests {
+            return Configuration(standard: AccountStorageTestStandard(), configurationsClosure)
+        } else {
+            return Configuration(configurationsClosure)
+        }
+    }
+
+    var configurationsClosure: () -> ModuleCollection {
+        {
+            self.configurations
+        }
+    }
+
+    @ModuleBuilder var configurations: ModuleCollection {
+        if FeatureFlags.accountStorageTests {
+            AccountConfiguration(configuration: [
+                .requires(\.userId),
+                .requires(\.name),
+                .requires(\.biography)
+            ])
+        } else {
             AccountConfiguration(configuration: [
                 .requires(\.userId),
                 .collects(\.name)
             ])
-            Firestore(settings: .emulator)
-            FirebaseAccountConfiguration(
-                authenticationMethods: [.emailAndPassword, .signInWithApple],
-                emulatorSettings: (host: "localhost", port: 9099)
-            )
-            FirebaseStorageConfiguration(emulatorSettings: (host: "localhost", port: 9199))
         }
+        Firestore(settings: .emulator)
+        FirebaseAccountConfiguration(
+            authenticationMethods: [.emailAndPassword, .signInWithApple],
+            emulatorSettings: (host: "localhost", port: 9099)
+        )
+        FirebaseStorageConfiguration(emulatorSettings: (host: "localhost", port: 9199))
     }
 }

Tests/UITests/TestAppUITests/FirebaseAccountStorageTests.swift

@@ -0,0 +1,86 @@
+//
+// This source file is part of the Stanford Spezi open-source project
+//
+// SPDX-FileCopyrightText: 2023 Stanford University and the project authors (see CONTRIBUTORS.md)
+//
+// SPDX-License-Identifier: MIT
+//
+
+import XCTest
+import XCTestExtensions
+
+
+final class FirebaseAccountStorageTests: XCTestCase {
+    @MainActor
+    override func setUp() async throws {
+        try await super.setUp()
+
+        continueAfterFailure = false
+
+        try disablePasswordAutofill()
+
+        try await FirebaseClient.deleteAllAccounts()
+        try await Task.sleep(for: .seconds(0.5))
+    }
+
+    @MainActor
+    func testAdditionalAccountStorage() async throws {
+        let app = XCUIApplication()
+        app.launchArguments = ["--account-storage"]
+        app.launch()
+
+        XCTAssert(app.buttons["FirebaseAccount"].waitForExistence(timeout: 10.0))
+        app.buttons["FirebaseAccount"].tap()
+
+        if app.buttons["Logout"].waitForExistence(timeout: 5.0) && app.buttons["Logout"].isHittable {
+            app.buttons["Logout"].tap()
+        }
+
+
+        try app.signup(
+            username: "[email protected]",
+            password: "TestPassword1",
+            givenName: "Test1",
+            familyName: "Username1",
+            biography: "Hello Stanford"
+        )
+
+
+        XCTAssertTrue(app.buttons["Account Overview"].waitForExistence(timeout: 2.0))
+        app.buttons["Account Overview"].tap()
+        XCTAssertTrue(app.staticTexts["Biography, Hello Stanford"].waitForExistence(timeout: 2.0))
+
+
+        // TEST ACCOUNT EDIT
+        XCTAssertTrue(app.navigationBars.buttons["Edit"].exists)
+        app.navigationBars.buttons["Edit"].tap()
+
+        try app.textFields["Biography"].enter(value: "2")
+
+        XCTAssertTrue(app.navigationBars.buttons["Done"].exists)
+        app.navigationBars.buttons["Done"].tap()
+
+        XCTAssertTrue(app.staticTexts["Biography, Hello Stanford2"].waitForExistence(timeout: 2.0))
+
+        // TEST ACCOUNT DELETION
+        XCTAssertTrue(app.navigationBars.buttons["Edit"].exists)
+        app.navigationBars.buttons["Edit"].tap()
+
+        XCTAssertTrue(app.buttons["Delete Account"].waitForExistence(timeout: 4.0))
+        app.buttons["Delete Account"].tap()
+
+        let alert = "Are you sure you want to delete your account?"
+        XCTAssertTrue(XCUIApplication().alerts[alert].waitForExistence(timeout: 6.0))
+        XCUIApplication().alerts[alert].scrollViews.otherElements.buttons["Delete"].tap()
+
+        XCTAssertTrue(app.alerts["Authentication Required"].waitForExistence(timeout: 2.0))
+        XCTAssertTrue(app.alerts["Authentication Required"].secureTextFields["Password"].waitForExistence(timeout: 0.5))
+        app.typeText("TestPassword1") // the password field has focus already
+        XCTAssertTrue(app.alerts["Authentication Required"].buttons["Login"].waitForExistence(timeout: 0.5))
+        app.alerts["Authentication Required"].buttons["Login"].tap()
+
+        sleep(2)
+        let accountsNew = try await FirebaseClient.getAllAccounts()
+        XCTAssertTrue(accountsNew.isEmpty)
+    }
+}