Skip to content

[CP Staging] fix regression copy to clipboard #30818

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.

Sign up for GitHub

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

Merged
Beamanator merged 1 commit into from
Nov 3, 2023
Merged

[CP Staging] fix regression copy to clipboard #30818

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4087,7 +4087,7 @@ function getIOUReportActionDisplayMessage(reportAction) {
let displayMessage;
if (originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY) {
const {IOUReportID} = originalMessage;
const {amount, currency} = originalMessage.IOUDetails;
const {amount, currency} = originalMessage.IOUDetails || originalMessage;

@jakub-trzebiatowski jakub-trzebiatowski Nov 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this.

This at least requires an explanation comment. Where are {amount, currency} expected to reside? Why are there sometimes here, sometimes here?

@saranshbalyan-1234 saranshbalyan-1234 Nov 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these values were expected to reside in the root level, but for only our original issue that we fixed, only in that case it is coming inside ioudetails key
It has to be similar behaviour across all the scenarios, if we don't go with this PR we have to revert the original pr which caused this regression and then handle it from backend

@jakub-trzebiatowski jakub-trzebiatowski Nov 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gonals @marcaaron I approved this as it's a deploy blocker, but I didn't really get the explanation above and I still believe something should be done here. I'm just not completely sure what.

@jakub-trzebiatowski jakub-trzebiatowski Nov 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saranshbalyan-1234

Both of these values were expected to reside in the root level, but for only our original issue that we fixed, only in that case it is coming inside ioudetails key

I need help understanding this. Would you re-phrase this sentence?

the original pr which caused this regression

Do you have a specific PR in mind?

@saranshbalyan-1234 saranshbalyan-1234 Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original PR -> my last PR which caused this regression

@jakub-trzebiatowski jakub-trzebiatowski Nov 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original PR -> my last PR which caused this regression

Oh, sure 👍

Still, we need to sketch a plan for removing this originalMessage.IOUDetails || originalMessage expression long-term. Do I understand correctly that you suggested that this inconsistency starts on the backend side?

@saranshbalyan-1234 saranshbalyan-1234 Nov 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I saw this inconsistency. I still have to take it to slack
Did not do due to being hospitalised from past few days.
Just replying to this chat so that I don't miss anything

@jakub-trzebiatowski jakub-trzebiatowski Nov 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'm sorry about your situation 🙁 If your health situation makes it difficult or impossible to work on this, let us know. We might put this on hold for the time of your recovery or something like that.

@mountiny mountiny Nov 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When money is sent (using send money) we store there details in IOUDetails, when money request is paid, the amount and currency is in the originalMessage field

@marcaaron marcaaron Nov 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? That seems weird 😄 Is there some way to explicitly tell whether it's one or the other? Maybe the code should have a comment about it if we are not going to improve this.

const formattedAmount = CurrencyUtils.convertToDisplayString(amount, currency);
const iouReport = getReport(IOUReportID);
const payerName = isExpenseReport(iouReport) ? getPolicyName(iouReport) : getDisplayNameForParticipant(iouReport.managerID, true);
Expand Down