[HOLD for payment 2024-03-18] [$500] Public room - Sign-in modal is hidden behind welcome modal after selecting an option

[kavimuru]

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: 1.4.31-2
Reproducible in staging?: y
Reproducible in production?: new feature
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Log out if logged in.

2. Navigate to public room https://staging.new.expensify.com/r/2376199970894587

3. Select any option in the welcome message modal.

Expected Result:

Welcome modal will dismiss and sign-in modal will appear after selecting an option.

Actual Result:

The link changes to sign-in URL, but the welcome modal does not dismiss and the sign-in modal is hidden behind it.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6354272_1706163569570.20240125_113158.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bdb196958ade050b
• Upwork Job ID: 1752411868444123136
• Last Price Increase: 2024-02-27
• Automatic offers:
• fedirjh | Reviewer | 0
• dukenv0307 | Contributor | 0

Issue Owner

Current Issue Owner: @kevinksullivan

[melvin-bot]

Triggered auto assignment to @marcaaron (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[aimane-chnaif]

Missing test case from #32154

[aimane-chnaif]

What's the expected behavior after opening public link while signed out?

• show FAB as usual
• show welcome modal and selecting any option dismiss modal and show sign-in modal

cc: @stitesExpensify

[s-alves10]

Proposal

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

Sign-in modal is hidden behind welcome modal after selecting an option in public room

What is the root cause of that problem?

We have recently introduced full screen engagement modal named PurposeForUsingExpensifyModal

We pass onPress handler to the MenuItem here

App/src/components/PurposeForUsingExpensifyModal.tsx

Line 123 in 0d0b0a8

onPress: () => completeModalAndClose(messageCopy[choice], choice),

When selecting an option, we redirect to the sign-in modal if the user is not signed in

App/src/components/MenuItem.tsx

Line 381 in 0d0b0a8

onPress={shouldCheckActionAllowedOnPress ? Session.checkIfActionIsAllowed(onPressAction, isAnonymousAction) : onPressAction}

App/src/libs/actions/Session/index.ts

Lines 134 to 139 in 0d0b0a8

	function checkIfActionIsAllowed<TCallback extends (...args: any[]) => any>(callback: TCallback, isAnonymousAction = false): TCallback | (() => void) {
	if (isAnonymousUser() && !isAnonymousAction) {
	return () => signOutAndRedirectToSignIn();
	}
	return callback;
	}

So menu item's onPress handler isn't executed at all.
This is the root cause

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

We can listen navigation state and hide the engagement modal if needed
We show this modal on mount here

App/src/components/PurposeForUsingExpensifyModal.tsx

Lines 91 to 101 in 0d0b0a8

	useEffect(() => {
	const navigationState = navigation.getState();
	const routes = navigationState.routes;
	const currentRoute = routes[navigationState.index];
	if (currentRoute && NAVIGATORS.CENTRAL_PANE_NAVIGATOR !== currentRoute.name && currentRoute.name !== SCREENS.HOME) {
	return;
	}
	Welcome.show(routes, () => setIsModalOpen(true));
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

We can add navigation listener here

    useEffect(() => {
        ~  ~  ~
        Welcome.show(routes, () => setIsModalOpen(true));
        return navigation.addListener('state', (event) => {
            const state = event.data.state;
            const route = state.routes[state.index];
            if (route.name === currentRoute.name) {
                return;
            }
            setIsModalOpen(false);
        });
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, []);

We compare the route's name to determine if we need to hide the modal in the above solution. We can use more optimized or suitable comparison

The solution works as expected

Result

35131.mp4

What alternative solutions did you explore? (Optional)

[thienlnam]

Interesting - will wait to let @stitesExpensify chime in. We'll definitely want to fix this soon, though I don't think this needs to block deploy. This relies on engaging with us from a public room and if you dismiss the screen you'll see the sign in screen

[stitesExpensify]

@MitchExpensify what do you think the intended functionality should be here? I'm thinking that we don't show the engagement modal at all to anonymous users both before and after they sign in. Thoughts?

[dukenv0307]

Bringing my proposal from #35238 (comment) since it will resolve this issue too, it's already based on #35131 (comment)

Proposal

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

Nothing happens. The close and back buttons unresponsive on touches.

What is the root cause of that problem?

The engagement modal is being shown for anonymous users, which is not correct because by design, it should only show for first time logged in users.

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

Do not show engagement modal for anonymous users, we can update here to early return if Session.isAnonymousUser() is true, or we can connect to the SESSION key to the BottomTabBar and use the session?.authTokenType to detect anonymous user, to make sure the block is rerun again when the session auth type changes.

If we don't want to show the engagement modal even after the anonymous user logs in, we can make sure to set isFirstTimeNewExpensifyUser to true and send back from the back-end after the user signs in successfully. Or we can do this in client side (if the user is anonymous) when the user begins the sign in API request in optimisticData and successData.

If instead we want to show the engagement modal after the anonymous user logs in, we have to make sure to remove the policies of the anonymous user in successData after the the user signs in (or the back-end can clear it in Onyx data when returning response). So that this logic will work correctly, otherwise there'll be 2 policies (1 of the signed in user and 1 of the anonymous user) and the engagement modal will not show.

What alternative solutions did you explore? (Optional)

NA

[MitchExpensify]

I'm thinking that we don't show the engagement modal at all to anonymous users both before and after they sign in. Thoughts?

For sure, yes! Lets not show the modal to anonymous users before they sign in

[marcaaron]

Do not show engagement modal for anonymous users, we can update here to early return if Session.isAnonymousUser() is true

Let's just do this.

or we can connect to the SESSION key to the BottomTabBar and use the session?.authTokenType to detect anonymous user, to make sure the block is rerun again when the session auth type changes.

What is the advantage of doing this? Let's do the first one I think.

If we don't want to show the engagement modal even after the anonymous user logs in, we can make sure to set isFirstTimeNewExpensifyUser to true and send back from the back-end after the user signs in successfully. Or we can do this in client side (if the user is anonymous) when the user begins the sign in API request in optimisticData and successData.

👎 - I don't really understand what problem we are trying to solve. Let's fix "sign in modal is hidden behind welcome modal".

If instead we want to show the engagement modal after the anonymous user logs in, we have to make sure to remove the policies of the anonymous user in successData after the the user signs in (or the back-end can clear it in Onyx data when returning response). So that this logic will work correctly, otherwise there'll be 2 policies (1 of the signed in user and 1 of the anonymous user) and the engagement modal will not show.

Again - not too sure what problem we are trying to solve. We agreed to not show them this modal.

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[dukenv0307]

@fedirjh The PR is ready for review.

[aimane-chnaif]

If the end goal is that we don't want to show this modal for anonymous users then we can just check if they are an anonymous user and then don't show the modal.

Looks like this was fixed already in #37673

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.49-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Hide engagement modal for anonymous user #37714

If no regressions arise, payment will be issued on 2024-03-18. 🎊

For reference, here are some details about the assignees on this issue:

• @fedirjh requires payment automatic offer (Reviewer)
• @dukenv0307 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kevinksullivan] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@kevinksullivan, @marcaaron, @fedirjh, @dukenv0307 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[fedirjh]

BugZero Checklist:

• Link to the PR: Add full screen engagement modal to NewDot #32154
• Link to comment: It's a missing test case from the new feature of engagement modal.
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: NA, We have the required regression test

[dukenv0307]

@kevinksullivan The PR was deployed to production 3 weeks ago, could we proceed with payments here?

Thank you!

[melvin-bot]

@kevinksullivan, @marcaaron, @fedirjh, @dukenv0307 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[marcaaron]

Bumping this one.

[melvin-bot]

@kevinksullivan, @marcaaron, @fedirjh, @dukenv0307 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[kevinksullivan]

Sorry for the delay here, all set.