Make sure to only pass complete account details to the account setup …

…complete closure (#79) # Make sure to only pass complete account details to the account setup complete closure ## ♻️ Current situation & Problem The `AccountSetup` accepts a `setupComplete` closure that will get called once the setup is completed giving the parent view the opportunity to perform additional checks or navigation steps based on the outcome. While a `AccountService` always calls an `ExternalStorageProvider` before submitting account details to `Account`, they have been decoupled in v2.0.0 and an `ExternalStorageProvider` must always return immediacy from its `load` call. In scenarios where account details are not locally cached yet, an `ExternalStorageProvider` returns details that are marked as [incomplete](https://swiftpackageindex.com/stanfordspezi/speziaccount/documentation/speziaccount/accountdetails/isincomplete). `AccountSetup` didn't check for this flag and forwarded incomplete details to the `setupComplete` closure. This PR fixes this issue and updates the behavior to only forward account details that are also marked as complete. ## ⚙️ Release Notes * Fixed an issue where `AccountSetup` would forward incomplete account details and didn't wait for an `ExternalStorageProvider` to load externally stored account details. * Fixed an issue where `flags` wouldn't be copied between account details. ## 📚 Documentation We improved some of the documentation discoverability. ## ✅ Testing Added UI tests to verify that the incomplete details are not passed to the `setupComplete` closure. ## 📝 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).
StanfordSpezi · Dec 9, 2024 · 37df11e · 37df11e
1 parent 0e4dcc7
commit 37df11e
Show file tree

Hide file tree

Showing 18 changed files with 110 additions and 40 deletions.
diff --git a/Package.swift b/Package.swift
@@ -34,7 +34,7 @@ let package = Package(
         .package(url: "https://github.com/StanfordBDHG/XCTestExtensions.git", from: "1.1.0"),
         .package(url: "https://github.com/apple/swift-collections.git", from: "1.1.2"),
         .package(url: "https://github.com/apple/swift-atomics.git", from: "1.2.0"),
-        .package(url: "https://github.com/swiftlang/swift-syntax.git", from: "600.0.0-prerelease-2024-08-14"),
+        .package(url: "https://github.com/swiftlang/swift-syntax.git", from: "600.0.0"),
         .package(url: "https://github.com/pointfreeco/swift-snapshot-testing.git", from: "1.17.0")
     ] + swiftLintPackage(),
     targets: [

diff --git a/Sources/SpeziAccount/AccountConfiguration.swift b/Sources/SpeziAccount/AccountConfiguration.swift
@@ -107,9 +107,9 @@ public final class AccountConfiguration {
     ///   - configuration: The user-defined configuration of account values that all user accounts need to support.
     ///   - activeDetails: The  account details you want to simulate.
     @_spi(TestingSupport)
-    public convenience init<Service: AccountService>( // swiftlint:disable:this function_default_parameter_at_end
+    public convenience init<Service: AccountService>(
         service: Service,
-        configuration: AccountValueConfiguration = .default,
+        configuration: AccountValueConfiguration = .default, // swiftlint:disable:this function_default_parameter_at_end
         activeDetails: AccountDetails
     ) {
         self.init(accountService: service, configuration: configuration, defaultActiveDetails: activeDetails)
@@ -123,18 +123,18 @@ public final class AccountConfiguration {
     ///   - configuration: The user-defined configuration of account values that all user accounts need to support.
     ///   - activeDetails: The  account details you want to simulate.
     @_spi(TestingSupport)
-    public convenience init<Service: AccountService, Storage: AccountStorageProvider>( // swiftlint:disable:this function_default_parameter_at_end
+    public convenience init<Service: AccountService, Storage: AccountStorageProvider>(
         service: Service,
         storageProvider: Storage,
-        configuration: AccountValueConfiguration = .default,
+        configuration: AccountValueConfiguration = .default, // swiftlint:disable:this function_default_parameter_at_end
         activeDetails: AccountDetails
     ) {
         self.init(accountService: service, storageProvider: storageProvider, configuration: configuration, defaultActiveDetails: activeDetails)
     }
 
-    init<Service: AccountService>( // swiftlint:disable:this function_default_parameter_at_end
+    init<Service: AccountService>(
         accountService: Service,
-        storageProvider: (any AccountStorageProvider)? = nil,
+        storageProvider: (any AccountStorageProvider)? = nil, // swiftlint:disable:this function_default_parameter_at_end
         configuration: AccountValueConfiguration,
         defaultActiveDetails: AccountDetails? = nil
     ) {

diff --git a/Sources/SpeziAccount/AccountSetup.swift b/Sources/SpeziAccount/AccountSetup.swift
@@ -88,10 +88,12 @@ public struct AccountSetup<Header: View, Continue: View>: View {
                     .frame(maxWidth: .infinity)
             }
         }
-            .onChange(of: [account.signedIn, account.details?.isAnonymous]) {
+            .onChange(of: [account.signedIn, account.details?.isAnonymous, account.details?.isIncomplete]) {
                 guard case .presentingSignup = setupState,
                       let details = account.details,
-                      !details.isAnonymous else {
+                      !details.isAnonymous,
+                      !details.isIncomplete // make sure all details are loaded from the storage provider 
+                else {
                     return
                 }
 

diff --git a/Sources/SpeziAccount/AccountStorageProvider.swift b/Sources/SpeziAccount/AccountStorageProvider.swift
@@ -34,6 +34,7 @@ public protocol AccountStorageProvider: Module {
     ///
     /// - Important: This method call must return immediately. Use `await` suspension only be for synchronization.
     ///     Return `nil` if you couldn't immediately retrieve the externally stored account details.
+    ///     If there is no data stored externally, you need to return an empty ``AccountDetails`` value.
     ///
     /// - Parameters:
     ///   - accountId: The primary identifier for stored record.

diff --git a/Sources/SpeziAccount/AccountValue/Collections/AccountDetails.swift b/Sources/SpeziAccount/AccountValue/Collections/AccountDetails.swift
@@ -35,12 +35,16 @@ private struct CopyVisitor: AccountValueVisitor {
         self.allowOverwrite = allowOverwrite
     }
 
-    mutating func visit<Key: AccountKey>(_ key: Key.Type, _ value: Key.Value) {
-        guard allowOverwrite || !details.contains(Key.self) else {
+    static func copyValue<Key: AccountKey>(to destination: inout AccountDetails, for key: Key.Type, value: Key.Value, allowOverwrite: Bool) {
+        guard allowOverwrite || !destination.contains(Key.self) else {
             return
         }
 
-        details.storage.set(key, value: value)
+        destination.set(key, value: value)
+    }
+
+    mutating func visit<Key: AccountKey>(_ key: Key.Type, _ value: Key.Value) {
+        Self.copyValue(to: &details, for: Key.self, value: value, allowOverwrite: allowOverwrite)
     }
 
     func final() -> AccountStorage {
@@ -65,11 +69,7 @@ private struct CopyKeyVisitor: AccountKeyVisitor {
             return
         }
 
-        guard allowOverwrite || !destination.contains(Key.self) else {
-            return
-        }
-
-        destination.storage.set(key, value: value)
+        CopyVisitor.copyValue(to: &destination, for: Key.self, value: value, allowOverwrite: allowOverwrite)
     }
 
     func final() -> AccountStorage {
@@ -121,6 +121,8 @@ private struct CopyKeyVisitor: AccountKeyVisitor {
 ///
 /// - ``isAnonymous``
 /// - ``isNewUser``
+/// - ``isIncomplete``
+/// - ``isVerified``
 /// - ``accountServiceConfiguration``
 /// - ``userIdType``
 ///
@@ -353,6 +355,11 @@ extension AccountDetails {
     public mutating func add(contentsOf values: AccountDetails, merge: Bool = false) {
         var visitor = CopyVisitor(self, allowOverwrite: merge)
         storage = values.acceptAll(&visitor)
+
+        // workaround as AccountDetailsFlagsKey is not an accountKey and won't be copied by the above visitor
+        if values.flags.rawValue != 0 {
+            self.flags.formUnion(values.flags)
+        }
     }
 
     /// Add the contents from another account details collection but filter for specific keys only.

diff --git a/Sources/SpeziAccount/AccountValue/Keys/AccountDetailsFlags.swift b/Sources/SpeziAccount/AccountValue/Keys/AccountDetailsFlags.swift
@@ -9,7 +9,7 @@
 import SpeziFoundation
 
 
-private struct AccountDetailsFlags: OptionSet, Sendable {
+struct AccountDetailsFlags: OptionSet, Sendable {
     let rawValue: UInt16
 
     init(rawValue: UInt16) {
@@ -34,7 +34,7 @@ extension AccountDetails {
         static let defaultValue: AccountDetailsFlags = []
     }
 
-    fileprivate var flags: AccountDetailsFlags {
+    var flags: AccountDetailsFlags {
         get {
             self[AccountDetailsFlagsKey.self]
         }

diff --git a/Sources/SpeziAccount/Mock/InMemoryAccountService.swift b/Sources/SpeziAccount/Mock/InMemoryAccountService.swift
@@ -459,8 +459,8 @@ extension InMemoryAccountService {
         var genderIdentity: GenderIdentity?
         var dateOfBirth: Date?
 
-        init( // swiftlint:disable:this function_default_parameter_at_end
-            accountId: UUID = UUID(),
+        init(
+            accountId: UUID = UUID(), // swiftlint:disable:this function_default_parameter_at_end
             userId: String?,
             password: String?,
             name: PersonNameComponents? = nil,

diff --git a/Sources/SpeziAccount/Mock/InMemoryAccountStorageProvider.swift b/Sources/SpeziAccount/Mock/InMemoryAccountStorageProvider.swift
@@ -25,15 +25,15 @@ public actor InMemoryAccountStorageProvider: AccountStorageProvider {
     public func load(_ accountId: String, _ keys: [any AccountKey.Type]) -> AccountDetails? {
         guard let details = cache[accountId] else {
             guard records[accountId] != nil else {
-                return nil
+                return AccountDetails() // no data present
             }
 
             // simulate loading from external storage
             Task {
                 try await Task.sleep(for: .seconds(1))
-                guard let details = records[accountId] else {
-                    return
-                }
+
+                let details = records[accountId] ?? AccountDetails()
+
                 cache[accountId] = details
                 storage.notifyAboutUpdatedDetails(for: accountId, details)
             }

diff --git a/Sources/SpeziAccount/Views/AccountSetup/SetupProvider/AccountServiceButton.swift b/Sources/SpeziAccount/Views/AccountSetup/SetupProvider/AccountServiceButton.swift
@@ -36,9 +36,9 @@ public struct AccountServiceButton<Label: View>: View {
         self.init(titleKey, systemImage: systemImage, state: .constant(.idle), action: action)
     }
 
-    public init( // swiftlint:disable:this function_default_parameter_at_end
+    public init(
         _ titleKey: LocalizedStringResource,
-        systemImage: String = "person.crop.square",
+        systemImage: String = "person.crop.square", // swiftlint:disable:this function_default_parameter_at_end
         state: Binding<ViewState>,
         action: @escaping @MainActor  () async throws -> Void
     ) where Label == SwiftUI.Label<Text, Image> {

diff --git a/Sources/SpeziAccount/Views/AccountSetup/SetupProvider/SignInWithAppleButton.swift b/Sources/SpeziAccount/Views/AccountSetup/SetupProvider/SignInWithAppleButton.swift
@@ -114,8 +114,8 @@ public struct SignInWithAppleButton: View {
     ///   - state: A view state binding that is used to set the error state if the `onCompletion` handler returns an error.
     ///   - onRequest: The authorization request for an Apple ID.
     ///   - onCompletion: The completion handler that the system calls when the sign-in completes.
-    public init( // swiftlint:disable:this function_default_parameter_at_end
-        _ label: AuthenticationServices.SignInWithAppleButton.Label? = nil,
+    public init(
+        _ label: AuthenticationServices.SignInWithAppleButton.Label? = nil, // swiftlint:disable:this function_default_parameter_at_end
         state: Binding<ViewState>,
         onRequest: @escaping (ASAuthorizationAppleIDRequest) -> Void,
         onCompletion: @escaping ((Result<ASAuthorization, any Error>) async throws -> Void)

diff --git a/Sources/SpeziAccountMacros/SpeziAccountDiagnostic.swift b/Sources/SpeziAccountMacros/SpeziAccountDiagnostic.swift
@@ -35,10 +35,10 @@ struct SpeziAccountDiagnostic: DiagnosticMessage {
 
 
 extension Diagnostic {
-    init<S: SyntaxProtocol>( // swiftlint:disable:this function_default_parameter_at_end
+    init<S: SyntaxProtocol>(
         syntax: S,
         message: String,
-        domain: String = "SpeziAccount",
+        domain: String = "SpeziAccount", // swiftlint:disable:this function_default_parameter_at_end
         id: SpeziAccountDiagnostic.ID,
         severity: SwiftDiagnostics.DiagnosticSeverity = .error
     ) {
@@ -48,10 +48,10 @@ extension Diagnostic {
 
 
 extension DiagnosticsError {
-    init<S: SyntaxProtocol>(  // swiftlint:disable:this function_default_parameter_at_end
+    init<S: SyntaxProtocol>(
         syntax: S,
         message: String,
-        domain: String = "SpeziAccount",
+        domain: String = "SpeziAccount", // swiftlint:disable:this function_default_parameter_at_end
         id: SpeziAccountDiagnostic.ID,
         severity: SwiftDiagnostics.DiagnosticSeverity = .error
     ) {

diff --git a/Tests/UITests/TestApp/AccountTestsView.swift b/Tests/UITests/TestApp/AccountTestsView.swift
@@ -15,6 +15,17 @@ import SwiftUI
 
 @MainActor
 struct AccountTestsView: View {
+    enum TestError: LocalizedError {
+        case incompleteAccount
+
+        var errorDescription: String? {
+            switch self {
+            case .incompleteAccount:
+                "Incomplete Account Details"
+            }
+        }
+    }
+
     @Environment(\.features)
     private var features
     @Environment(InMemoryAccountService.self)
@@ -29,6 +40,8 @@ struct AccountTestsView: View {
     @State private var showOverview = false
     @State private var accountIdFromAnonymousUser: String?
 
+    @State private var setupState: ViewState = .idle
+
 
     var body: some View {
         NavigationStack {
@@ -67,6 +80,10 @@ struct AccountTestsView: View {
                     details.remove(AccountKeys.name)
                 }
 
+                if features.includeInvitationCode {
+                    details.invitationCode = "123456789"
+                }
+
                 do {
                     switch features.credentials {
                     case .create:
@@ -150,11 +167,16 @@ struct AccountTestsView: View {
     @ViewBuilder
     func setupSheet(closeable: Bool = true) -> some View {
         NavigationStack {
-            AccountSetup { _ in
-                showSetup = false
+            AccountSetup { details in
+                if details.isIncomplete {
+                    setupState = .error(TestError.incompleteAccount)
+                } else {
+                    showSetup = false
+                }
             } continue: {
                 finishButton
             }
+                .viewStateAlert(state: $setupState)
                 .toolbar {
                     if closeable {
                         toolbar(closing: $showSetup)

diff --git a/Tests/UITests/TestApp/Features.swift b/Tests/UITests/TestApp/Features.swift
@@ -47,6 +47,9 @@ struct Features: ParsableArguments {
 
     @Flag(help: "Set no name if default credentials are used")
     var noName = false
+
+    @Flag(help: "Include biography in default details")
+    var includeInvitationCode = false
 }
 
 

diff --git a/Tests/UITests/TestApp/Utils/AccountDetails+Default.swift b/Tests/UITests/TestApp/Utils/AccountDetails+Default.swift
@@ -18,12 +18,18 @@ extension AccountDetails {
         .day(.twoDigits)
 
     static var defaultDetails: AccountDetails {
+        let date: Date
+        do {
+            date = try Date("09.03.1824", strategy: dateStyle)
+        } catch {
+            preconditionFailure("Failed to parse date: \(error)")
+        }
         var details = AccountDetails()
         details.userId = "[email protected]"
         details.password = "StanfordRocks123!"
         details.name = PersonNameComponents(givenName: "Leland", familyName: "Stanford")
         details.genderIdentity = .male
-        details.dateOfBirth = try? Date("09.03.1824", strategy: dateStyle)
+        details.dateOfBirth = date
         return details
     }
 }
diff --git a/Tests/UITests/TestAppUITests/AccountSetupTests.swift b/Tests/UITests/TestAppUITests/AccountSetupTests.swift
@@ -524,6 +524,29 @@ final class AccountSetupTests: XCTestCase { // swiftlint:disable:this type_body_
         XCTAssertTrue(app.staticTexts["Biography, Hello Stanford2"].waitForExistence(timeout: 2.0))
     }
 
+    @MainActor
+    func testLoginWithAdditionalStorage() throws {
+        // Ensure AccountSetup properly handles the `incomplete` flag
+        // https://github.com/StanfordSpezi/SpeziAccount/pull/79
+
+        let app = XCUIApplication()
+        app.launch(config: .default, credentials: .create, includeInvitationCode: true)
+
+
+        XCTAssertTrue(app.wait(for: .runningForeground, timeout: 2.0))
+        XCTAssertTrue(app.staticTexts["Spezi Account"].exists)
+
+        app.openAccountSetup()
+
+        try app.login(email: Defaults.email, password: Defaults.password)
+
+        // make sure our incomplete error never pops up
+        XCTAssert(app.alerts["Error"].waitForNonExistence(timeout: 5.0))
+
+        // verify we are back at the start screen
+        XCTAssertTrue(app.staticTexts[Defaults.email].waitForExistence(timeout: 2.0))
+    }
+
     @MainActor
     func testAccountRequiredModifier() throws {
         let app = XCUIApplication()

diff --git a/Tests/UITests/TestAppUITests/Utils/XCUIApplication+AccountSetup.swift b/Tests/UITests/TestAppUITests/Utils/XCUIApplication+AccountSetup.swift
@@ -18,12 +18,12 @@ extension XCUIApplication {
         XCTAssertTrue(staticTexts["Please fill out the details below to create your new account."].waitForExistence(timeout: 3.0))
     }
 
-    func fillSignupForm( // swiftlint:disable:this function_default_parameter_at_end
+    func fillSignupForm(
         email: String,
         password: String,
         name: PersonNameComponents? = nil,
         genderIdentity: String? = nil,
-        supplyDateOfBirth: Bool = false,
+        supplyDateOfBirth: Bool = false, // swiftlint:disable:this function_default_parameter_at_end
         biography: String
     ) throws {
         try fillSignupForm(email: email, password: password, name: name, genderIdentity: genderIdentity, supplyDateOfBirth: supplyDateOfBirth)

diff --git a/Tests/UITests/TestAppUITests/Utils/XCUIApplication+TestApp.swift b/Tests/UITests/TestAppUITests/Utils/XCUIApplication+TestApp.swift
@@ -31,18 +31,20 @@ enum DefaultCredentials: String {
 
 
 extension XCUIApplication {
-    func launch( // swiftlint:disable:this function_default_parameter_at_end
+    func launch(
         serviceType: ServiceType = .mail,
         config: Config = .default,
         credentials: DefaultCredentials? = nil,
         accountRequired: Bool = false,
         noName: Bool = false,
+        includeInvitationCode: Bool = false, // swiftlint:disable:this function_default_parameter_at_end
         flags: String...
     ) {
         launchArguments = ["--service-type", serviceType.rawValue, "--configuration-type", config.rawValue]
         launchArguments += credentials.map { ["--credentials", $0.rawValue] } ?? []
         launchArguments += (accountRequired ? ["--account-required-modifier"] : [])
         launchArguments += (noName ? ["--no-name"] : [])
+        launchArguments += (includeInvitationCode ? ["--include-invitation-code"] : [])
         launchArguments += flags
         launch()
     }

diff --git a/Tests/UITests/UITests.xcodeproj/xcshareddata/xcschemes/TestApp.xcscheme b/Tests/UITests/UITests.xcodeproj/xcshareddata/xcschemes/TestApp.xcscheme
@@ -94,7 +94,11 @@
             isEnabled = "YES">
          </CommandLineArgument>
          <CommandLineArgument
-            argument = "createAndSignIn"
+            argument = "create"
+            isEnabled = "YES">
+         </CommandLineArgument>
+         <CommandLineArgument
+            argument = "--include-invitation-code"
             isEnabled = "YES">
          </CommandLineArgument>
       </CommandLineArguments>