-
Notifications
You must be signed in to change notification settings - Fork 114
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
Sync: merge onboarding_routing_sept_2023
into service_rewrite_2023
#1067
Merged
shankari
merged 18 commits into
e-mission:service_rewrite_2023
from
JGreenlee:merge-onboarding-servicerewrite
Oct 14, 2023
Merged
Sync: merge onboarding_routing_sept_2023
into service_rewrite_2023
#1067
shankari
merged 18 commits into
e-mission:service_rewrite_2023
from
JGreenlee:merge-onboarding-servicerewrite
Oct 14, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We had this issue (e-mission/e-mission-docs#999) where if the app was exited in the middle of onboarding, and then opened again, this error would show. I found out that this occured because the LabelTab was being rendered for a split second until the onboarding state was still being resolved. It takes some time to determine the onboarding state, and it requires waiting for native calls, which is why `getPendingOnboardingState` in onboardingHelper returns a Promise. But until this promise is resolved, we don't really know if we should show a) onboarding flow or b) the main app with tab navigation. We had been keeping `pendingOnboardingState` as `null` if the onboarding was finished. In this case, there was no 'pending' onboarding state and we would just continue to the main app. But a safer thing to do here is have onboarding state be null **only** if it hasn't been determined yet. If it *has* been determined, then we will explictly mark it with a route of DONE. So if it's DONE we show the main app, if it's something other than DONE, we show the onboarding flow, and if it's null, we can show a big loading spinner while we determine what state we are in. In doing this, we no longer just keep track of *pending* onboarding states - we must always keep track of an onboarding state, whether it is DONE or not. So while making this adjustment, I renamed the variable throughout as simply 'onboardingState'.
- increase page padding - increase gap between paragraphs - increase font sizes - have content vertically centered by margin: 'auto'
🐛 Onboarding fixes 10/3
the old "Logout" functionality cleared the UI config but not other storage keys, such as those for marking consented and marking intro done. A backwards-compat check is needed: if the UI config is blank but intro done or consented are marked true, we need to clear everything.
🐛 Backwards Compat Fix for New Onboarding
in the logs for the bug runs: e-mission/e-mission-docs#1002 I'm not seeing log statements indicating that the user is logged in (which I should see) and so I'm wondering how the onboarding state is moving on in that case. Adding this log statement to investigate.
More token log statements
Now that we have an App component, we can think about hoisting state higher in the component tree so it can be shared instead of being redundant. Rather than hooking into usePermissionStatus from multiple different components, it will be cleaner to hook into it once in the App component and share it downstream via the AppContext. When there were multiple instances of usePermissionStatus, we ran into issues where only one was recieving updates (e-mission/e-mission-docs#1002) Hoisting the permission state to the App component resolves these issues because now there is only one instance of usePermissionStatus
If the token that the user enters belongs to a valid program, but is not a valid user token for that program, registration will fail. We should catch this and not allow the user to go any further through onboarding - rather, we should give them a message explaining what happened and send them back to the "Welcome" page so they can try again with a valid OPCode. Unfortunately they have to go through onboarding again - they will not have to re-do permissions though. The best UX would be achieved if we could ensure that the token is valid BEFORE they go through everything else. However, this would require contacting the server before the user consented, which we cannot do. --In addition, we'll add an error message if the user has not yet registered, but has advanced to preloading survey responses. This will not happen unless something is broken.
on staging, I noticed that "why we need this" had an explanation of "how to fix" rather than what the permissions are used for. I have fixed this by allowing the default explanation to persist for ios -- custom messages for how to fix permissions are maintained in the descriptions of the permissions in `checklist`
addressing comment from: Cleanup notes for new onboarding
As a result of e-mission/e-mission-data-collection#209, we now have a popup for this permission, so all the user needs to do is allow it! Translate PR to follow with this small change to en.json
🐛 Onboarding Fixes Oct 6
…ission/e-mission-phone into service_rewrite_2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were some hairy merge conflicts making it difficult to sync these.
I resolved them and then did a quick check to make sure the app still works.