Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Deployment Setup #108

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Improve Deployment Setup #108

wants to merge 21 commits into from

Conversation

PSchmiedmayer
Copy link
Member

Improve Deployment Setup

⚙️ Release Notes

  • Improve Deployment Setup

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Oct 29, 2024
@PSchmiedmayer PSchmiedmayer self-assigned this Oct 29, 2024
Copy link
Contributor

@nriedman nriedman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through and identified some changes that would prevent SwiftLint from throwing errors.

fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Outdated Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
fastlane/SnapshotHelper.swift Show resolved Hide resolved
@PSchmiedmayer
Copy link
Member Author

@nriedman We had a short Slack conversation about this PR. As you can see in the GitHub Action logs, updating some of the dependencies and fixing smaller bugs is currently causing some UI tests to fail. It would be amazing if you can take a look at the current set of tests and see why they are currently failing and fix them so we can merge the PR.

@Supereg While investigating some login issues, I have encountered that the updated Account + Firebase Module no longer reliably populates the account details in the AccountSetup { details in closure:

AccountSetup { details in
if details.invitationCode != nil {
onboardingNavigationPath.nextStep()
} else {
onboardingNavigationPath.append(customView: InvitationCodeView())
}

The check above always results in the InvitationCodeView being shown despite the fact that an invitation account is attached to the user account when logging in with an existing user account. I suspect that there is a concurrent mismatch that is not waiting until the Account Storage provider returns the initial setup of user details or that they are not populated before the closure is called. I think it would make sense to continue to show the loading state until the details are loaded (and maybe show some state in the UI that the user account details are loaded).

At this point, I added the following workaround in the Invitation Code view that continuously checks if the invitation code is loaded and then proceeds with the onboarding flow:

.task {
try? await Task.sleep(for: .seconds(1))
guard account.details?.invitationCode == nil else {
onboardingNavigationPath.removeLast()
onboardingNavigationPath.nextStep()
return
}
accountNotificationsTask = Task.detached { @MainActor in
for await event in accountNotifications.events where event.accountDetails?.invitationCode != nil {
guard (accountNotificationsTask?.isCancelled ?? true) == false else {
return
}
onboardingNavigationPath.removeLast()
onboardingNavigationPath.nextStep()
accountNotificationsTask?.cancel()
}
}
viewState = .idle
}

You can replicate the issue using the local emulator. You can clone the repo and log in with one of the provided accounts by first seeding the Firebase Emulator & then running the emulator.

npm --prefix ENGAGE-HF-Firebase run prepare && firebase emulators:exec -c ./ENGAGE-HF-Firebase/firebase.json --export-on-exit=./firebase 'npm --prefix ./ENGAGE-HF-Firebase/functions run serve:seed'
firebase emulators:start -c ./ENGAGE-HF-Firebase/firebase.json --import ./firebase

If we can guarantee that the account details are loaded before the closure is called, we could remove that workaround and directly navigate to the correct screen in the next step. Let me know what you think about the issue @Supereg 🚀

@Supereg
Copy link
Member

Supereg commented Nov 24, 2024

@PSchmiedmayer StanfordSpezi/SpeziAccount#79 fixes the issue. As described in the PR description, AccountServices did always call the storage provider "in sync" before submitting account details. However, a new requirement with SpeziAccount 2.0 is that StorageProvider must return immediately and perform the loading step asynchronously (e.g., the concept of "incomplete" account details exists). This wasn't checked for in the AccountSetup view. We added this little check and a new unit test checking that behavior.

I verified in this branch removing some of the code that this resolved the issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants