Skip to content
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

fix: allow creating Distance request from expense reports #30881

Merged
merged 3 commits into from
Nov 8, 2023
Merged

fix: allow creating Distance request from expense reports #30881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4dd2eb1
Support distance request from expense reports
paultsimura Oct 27, 2023
25a5538
Merge branch 'main' into fix/30407-distance-from-expense
paultsimura Nov 4, 2023
98327c2
Lint
paultsimura Nov 4, 2023
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
8 changes: 6 additions & 2 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,12 +621,16 @@ function getMoneyRequestInformation(
* @param {Boolean} [billable]
*/
function createDistanceRequest(report, participant, comment, created, transactionID, category, tag, amount, currency, merchant, billable) {
// If the report is an iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an incorrect change. Why not pass the proper report object to createDistanceRequest() in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the same approach when creating a regular money request:

App/src/libs/actions/IOU.js

Lines 847 to 851 in 1dc4593

// If the report is iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
const {payerAccountID, payerEmail, iouReport, chatReport, transaction, iouAction, createdChatReportActionID, createdIOUReportActionID, reportPreviewAction, onyxData} =
getMoneyRequestInformation(currentChatReport, participant, comment, amount, currency, created, merchant, payeeAccountID, payeeEmail, receipt, undefined, category, tag, billable);

What exactly do you mean by a "proper report object" here: is it the one we are fetching as currentChatReport? If so, we'll still need to tell apart a request created from a chat and the one created from an expense report so that we can navigate the user properly. Would you have any suggestions for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just because we do it somewhere else, doesn't mean it's right. It just means that you get to fix it in two places 🎉

What I am saying is that the logic that you added inside the method should have been done outside of the method, and then the proper "chat report" is what is passed to the method in the report param.

If you need something else for the navigation, then I would consider:

  • Moving navigation out of this method (I also dislike this method is doing navigation, it is an anti-pattern of separating concerns)
  • Using a different param to control the navigation specifically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Since this PR is already merged, would you want me to create a cleanup one?


const optimisticReceipt = {
source: ReceiptGeneric,
state: CONST.IOU.RECEIPT_STATE.OPEN,
};
const {iouReport, chatReport, transaction, iouAction, createdChatReportActionID, createdIOUReportActionID, reportPreviewAction, onyxData} = getMoneyRequestInformation(
report,
currentChatReport,
participant,
comment,
amount,
Expand Down Expand Up @@ -660,7 +664,7 @@ function createDistanceRequest(report, participant, comment, created, transactio
},
onyxData,
);
Navigation.dismissModal(chatReport.reportID);
Navigation.dismissModal(isMoneyRequestReport ? report.reportID : chatReport.reportID);
Report.notifyNewAction(chatReport.reportID, userAccountID);
}

Expand Down
5 changes: 3 additions & 2 deletions src/pages/iou/MoneyRequestSelectorPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ function MoneyRequestSelectorPage(props) {
[CONST.IOU.TYPE.SPLIT]: translate('iou.splitBill'),
};
const isFromGlobalCreate = !reportID;
const isExpenseRequest = ReportUtils.isPolicyExpenseChat(props.report);
const shouldDisplayDistanceRequest = isExpenseRequest || isFromGlobalCreate;
const isExpenseChat = ReportUtils.isPolicyExpenseChat(props.report);
const isExpenseReport = ReportUtils.isExpenseReport(props.report);
const shouldDisplayDistanceRequest = isExpenseChat || isExpenseReport || isFromGlobalCreate;

const resetMoneyRequestInfo = () => {
const moneyRequestID = `${iouType}${reportID}`;
Expand Down