fix: [P2P Distance] - Distance unit in the system message changes from miles to mi after changing rate.#47605
Merged
Conversation
…m miles to mi after changing rate. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
melvin-bot
Bot
requested review from
paultsimura
and removed request for
a team
|
@paultsimura 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
paultsimura
reviewed
Aug 20, 2024
paultsimura
left a comment
paultsimura
left a comment
Contributor
There was a problem hiding this comment.
A couple of small notes.
| const unitString = distanceInUnits === '1' ? singularDistanceUnit : distanceUnit; | ||
|
|
||
| return `${distanceInUnits} ${unitString}`; | ||
| return `${distanceInUnits} ${useShortFormUnit ? unit : unitString}`; |
Contributor
There was a problem hiding this comment.
Let's avoid all the unnecessary unitString calculations by returning early if useShortFormUnit:
if (!hasRoute || !rate || !unit || !distanceInMeters) {
return translate('iou.fieldPending');
}
const distanceInUnits = getRoundedDistanceInUnits(distanceInMeters, unit);
if (useShortFormUnit) {
return `${distanceInUnits} ${unit}`;
}
const distanceUnit = unit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? translate('common.miles') : translate('common.kilometers');
const singularDistanceUnit = unit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES ? translate('common.mile') : translate('common.kilometer');
const unitString = distanceInUnits === '1' ? singularDistanceUnit : distanceUnit;
return `${distanceInUnits} ${unitString}`;
}This should work, right?
Contributor
Author
There was a problem hiding this comment.
This should work, right?
yes, updating...
| unit: Unit | undefined, | ||
| rate: number | undefined, | ||
| translate: LocaleContextProps['translate'], | ||
| useShortFormUnit?: boolean, |
Contributor
There was a problem hiding this comment.
Please add it to JsDoc.
| translate: LocaleContextProps['translate'], | ||
| toLocaleDigit: LocaleContextProps['toLocaleDigit'], | ||
| isOffline?: boolean, | ||
| useShortFormUnit?: boolean, |
Contributor
There was a problem hiding this comment.
Please add it to JsDoc.
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: Native2024-08-20.-.20.36.-.android.mp4Android: mWeb Chrome2024-08-20.-.20.36.-.chrome.mp4iOS: Native2024-08-20.-.20.36.-.Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-20.at.20.32.43.mp4iOS: mWeb Safari2024-08-20.-.20.36.-.Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-20.at.20.35.45.mp4MacOS: Chrome / Safari2024-08-20.-.20.36.-.Screen.Recording.2024-08-20.at.19.57.35.mp4MacOS: Desktop2024-08-20.-.20.36.-.Screen.Recording.2024-08-20.at.20.02.35.mp4 |
paultsimura
reviewed
Aug 20, 2024
| currency: string, | ||
| translate: LocaleContextProps['translate'], | ||
| toLocaleDigit: LocaleContextProps['toLocaleDigit'], | ||
| useShortFormUnit?: boolean, |
Contributor
There was a problem hiding this comment.
We are now always building the merchant with the short unit form.
So we can remove this param and just call both getDistanceForDisplay and getRateForDisplay with useShortFormUnit: true
Contributor
|
@Krishna2323 the PR tests good in general, thanks. There are a couple of things to change though. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
paultsimura
suggested changes
Aug 21, 2024
Comment on lines
+146
to
+148
| if (!Number.isNaN(Number(numValue)) && withDecimals) { | ||
| return Number(numValue).toFixed(2).toString().replace('.', toLocaleDigit('.')); | ||
| } |
Contributor
There was a problem hiding this comment.
There is an issue with this approach.
When the rate is with decimals, it uses 3 digits after a comma, and here you cut it to fixed 2, which leads to a discrepancy with the BE merchant:
Line 2221 in c0a9cbf
We want to build the following logic:
- If the value has 3 decimals, leave them as is
- If it has less than 2 decimals, add trailing zeros to have 2 after comma
A potential solution would be to round to 3 decimals by default and cut the last one off if it's a 0.
Contributor
There was a problem hiding this comment.
Also, since we perform this check and return early:
Lines 142 to 144 in 4daf8df
Do you think the !Number.isNaN(Number(numValue)) check is necessary here?
Can't we use just if (withDecimals) assuming that !Number.isNaN(Number(numValue)) would have caused the early return?
Contributor
There was a problem hiding this comment.
Let's handle this properly:
- Rewrite this function to handle the digits correctly.
- Add
PolicyUtilsunit tests to cover thegetRateDisplayValuefunction with different params.
Contributor
Author
There was a problem hiding this comment.
I will start updating accordingly in few hours.
Contributor
Author
There was a problem hiding this comment.
@paultsimura, can you please check the updated code?
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Contributor
|
Thanks @Krishna2323, this tests well. Please add the unit test – import * as PolicyUtils from '../../src/libs/PolicyUtils';
function toLocaleDigitMock(dot: string): string {
return dot;
}
describe('PolicyUtils', () => {
describe('getRateDisplayValue', () => {
it('should return an empty string for NaN', () => {
const rate = PolicyUtils.getRateDisplayValue('invalid' as unknown as number, toLocaleDigitMock);
expect(rate).toEqual('');
});
describe('withDecimals = false', () => {
it('should return integer value as is', () => {
const rate = PolicyUtils.getRateDisplayValue(100, toLocaleDigitMock);
expect(rate).toEqual('100');
});
it('should return non-integer value as is', () => {
const rate = PolicyUtils.getRateDisplayValue(10.5, toLocaleDigitMock);
expect(rate).toEqual('10.5');
});
});
describe('withDecimals = true', () => {
it('should return integer value with 2 trailing zeros', () => {
const rate = PolicyUtils.getRateDisplayValue(10, toLocaleDigitMock, true);
expect(rate).toEqual('10.00');
});
it('should return non-integer value with up to 2 trailing zeros', () => {
const rate = PolicyUtils.getRateDisplayValue(10.5, toLocaleDigitMock, true);
expect(rate).toEqual('10.50');
});
it('should return non-integer value with 3 decimals as is', () => {
const rate = PolicyUtils.getRateDisplayValue(10.531, toLocaleDigitMock, true);
expect(rate).toEqual('10.531');
});
it('should return non-integer value with 3+ decimals cut to 3', () => {
const rate = PolicyUtils.getRateDisplayValue(10.531345, toLocaleDigitMock, true);
expect(rate).toEqual('10.531');
});
});
});
}); |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Contributor
Author
|
@paultsimura, unit test added. |
paultsimura
approved these changes
Aug 23, 2024
paultsimura
left a comment
paultsimura
left a comment
Contributor
There was a problem hiding this comment.
Solid work done here, thanks ✔️
puneetlath
approved these changes
Aug 27, 2024
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
93 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.0.26-1 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.26-6 🚀
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #46873
PROPOSAL: #46873 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4