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

[HOLD for payment 2024-10-09] [$250] Onboarding - Onboarding Modal opens when returning back online #49499

Closed
2 of 6 tasks
IuliiaHerets opened this issue Sep 19, 2024 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 19, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: v9.0.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

  1. Sign in with a new user
  2. On the Onboarding modal turn off the internet
  3. Go through the onboarding flow and click continue
  4. Refresh the page to kill the app
  5. Go back online and reload

Expected Result:

Reloading after going back online should not bring the Onboarding flow back

Actual Result:

Reloading after going back online bring back the Onboarding flow for a moment

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6608836_1726747731805.Onboarding_01.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836961456209861456
  • Upwork Job ID: 1836961456209861456
  • Last Price Increase: 2024-09-20
  • Automatic offers:
    • allgandalf | Contributor | 104080039
Issue OwnerCurrent Issue Owner: @zanyrenney
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@IuliiaHerets
Copy link
Author

@jliexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@jliexpensify
Copy link
Contributor

Able to repro this one

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021836961456209861456

@melvin-bot melvin-bot bot changed the title Onboarding - Onboarding Modal opens when returning back online [$250] Onboarding - Onboarding Modal opens when returning back online Sep 20, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

@dominictb
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Reloading after going back online bring back the Onboarding flow for a moment

What is the root cause of that problem?

When calling CompleteGuidedSetup API, we didn't set hasCompletedGuidedSetupFlow: true

API.write(WRITE_COMMANDS.COMPLETE_GUIDED_SETUP, parameters, {optimisticData, successData, failureData});

so when users reload, the onboarding flow will be shown again

What changes do you think we should make in order to solve the problem?

set hasCompletedGuidedSetupFlow: true optimistically when calling CompleteGuidedSetup API. We should reset hasCompletedGuidedSetupFlow: false in failureData

What alternative solutions did you explore? (Optional)

@huult
Copy link
Contributor

huult commented Sep 20, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Onboarding Modal opens when returning back online

What is the root cause of that problem?

CompleteGuidedSetup will be stored locally when there is no internet connection, and hasCompletedGuidedSetupFlow will not be updated because CompleteGuidedSetup has not been executed yet. Once the connection is restored, the condition checking hasCompletedGuidedSetupFlow will still return false until the CompleteGuidedSetup API is fully executed. As a result, the onboarding process will reappear because the condition to show onboarding is checked before CompleteGuidedSetup is completed.

API.write(WRITE_COMMANDS.COMPLETE_GUIDED_SETUP, parameters, {optimisticData, successData, failureData});

return onboarding?.hasCompletedGuidedSetupFlow ?? true;

What changes do you think we should make in order to solve the problem?

We need to add a condition for displaying onboarding. If there is a CompleteGuidedSetup pending for the account, onboarding will not be displayed. By using this method, we can avoid the scenario where, if the CompleteGuidedSetup API call encounters an error, onboarding will still follow the correct flow

// .src/libs/hasCompletedGuidedSetupFlowSelector.ts#L6
function hasCompletedGuidedSetupFlowSelector(onboarding: OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>): boolean {
    // onboarding is an array for old accounts and accounts created from olddot
    if (Array.isArray(onboarding)) {
        return true;
    }

+    const persistedRequests = PersistedRequests.getAll();
+    const hasPendingCompleteGuidedSetupRequest = persistedRequests.some((item) => item.command === WRITE_COMMANDS.COMPLETE_GUIDED_SETUP);

+    // If there's a pending request to complete guided setup, return false
+    if (hasPendingCompleteGuidedSetupRequest) {
+        return false;
+    }

    return onboarding?.hasCompletedGuidedSetupFlow ?? true;
}
POC
Screen.Recording.2024-09-20.at.14.57.18.mp4

@CyberAndrii
Copy link
Contributor

This is a regression from #49263 and was fixed in #49516 by removing the hasCompletedGuidedSetupFlowSelector call. Ideally we should still revert #49263 to prevent bugs in other places

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 22, 2024
Copy link

melvin-bot bot commented Sep 22, 2024

📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@jliexpensify
Copy link
Contributor

Thanks @CyberAndrii - have assigned Vit and @allgandalf to have a look at this and confirm it's a regression fornm their PR.

@mountiny
Copy link
Contributor

I think this is minor, the original bug the Pr was fixing was much more critical

@blazejkustra would you be able to follow up on this one and see why the modal is opening for a bit? Thanks!

@CyberAndrii
Copy link
Contributor

To clarify, this issue was already fixed by #49516. It's just that #49263 changed the default value in the hasCompletedGuidedSetupFlowSelector function from false to true. Then the follow-up PR fixed the initial issue using a completely different appoach (to avoid bugs like this) that no longer uses hasCompletedGuidedSetupFlowSelector. And since it's no longer used, it would make sense to revert the default value to what it was previously.

@huult
Copy link
Contributor

huult commented Sep 23, 2024

We can use my proposal to fix this issue because it solves the problem without affecting other flows.

@blazejkustra
Copy link
Contributor

Update: I have a working fix for this here, but I'm struggling to make the exit animation of the Modal to work correctly, I'm sure I'll update on the progress rather quickly

@blazejkustra
Copy link
Contributor

I'm sure I'll update on the progress rather quickly

Famous last words 💀

Update: We investigated this with @adamgrzybowski, as it is deeply connected to navigation logic, there are several issues with the current setup of onboarding (redundant logic, weird flows etc)

  • hasCompletedGuidedSetupFlow is not optimistically updated when onboarding is finished
  • onboardingFlow is initiated from BottomTabBar.tsx component (❓❗️)
  • For some reason NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR is always mounted, while nested screens are not always accessible
  • There is a sophisticated logic inside OnboardingModalNavigator.tsx to navigate back once the onboarding is finished

All that being said we come up with a solution that refactors a lot of code, and could potentially affect HybridApp - that's why first thing in the morning I'll check with the HybridApp team to make sure we don't introduce any new bugs.

cc @mountiny @allgandalf

@mountiny
Copy link
Contributor

@blazejkustra thanks for the update, how its going?

@blazejkustra
Copy link
Contributor

I'll update shortly, we are testing the refactor on HybridApp with @mateuuszzzzz

@allgandalf
Copy link
Contributor

double mention @blazejkustra 😆

@jliexpensify
Copy link
Contributor

Just a heads up I'm OOO from 3rd to 14th, but I don't think anything is needed from me during this period. Feel free to reassign to another B0 if needed - otherwise I will catch up when I'm back.

Copy link

melvin-bot bot commented Oct 1, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@allgandalf
Copy link
Contributor

PR hit production today, payment date should be 9th. @mountiny can you reassign a BZ since @jliexpensify is OoO? Also what would be the final payment here given that it was a full logic refactor of the onboarding flow?

@mountiny mountiny added Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 2, 2024
@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2024
@mountiny mountiny added Overdue Awaiting Payment Auto-added when associated PR is deployed to production labels Oct 2, 2024
@mountiny mountiny changed the title [$250] Onboarding - Onboarding Modal opens when returning back online [HOLD for payment 2024-10-09] [$250] Onboarding - Onboarding Modal opens when returning back online Oct 2, 2024
@mountiny mountiny added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 2, 2024
Copy link

melvin-bot bot commented Oct 2, 2024

Triggered auto assignment to @zanyrenney (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@mountiny
Copy link
Contributor

mountiny commented Oct 2, 2024

$250 to @allgandalf for their review

@melvin-bot melvin-bot bot added the Overdue label Oct 4, 2024
@allgandalf
Copy link
Contributor

$250 to @allgandalf for their review

Can the compensation be $375 for this one ?, I tested lot of cases from testrail for this logic refactor 😅

Copy link

melvin-bot bot commented Oct 7, 2024

@mountiny, @blazejkustra, @zanyrenney, @allgandalf Eep! 4 days overdue now. Issues have feelings too...

@mountiny
Copy link
Contributor

mountiny commented Oct 8, 2024

Yep I think this is fair, thanks for your willingness to step up and help with this one

@zanyrenney
Copy link
Contributor

Hey @allgandalf well done for the extra effort here.

payment summary

$250 + $100 bonus for the work, paid via upwork to @allgandalf

@github-project-automation github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Oct 8, 2024
@melvin-bot melvin-bot bot removed the Overdue label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Done
Development

No branches or pull requests

10 participants