-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$250] Android – Sign in – Able to dismiss the Onboarding modal using back button #50177
Comments
Triggered auto assignment to @twisterdotcom ( |
@twisterdotcom 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 |
ProposalPlease re-state the problem that we are trying to solve in this issue.Android – Sign in – Able to dismiss the Onboarding modal using back button What is the root cause of that problem?We didn't prevent pressing Back button on native platform for What changes do you think we should make in order to solve the problem?Using the similar approach with // OnboardingPersonalDetails/index.native.tsx
const currentRoute: NavigationPartialRoute | undefined = navigationRef.getCurrentRoute();
const rootState = navigationRef.getRootState();
const onboardingNavigatorStackLength = adaptedState.routes.filter(
(route) => route.name === NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR,
).length
useFocusEffect(
useCallback(() => {
// Return true to indicate that the back button press is handled here
const backAction = () => {
if (currentRoute?.name === SCREENS.ONBOARDING.PERSONAL_DETAILS && onboardingNavigatorStackLength === 1) {
return true;
}
return false;
};
const backHandler = BackHandler.addEventListener('hardwareBackPress', backAction);
return () => backHandler.remove();
}, [currentRoute?.name]),
); What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
Edited by proposal-police: This proposal was edited at 2024-10-04 05:42:22 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.User is able to dismiss the onboarding flow on the personal details step. What is the root cause of that problem?The onboarding has 3 steps, purpose, work information, and personal details. On the purpose page, we already override the back button to prevent the user from going back. App/src/pages/OnboardingPurpose/index.native.tsx Lines 8 to 18 in 0ed41d7
But this isn't present in either work information or personal details. However, that's not the problem. When going back from the work information or personal details, the user should be going back to the correct step instead of preventing them from going back. What happens in this issue is, that when we reopen the app, the app will show the last step of the onboarding step, in our case, the personal details step. App/src/libs/actions/Welcome/OnboardingFlow.ts Lines 106 to 113 in 0ed41d7
That's possible because of
So, in the nav stack, only the onboarding personal details page is present. So, when going back, instead of going back to the purpose step, the page is closed. Why this doesn't happen when we press go back using the in-app back button? That's because we reset the root state which contains the correct state for the onboarding navigator. App/src/libs/actions/Welcome/OnboardingFlow.ts Lines 140 to 145 in 0ed41d7
App/src/libs/actions/Welcome/OnboardingFlow.ts Lines 52 to 101 in e4969b2
But this solution will cause a brief LHN to be seen. What changes do you think we should make in order to solve the problem?We should improve First, create a new function in a separate file called
Next, modify the onboarding navigator state here to use the new state from App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 255 to 257 in 0ed41d7
Last, we can remove |
@blazejkustra we fixed this one right!!!!!!!!!!!!!!!!!!!!!!!!!!!! 😮 |
Sorry, should this be closed or are you saying it's a regression @allgandalf? |
@allgandalf / @blazejkustra I can recreate this, so I'm going to make it external... but let me know if you think this should be handled by somebody specific. |
Job added to Upwork: https://www.upwork.com/jobs/~021844399665498037051 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox ( |
@bernhardoj Your proposal is a potential solution, but could you please update it with the latest main branch and handle the duplicate OnboardingEmployees state issue as well? Screen.Recording.2024-10-12.at.13.18.17.mp4 |
Updated my proposal.
I can't repro it with my proposal. |
@bernhardoj It still reproduces on my side. Screen.Recording.2024-10-13.at.18.16.30.mp4 |
Ah, I think it's the same issue as this one. Can you check if applying the solution there fixes the issue? |
👋 @twisterdotcom onboarding related bugs, I'd put those in #convert going forward. |
Added to Convert. |
@bernhardoj The selected proposal from #50064 retains the use of |
I don't think they will use |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@twisterdotcom @suneox this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
@suneox any thoughts on @bernhardoj's response, or are we waiting for more proposals? |
I'll check @bernhardoj's response with issue #50064 in few hours |
I’ve verified that this issue can be resolved in #50064, so @bernhardoj’s proposal LGTM. Based on the onboarding steps, we will push the missing onboarding route during initialization 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@bernhardoj Please tag me on the PR once it's ready 🙏 |
@blazejkustra sure, @danieldoglas waiting for an assignment |
@danieldoglas, @twisterdotcom, @suneox Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
Assigned. |
PR is ready cc: @suneox @blazejkustra |
It looks good to me. However, I'm wondering if we want to have the swipe back action possible after freshly opening the app. e.g. swipe back on the mobile browser ios means go back in history so that would be inconsistent. If yes then 🟢 If not then I guess we could use a param for the |
I’ve verified that it still works as expected |
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: 9.0.44-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5043083
Email or phone of affected tester (no customers): ponikarchuks+1331024@gmail.com
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
Onboarding modal present
Actual Result:
Onboarding modal disappears
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6623468_1727975678459.And_signin.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @suneoxThe text was updated successfully, but these errors were encountered: