Fix currency changes after refresh although URL has currency params

[makiour]

Details

Changing the MoneyRequestAmountPage to check currency URL param. Thus, after refresh, the currency selected will not change.

Fixed Issues

$ #23436
PROPOSAL: #23436 (comment)

Tests

1. Open a chat in the app
2. Open the request money tab
3. Change the currency and notice it has changed in the URL
4. Refresh the page
5. Notice that the currency has not changed

• Verify that no errors appear in the JS console

QA Steps

Same as test.

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

fixCurrencyBug.webm

Mobile Web - Chrome

WhatsApp.Video.2023-07-27.at.12.01.45.mp4

Mobile Web - Safari

WhatsApp.Video.2023-07-27.at.12.01.44.mp4

Desktop

desktopFix.webm

iOS

Screen.Recording.2023-07-27.at.9.48.42.PM.mov

Android

androidFix.webm

[melvin-bot]

@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[makiour]

I have read the CLA Document and I hereby sign the CLA

[rushatgabhane]

@makiour please add the checklist again, and complete it.
All PRs should have the checklist and screenshots for all platforms.

For native, you can use a command like

npx uri-scheme open new-expensify://request/new/123?currency=MDL --android
npx uri-scheme open new-expensify://request/new/123?currency=MDL --ios

[rushatgabhane]

@makiour please add the checklist in the PR body. Or it'll fail a checklist workflow.
But I'd recommend that you open a new PR and complete the checklist inside it.

[makiour]

@rushatgabhane Thank you for your feedback! I have added video tests for all platforms, added the checklist to the body as well!

[rushatgabhane]

Reviewer Checklist

• I have verified the author checklist is complete (all boxes are checked off).
• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I included screenshots or videos for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-08-02.at.03.31.36.mov

Mobile Web - Chrome

WhatsApp.Video.2023-08-02.at.03.29.01.mp4

Mobile Web - Safari

Screen.Recording.2023-08-02.at.03.25.48.mov

Desktop

Screen.Recording.2023-08-02.at.03.35.03.mov

iOS

Screen.Recording.2023-08-02.at.03.21.57.mov

Android

Screen.Recording.2023-08-02.at.03.22.30.mov

[makiour]

Hey @rushatgabhane, is everything looking okay for you? If yes, can we proceed to merge the branch within the 3 days if that is possible?

Also, I have updated the PR Author checklist as I have added the wrong one before. Now, everything is as intended.

Thank you so much for your cooperation!

[rushatgabhane]

yes all is good, i just need to complete the checklist after I reach home :)

[makiour]

Amazing, thank you @rushatgabhane! The 3 days limit is 29th July at 1 PM(GMT+1), I will be very thankful if you manage to finish them before that!

[rushatgabhane]

@makiour i found a bug - app crash when navigating to /request/new

1. Run iOS app
2. Run npx uri-scheme open new-expensify://request/new --ios

Platform iOS

[makiour]

@rushatgabhane Thank you for the review and comments.

It seems that the issue you found has nothing to do with my branch. It is actually happening even in the main branch. Here is an illustration:

Main branch:

Untitled.mp4

Concerning my fix, it works all fine for the correct URL with a selected currency.

[makiour]

It also happens with normal request money process through the simulator on the main branch.

[rushatgabhane]

@makiour okay, thanks for confirming

[makiour]

@rushatgabhane Amazing, let me know of anything else

[rushatgabhane]

@makiour i know that it's an unrelated bug, but it's preventing testing on iOS. Can we fix this?

Screen.Recording.2023-08-01.at.15.52.34.mov

[makiour]

@rushatgabhane
After I investigated a bit, It is the same in the web version as well. It throws the same error. Currently, investigate the Root cause

[makiour]

@rushatgabhane It is completely a new bug. Here is what I found:

Issue

Basically, the app crashes when we try to open the IOU Request route in the app. It is spotted first on IOS and then I found that it also happens at the level of all other platforms.
Here is an illustration at the level of the web:

Cause

After investigating, the bug only happens when we Sign-in. Basically, the currency set for the profile isn't fetched yet or isn't ready to use through the "request/new" URL immediately. Thus, whenever we open it the first time, we get it as undefined. However, if we go through the normal flow, of clicking on Request Money, the currency then is fetched as ONYX has correctly subscribed and set the IOU.currency from profile details that are fetched:

Untitled.1.mp4

What causing the issue is that the following hook executes and changes the selectedCurrency to undefined:

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

Lines 286 to 288 in 5034433

	useEffect(() => {
	setSelectedCurrencyCode(props.iou.currency);
	}, [props.iou.currency]);

Thus, these functions, which are called from the child component: "TextInputWithCurrencySymbol", throw an exception:

App/src/libs/CurrencyUtils.js

Lines 51 to 73 in 5034433

	function getLocalizedCurrencySymbol(currencyCode) {
	const parts = NumberFormatUtils.formatToParts(BaseLocaleListener.getPreferredLocale(), 0, {
	style: 'currency',
	currency: currencyCode,
	});
	return _.find(parts, (part) => part.type === 'currency').value;
	}
	/**
	* Whether the currency symbol is left-to-right.
	*
	* @param {String} currencyCode
	* @returns {Boolean}
	*/
	function isCurrencySymbolLTR(currencyCode) {
	const parts = NumberFormatUtils.formatToParts(BaseLocaleListener.getPreferredLocale(), 0, {
	style: 'currency',
	currency: currencyCode,
	});
	// Currency is LTR when the first part is of currency type.
	return parts[0].type === 'currency';
	}

Solution

The fix to this bug, including my current solution, is just to add a small checking here. Basically, a constant USD is already set initially in the props:

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

Lines 59 to 72 in 5034433

	const defaultProps = {
	route: {
	params: {
	iouType: '',
	reportID: '',
	},
	},
	report: {},
	iou: {
	id: '',
	amount: 0,
	currency: CONST.CURRENCY.USD,
	participants: [],
	},

However we need to make sure to not assign directly to props.IOU.currency in the useEffect as it is undefined at the first time. To handle this we can just add a condition statement here in my changes:

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

Lines 278 to 284 in 0176b1f

	useEffect(() => {
	if (props.route.params.currency) {
	setSelectedCurrencyCode(props.route.params.currency);
	return;
	}
	setSelectedCurrencyCode(props.iou.currency);
	}, [props.route.params.currency, props.iou.currency]);

to:

    useEffect(() => {
        if (props.route.params.currency) {
            setSelectedCurrencyCode(props.route.params.currency);
            return;
        }
        **if (props.iou.currency)** {
            setSelectedCurrencyCode(props.iou.currency);
        }
    }, [props.route.params.currency, props.iou.currency]);

The following commit includes this fix!

[rushatgabhane]

Thanks for the fix @makiour, I've reported the bug here. I'll make sure that you get assigned and rewarded for it.

I think it's okay to fix a seperate bug in this PR because it's preventing testing.

[rushatgabhane]

LGTM! 🚀

@MariaHCD because it was preventing testing, we fixed an additional bug in this PR too - #23636 (comment)

[makiour]

Thank you so much @rushatgabhane for your cooperation! The new bug did not allow us to land our changes quickly as we wanted to add all the issues, and so happy to work on both of them!

[MariaHCD]

Nice work, thanks!

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.3.50-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[puneetlath]

QA reported that this doesn't seem to be working on staging. I also just tested this and it doesn't seem that the currency is added to the URL params.

[rushatgabhane]

okayy, the url has changed. we added manual and scan request

[rushatgabhane]

@makiour wanna investigate what happened here?

cc @allroundexperts because you reviewed scan receipt PR

[OSBotify]

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.50-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[makiour]

@rushatgabhane @puneetlath I tried to check the new flow, but creating an account, and testing request money in staging hasn't changed. Here is a video showing the changes working all fine in staging!

Screencast.from.07.2023.+01.07.38.27.webm

[OSBotify]

🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.3.51-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[rushatgabhane]

@makiour oh you must not be on the beta.

Thanks for looking into this, I'll investigate it further

[rushatgabhane]

Alright, i found the PR :=
https://github.com/Expensify/App/pull/23046/files#r1286399372

While adding scan receipt feature, we refactored money request page and didn't take into account the query params.
@puneetlath how should we proceed?

cc: @s77rt (c+)

[s77rt]

I'm not sure about the bug here (I can set the currency in url w/wo beta). Regardless, I think we should not worry about the currency for now. The currency selection should not have its own route (and no query params). Instead we'd render the currency modal in the amount form and just pass the currency selection through a callback (same way done for year picker, state picker and country picker). However I don't know if we have an issue for that or when we are going to work on that.

[makiour]

@s77rt Thank you for your comment. You are not seeing the bug now as it is fixed without beta in staging and production. Refer to the following issue and video to check it before the fix: #23436 (comment)
Indeed the refactoring has tiggered the currency param to pops up when we click on selection:

[makiour]

Also, this is not happening now: after refactoring and adding scan feature, the currency selection is now under the new flow for no beta users as well. You can pull the branch before this one and check the video for the previous bug.

A small comment here, I am not sure if it is a bug but I might consider it as UI/UX inconsistency. If we set a currency through the URL:

It shows that the currency is already selected, so going back should not remove the selection, however, it does and goes back to the previous currency. What do you think @s77rt @rushatgabhane?

[OSBotify]

🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅