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

Jump to bottom

Optimistic report previews #21578

Merged
merged 28 commits into from
Jul 11, 2023
Merged

Optimistic report previews #21578

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b70230f
Optimistic report preview when sending money.
cristipaval Jun 21, 2023
623b8a1
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jun 23, 2023
7b177a5
Improve optimistic report preview when requesting money.
cristipaval Jun 23, 2023
1685f56
Optimistically update report preview when requesting money in an exis…
cristipaval Jun 23, 2023
62d0420
Optimistic report preview when pying money requests
cristipaval Jun 26, 2023
aa3a4b0
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jun 27, 2023
723a5d6
Fix optimistic report preview when sending money.
cristipaval Jun 27, 2023
50ddd45
Fix optimistic report preview when paying money request
cristipaval Jun 27, 2023
f5e4f02
Fix lint.
cristipaval Jun 28, 2023
279732d
run prettier
cristipaval Jun 28, 2023
769b91b
Update src/libs/ReportUtils.js
cristipaval Jun 30, 2023
f3631c1
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jun 30, 2023
b4a361e
Reuse logic to build report preview message.
cristipaval Jul 3, 2023
1679480
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jul 3, 2023
daf707f
Remove unecessary param
cristipaval Jul 3, 2023
8b4ec46
update report preview when already exists at money request
cristipaval Jul 3, 2023
91e9371
improve optimistic report preview when paying money request
cristipaval Jul 3, 2023
6f88bd1
Cleanup report preview when splitting the bill.
cristipaval Jul 3, 2023
544604f
Fix console warning.
cristipaval Jul 3, 2023
80c24b1
Make Lint happy.
cristipaval Jul 4, 2023
40a62ad
Improve code readability in buildOptimisticReportPreview
cristipaval Jul 5, 2023
e17a359
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jul 5, 2023
70bff8c
Update src/libs/ReportUtils.js
cristipaval Jul 6, 2023
7c97057
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jul 6, 2023
bfe57c0
Fix.
cristipaval Jul 6, 2023
1f20f9a
Make optimistic reportActions data more readable
cristipaval Jul 6, 2023
201f426
Merge remote-tracking branch 'origin/main' into cristi_optimistic-rep…
cristipaval Jul 8, 2023
c379a5c
Always update successData
cristipaval Jul 10, 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
43 changes: 27 additions & 16 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,28 +1426,39 @@ function buildOptimisticIOUReportAction(type, amount, currency, comment, partici
};
}

function buildOptimisticReportPreview(reportID, iouReportID, payeeAccountID) {
return {
function buildOptimisticReportPreview(chatReport, iouReport, existingReportPreviewAction, justPaidOutstandingIOU) {
cristipaval marked this conversation as resolved.
Show resolved Hide resolved
const reportPreviewAction = existingReportPreviewAction || {
reportActionID: NumberUtils.rand64(),
reportID,
created: DateUtils.getDBTime(),
reportID: chatReport.reportID,
actionName: CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
accountID: payeeAccountID,
message: [
{
html: '',
text: '',
isEdited: false,
type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
},
],
originalMessage: {
linkedReportID: iouReportID,
linkedReportID: iouReport.reportID,
},
actorEmail: currentUserEmail,
actorAccountID: currentUserAccountID,
};
cristipaval marked this conversation as resolved.
Show resolved Hide resolved

reportPreviewAction.created = DateUtils.getDBTime();
reportPreviewAction.accountID = iouReport.managerID;

const reportAmount = CurrencyUtils.convertToDisplayString(getMoneyRequestTotal(iouReport), iouReport.currency);
const payerName = isPolicyExpenseChat(chatReport) ? getPolicyName(chatReport) : getDisplayNameForParticipant(iouReport.managerID || '', true);
cristipaval marked this conversation as resolved.
Show resolved Hide resolved
const message =
iouReport.hasOutstandingIOU && !justPaidOutstandingIOU
? Localize.translateLocal('iou.payerOwesAmount', {payer: payerName, amount: reportAmount})
: Localize.translateLocal('iou.payerSettled', {amount: reportAmount});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ReportUtils.isSettled?

reportPreviewAction.message = [
{
html: message,
text: message,
isEdited: false,
type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
},
];

reportPreviewAction.actorEmail = iouReport.managerEmail;
reportPreviewAction.actorAccountID = iouReport.managerID;

return reportPreviewAction;
}

function buildOptimisticTaskReportAction(taskReportID, actionName, message = '') {
Expand Down
112 changes: 81 additions & 31 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,7 @@ function buildOnyxDataForMoneyRequest(
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
...(isNewChatReport ? {[chatCreatedAction.reportActionID]: chatCreatedAction} : {}),
[reportPreviewAction.reportActionID]: {
...(isNewReportPreviewAction ? reportPreviewAction : {created: DateUtils.getDBTime()}),
},
[reportPreviewAction.reportActionID]: reportPreviewAction,
Julesssss marked this conversation as resolved.
Show resolved Hide resolved
},
},
{
Expand Down Expand Up @@ -386,12 +384,9 @@ function requestMoney(report, amount, currency, payeeEmail, payeeAccountID, part
},
};

let isNewReportPreviewAction = false;
let reportPreviewAction = isNewIOUReport ? null : ReportActionsUtils.getReportPreviewAction(chatReport.reportID, iouReport.reportID);
if (!reportPreviewAction) {
isNewReportPreviewAction = true;
reportPreviewAction = ReportUtils.buildOptimisticReportPreview(chatReport.reportID, iouReport.reportID);
}
let reportPreviewAction = ReportActionsUtils.getReportPreviewAction(chatReport.reportID, iouReport.reportID);
const isNewReportPreviewAction = Boolean(!reportPreviewAction);
reportPreviewAction = ReportUtils.buildOptimisticReportPreview(chatReport, iouReport, reportPreviewAction);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks odd and will likely introduce a bug when you try to rollback data, since you no longer have the original created date anymore. As in my first comment, I think we should build an optimistic action only if we don't have an existing one.


// STEP 5: Build Onyx Data
const [optimisticData, successData, failureData] = buildOnyxDataForMoneyRequest(
Expand Down Expand Up @@ -648,12 +643,9 @@ function createSplitsAndOnyxData(participants, currentUserLogin, currentUserAcco
},
};

let isNewOneOnOneReportPreviewAction = false;
let oneOnOneReportPreviewAction = ReportActionsUtils.getReportPreviewAction(oneOnOneChatReport.reportID, oneOnOneIOUReport.reportID);
if (!oneOnOneReportPreviewAction) {
isNewOneOnOneReportPreviewAction = true;
oneOnOneReportPreviewAction = ReportUtils.buildOptimisticReportPreview(oneOnOneChatReport.reportID, oneOnOneIOUReport.reportID);
}
const isNewOneOnOneReportPreviewAction = Boolean(!oneOnOneReportPreviewAction);
oneOnOneReportPreviewAction = ReportUtils.buildOptimisticReportPreview(oneOnOneChatReport, oneOnOneIOUReport, oneOnOneReportPreviewAction);

// STEP 5: Build Onyx Data
const [oneOnOneOptimisticData, oneOnOneSuccessData, oneOnOneFailureData] = buildOnyxDataForMoneyRequest(
Expand Down Expand Up @@ -942,14 +934,16 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
true,
);

const reportPreviewAction = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticIOUReport);

// First, add data that will be used in all cases
const optimisticChatReportData = {
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${chatReport.reportID}`,
value: {
...chatReport,
lastReadTime: DateUtils.getDBTime(),
lastVisibleActionCreated: optimisticIOUReportAction.created,
lastVisibleActionCreated: reportPreviewAction.created,
},
};
const optimisticIOUReportData = {
Expand All @@ -961,16 +955,28 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
lastMessageHtml: optimisticIOUReportAction.message[0].html,
},
};
const optimisticReportActionsData = {
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticIOUReport.reportID}`,
value: {
[optimisticIOUReportAction.reportActionID]: {
...optimisticIOUReportAction,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
const optimisticReportActionsData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticIOUReport.reportID}`,
value: {
[optimisticIOUReportAction.reportActionID]: {
...optimisticIOUReportAction,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
},
},
};
{
Copy link
Member

@parasharrajat parasharrajat Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • Fix, Line 1037.
optimisticReportActionsData.onyxMethod = Onyx.METHOD.SET;

optimisticReportActionsData is now an array.

  • Fix, line 1083,.

Copy link
Member

@parasharrajat parasharrajat Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It might be better to break this array into two objects. More readability.

optimisticChatReportActionData
optimisticIOUReportActionData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be better to break this array into two objects. More readability.

I can do this, but I'd rather do it in a separate PR. We want to get this one merged as it is part of the wave3 initiative and we want to wrap it up.

Copy link
Member

@parasharrajat parasharrajat Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But we still need to fix the lines #21578 (comment). They will cause issues.

we can not do optimisticReportActionsData.onyxMethod = Onyx.METHOD.SET; anymore. optimisticReportActionsData is an array so we need to set the onyxMethod on each item in the array.

Copy link
Member

Choose a reason for hiding this comment

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

It might be better to break this array into two objects. More readability.

I just feel that it is a quick change and we are still working on the code. But not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I missed the fixes, it was also not clear what you were pointing out. Good catch though! They are now fixed.

onyxMethod: Onyx.METHOD.MERGE,
cristipaval marked this conversation as resolved.
Show resolved Hide resolved
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[reportPreviewAction.reportActionID]: {
...reportPreviewAction,
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
cristipaval marked this conversation as resolved.
Show resolved Hide resolved
},
},
},
];

const successData = [
{
Expand All @@ -987,6 +993,15 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${optimisticTransaction.transactionID}`,
value: {pendingAction: null},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[reportPreviewAction.reportActionID]: {
pendingAction: null,
},
},
},
];

const failureData = [
Expand Down Expand Up @@ -1054,18 +1069,29 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
// Add an optimistic created action to the optimistic reportActions data
optimisticReportActionsData.value[optimisticCreatedAction.reportActionID] = optimisticCreatedAction;
} else {
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticIOUReport.reportID}`,
value: {
[optimisticIOUReportAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.other'),
failureData.push(
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticIOUReport.reportID}`,
value: {
[optimisticIOUReportAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.other'),
},
},
},
});
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[reportPreviewAction.reportActionID]: {
created: reportPreviewAction.created,
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 might not be right since we are overriding the old preview action with a new one in line 389.

},
},
},
);
}

const optimisticData = [optimisticChatReportData, optimisticIOUReportData, optimisticReportActionsData, optimisticTransactionData];
const optimisticData = [optimisticChatReportData, optimisticIOUReportData, ...optimisticReportActionsData, optimisticTransactionData];
if (!_.isEmpty(optimisticPersonalDetailListData)) {
optimisticData.push(optimisticPersonalDetailListData);
}
Expand All @@ -1079,6 +1105,7 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
transactionID: optimisticTransaction.transactionID,
newIOUReportDetails,
createdReportActionID: isNewChat ? optimisticCreatedAction.reportActionID : 0,
reportPreviewReportActionID: reportPreviewAction.reportActionID,
},
optimisticData,
successData,
Expand Down Expand Up @@ -1113,6 +1140,13 @@ function getPayMoneyRequestParams(chatReport, iouReport, recipient, paymentMetho
login: recipient.login,
};

const optimisticReportPreviewAction = ReportUtils.buildOptimisticReportPreview(
chatReport,
iouReport,
ReportActionsUtils.getReportPreviewAction(chatReport.reportID, iouReport.reportID),
true,
);

const optimisticData = [
{
onyxMethod: Onyx.METHOD.MERGE,
Expand All @@ -1137,6 +1171,13 @@ function getPayMoneyRequestParams(chatReport, iouReport, recipient, paymentMetho
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[optimisticReportPreviewAction.reportActionID]: optimisticReportPreviewAction,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
Expand Down Expand Up @@ -1194,6 +1235,15 @@ function getPayMoneyRequestParams(chatReport, iouReport, recipient, paymentMetho
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[optimisticReportPreviewAction.reportActionID]: {
created: optimisticReportPreviewAction.created,
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${optimisticTransaction.transactionID}`,
Expand Down