Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.87k forks source link

[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 #22497

Closed kavimuru closed 11 months ago

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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?

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

https://github.com/Expensify/App/assets/43996225/6a06fd67-b83f-4784-a0d6-586fcedb8f3b

https://github.com/Expensify/App/assets/43996225/7296c3ad-ad0c-42a3-953e-04c576205cb4

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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

ginsuma commented 1 year ago

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 commented 1 year ago

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) https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/pages/iou/steps/MoneyRequestAmountPage.js#L410-L411

When we select a currency, we will navigate back to the backTo route and append a currency params. https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/pages/iou/IOUCurrencySelection.js#L92

The currency will be updated if the currency params exist. https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/pages/iou/steps/MoneyRequestAmountPage.js#L144-L148

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. https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/pages/iou/IOUCurrencySelection.js#L89-L90

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. https://github.com/Expensify/App/blob/25f7ac14d3dcadbb60e24074494b12d181567ba3/src/components/Pressable/PressableWithFeedback.js#L88-L90

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. https://github.com/Expensify/App/blob/25f7ac14d3dcadbb60e24074494b12d181567ba3/src/components/Pressable/GenericPressable/BaseGenericPressable.js#L137

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. https://github.com/Expensify/App/blob/25f7ac14d3dcadbb60e24074494b12d181567ba3/src/components/Pressable/GenericPressable/BaseGenericPressable.js#L145

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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

maddylewis commented 1 year ago

assigned C+ to review existing proposals

maddylewis commented 1 year ago

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

eVoloshchak commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. https://github.com/Expensify/App/blob/67bac9d50b9fbc1b8a1b1a14e8cc567ca3d0580b/src/pages/iou/steps/MoneyRequestAmountPage.js#L361-L365

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

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

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 commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

maddylewis commented 1 year ago

@eVoloshchak - bump on this one - ty!

eVoloshchak commented 1 year ago

@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 commented 1 year ago

@eVoloshchak included it on the alternative solution

eVoloshchak commented 1 year ago

@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[bot] commented 1 year ago

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

francoisl commented 1 year ago

@bernhardoj's proposal looks like the best to me as well, let's go with that.

melvin-bot[bot] commented 1 year ago

πŸ“£ @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

πŸ“£ @bernhardoj You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 year ago

πŸ“£ @tranvantoan-qn πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reporter role πŸŽ‰ Thanks for contributing to the Expensify app!

Upwork job

bernhardoj commented 1 year ago

While testing, I found out that my solution does not fully solve the issue. It is preventing the press callback to be called multiple times, but I can still reproduce the issue when keep quickly pressing the currency button non-stop until the navigation animation is fully complete. I see the log in the terminal that the press callback is called twice and the 2nd one is called after the navigation is done. Below is the video after applying my solution.

https://github.com/Expensify/App/assets/50919443/db91bde0-8cb9-46df-bd06-d1b99b0f078b

So, we should find a new solution. I have updated my proposal to include another solution.

@eVoloshchak @francoisl

melvin-bot[bot] commented 1 year ago

@francoisl @eVoloshchak @maddylewis @bernhardoj this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

maddylewis commented 1 year ago

thanks for the update @bernhardoj - once @eVoloshchak and @francoisl review, we'll move forward with the fix. thanks!

francoisl commented 1 year ago

@bernhardoj it makes sense to me to move that logic up to BaseGenericPressable, though in the end I don't see how that would work any better than your original proposal. What exactly keeps causing an issue with your original proposal?

bernhardoj commented 1 year ago

Oops, I think I forget to save the updated solution 🀦.

move that logic up to BaseGenericPressable

That alternative solution is the old one that is based on https://github.com/Expensify/App/issues/22497#issuecomment-1637210760 which still has the issue https://github.com/Expensify/App/issues/22497#issuecomment-1645027294.

I have made sure it's updated this time πŸ˜…. Please check again.

eVoloshchak commented 1 year ago

@bernhardoj, the updated proposal does resolve the issue, but it's a workaround, navigate is still called twice, just with the correct route. Is there a way we could disable Pressable while navigation action is in progress?

bernhardoj commented 1 year ago

@eVoloshchak you're right, the navigate action will be called multiple times.

Is there a way we could disable Pressable while navigation action is in progress?

As InteractionManager is not enough and navigation action is not a promise, the only idea I have is to disable it on press and enable it when we come back to the page, but I think that doesn't feel right.

melvin-bot[bot] commented 1 year ago

@francoisl @eVoloshchak @maddylewis @bernhardoj this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

maddylewis commented 1 year ago

@eVoloshchak - can you provide an update on where we are with this fix? ty!

eVoloshchak commented 1 year ago

As InteractionManager is not enough and navigation action is not a promise, the only idea I have is to disable it on press and enable it when we come back to the page, but I think that doesn't feel right

Agree, @bernhardoj, that isn't right

can you provide an update on where we are with this fix

@maddylewis, there isn't a proposal that would resolve the root cause (we essentially need to disable a button while navigation action is in progress), but there are a couple of workarounds that would work. I think this isn't critical enough to apply a workaround and we should wait for more proposals

melvin-bot[bot] commented 1 year ago

@francoisl @eVoloshchak @maddylewis @bernhardoj this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @eVoloshchak is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

maddylewis commented 1 year ago

I am OOO until ~Aug 20 - here is the latest update for this one - https://github.com/Expensify/App/issues/22497#issuecomment-1663990410

it sounds like this isn't "critical" enough to warrant moving forward with the existing proposals. and we should wait for some new proposals.

i will double the bounty on this one, though!

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $2000

pradeepmdk commented 1 year ago

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?

https://github.com/Expensify/App/blob/014b326a1cb193448746a7fec90cf0411563a35a/src/pages/iou/steps/NewRequestAmountPage.js?#L143 navigateToCurrencySelectionPage called twice on the double tab so each time navigation state is changed https://github.com/Expensify/App/blob/014b326a1cb193448746a7fec90cf0411563a35a/src/pages/iou/steps/NewRequestAmountPage.js?#L145

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

1) prevent the double tab (using throttle) 2) we can modify the active route variable instead of getting getActiveRoute

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

this will help us maintain same path (params) in the state https://reactnavigation.org/docs/navigating/#navigate-to-a-route-multiple-times

when we use getActiveRoute 
[{"key": "Money_Request-arW5zdfhJxvb46XnuZTB3", "name": "Money_Request", "params": {"iouType": "split"}}, {"key": "Money_Request_Currency-sjEX9FW3WU4LCzMRjSbg_", "name": "Money_Request_Currency", "params": {"backTo": "/split/new/currency/", "currency": "INR", "iouType": "split"}, "path": "/split/new/currency/?currency=INR&backTo=%2Fsplit%2Fnew%2Fcurrency%2F"}]

when we use getMoneyRequestRoute
 [{"key": "Money_Request-pm_1CchdHXSkKqmbtoq-f", "name": "Money_Request", "params": {"currency": "JMD", "iouType": "split"}, "path": "/split/new/?currency=JMD"}, {"key": "Money_Request_Currency-TVR61U8r19agX6bFgRBsU", "name": "Money_Request_Currency", "params": {"backTo": "split/new/", "currency": "JMD", "iouType": "split"}, "path": "/split/new/currency/?currency=JMD&backTo=split%2Fnew%2F"}]

This is exact issue on when we use
getActiveRoute

when we double tab its updating backTo with '/split/new/currency/' path in the same state. so that its after selecting the currency its navigate to same currency route and at the time no backTo Param . after that in the second time when we select 'params.backTo' goes empty and redirect to back screen without currencyCode https://github.com/Expensify/App/blob/014b326a1cb193448746a7fec90cf0411563a35a/src/pages/iou/IOUCurrencySelection.js?#L83-L96

Result

https://github.com/Expensify/App/assets/16595846/147e4e46-04b4-4f9a-80b0-a5723c7c9fcc

WikusKriek commented 1 year ago

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 function navigateToCurrencySelectionPage is called twice on the double click, this cause the goBack route to append an additional 'currency' keyword to the route. This messes with the parameters of the url upon returning and we are not able to update the currency.

The symptoms are the incorect return url, but the root issue is we should not allow navigateToCurrencySelectionPage to be called twice.

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

We should add an isEditingCurrency variable that we set to true when onCurrencyButtonPress inside navigateToCurrencySelectionPage and clear it when we navigate back to the request money page.

const [isEditingCurrency, setIsEditingCurrency] = useState(false);

We can setIsEditingCurrency(false); on the useFocusEffect. This way we know we are back on the request money page and we are not selecting the currency anymore.

useFocusEffect(
        useCallback(() => {
            focusTextInput();
            setIsEditingCurrency(false);
        }, []),
    );

And then just add a !isEditingCurrency to the currency button press.

onCurrencyButtonPress={ !isEditingCurrency && navigateToCurrencySelectionPage}

What alternative solutions did you explore? (Optional)

If we want a more universal approach the best solution would be to debounce the NativeGenericPressable

onPress={_.debounce(props.onPress, 300, true)}

add the below to navigateToCurrencySelectionPage function.

if (activeRoute.includes('currency')) {
    return;
  }

But we really should not add anything to the navigateToCurrencySelectionPage function in my opinion because it works fine and doing this will be fixing the symptoms, we need to prevent the navigateToCurrencySelectionPage from being called twice in the first place.

stephanieelliott commented 1 year ago

Some more proposals here and here, @eVoloshchak can you please review?

pradeepmdk commented 1 year ago

@eVoloshchak any update

eVoloshchak commented 1 year ago

Reviewing shortly

eVoloshchak commented 1 year ago

prevent the double tab (using throttle)

@pradeepmdk, this was proposed before, while would technically work, this is working around the problem

we can modify the active route variable instead of getting getActiveRoute

A similar approach was proposed before (https://github.com/Expensify/App/issues/22497#issuecomment-1627690548), while your implementation is a bit different, I think the general idea is the same

eVoloshchak commented 1 year ago

We should add an isEditingCurrency variable that we set to true when onCurrencyButtonPress inside navigateToCurrencySelectionPage and clear it when we navigate back to the request money page.

@WikusKriek, while this would work, I wonder if there's a more universal approach. This problem can be observed everywhere throughout the app, every button that triggers navigation will have the same problem. For instance, pressing back button twice will navigate back twice (notice the animation)

https://github.com/Expensify/App/assets/9059945/d5ff4a71-cfe3-43dd-aa08-b85acb105b38

Here are our options

  1. Extend this issue to fix this bug throughout the app (every button that triggers navigation will have the same problem when double-clicking). In principle, we need to disable all navigation buttons while the navigation transition is in progress. Not sure how complex that would be and what the value is, since mostly the bug is animation being played twice. On the other hand, this is a legitimate bug and it needs to be fixed at some point
  2. Implement a fix for this particular case (i.e. fix this particular issue with not being able to change the currency after double-clicking on the current currency). (Get the currently active route once instead of getting it on each click. https://github.com/Expensify/App/issues/22497#issuecomment-1627690548)

@arosiclair, what do you think?

pradeepmdk commented 1 year ago

@eVoloshchak thanks for reviewing my proposal. why I am suggesting using ROUTES.getMoneyRequestRoute(iouType, reportID) means we are already in the IOUCurrencySelection.js using this only on goback session. i am not sure why still we need to pass activeroute from MoneyRequestAmountPage.js https://github.com/Expensify/App/blob/014b326a1cb193448746a7fec90cf0411563a35a/src/pages/iou/IOUCurrencySelection.js?#L140

Another solution

and here our main issue on double tap. we want need only single tap. we deviate two events from pressable component single or double here we can do that https://github.com/Expensify/App/blob/60f48175c9e3342ef192d878218820e5ed5a76cd/src/components/Pressable/GenericPressable/index.native.js

const NativeGenericPressable = forwardRef((props, ref) => {
    const lastTap = useRef(null);

    return (
        <GenericPressable
            focusable
            accessible
            accessibilityHint={props.accessibilityHint || props.accessibilityLabel}
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...props}
            onPress={() => {
                const now = Date.now();
                const DOUBLE_PRESS_DELAY = 300; 
                if (lastTap.current && now - lastTap.current < DOUBLE_PRESS_DELAY) {
                    // Double tap logic
                    console.log('Double tap detected');
                    lastTap.current = now;
                    props.onDoublePress && props.onDoublePress();
                  } else {
                    // Single tap logic
                    console.log('Single tap detected');
                    lastTap.current = now;
                    props.onPress && props.onPress();
                  }
            }}
            ref={ref}
        />
    )
});

so that we can avoid this in all the places on the double tap.. and also it should be a feature request. if we need anything on the double tap we can use this.

WikusKriek commented 1 year ago

@eVoloshchak Yeah my solution only works on the navigation buttons. I will not be able to apply that to the general pressable.

The best solution to fix this across the app would be to apply a debounce as mentioned in this stack overflow solution

So since this issue is only reproducible on native I agree with @pradeepmdk we should add a solution to NativeGenericPressable I am just not sure what exactly he is doing in his solution.

All that is needed is:

onPress={_.debounce(props.onPress, 300)}
WikusKriek commented 1 year ago

Proposal

Updated