[HOLD for payment 2024-03-20] HIGH: [$500] Deeplink - User is not redirected to public room if the app is already open

@stitesExpensify

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.42.0

Reproducible in staging?: Yes
Reproducible in production?: Yes

Issue reported by: Applause -Internal team
Slack conversation: Found while executing tests for https://expensify.slack.com/archives/C9YU7BX5M/p1708359665549809

Action Performed:

Open the app and Sign out if needed
Deep link to this public room from another app like Gmail or slack: https://staging.new.expensify.com/r/6776919265133947

Expected Result:

Navigating to the deeplink should work and take the user to the public room.

Actual Result:

If the app is already open, the user is navigated to the login screen instead of the room.

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?

Screenshots/Videos

Bug6384793_1708366194698.Screen_Recording_20240219_201845_New_Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01615ede560b1898a9
Upwork Job ID: 1761064938880024576
Last Price Increase: 2024-02-23
Automatic offers:

dukenv0307 | Contributor | 0

Issue Owner

Current Issue Owner: @stitesExpensify

@laurenreidexpensify

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

@isagoico

@isagoico thinks this might be related to #36812
Adding to #vip-vsb project.

@laurenreidexpensify

@laurenreidexpensify Is this open to contributors? (I see it doesn't have Help Wanted label but not sure if it's intentional)

Proposal

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

Can't deep link to public room after sign-out from the app.

What is the root cause of that problem?

When we deep link to a report, it will check whether authenticated status is false.

App/src/libs/actions/Report.ts

Lines 2204 to 2206 in d33bf6e

    
           if (reportID && !isAuthenticated) { 
        
               // Call the OpenReport command to check in the server if it's a public room. If so, we'll open it as an anonymous user 
        
               openReport(reportID, [], {}, '0', true);

The authenticated status is passed from this useEffect with an empty deps which means the value is never updated.

App/src/Expensify.js

Lines 200 to 212 in d33bf6e

    
               // Open chat report from a deep link (only mobile native) 
        
               Linking.addEventListener('url', (state) => { 
        
                   Report.openReportFromDeepLink(state.url, isAuthenticated); 
        
               }); 
        
               return () => { 
        
                   if (!appStateChangeListener.current) { 
        
                       return; 
        
                   } 
        
                   appStateChangeListener.current.remove(); 
        
               }; 
        
               // eslint-disable-next-line react-hooks/exhaustive-deps -- we don't want this effect to run again 
        
           }, []);

So, when we sign out, the authenticated status is still true, thus we can't deep link to a public room.

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

We should put the linking listener on a new useEffect with authenticated status as the deps and reregister the listener.

useEffect(() => {
    const linking = Linking.addEventListener('url', (state) => {
        Report.openReportFromDeepLink(state.url, isAuthenticated);
    });
    return linking.remove;
}, [isAuthenticated])

What alternative solutions did you explore? (Optional)

Create a ref for isAuthenticated so we don't need to re-register the listener.

Proposal

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

If the app is already open, the user is navigated to the login screen instead of the room.

What is the root cause of that problem?

In here, when we register the listener, we use isAuthenticated from the component state, but isAuthenticated is not in the useEffect dependency list here, so even when isAuthenticated turns from true to false, it will still remain true in the listener. Then it will not openReport on the public room here, leading to the issue.

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

We should not depend on the component state inside openReportFromDeepLink, that utility should remain independent from any component's lifecycle and works on it's own.

To do that we can use the same approach we're already using for the isAnonymousUser check.

Define a isAuthenticated method here and export it.

function isAuthenticated(): boolean {
    return !!sessionAuthToken;
}

Use it here and remove the isAuthenticated param that relies on component lifecycle

if (reportID && !Session.isAuthenticated()) {

function openReportFromDeepLink(url: string) {

The openReportFromDeepLink cannot be used independent of any specific component, and it won't have such issues like this. The Session.isAuthenticated can be used where the authentication status is needed too.

What alternative solutions did you explore? (Optional)

NA

@laurenreidexpensify

@laurenreidexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

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

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

@sobitneupane

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

fix public room is not opened #37664

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

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

@sobitneupane requires payment through NewDot Manual Requests
@dukenv0307 requires payment automatic offer (Contributor)

@sobitneupane

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:

[@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
[@sobitneupane] 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:
[@sobitneupane] 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:
[@sobitneupane] Determine if we should create a regression test for this bug.
[@sobitneupane] 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.
[@laurenreidexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@sobitneupane

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:

[@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#18293

[@sobitneupane] 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:

#18293 (comment)

[@sobitneupane] 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:

It is an edge case which could easily be missed.

[@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

[@sobitneupane] 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.

#36811 (comment)

Regression Test Proposal

Open the app and Sign out if needed
Click on the deep link to any public room from another app like Gmail or slack
Verify that navigating to the deep link works and user is directed to the public room in Expensify app.

Do we agree 👍 or 👎

@sobitneupane

Payment Summary:

C+ @sobitneupane $500 please request in newdot
Contributor: @dukenv0307 $500 payment issued in Upwork

@sobitneupane

$500 approved for @sobitneupane based on summary.

Navigation Menu

Repository navigation

[HOLD for payment 2024-03-20] HIGH: [$500] Deeplink - User is not redirected to public room if the app is already open #36811

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

25 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (reportID && !isAuthenticated) {
	// Call the OpenReport command to check in the server if it's a public room. If so, we'll open it as an anonymous user
	openReport(reportID, [], {}, '0', true);

	// Open chat report from a deep link (only mobile native)
	Linking.addEventListener('url', (state) => {
	Report.openReportFromDeepLink(state.url, isAuthenticated);
	});

	return () => {
	if (!appStateChangeListener.current) {
	return;
	}
	appStateChangeListener.current.remove();
	};
	// eslint-disable-next-line react-hooks/exhaustive-deps -- we don't want this effect to run again
	}, []);

[HOLD for payment 2024-03-20] HIGH: [$500] Deeplink - User is not redirected to public room if the app is already open #36811

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Activity

melvin-bot commented on Feb 19, 2024

mallenexpensify commented on Feb 19, 2024

dukenv0307 commented on Feb 22, 2024

bernhardoj commented on Feb 22, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

dukenv0307 commented on Feb 22, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot commented on Feb 23, 2024

melvin-bot commented on Feb 23, 2024

25 remaining items

melvin-bot commented on Mar 13, 2024

melvin-bot commented on Mar 13, 2024

melvin-bot commented on Mar 13, 2024

sobitneupane commented on Mar 20, 2024

sobitneupane commented on Mar 20, 2024

laurenreidexpensify commented on Mar 21, 2024

JmillsExpensify commented on Mar 22, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions