[HOLD for payment 2024-02-14] [$500] Long pressing "Go to Expensify Classic" option in Settings Crashes the App

[m-natarajan]

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

---

Version Number: 1.4.32-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
Expensify/Expensify Issue URL:
Issue reported by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1706307323470669

Action Performed:

1. Go to settings
2. Long press Go to expensify classic

Expected Result:

App should not crash

Actual Result:

App crashed

Workaround:

unknown

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

RPReplay_Final1706302579.MP4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017b2c107d082607ce
• Upwork Job ID: 1751947232616599552
• Last Price Increase: 2024-01-29
• Automatic offers:
• ishpaul777 | Contributor | 28134405

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[ishpaul777]

Proposal

Problem Statement

Users are experiencing app crashes when performing a long press on the "Go to Expensify Classic" option in Settings.

Root Cause

We are not using Link.buildOldDotURL properly in the getDefaultMenuItems in the InitialSettingsPage. The function returns a promise instead of a string, leading to unexpected behavior and crash.

App/src/pages/settings/InitialSettingsPage.js

Line 274 in c5ca9a8

link: Link.buildOldDotURL(CONST.OLDDOT_URLS.INBOX),

Proposed Solution

To address the problem, we should handle the link properly similar to what we have done for MenuItemList:

App/src/pages/settings/InitialSettingsPage.js

Line 274 in c5ca9a8

link: Link.buildOldDotURL(CONST.OLDDOT_URLS.INBOX),

to:

 link: () => Link.buildOldDotURL(CONST.OLDDOT_URLS.INBOX), 

App/src/components/MenuItemList.tsx

Lines 34 to 40 in c5ca9a8

	const secondaryInteraction = (link: MenuItemLink | undefined, event: GestureResponderEvent | MouseEvent) => {
	if (typeof link === 'function') {
	link().then((url) => ReportActionContextMenu.showContextMenu(CONST.CONTEXT_MENU_TYPES.LINK, event, url, popoverAnchor.current));
	} else if (link) {
	ReportActionContextMenu.showContextMenu(CONST.CONTEXT_MENU_TYPES.LINK, event, link, popoverAnchor.current);
	}
	};

// src/pages/settings/InitialSettingsPage.js

const onSecondaryInteraction = (link, event) => {
    if (typeof link === 'function') {
        console.debug('InitialSettingsPage: secondaryInteraction: link is a function, executing it');
        link().then((url) => ReportActionContextMenu.showContextMenu(CONST.CONTEXT_MENU_TYPES.LINK, event, url, popoverAnchor.current));
    } else if (link) {
        ReportActionContextMenu.showContextMenu(CONST.CONTEXT_MENU_TYPES.LINK, event, link, popoverAnchor.current);
    }
};

<MenuItem
    // rest of props
    onSecondaryInteraction={item.link !== undefined ? (e) => onSecondaryInteraction(item.link, e) : undefined}
/>

[samilabud]

Proposal

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

Secondary action for button "Go to Expensify Classic" in Settings Crashes the App

What is the root cause of that problem?

We are using buildOldDotURL method as secondary action, but this return an promise-string and we are expecting just a string, this cause that our ReportActionContextMenu.showContextMenu rise the error.

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

We should make the same as our main action and call the openOldDotLink method.

Something like:

          {
                translationKey: 'initialSettingsPage.goToExpensifyClassic',
                icon: Expensicons.NewExpensify,
                action: () => {
                    Link.openOldDotLink(CONST.OLDDOT_URLS.INBOX);
                },
                shouldShowRightIcon: true,
                iconRight: Expensicons.NewWindow,
                link: () => Link.openOldDotLink(CONST.OLDDOT_URLS.INBOX),
            },

Result:

long.pressing.crashes.the.app.test.mp4

[mananjadhav]

@ishpaul777's proposal looks good.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[melvin-bot]

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

[sonialiap]

Not overdue, contributer has just been assigned 🎉

[ishpaul777]

PR is open for review.

@mananjadhav

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

• fixes crash on long press expensify classic option #35601

If no regressions arise, payment will be issued on 2024-02-14. 🎊

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

• @mananjadhav requires payment through NewDot Manual Requests
• @ishpaul777 requires payment automatic offer (Contributor)
• @ishpaul777 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:

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

[mananjadhav]

I tried finding the root PR, the best one where we refactored the links in InitialSettingsPage is this PR. @ishpaul777 if you could identify the offending PR?

I don't think we need a regression test for this one.

[sonialiap]

@mananjadhav $500 - please request via ND
@ishpaul777 Contributor - $500 paid ✔️ (not sure why melvin decided there should be two offers. In fact looks like it might have duplicated by accident)

[JmillsExpensify]

$500 approved for @mananjadhav based on summary above.

[ishpaul777]

not sure why melvin decided there should be two offers. In fact looks like it might have duplicated by accident

could be because i reported the bug also, maybe there is still bug report bounty automation in place for these messages?

[ishpaul777]

@ishpaul777 if you could identify the offending PR?

sorry missed this earlier, will look into this

[ishpaul777]

This PR https://github.com/Expensify/App/pull/32947/files added a new menu item in settings page and this one is the offending PR #33863 to be specific

[sonialiap]

@mananjadhav @ishpaul777 could you please complete the checklist :)

[ishpaul777]

• The PR that introduced the bug has been identified. Link to the PR: Fix/32875/svg color and olddot url #33863
• 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: https://github.com/Expensify/App/pull/33863/files#r1492831004
• 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: not required
• Determine if we should create a regression test for this bug. not required
• 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. - not required

[ishpaul777]

not overdue, we can close this now