-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2022-02-01] $1000 - IOU - Decimal separator not updated when set to Spanish on some screens #6427
Comments
Triggered auto assignment to @roryabraham ( |
Triggered auto assignment to @bfitzexpensify ( |
Posted to Upwork - job |
Triggered auto assignment to @parasharrajat ( |
Triggered auto assignment to @Luke9389 ( |
ProposalI will format the value of this.state.amount App/src/pages/iou/steps/IOUAmountPage.js Line 196 in 96ba12d
in WithLocalise
|
@anthony-hull Thanks for the proposal. How would you solve the first screen issue? I feel that it would inconsistent for the Request Money screen if the user types |
I am submitting this proposal because I've got an email that was allowed to be hired for another jobs by this pull request approval 1. ReplicationI was able to reproduce this issue in Web and Android versions and absolutely sure that this issue will also occur on other platforms. 2. ProblemMainly, we have 2 problems that are related to this issue.
We can easily update this module in order to display
I think it is not recommended way to save the values with formatted and is better to reformat the values before displaying in the frontend (after getting the values from storage). 3. Solution.
//define state like this.
- amount: props.selectedAmount,
+ amount: props.selectedAmount ? props.numberFormat(props.selectedAmount) : '',
---
//update validate rule
validateAmount(amount) {
- const decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,3})?$/, 'i');
- return amount === '' || (decimalNumberRegex.test(amount) && (parseFloat((amount * 100).toFixed(3)).toString().length <= CONST.IOU.AMOUNT_MAX_LENGTH));
+ let decimalNumberRegex = new RegExp(/^\d+(,\d+)*(\.\d{0,3})?$/, 'i');
+ let enFormattedAmount = amount;
+ if (this.props.preferredLocale == 'es') {
+ enFormattedAmount = amount.replace(/\./g, '').replace(/,/g, '.');
+ }
+ return enFormattedAmount === '' || (decimalNumberRegex.test(enFormattedAmount) && (parseFloat((enFormattedAmount * 100).toFixed(3)).toString().length <= CONST.IOU.AMOUNT_MAX_LENGTH));
}
stripCommaFromAmount(amount) {
- return amount.replace(/,/g, '');
+ const stripped = this.props.preferredLocale == 'es' ? amount.replace(/\./g, '') : amount.replace(/,/g, '');
+ return stripped
}
----
//format saving values to normal number format
+ getENFormattedAmount() {
+ if(this.props.preferredLocale == 'es') {
+ return this.state.amount.replace(/\./g, '').replace(/,/g, '.');
+ }
+ return this.state.amount;
+ }
---
<Button
success
style={[styles.w100, styles.mt5]}
- onPress={() => this.props.onStepComplete(this.state.amount)}
+ onPress={() => this.props.onStepComplete(this.getENFormattedAmount())}
pressOnEnter
- isDisabled={!this.state.amount.length || parseFloat(this.state.amount) < 0.01}
+ isDisabled={!this.state.amount.length || parseFloat(this.getENFormattedAmount()) < 0.01}
text={this.props.translate('common.next')}
/>
//ReportTransaction.js
+ import compose from '../libs/compose';
+ import withLocalize from './withLocalize';
+ import * as ReportUtils from './../libs/reportUtils'
---
<Text style={[styles.chatItemMessage]}>
- {this.props.action.message[0].text}
+ {ReportUtils.changeNumberFormatFromMessage(this.props.action.message[0].text, this.props.numberFormat)}
</Text>
---
-export default withOnyx({
- transactionsBeingRejected: {
- key: ONYXKEYS.TRANSACTIONS_BEING_REJECTED,
- },
-})(ReportTransaction);
+export default compose(
+ withLocalize,
+ withOnyx({
+ transactionsBeingRejected: {
+ key: ONYXKEYS.TRANSACTIONS_BEING_REJECTED,
+ },
+ }),
+)(ReportTransaction);
// libs/reportUtils.js
+ function changeNumberFormatFromMessage(messageText, changeNumberFormat) {
+ const numRegex = new RegExp(/[\d,]+[\.\d+]*/g);
+ const matches = messageText.match(numRegex)
+ if (matches) {
+ const matchStrNum = matches[0].replace(/,/g, '');
+ const formattedNumber = changeNumberFormat(matchStrNum);
+ messageText = messageText.replace(numRegex, formattedNumber);
+ }
+ return messageText
+ }
|
Well.. I tried to submit my proposal in upwork but got a strange issue. Maybe, is it related open jobs that I worked for Magnifying icon issue? Please help me how can I do |
Looking at the proposals.... |
@railway17 I really liked your thorough summary of the problems, but wasn't a big fan of your proposal for a few reasons:
That said, here are some high-level goals we should strive for in a solution for this issue:
|
I think we're still looking for a proposal that can encapsulate the points that @roryabraham has mentioned above. Maybe we need a |
Thank you. |
I agree with @roryabraham points.
if (this.props.preferredLocale == 'es') {
+ enFormattedAmount = amount.replace(/\./g, '').replace(/,/g, '.');
+ } we don't do this. The app is not specifically configured for Spanish. There could be many languages. The solution should work with any language that we have or add in the future. for this purpose, we have
I think, we are not concerned with the number of segments ATM. Only the decimal part matters. it's totally fine that if you enter |
I think we had another issue to localize IOU strings. cc: @iwiznia |
I agree that onyx should not save localized data. |
Yes, I think these messages are generated from the backend. |
Any texts coming from the server can remain unlocalized, since we yet have to add support for localization in the server, for now only the frontend has the framework to localize things. |
If you are referring to the comment you see in the chat after doing the request, which is generated in the server, then no. I don't like @songlang1994's proposal of putting the decimal and thousand separator in the translation files, mostly because they are already defined inside Intl.NumberFormat. The proposal from @mdneyazahmad is a bit better, but kind of odd too, I would recommend using Regarding the big number pad UI, I agree it should display only the decimal separator of the current locale (grabbed using the method described in the paragraph above). Regarding the physical keyboard, nothing needs to be done there, if you type a number, it shows up, if you type your locale's decimal separator it will add it and if you type anything else, then nothing should happen. As for how we return the value to a component using the BigNumberPad, it should always be as a number, not as a string formatted number. |
@iwiznia I've post a new implement above which use Intl.NumberFormat to grab the locale digits. See LocaleDiditUtils in this comment. #6427 (comment) It's just a demo so I didn't optimize the performance yet. |
Oh, I think I missed that one, sorry. Did not even think about other numbers like arabic. |
Thanks for the input @iwiznia.
Based on this, We won't update the message that is coming from the backend in https://user-images.githubusercontent.com/43996225/143068980-41848fd5-bc85-4b2b-9a6b-1315610211ff.jpg. But some parts of it can be localized such as IOU Preview and Header of Split Page. @songlang1994 your proposal is good for BigNumberpad but could you please add more details for these changes as well. Thanks for your patience and for helping us understand your proposal. |
First of all, I'd like to thank @songlang1994 for helping us clear this up in this comment. @parasharrajat has a good point. So far, we haven't addressed how we will localize numbers that are coming from the back-end. I'm assuming @songlang1994 would want to use @songlang1994, would you be willing to show a proposal for how that would work? |
The messages from API are actually TEXT rather than numbers. So I think the localization process should be taken care of by the server. BTW, // File: src/components/ReportActionItem/IOUPreview.js
// The original `cachedTotal` from API was something like "(HK$63.69)".
// `IOUPreview` convert it to "HK$63.69".
const cachedTotal = props.iouReport.cachedTotal ? props.iouReport.cachedTotal.replace(/[()]/g, '') : ''; |
yeah, IOUQuote is populated from the backend and we can skip that for now. But we can still change the IOUPreview amount and currency. Look at the IOUBadge for this. And there is one more screen that needs to be localized the header of It would be great if you can submit a finalized proposal in one place so that others are aware of it. But Overall, I like @songlang1994 's proposal. cc: @Luke9389 🎀 👀 🎀 C+ reviewed |
We're hoping to do localization entirely on the front end. @songlang1994, would you be willing to post the exact response the server is giving? Is it literally returning "Requested HK$12.88 from Test"? If so, maybe we should change the return to be an object, like
That way the front end can localize it more easily. |
Yep, I'm in the same boat as @parasharrajat. I'm ready to give the green light to @songlang1994 once a final proposal with all front end screens is submitted. It's OK to skip |
@Luke9389 Yes. The data from server related to "message": [
{
"type": "COMMENT",
"html": "Requested HK$12.58 from Test",
"text": "Requested HK$12.58 from Test",
"isEdited": false
}
], Click to expand the full response// POST /api?command=Report_GetHistory
// [FormData]
//
// reportID: 85812791
// reportActionsOffset: -1
// reportActionsLimit: 50
// shouldRetry: true
// email: songlang1994@outlook.com
// authToken: 01BC5DB123416766C2665414B243244FA25B6CC8666E3C5449D3E3B5C83C03C5F4434C55E7B7047F90C951D410624C855E1FBB6D8A682A9E47B3297182318EF3DDDA6E744F669B0BC28C92DC4778548272BBE38C19B7C63EDC046F1A096B9FFB0FC7E0000943BE6117129D0233FDB6CDC15CB21998F0FE4E9DE7F5AB021E353EB97F55F99ECA36D83CA47E3F77A8F517A742D7837F47EE0E1403DF5A37B2DDB188DCF2930D3A36E94D5A5AC4EBC9D4781FDD382075DE91293901CCAE276198AA08CC22ACB0E7D5564EFA5B1AABB2C31C6FFCF5104BF029C8D0A49D3F8FC1F28E9CFD58A239416CE0045A9D5BC3148C1E9DFBF4DC4FB9A21B7464A332E369E2ECDD1DE117B5AA0CC85D618C6D9E48D5E19FC7F3D8C5585A46A3CC8F603634227DB9CFC3B57BFA9FD95FB97B7AC200D6BE08932CDEA7FBF5B293674FE1D8CE4AD514B61B9E50BB328399EFA525D576BB95
// referer: ecash
// api_setCookie: false
{
"history": [
{
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "[email protected]"
}
],
"actorEmail": "[email protected]",
"actorAccountID": 11012599,
"message": [
{
"type": "COMMENT",
"html": "Requested HK$12.99 from Test",
"text": "Requested HK$12.99 from Test",
"isEdited": false
}
],
"originalMessage": {
"IOUReportID": 86311814,
"IOUTransactionID": "58630061686784005",
"amount": 1299,
"comment": "",
"currency": "HKD",
"type": "create"
},
"avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
"created": "Dec 31 2021 12:05am PST",
"timestamp": 1640937928,
"automatic": false,
"sequenceNumber": 5,
"actionName": "IOU",
"shouldShow": true,
"clientID": "",
"reportActionID": "917927660"
},
{
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "[email protected]"
}
],
"actorEmail": "[email protected]",
"actorAccountID": 11012599,
"message": [
{
"type": "COMMENT",
"html": "Requested HK$12.66 from Test",
"text": "Requested HK$12.66 from Test",
"isEdited": false
}
],
"originalMessage": {
"IOUReportID": 86311814,
"IOUTransactionID": "58630061686784004",
"amount": 1266,
"comment": "",
"currency": "HKD",
"type": "create"
},
"avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
"created": "Dec 31 2021 12:04am PST",
"timestamp": 1640937899,
"automatic": false,
"sequenceNumber": 4,
"actionName": "IOU",
"shouldShow": true,
"clientID": "",
"reportActionID": "917927544"
},
{
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "[email protected]"
}
],
"actorEmail": "[email protected]",
"actorAccountID": 11012599,
"message": [
{
"type": "COMMENT",
"html": "Requested HK$12.88 from Test",
"text": "Requested HK$12.88 from Test",
"isEdited": false
}
],
"originalMessage": {
"IOUReportID": 86311814,
"IOUTransactionID": "58630061686784003",
"amount": 1288,
"comment": "",
"currency": "HKD",
"type": "create"
},
"avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
"created": "Dec 31 2021 12:03am PST",
"timestamp": 1640937829,
"automatic": false,
"sequenceNumber": 3,
"actionName": "IOU",
"shouldShow": true,
"clientID": "",
"reportActionID": "917927269"
},
{
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "[email protected]"
}
],
"actorEmail": "[email protected]",
"actorAccountID": 11012599,
"message": [
{
"type": "COMMENT",
"html": "Requested HK$12.58 from Test",
"text": "Requested HK$12.58 from Test",
"isEdited": false
}
],
"originalMessage": {
"IOUReportID": 86311814,
"IOUTransactionID": "58630061686784002",
"amount": 1258,
"comment": "",
"currency": "HKD",
"type": "create"
},
"avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
"created": "Dec 30 2021 11:38pm PST",
"timestamp": 1640936298,
"automatic": false,
"sequenceNumber": 2,
"actionName": "IOU",
"shouldShow": true,
"clientID": "",
"reportActionID": "917920802"
},
{
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "[email protected]"
}
],
"actorEmail": "[email protected]",
"actorAccountID": 11012599,
"message": [
{
"type": "COMMENT",
"html": "Requested HK$12.58 from Test",
"text": "Requested HK$12.58 from Test",
"isEdited": false
}
],
"originalMessage": {
"IOUReportID": 86311814,
"IOUTransactionID": "58630061686784001",
"amount": 1258,
"comment": "",
"currency": "HKD",
"type": "create"
},
"avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
"created": "Dec 30 2021 11:32pm PST",
"timestamp": 1640935967,
"automatic": false,
"sequenceNumber": 1,
"actionName": "IOU",
"shouldShow": true,
"clientID": "",
"reportActionID": "917920460"
},
{
"actionName": "CREATED",
"created": "Dec 20 2021 10:51pm PST",
"timestamp": 1640069488,
"avatar": "https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png",
"message": [
{
"type": "TEXT",
"style": "strong",
"text": "__fake__"
},
{
"type": "TEXT",
"style": "normal",
"text": " created this report"
}
],
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "__fake__"
}
],
"automatic": false,
"sequenceNumber": 0,
"shouldShow": true
}
],
"jsonCode": 200,
"requestID": "6c93e6470c2e3b4c-SJC"
} |
@parasharrajat
Proposal (v3)
// File: src/libs/NumberFormatUtils/index.js, src/libs/NumberFormatUtils/index.native.js
function format(locale, number, options) {
return new Intl.NumberFormat(locale, options).format(number);
}
function formatToParts(locale, number, options) {
return new Intl.NumberFormat(locale, options).formatToParts(number);
}
export {
format,
formatToParts,
};
// File: src/libs/LocaleDigitUtils.js
import invariant from 'invariant';
import _ from 'underscore';
import * as NumberFormatUtils from './NumberFormatUtils';
const CACHE = {};
const STANDARD_DIGITS = Object.freeze(['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '.', '-', ',']);
const INDEX_DECIMAL = 10;
const INDEX_MINUS_SIGN = 11;
const INDEX_GROUP = 12;
function getLocaleDigits(locale) {
if (!CACHE[locale]) {
const localeDigits = _.clone(STANDARD_DIGITS);
for (let i = 0; i <= 9; i++) {
localeDigits[i] = NumberFormatUtils.format(locale, i);
}
_.forEach(NumberFormatUtils.formatToParts(locale, 1000000.5), (part) => {
switch (part.type) {
case 'decimal':
localeDigits[INDEX_DECIMAL] = part.value;
break;
case 'minusSign':
localeDigits[INDEX_MINUS_SIGN] = part.value;
break;
case 'group':
localeDigits[INDEX_GROUP] = part.value;
break;
default:
break;
}
});
CACHE[locale] = Object.freeze(localeDigits);
}
return CACHE[locale];
}
function isValidLocaleDigit(locale, digit) {
return getLocaleDigits(locale).includes(digit);
}
function toLocaleDigit(locale, digit) {
const index = _.indexOf(STANDARD_DIGITS, digit);
invariant(index > -1, `"${digit}" must be in ${JSON.stringify(STANDARD_DIGITS)}`);
return getLocaleDigits(locale)[index];
}
function fromLocaleDigit(locale, localeDigit) {
const index = _.indexOf(getLocaleDigits(locale), localeDigit);
invariant(index > -1, `"${localeDigit}" must be in ${JSON.stringify(getLocaleDigits(locale))}`);
return STANDARD_DIGITS[index];
}
export {
isValidLocaleDigit,
toLocaleDigit,
fromLocaleDigit,
};
// File: src/components/BigNumberPad.js
<Button
key={column}
style={[styles.flex1, marginLeft]}
- text={column}
+ text={column === '<' ? column : this.props.toLocaleDigit(column)}
onLongPress={() => this.handleLongPress(column)}
// File: src/pages/iou/steps/IOUAmountPage.js
class IOUAmountPage {
// ...
get formattedInputingAmount() {
if (!this.state.amount) {
return '';
}
const endsWithDecimalPoint = this.state.amount.endsWith('.');
if (endsWithDecimalPoint) {
const localized = this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: 1});
return localized.slice(0, -1);
}
const fraction = this.state.amount.split('.')[1];
const fractionDigits = fraction ? fraction.length : 0;
return this.props.numberFormat(this.state.amount, {useGrouping: false, minimumFractionDigits: fractionDigits});
}
updateAmount(text) {
this.setState((prevState) => {
const amount = this.tryConvertToStandardDigits(text);
return amount !== undefined && this.validateAmount(amount)
? {amount: this.stripCommaFromAmount(amount)}
: prevState;
});
}
tryConvertToStandardDigits(localeDigits) {
try {
return _.chain([...localeDigits])
.map(char => this.props.fromLocaleDigit(char))
.join('')
.value();
} catch {
return undefined;
}
}
// ...
}
// File: src/components/ReportActionItem/IOUPreview.js
- const cachedTotal = props.iouReport.cachedTotal ? props.iouReport.cachedTotal.replace(/[()]/g, '') : '';
+ const cachedTotal = props.iouReport.total && props.iouReport.currency
+ ? props.numberFormat(
+ props.iouReport.total / 100, // Divide by 100 because total is in cents
+ {style: 'currency', currency: props.iouReport.currency},
+ ) : ''; |
Is |
Yup. Overall, the proposal is fine. There are a few modifications and simplifications needed but I will point them in review. Waiting for @Luke9389's decision here... Changes could be:
We don't need to rename this.
_.memoize instead of local cache.
we know that amount will always have at most 2 fraction digits so we can hardcode it. Missing parts
|
I think I'm actually a fan of the name @songlang1994 thank you very much for your patience in developing this proposal. You've got the 🟢 green light 🟢 to start a PR! |
No. It does not matter much. Happy with anything. |
Thank you @Luke9389. I will send a PR within 2 days. Should I apply for the Upwork job now? The job link mentioned in this comment is unavailable. |
Hey @songlang1994, please apply here (I updated the link, but it got lost in the hidden comments in this issue). Thanks! |
Aha, thank you @bfitzexpensify. I forgot the hidden comments. |
Deployed to staging. @songlang1994, once this is deployed to production, this issue will automatically update, and then we'll pay out the fee once there have been no regressions after 7 days. |
@bfitzexpensify Okay. That sounds good for me. Thank you. |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.32-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-02-01. 🎊 |
Paid out contracts. Thanks @songlang1994! |
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:
Expected Result:
All money amounts are localized to Spanish, comma as decimal separator. Eg: C$ 23,85
Actual Result:
Decimal separators are not localized in some screens. They are shown as English, in dots. Eg: C$ 23.85
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.1.16-1
Reproducible in staging?: Yes
Reproducible in production?: yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Expensify/Expensify Issue URL:
Issue reported by: Applause
Slack conversation:
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: