Call onboarding dialog only once #144

Open
jose wants to merge 1 commits from onboarding-dialog-fix into master
jose commented 3 weeks ago
Owner
  • Removed earlier call to onboarding dialog, choosing to call it only at the end to make way for any error notifications
- Removed earlier call to onboarding dialog, choosing to call it only at the end to make way for any error notifications
jose added 1 commit 3 weeks ago
3e6e959b19 Call onboarding dialog only once
trentlarson reviewed 3 weeks ago
}
if (!settings.finishedOnboarding) {
if (this.needsOnboarding) {
Poster
Owner

Note that there is a hole in the logic: needsOnboarding is never set to false after they have seen it. (It may not cause a problem because we always recheck, but it does mean that this new variable doesn't always reflect the true state of the user.)

Another possible complication is: after they finish onboarding, they no longer need onboarding, so this method will always check the DB again. (That doesn't seem like it'll cause a problem; I just mention it because I think it's not intended because we keep checking the flag.)

This is a common problem with new state variables. If you ever get a chance to play with functional programming (eg. Haskell, Scheme, LISP) then it'll be interesting how it makes you think differently... and you're able to avoid a lot of state variables in the classes because you always pass all the variables you need.

Anyway, I think it would be fine to eliminate the needsOnboarding flag and always check with retrieveSettingsForActiveAccount in here. That way we don't have to worry about setting a variable. (Another more efficient option is to eliminate that additional DB check here and pass in the flag to the checkOnboarding method from above -- but if it makes things more complicated and harder to understand then it's not worthwhile.)

Note that there is a hole in the logic: needsOnboarding is never set to false after they have seen it. (It may not cause a problem because we always recheck, but it does mean that this new variable doesn't always reflect the true state of the user.) Another possible complication is: after they finish onboarding, they no longer need onboarding, so this method will always check the DB again. (That doesn't seem like it'll cause a problem; I just mention it because I think it's not intended because we keep checking the flag.) This is a common problem with new state variables. If you ever get a chance to play with functional programming (eg. Haskell, Scheme, LISP) then it'll be interesting how it makes you think differently... and you're able to avoid a lot of state variables in the classes because you always pass all the variables you need. Anyway, I think it would be fine to eliminate the needsOnboarding flag and always check with retrieveSettingsForActiveAccount in here. That way we don't have to worry about setting a variable. (Another more efficient option is to eliminate that additional DB check here and pass in the flag to the checkOnboarding method from above -- but if it makes things more complicated and harder to understand then it's not worthwhile.)
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.