-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Supporting to distinguish multiple consent forms during onboarding via an identifier (#50) #51
Conversation
…ew parameter "identifier" added to OnboardingConsentView and OnboardingConstraint.store(...). The parameter can be optionally specified when using an OnboardingConsentView to distinguish multiple consent forms when storing. If not specified in OnboardingConsentView, the default value „DefaultConsentDocument“ is set. UITests/TestApp has been changed accordingly to test the new functionality of having multiple (in this case two) consent forms. OnboardingConsentMarkdownTestView and OnboardingConsentMarkdownRenderingView have been renamed to OnboardingFirstConsentMarkdownRenderingView and OnboardingFirstConsentMarkdownRenderingView accordingly. Views for a second consent form were added (OnboardingSecondConsentMarkdownRenderingView and -TestView). ExampleStandard has been changed to distinguish the two documents based on their identifiers ("FirstConsentDocument" and "SecondConsentDocument") during store and loadConsentDocument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @RealLast for your first contribution to Spezi and tackling this issue. This has been a long-running limit of the onboarding module and is great to be addressed!
Sorry for the delay, we have been quite busy with a few end-of-June deadlines, sorry about this!
I have added a few comments across the PR with he majority focusing on how we can
- Make this a backward-compatible change
- Make sure that we use this as an opportunity to address some limitations that we currently have in the current implementation (e.g. error propagation from the standard to the view)
Apart from this, it would be great if you can address the failing UI tests on some platforms as we go though this PR.
Please let us know if you have any follow-up questions based on the feedback and additional suggestions!
Thanks again for taking the time to make an open-source contribution!
Tests/UITests/TestApp/Views/OnboardingConsentMarkdownRenderingView1.swift
Outdated
Show resolved
Hide resolved
Thank you very much @PSchmiedmayer for the detailed and thoughtful feedback! I went over it carefully and incorporated your suggestions. I have also added a reply to most of your comments, including some considerations. Regarding the failing test, I saw that we got the following error on iPadOS. "The request was denied by service delegate (SBMainWorkspace) for reason: NotFound ("Application "edu.stanford.spezi.onboarding.testappuitests.xctrunner" is unknown to FrontBoard")" I am not sure how to solve this, can I do anything on my side to fix this? Thanks again! |
…r the Standard is ConsentConstrain or OnboardingConstraint and invoke the appropriate store function. Added progress view to OnboardingConsentView, which is shown while the PDF is exported. Added new state .storing and .stored to ConsentViewState. The view enters the storing state when the user presses "I consent". While in .storing state, the consent button is disabled. It is very important that the user cannot press "I consent" more than once.
…due to bad timing.
There was a problem hiding this 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 improvements @RealLast!
I took a look at the iPad GitHub runner problem and it seems like the relevant iPad we used is no longer installed by default so I switched it to the newer iPad Air model to successfully start the test.
Still seems like some of the tests are failing on the CI which might be due to multiple changes in the UI and elements. The best way to identify what is going wrong is going to the checks of the PR (https://github.com/StanfordSpxezi/SpeziOnboarding/pull/51/checks) and downloading the .xcresult
files that are provided as artifacts of the failed builds. They can be opened in Xcode and inspected (including a video recording of the failed test). These things can be frustrating so feel free to let me or @philippzagar know if you run into any issues or elements where you are not sure how to solve a failing test!
Apart from this: I took a look at the PR code and we are making some great progress! Thanks for all the improvements and incorporating this in the code base. I have looked at the current state and provided some additional feedback and suggestions based on the new additions and improvements. Most of them are now UI-state related and would be great to be incorporated with the latest improvements in the PR 🚀
Let me know if you have any questions or follow-up comments. I think after resolving those issues the PR should be ready to be merged, thanks for all the work 👍
Sources/SpeziOnboarding/SpeziOnboarding.docc/ObtainingUserConsent.md
Outdated
Show resolved
Hide resolved
Tests/UITests/TestApp/Views/OnboardingConsentMarkdownRenderingView.swift
Outdated
Show resolved
Hide resolved
…cator directly to the button and removed previous progress overlay. Added progress indicator to share button, which is active while the share sheet is being shown. Changed share button to be disabled while the PDF is being exported. Disabled back button while PDF is being exported. Removed unused view state "stored".
Hi @PSchmiedmayer, thanks again for the thoughtful feedback! I went over it carefully and have incorporated all of it, and hope I haven't overlooked anything. I also closed most of the conversations except two which I left open for potential further discussion. Let me know what you think :) Thanks again! Best |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @RealLast for the great changes and improvements; I think we are getting really close.
It seems like the UI tests are failing due to some changes to iOS 18, and I have pushed some elements for this in #52 that we should also incorporate here or merge the other PR first and then merge this one.
I have added some last comments with one larger one that handles future backward compatibility but should be easily doable.
We can keep the OnboardingDataSource
as is for now, but we will then deprecate it in an other PR where we might introduce additional context to the consent document rendering and eventual export.
… PDF and it's identifier. Changed protocol signature of ConsentConstraint to expect a ConsentDocumentExport, instead of the previous implementation using document identifier strings.
Thanks again @PSchmiedmayer for the suggestions! Also thanks for checking again the test runners! Curious to see whether they will run through now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RealLast Thank you for the further improvements; the major last element would be a removal of a breaking change + suggestions to structure the ConsentDocumentExport types.
After that I see this PR as ready to be merged; thanks for all the time and effort that went into the PR!
Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift
Outdated
Show resolved
Hide resolved
Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift
Outdated
Show resolved
Hide resolved
Sources/SpeziOnboarding/ConsentView/ConsentDocumentIdentifier.swift
Outdated
Show resolved
Hide resolved
Reverted changes made to OnboardingDataSource.
Thanks @PSchmiedmayer, I have incorporated the suggestions! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @RealLast; the changes look great!
The only change left is the discussion above, apart from this the PR would be ready to be merged; great job! 🚀
I also see that the UI tests are currently failing again. Let me know if that looks like a performance regression on the runners.
I can then re-check our CI later this week, I might need to reduce the runner instances on one machine a bit to make this more performant.
Thank you very much @PSchmiedmayer, excited to see the code getting merged into Spezi! Regarding the failing tests, to me it looks like a performance issue, or in general an issue with the runners. I attached a screenshot of the error from the xcresult file: |
Thanks for taking a look at the tests; I will try to investigate this later this week! |
…r a default documentIdentifier. Changed OnboardingConsentView and OnboardingDataSource to use this default identifier, if no document identifier was provided.
Thanks! I have incorporated the last changes regarding the default document identifier. |
@RealLast We have merged the fixed for the builds on the main branch and have also updated out build agents; it would be amazing if you can merge main to this branch to eventually merge this PR; let me know if you have any questions or run into any issues! |
Hi @PSchmiedmayer that's great, thanks for the update! I just pulled in the changes and merged them into this branch. I tested everything again locally on my PC and it worked; hope it will work on the test runners too! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RealLast Looks great, all the builds are passing now!
I only had a very small comment that occurred in the merge, once these are resolved we can merge the PR. The CodeCov element can't pass as codecov changed their logic that breaks public forks but we can merge it despite that.
Feel free to tag me once you have resolved the last comments and I can then merge the PR with my admin permissions.
Thanks for all the work here that went into the PRs and improving Spezi Onboarding!
Thanks @PSchmiedmayer for checking it, I have resolved the comment! Waiting for the merge :) |
Support for storing multiple consent forms (#50)
♻️ Current situation & Problem
Closes #50
In the case of multiple
OnboardingConsentView
, it was impossible to distinguish the individual consent forms when storing in theStandard
, as the "store" function of theOnboardingConstraint
only receives aPDFDocument
as a parameter and no additional information.⚙️ Release Notes
OnboardingConsentView
andOnboardingConstraint
.ExampleStandard
has been changed to distinguish the two consent forms based on their identifiers "FirstConsentDocument" and "SecondConsentDocument"📚 Documentation
*Documented changes in the appropriate Markdown files.
✅ Testing
OnboardingConsentView
with an identifier different from the first one.If you accept this PR, there are implications for SpeziTemplateApplication and SpeziStudyApplication, as both use an
OnboardingConsentView
and require to add the "identifier" parameter to the respectiveStandard
. I am happy to make those changes if this PR gets accepted.📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: