Closed kbecciv closed 9 months ago
Triggered auto assignment to @adelekennedy (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Job added to Upwork: https://www.upwork.com/jobs/~017b6ff5aa114aa173
Platforms
in OP are ✅)Triggered auto assignment to @flaviadefaria (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External
)
Distance message changes from 'mile' to 'mi' on IOU detail page
The root cause of the problem is that the unit of measurement for distance is hardcoded as 'mi' in the CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES
. This constant is defined as 'mi', not 'miles' or 'mili'.
If we only want to use miles
instead of mili
, we just need to change the const into miles
We can also keep the mi
but change it to miles
instead on Report Screen after the map is loading
Distance message changes from mile
to mi
on IOU detail page
The root cause of the problem is located inside the function getTransactionDetails function located at src/libs/ReportUtils.js
function getTransactionDetails(transaction) {
const report = getReport(transaction.reportID);
return {
created: TransactionUtils.getCreated(transaction),
amount: TransactionUtils.getAmount(transaction, isExpenseReport(report)),
currency: TransactionUtils.getCurrency(transaction),
comment: TransactionUtils.getDescription(transaction),
merchant: TransactionUtils.getMerchant(transaction), // THE FORMAT ISSUE IS LOCATED HERE
};
}
At field merchant, it is returning in this following format:
2921.48 mi @ R$0.655 / mi
.
Since we want to display in this format below:
2921.48 miles @ R$0.655 / mile
We need to implement a way to parse mi
to miles
or mile
(if singular) and same to km
to kilometers
or kilometer
(if singular).
We need to implement a comparison like the below before assign to the field merchant
at function getTransactionDetails
. Solution is similar to how it is done at getDistanceMerchant
That is why at IOU creation page (where we create the payment request) we see the expected format and at IOU detail page we don't see the same format, because it is using the default CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES.
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');
function getTransactionDetails(transaction) {
const report = getReport(transaction.reportID);
return {
created: TransactionUtils.getCreated(transaction),
amount: TransactionUtils.getAmount(transaction, isExpenseReport(report)),
currency: TransactionUtils.getCurrency(transaction),
comment: TransactionUtils.getDescription(transaction),
merchant: TransactionUtils.getMerchant(transaction), // HERE NEED TO CALL A FUNCTION TO CONVERT UNITS OR DO IT INSIDE getMerchant.
};
}
Alternatively, I took a look into transaction.merchant
located at MoneyRequestPreview.js (look at props.transaction
)
That value is already set as the wrong format:
2921.48 mi @ R$0.655 / mi
Seems like this value comes with that format directly from backend, from /api?command=OpenReport
So if it is not supposed to receive such value, we should create tests to avoid that creating testing.
We could also create a separated field called transaction.formattedMerchant
from the API, with the expected format, similar to what is done with formattedAmount
. That way we would keep the transaction.merchant
as it is and just where we want to use the full format naming (Miles/Mile or Kilometers/Kilometer) by accessing the transaction.formattedMerchant
(we can improve naming to a more meaningful one)
If the format 2921.48 mi @ R$0.655 / mi
is never used, I would recommend implementing a temporary fix on the website, but the API should be returning the right value.
📣 @lcsvcn! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Contributor details Your Expensify account email: lucas.nicolau.developer@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01686d9f62c3ed0541
✅ Contributor details stored successfully. Thank you for contributing to Expensify!
Distance money request has different subtitle for distance/amount text in chat details pages compared with while making a distance money request.
Root cause of the problem is API syncing. While making a CreateDistanceRequest API call, code just sends transaction ID and remaining calculations are done at serverside. So basically format shown while making a new distance money request is client side calculation ONLY and when server sends back response, Onyx gets updated with server response below:
So Basically this issue just happening in online mode.
In ReportsPreview and MoneyRequestPreview we need to identify which chats are Money Requests chats and re-calculate subtitle instead of printing what server sent us.
In ReportsPreview we need to update following section:
with
const isDistanceRequest = hasOnlyOneReceiptRequest && transactionsWithReceipts[0] && TransactionUtils.isDistanceRequest(transactionsWithReceipts[0]);
const distanceInMeters = hasOnlyOneReceiptRequest && lodashGet(transactionsWithReceipts[0], 'routes.route0.distance', undefined);
const getUpdatedTransactionSubtitle= (distanceInMeters) => {
const { currency, rate } = props.mileageRate;
return DistanceRequestUtils.getDistanceMerchant(distanceInMeters, CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, rate, currency, props.translate)
}
Update previewSubtitle to:
const previewSubtitle = hasOnlyOneReceiptRequest
? (isDistanceRequest && distanceInMeters ? getUpdatedTransactionSubtitle(distanceInMeters) : TransactionUtils.getMerchant(transactionsWithReceipts[0]))
: props.translate('iou.requestCount', {
count: numberOfRequests,
scanningReceipts: numberOfScanningReceipts,
});
This will render it back in client side format.
In MoneyRequestPreview we need to update in similar fashion here:
https://github.com/Expensify/App/blob/3fe7ab1c7d860f40eeb9a52b911f8d7bc4781dd5/src/components/ReportActionItem/MoneyRequestPreview.js#L161
update requestMerchant using getUpdatedTransactionSubtitle
method in step 4 above.
Since getUpdatedTransactionSubtitle
is generic we can add it in DistanceRequestUtils
class and use it in both MoneyRequestPreview and the ReportsPreview.
We also need to have mileageRates using:
DistanceRequestUtils.getDefaultMileageRate(ReportUtils.getPolicy(props.chatReport.policyID));
in getUpdatedTransactionSubtitle
or just pass through props
To synchronize with the backend, we can use the same format returned by the backend on the client side. Otherwise, we would need to recalculate subtitles for distance requests ONLY as describe above.
@adelekennedy Looks like I was incorrectly double assigned here by the external label. We are discussing fixing this here. In the meantime, going to un-assign as this doesn't need two BZ assignees.
@mananjadhav any update regarding my proposal?
I think this should be fixed from the backend?
🎀 👀 🎀 C+ reviewed.
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@mananjadhav yes you are right but if backend is getting overloaded with work then we can quickly deploy this fix in the front end. What do you think? Shall I share updated code with you in video so you may double check if everything is alright?
@mananjadhav and @marcochavezf yes, it should be fixed in the backend.
As per my proposal, I still think we are missing verification to validate the back-end consistency, so we should implement a way to log a warning for developers that if we spot that the format is coming in a wrong format, not exclusive for this issue, but for easy identification for further issues.
I also suggested a quick fix for now, if back-end fix would take a lot of time to complete.
What do you think of my proposal?
@mananjadhav, @marcochavezf, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@mananjadhav @marcochavezf pending response on the above
I still feel the ideal fix will be on the backend.
@marcochavezf what are your thoughts should we move this internal?
@mananjadhav @marcochavezf @adelekennedy 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!
Yup, moving it to internal. I have a few other high priorities on my plate I will check this out soon
Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.
No update
no updated
@mananjadhav @marcochavezf @adelekennedy this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
I'm swamped with other high-priority items, I will come back to this one after I finish the other pending items
@mananjadhav, @marcochavezf, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
No update, but I am prioritizing this one for this week
@mananjadhav @marcochavezf @adelekennedy this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!
I will work on this one today
Still wrapping up some priorities for wave7, no update atm
No update
No update, focused on TU launch
Small issue - deprioritizing
@marcochavezf should we make this monthly or will it just get lost?
Yup, monthly will be fine. It's a minor bug and I'm still wrapping up some other higher-priority items
@marcochavezf we're at the month mark here - is this still something you can get to or is this small enough we should close at this point?
I would like to check it out eventually, but given the critical priority of wave issues, I will come back to this one after wrapping up my pending tasks.
@marcochavezf with waves a little slower right now can this be worked on? Otherwise it's a small enough bug I wonder if we should close?
Yeah, it would be better to close it. I don't think the ROI is worth the effort to investigate and fix it given our priorities. We can re-open later if we think it's important or have more capacity.
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:
Expected Result:
The distance message on the IOU detail page should display 'mile' or 'miles' as the unit of measurement
Actual Result:
The distance message on the IOU detail page displays 'mi' as the unit of measurement.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.68.12 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/93399543/e59fb75a-139a-47d1-8e5f-dd219ae2970d
https://github.com/Expensify/App/assets/93399543/f51f961c-e45a-458f-ba90-8593c3f8c691
Expensify/Expensify Issue URL: Issue reported by: @jo-ui Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694256759509979
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @marcochavezfIssue Owner
Current Issue Owner: @marcochavezf