[HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$2000] Unable to change the currency after double-clicking on the current currency

[kavimuru]

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. Log in to your account.
2. Click on the FAB menu and select either the "Request Money" or "Split Bill" option.
3. Double-click on the current currency.
4. In the Currency list, select a different currency.
5. Observe that the selected currency does not change.

Expected Result:

The currency should change to the selected one.

Actual Result:

The selected currency remains unchanged after double-clicking on the current currency.

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 / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.38-3
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

DOUBLE-CLICKED-Currency-not-changed.MP4

QWVO2990.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tranvantoan-qn
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688821290443479

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ac5f2b1e902a7171
• Upwork Job ID: 1678505607142854656
• Last Price Increase: 2023-08-06

[melvin-bot]

Triggered auto assignment to @maddylewis (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

[ginsuma]

Proposal

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

Unable to change the currency after double-clicking on the current currency

What is the root cause of that problem?

When we double click, we will navigate to currency selector page two times if the navigation time is long enough.

Because of 2nd click, activeRoute in navigateToCurrencySelectionPage function is currency selector page already.
By the logic here, we will use backTo to navigate when selecting currency => back to that page again.

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

We can debounce navigateToCurrencySelectionPage function in MoneyRequestAmountPage

this.navigateToCurrencySelectionPage = _.debounce(this.navigateToCurrencySelectionPage.bind(this), 300);

What alternative solutions did you explore? (Optional)

We can check activeRoute is not currency selector page before navigation.

if (!decodeURIComponent(activeRoute).includes('/new/currency/')) {
    Navigation.navigate(ROUTES.getMoneyRequestCurrencyRoute(this.iouType, this.reportID, this.state.selectedCurrencyCode, activeRoute));
}

[bernhardoj]

Proposal

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

Pressing the currency symbol twice will make the currency selection doesn't work.

What is the root cause of that problem?

When we navigate to currency selection, we pass backTo params and the value is from the currently active route (or path, for example,/request/new or /split/new)

App/src/pages/iou/steps/MoneyRequestAmountPage.js

Lines 410 to 411 in 60f4817

	const activeRoute = encodeURIComponent(Navigation.getActiveRoute().replace(/\?.*/, ''));
	Navigation.navigate(ROUTES.getMoneyRequestCurrencyRoute(this.iouType, this.reportID, this.state.selectedCurrencyCode, activeRoute));

When we select a currency, we will navigate back to the backTo route and append a currency params.

App/src/pages/iou/IOUCurrencySelection.js

Line 92 in 60f4817

Navigation.navigate(`${props.route.params.backTo}?currency=${option.currencyCode}`);

The currency will be updated if the currency params exist.

App/src/pages/iou/steps/MoneyRequestAmountPage.js

Lines 144 to 148 in 60f4817

	const prevCurrencyParam = lodashGet(prevProps.route.params, 'currency', '');
	const currencyParam = lodashGet(this.props.route.params, 'currency', '');
	if (currencyParam !== '' && prevCurrencyParam !== currencyParam) {
	this.setState({selectedCurrencyCode: currencyParam});
	}

When we quickly press the currency symbol multiple times, we will do the navigation action multiple times and the active route for the first and the rest action is different. Let's take request money as an example.

1. First click will get the active route which is /request/new and navigation (to currency selection) action is dispatched. This action will update the route in the navigation state. (current route now is: /request/new/currency?backTo=/request/new)
2. Second click will get the active route which is /request/new/currency?backTo=request/new, but we remove all query params so the active route is /request/new/currency. (current route now is: /request/new/currency?backTo=/request/new/currency)
3. The next multiple clicks will always have the same active route.

Notice that the backTo param is /request/new/currency, therefore, when we select a currency, it will navigate to itself with a currency param appended to it, which means the current route now will be /request/new/currency?currency=ANYTHING. Now, the backTo param is gone and when we select a currency again, it will simply navigate back.

App/src/pages/iou/IOUCurrencySelection.js

Lines 89 to 90 in 60f4817

	if (_.isEmpty(backTo) || props.navigation.getState().routes.length === 1) {
	Navigation.goBack();

Let's say we fix the above issue by disabling the button press when navigating.

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

We should prevent the user from clicking the currency symbol multiple times. In PressableWithFeedback, we already use useSingleExecution to disable the button while navigating.

App/src/components/Pressable/PressableWithFeedback.js

Lines 88 to 90 in 25f7ac1

	onPress={(e) => {
	singleExecution(() => props.onPress(e))();
	}}

Currently, currency symbols use PressableWithoutFeedback which doesn't have this logic (idk the reason the press logic is only applied to PressableWithFeedback). So, the solution is to move the press logic above from PressableWithFeedback to BaseGenericPressable, so both PressableWithFeedback and PressableWithoutFeedback will have the same logic.

However, this isn't enough. On some platforms, long-pressing a button will trigger the onPress callback. This becomes a problem because currently, when we "disable" the button/Pressable, it will just pass undefined handlers to the Pressable, for example.

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Line 137 in 25f7ac1

onPress={!isDisabled ? onPressHandler : undefined}

So, even when we "disable" the button, the Pressable can still receive touch and "start the long press delay timer" and trigger the long press after the button is re-enabled. To fix this, we should fully disable the Pressable by disabling it through disabled props.

disabled={isDisabled}

This will set pointerEvents to none, thus disabling any interaction with the component. The downside of this is, we can't have a not-allowed cursor. This is normal, but as we want to show the not-allowed cursor when it's disabled, we can wrap the Pressable with a View and move the cursor style from Pressable to View.

App/src/components/Pressable/GenericPressable/BaseGenericPressable.js

Line 145 in 25f7ac1

getCursorStyle(shouldUseDisabledCursor, [props.accessibilityRole, props.role].includes('text')),

With these changes, we don't need isDisabled && onPress and such anymore.

I also suggested splitting the PR into 2 PRs. The first PR is to move the useSingleExecution logic to BaseGenericPressable and the second PR is to fix the long press issue.

[melvin-bot]

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

[melvin-bot]

Current assignee @maddylewis is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[maddylewis]

assigned C+ to review existing proposals

[maddylewis]

bumping @eVoloshchak to review existing proposals. lmk if you're overloaded and i can reach out to another C+ - thx!

[eVoloshchak]

@ginsuma, thanks for the proposal!

Debouncing navigate would technically resolve the issue, but I think that's more of a workaround. Let's resolve the root cause instead

@bernhardoj, can we implement the same logic in PressableWithoutFeedback instead? (ideally, shouldn't be this implemented in GenericPressable component, so all Pressables support this behavior?)

[bernhardoj]

ideally, shouldn't be this implemented in GenericPressable component,

That's what I thought too, that's why I wrote

idk the reason the press logic is only applied to PressableWithFeedback

in my proposal 😄

@priyeshshah11 @s77rt maybe can help answer it.

[priyeshshah11]

ideally, shouldn't be this implemented in GenericPressable component,

That's what I thought too, that's why I wrote

idk the reason the press logic is only applied to PressableWithFeedback

in my proposal 😄

@priyeshshah11 @s77rt maybe can help answer it.

even I don't know the actual reason why we just implemented it in PressableWithFeedback & not in a more generic component but that's what was suggested by @roryabraham / @robertKozik at that time, better to check with them.

[tranvantoan-qn]

Proposal

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

Unable to change the currency after double-clicking on the current currency

What is the root cause of that problem?

The navigation to the Currency selection page is handled in the following code.

App/src/pages/iou/steps/MoneyRequestAmountPage.js

Lines 361 to 365 in 67bac9d

	const navigateToCurrencySelectionPage = () => {
	// Remove query from the route and encode it.
	const activeRoute = encodeURIComponent(Navigation.getActiveRoute().replace(/\?.*/, ''));
	Navigation.navigate(ROUTES.getMoneyRequestCurrencyRoute(iouType.current, reportID.current, selectedCurrencyCode, activeRoute));
	};

In normal cases when the currency is clicked, the activeRoute will be as follows:

• Send Money: /send/new
• Split Money: /split/new
• Request Money: /request/new

But when the current currency is double-clicked, it somehow adds /currency/ to the end and an error occurs.

• Send Money: /send/new/currency/
• Split Money: /split/new/currency/
• Request Money: /request/new/currency/

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

Add a condition to check if the activeRoute contains /currency. Only navigate if /currency is not present

    const navigateToCurrencySelectionPage = () => {
        // Remove query from the route
        const activeRoute = Navigation.getActiveRoute().replace(/\?.*/, '')
        if (!activeRoute.includes('/currency')) {
            // Encode the route.
            const backToRoute = encodeURIComponent(activeRoute);
            Navigation.navigate(ROUTES.getMoneyRequestCurrencyRoute(iouType.current, reportID.current, selectedCurrencyCode, backToRoute));
        }
    };

What alternative solutions did you explore? (Optional)

N/A

[tranvantoan-qn]

@eVoloshchak
I don't think adding Debouncing will help
I've added my proposal here, please have a look.

[melvin-bot]

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

[maddylewis]

@eVoloshchak - bump on this one - ty!

[eVoloshchak]

@tranvantoan-qn, this is a workaround, let's prevent this function from being called twice in the first place

@bernhardoj, I think we should implement this in the generic pressable, so it's universally available
Would you mind posting an updated proposal?

[bernhardoj]

@eVoloshchak included it on the alternative solution

[eVoloshchak]

@bernhardoj's updated proposal looks good to me
Let's make sure this is fixed for all pressables by implementing the alternative solution

We can move the press logic above from PressbaleWithFeedback to BaseGenericPressable, so both WithFeedback and WithoutFeedback will have the same logic

🎀👀🎀 C+ reviewed!

[melvin-bot]

@francoisl, @eVoloshchak, @chiragsalian, @maddylewis, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

[maddylewis]

lol what is happening with this issue? am i good to start processing payments to:

• @eVoloshchak
• @bernhardoj
• @tranvantoan-qn

[bernhardoj]

@maddylewis we are still working out on the 2nd PR. Waiting for @eVoloshchak

[melvin-bot]

@francoisl, @eVoloshchak, @chiragsalian, @maddylewis, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[maddylewis]

how are we looking @bernhardoj? are we still waiting on the 2nd PR? #22497 (comment)

[bernhardoj]

Yes. Last week, @eVoloshchak started a discussion on Slack (c+ channel I guess?) about the PR solution. @eVoloshchak would you mind sharing the result of the discussion?

[melvin-bot]

@francoisl, @eVoloshchak, @chiragsalian, @maddylewis, @bernhardoj Whoops! This issue is 2 days overdue. Let's get this updated quick!

[eVoloshchak]

In our discussion, we didn't reach a definite answer to the question "Is this worth fixing?", but we did figure out that this behavior is not caused by our code and can be reproduced even in an empty project, so it's a default behavior

if you tap and hold and there is no onLongPress handler, so the Pressable will "wait" indefinitely until the finger is lifted, triggering the onPress handler

Given that

1. The original issue (Unable to change the currency after double-clicking on the current currency) is fully resolved by the first PR
2. The second issue (tap and hold) is a default behavior we're trying to work around
3. The second issue is essentially an edge case and I don't think users would intentionally perform an action of tapping and then immediately holding the currency button
4. The risk of potential regressions and further complication of logic of one of our core components (Pressable) outweighs the value of resolving the said issue

I think we should just close the second PR and not proceed with the fix further
@chiragsalian, what are your thoughts?

[bernhardoj]

@chiragsalian any opinion?

I think we can handle the 2nd problem on a separate issue here #26978.

I have a new completely different solution ready for the 2nd problem, but I think we need to discuss it more on the new linked issue above

[chiragsalian]

Yeah i think discussing the 2nd problem in a separate issue makes sense to me. I like having an issue for a very specific usecase like this because if we don't want to solve it now and we close it that's fine. But if we ever want to tackle it in the future there is an existing issue we can open/refer to continue the discussion as needed.

I think considering the first PR solving the main issue here is good enough for me.

[bernhardoj]

Okay great, I have closed the 2nd PR. We can address it on this new issue #26978. I think the next step is the checklist

[eVoloshchak]

I'll fill out the BZ checklist today

[maddylewis]

got it - waiting for @eVoloshchak to fill out the BZ checklist.

this one has a lot of moving parts so i need a little guidance on how payments are looking for this one. let me know how this is looking:

• Reporting bonus: $250 to @tranvantoan-qn
• Contributor: $2k to @bernhardoj
• C+: $2k to @eVoloshchak

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: there isn't a PR that caused this, this was present in the initial implementations
• 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: N/A
• 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: I don't think an additional discussion is necessary, this is a fairly simple bug (from the testing perspective)

Regression Test Proposal

1. Open Request Money page
2. Switch to Manual tab
3. Double-click the currency symbol button
4. Select any currency
5. Verify you are navigated back and the currency is updated

Do we agree 👍 or 👎

[maddylewis]

Payments

• Reporting bonus: $250 to @tranvantoan-qn (Upwork)
• Contributor: $2k to @bernhardoj (Upwork)
• C+: $2k to @eVoloshchak (NewDot)

[maddylewis]

everyone is paid

[tranvantoan-qn]

@maddylewis There is a previous contract for this one, should it be closed now?
https://www.upwork.com/nx/wm/workroom/34271376/overview

[JmillsExpensify]

$2,000 payment approved for @eVoloshchak based on summary above.