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

Allow AccountSetup to be created with async function to handle completeness of the account setup #73

Merged
merged 24 commits into from
Nov 7, 2024

Conversation

pauljohanneskraft
Copy link
Contributor

@pauljohanneskraft pauljohanneskraft commented Sep 3, 2024

Allow AccountSetup to be created with async throwing function to handle completeness of the account setup

♻️ Current situation & Problem

When an account setup is successful, one may want to run additional code that is async and throwing to finish the account setup completely.

Specifically in ENGAGE-HF, we need to call a Firebase function when a new account has been created before the account can actually be used.

⚙️ Release Notes

  • Allow setupComplete parameter in AccountSetup to be async-throwing.

📚 Documentation

Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.

✅ Testing

Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.

📝 Code of Conduct & Contributing Guidelines

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

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 74.71264% with 44 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (7ffb73d) to head (f082fa8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...Account/Views/AccountSetup/AccountSetupState.swift 0.00% 35 Missing ⚠️
Sources/SpeziAccount/AccountSetup.swift 94.74% 3 Missing ⚠️
...Account/ViewModifier/AccountRequiredModifier.swift 81.25% 3 Missing ⚠️
Sources/SpeziAccount/Views/AccountSummaryBox.swift 91.67% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
- Coverage   83.79%   83.23%   -0.55%     
==========================================
  Files         122      121       -1     
  Lines        5204     5245      +41     
==========================================
+ Hits         4360     4365       +5     
- Misses        844      880      +36     
Files with missing lines Coverage Δ
...ces/SpeziAccount/Environment/AccountViewType.swift 100.00% <ø> (ø)
...es/SpeziAccount/Environment/FollowUpBehavior.swift 50.00% <ø> (ø)
...s/SpeziAccount/Environment/PasswordFieldType.swift 95.00% <ø> (-1.15%) ⬇️
...SpeziAccount/Environment/PreferredSetupStyle.swift 100.00% <ø> (ø)
...Views/AccountSetup/DefaultAccountSetupHeader.swift 100.00% <100.00%> (ø)
...count/Views/AccountSetup/ExistingAccountView.swift 100.00% <100.00%> (ø)
...Account/Views/AccountSetup/FollowUpInfoSheet.swift 88.97% <100.00%> (+0.16%) ⬆️
Sources/SpeziAccount/Views/SignupFormHeader.swift 100.00% <100.00%> (ø)
Sources/SpeziAccount/AccountSetup.swift 91.42% <94.74%> (-0.66%) ⬇️
...Account/ViewModifier/AccountRequiredModifier.swift 87.10% <81.25%> (-3.28%) ⬇️
... and 2 more

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ffb73d...f082fa8. Read the comment docs.

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks for the great work 🚀

I think the PR is generally in a good shape. Just had a few comments and ideas that discusses some of the detailed design of this PR.

Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
@pauljohanneskraft pauljohanneskraft force-pushed the feature/async-throwing-setup branch from 318ed30 to 71b8a89 Compare September 12, 2024 18:04
@pauljohanneskraft pauljohanneskraft changed the title Allow AccountSetup to be created with async throwing function to handle completeness of the account setup Allow AccountSetup to be created with async function to handle completeness of the account setup Sep 12, 2024
@pauljohanneskraft
Copy link
Contributor Author

I reverted the changes that are needed in ENGAGE-HF and will open a different PR instead, since this might still be a nice addition, but actually isn't needed urgently anymore.

Feel free to keep this open / ask for adaptions though!

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the work here @pauljohanneskraft! I had a comment about the API surface were we could improve the naming; apart from this the change looks good and once @Supereg's comment are addressed we can merge this as a nicer generalization of this feature.

Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the updates @pauljohanneskraft!

@Supereg feel free to provide a last review and merge the PR before we tag a next major version.

@PSchmiedmayer PSchmiedmayer requested a review from Supereg October 19, 2024 22:27
Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the delay here. The PR is in a very good shape. Great work 🚀 I only had some smaller comments.

Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Sources/SpeziAccount/SpeziAccount.docc/SpeziAccount.md Outdated Show resolved Hide resolved
Sources/SpeziAccount/Environment/AccountRequiredKey.swift Outdated Show resolved Hide resolved
pauljohanneskraft and others added 2 commits November 4, 2024 12:34
# Add `AccountKeyRequirement.manual`

## ♻️ Current situation & Problem
There is currently no way to add AccountKeys that shouldn't be shown to
the user. Since this might still be reasonable though (e.g. to check a
status of the user like enrollment in a study), we introduce
`AccountKeyRequirement.manual`.


## ⚙️ Release Notes 
- Add `AccountKeyRequirement.manual` to allow account keys that are not
automatically shown to the user, either used entirely internally or
exposed by custom UI elements.


## 📚 Documentation
*Please ensure that you properly document any additions in conformance
to [Spezi Documentation
Guide](https://github.com/StanfordSpezi/.github/blob/main/DOCUMENTATIONGUIDE.md).*
*You can use this section to describe your solution, but we encourage
contributors to document your reasoning and changes using in-line
documentation.*


## ✅ Testing
*Please ensure that the PR meets the testing requirements set by CodeCov
and that new functionality is appropriately tested.*
*This section describes important information about the tests and why
some elements might not be testable.*


## 📝 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).

---------

Co-authored-by: Andreas Bauer <[email protected]>
@Supereg Supereg force-pushed the feature/async-throwing-setup branch from 76ceb51 to fb27fe9 Compare November 4, 2024 13:02
@Supereg
Copy link
Member

Supereg commented Nov 4, 2024

To save you some context switches and time, I went forward and quickly incorporated the changes I had in my comments and rebased onto the latest changes from main. I noticed that in your implementation the loadingExistingAccount was never actually set anymore. I fixed that and merged that state with the isCompletingSetup state you introduced. I used this step to also polish the AccountSetupState implementation a bit and fully exposed it, replacing the previous underscore implementation.
Hope that works and feel free to merge the PR if you are happy with the changes. We can release these as 2.1.0 then 👍

@PSchmiedmayer
Copy link
Member

Amazing; thank you @Supereg!

Feel free to merge the PR if it looks good to you @pauljohanneskraft 🚀

Copy link
Contributor Author

@pauljohanneskraft pauljohanneskraft left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, just two minor comments that may be irrelevant - let me know what you think 😊 otherwise I'd be happy to merge, especially that new @Entry API looks really nice 🎉

Sources/SpeziAccount/AccountSetup.swift Outdated Show resolved Hide resolved
Sources/SpeziAccount/AccountSetup.swift Show resolved Hide resolved
@Supereg Supereg merged commit 0e4dcc7 into main Nov 7, 2024
11 checks passed
@Supereg Supereg deleted the feature/async-throwing-setup branch November 7, 2024 19:28
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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants