[HOLD for payment 2023-11-02] [$500] Web - Room - Search list can be opened with CMD + K in public room as anonymous user

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to a public room as an anonymous user like https://staging.new.expensify.com/r/6874216302070473
2. Click on the search icon.
3. Close the login menu.
4. Use the shortcut CMD + K.

Expected Result:

Login menu will open when using the shortcut CMD + K, just like when clicking on the search icon in Step 2.

Actual Result:

The search list is opened when using CMD + K.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: v1.3.74-2

Reproducible in staging?: Y

Reproducible in production?: Y

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6214634_20230926_023604.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause-Internal Team

Slack conversation: @

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e7ac225db079b27f
• Upwork Job ID: 1706839068229685248
• Last Price Increase: 2023-10-04
• Automatic offers:
• 0xmiroslav | Reviewer | 27126702
• kushu7 | Contributor | 27126704

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[kushu7]

Proposal

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

Fixing redirect issue with using shortcut CMD + K or CMD + SHIFT + K in public room as anonymous user

What is the root cause of that problem?

The problem is we are not checking if user is anonymous or not. we are just redirecting it to search page or chat page.

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Lines 182 to 191 in 3402597

	this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
	searchShortcutConfig.shortcutKey,
	() => {
	Modal.close(() => {
	if (Navigation.isActiveRoute(ROUTES.SEARCH)) {
	return;
	}
	return Navigation.navigate(ROUTES.SEARCH);
	});
	},

unlike we are checking on Search icon click here

App/src/pages/home/sidebar/SidebarLinks.js

Line 173 in 3402597

onPress={Session.checkIfActionIsAllowed(this.showSearchPage)}

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

We should also check this condition on shortcut click. we can update this function to like we are checking on search icon click.

Modal.close(Session.checkIfActionIsAllowed(() => {
    if (Navigation.isActiveRoute(ROUTES.SEARCH)) {
        return;
    }
    return Navigation.navigate(ROUTES.SEARCH);
}));

and we should update this chat shortcut listener also to respect anonymous users.

and we should add early return condition here to avoid re-navigating to sign-in modal page.

if (Navigation.isActiveRoute(ROUTES.SIGN_IN_MODAL)) {
    return
};

Result

Screen.Recording.2023-09-26.at.5.50.52.PM.mov

What alternative solutions did you explore? (Optional)

We can disable shortcut for anonymous users

[zanyrenney]

adding external.

[melvin-bot]

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

[melvin-bot]

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

[DylanDylann]

Proposal

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

• Anonymous user can access new chat page

What is the root cause of that problem?

• Currently, when pressing "Ctrl + K" without login, we still allow the user to open the search modal

  App/src/libs/Navigation/AppNavigator/AuthScreens.js

  Lines 167 to 180 in 7d1f5c6

  	this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
  	searchShortcutConfig.shortcutKey,
  	() => {
  	Modal.close(() => {
  	if (Navigation.isActiveRoute(ROUTES.SEARCH)) {
  	return;
  	}
  	return Navigation.navigate(ROUTES.SEARCH);
  	});
  	},
  	searchShortcutConfig.descriptionKey,
  	searchShortcutConfig.modifiers,
  	true,
  	);

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

• When user pressing "Ctrl + K", we need to redirect user to login modal like below:

this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
            searchShortcutConfig.shortcutKey,
            () => {
                interceptAnonymousUser(() => {
                    Modal.close(() => {
                        if (Navigation.isActiveRoute(ROUTES.SEARCH)) {
                            return;
                        }
                        return Navigation.navigate(ROUTES.SEARCH);
                    });
                });
            },
            searchShortcutConfig.descriptionKey,
            searchShortcutConfig.modifiers,
            true,
        );

• The interceptAnonymousUser function has been used in other sections in our source code, used to check if the user is an anonymous user:

const isAnonymousUser = Session.isAnonymousUser();
const interceptAnonymousUser = (callback) => {
    if (isAnonymousUser) {
        Session.signOutAndRedirectToSignIn();
    } else {
        callback();
    }
};

What alternative solutions did you explore? (Optional)

• We can also disable the event listener when pressing "Ctrl + K" without login like:

 if (!Session.isAnonymousUser()) {
            this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
                // Handle event logic
            );
        }

Result:

Solution 1:

Screencast.from.29-08-2023.17.31.39.webm

[melvin-bot]

@zanyrenney, @0xmiroslav Eep! 4 days overdue now. Issues have feelings too...

[zanyrenney]

@0xmiroslav please can you review the proposals?

[0xmiros]

reviewing today

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[0xmiros]

@kushu7's proposal looks good to me.
Let's fix Ctrl+Shift+K shortcut as well
🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[pradeepmdk]

@0xmiroslav we need to cover the user entering the URL directly also?

ex: https://staging.new.expensify.com/search

[0xmiros]

@0xmiroslav we need to cover the user entering the URL directly also?

I think that can be separate issue. None of direct urls seem to work correctly for me

[pradeepmdk]

got it Thanks let me raise it separately.

[0xmiros]

i.e. /settings/profile, /settings/profile/display-name

It's weird that I can edit anonymous user profile

[pradeepmdk]

@0xmiroslav ya yes we need to hide that as well.

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @kushu7 🎉 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 📖

[kushu7]

@0xmiroslav PR is ready for review.

cc @AndrewGable

[0xmiros]

I think we should hold this issue until this regression is fixed. This happens on main right now.

[0xmiros]

Let's put this on hold for #29341

[kushu7]

Let's put this on hold for #29341

@0xmiroslav This issue is fixed by reverting those related PR. #29341 (comment)
We are good to go, can't reproduce that issue now.
I think we can proceed with reviewing PR. i merged main into our PR.

[0xmiros]

yes, let's remove hold.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @kushu7 got assigned: 2023-10-10 19:21:25 Z
• when the PR got merged: 2023-10-25 16:13:28 UTC
• days elapsed: 10

On to the next one 🚀

[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.3.91-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• redirect Anon user to sign in page on shortcut use #29323

If no regressions arise, payment will be issued on 2023-11-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @kushu7 requires payment offer (Contributor)
• @0xmiroslav requires payment offer (Reviewer)

[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:

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

[zanyrenney]

@kushu7 requires payment offer (Contributor) - PAID $500
@0xmiroslav requires payment offer (Reviewer)fuow - paid $500

no urgency bonus due to 10 day merge time.
no reporting bonus due to applause report.

[kushu7]

yes, let's remove hold.

@0xmiroslav @zanyrenney it was on hold i guess 🤔

[0xmiros]

The PR was approved (both C+ and engineer) quickly after removing hold.
As the delay (hold, conflict) was out of our control, I think bonus still applies here.
@AndrewGable can you please confirm?

[AndrewGable]

Yes this was on hold for 10 days, it was reviewed/merged in 3 days after the hold. cc @zanyrenney

[0xmiros]

@zanyrenney Andrew confirmed. Thanks