-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Display displayName instead of primaryLogin on the ReportPreview message #20446
Changes from 17 commits
d2c07a2
5e1b4c7
4338186
b58a8f8
3b8bb48
3788f8f
9171962
0da6a76
644d71f
d58e95c
1e1e34d
e329173
caa45ea
b57b897
9009632
f4e1390
bad0519
5bd7268
0320938
20b7a1a
e59d39f
d1c6412
117c9f0
a5d0633
e018913
8942536
ba57e73
8142f02
d1e4aae
638fe01
c9a25e5
0fc3ed0
331cc43
0f106df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -978,6 +978,37 @@ function getTransactionReportName(reportAction) { | |
}); | ||
} | ||
|
||
/** | ||
* Get money request message for a IOU report | ||
hoangzinh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @param {Object} report | ||
* @param {Object} reportAction | ||
* @returns {String} | ||
*/ | ||
function getMoneyRequestReportActionMessage(report, reportAction) { | ||
Julesssss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let formattedAmount = null; | ||
let reportHasOutstandingIOU = null; | ||
const totalAmount = getMoneyRequestTotal(report); | ||
const payerName = isExpenseReport(report) ? getPolicyName(report) : getDisplayNameForParticipant(report.managerEmail, true); | ||
|
||
if (totalAmount > 0) { | ||
formattedAmount = CurrencyUtils.convertToDisplayString(totalAmount, report.currency); | ||
reportHasOutstandingIOU = report.hasOutstandingIOU; | ||
} else { | ||
const reportActionMessage = lodashGet(reportAction, 'message[0].html', ''); | ||
|
||
// The totalAmount is 0 after Sign in because OpenApp API won't return iouReports if it's paid, so we need to retrieve it from the reportActionMessage | ||
// reportActionMessage is in the format of either "payer@mail.com owes $1.00" or "paid $1.00" | ||
formattedAmount = _.last(reportActionMessage.split(' ')); | ||
reportHasOutstandingIOU = Boolean(reportActionMessage.match(/ owes /)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems very brittle. Can we check for the report action type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, I think we don't really need it if we only use this utils for ReportPreview action type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this would break if we changed the message, no? I think we should check for action = IOU, type = 'create' instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry @luacmartins we're fixing for ReportPreview action type. so could you explain how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah in that case, we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. honestly not sure what would be prefered here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc: @mountiny for thoughts on #20446 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could:
That will work for this PR and then we can address if we need to start adding more info to the preview action. we usually dont want any duplication of data in the databse so we would have to think how to solve this the best There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @mountiny If we agree with above ^. I will try to update this PR during weekend and it will be ready to review again next week 🚀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just in case we all agree with above. Sorry this is first time I work on a task that need some translation. As @luacmartins mentioned above #20446 (comment). BE returns the settled message either "paid {amount} elsewhere" or "paid {amount} using PayPal.me" (before only "paid {amount}") => How can I get those messages in Spain? Thanks |
||
} | ||
|
||
if (reportHasOutstandingIOU) { | ||
return Localize.translateLocal('iou.payerOwesAmount', {payer: payerName, amount: formattedAmount}); | ||
} | ||
return Localize.translateLocal('iou.payerSettled', {amount: formattedAmount}); | ||
} | ||
|
||
/** | ||
* Get the title for a report. | ||
* | ||
|
@@ -2317,4 +2348,5 @@ export { | |
getMoneyRequestAction, | ||
getBankAccountRoute, | ||
getParentReport, | ||
getMoneyRequestReportActionMessage, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is not used here anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep message here even thought it's not used. Because according to the underscore doc https://underscorejs.org/#map, the fist param is value, and 2nd is index and we would like to use index when build list key
App/src/components/ReportActionItem/ReportPreview.js
Line 107 in 23ca9a7
Btw, I guess we can remove _.map here. It seems all the follow-up messages has been moved into iouReport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think we can remove the map altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mountiny do you agree we can remove the map altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoangzinh Yep lets try removing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the
map
. Feel free to review the PR again. Thanks