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

Followup #23961: update IOU report total in optimistic data on edit money request #25498

Merged
merged 14 commits into from
Sep 14, 2023
Merged

Followup #23961: update IOU report total in optimistic data on edit money request #25498

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f0ff0a7
feat: update IOU report total in optimistic data on edit money request
BeeMargarida Aug 18, 2023
72784d4
Merge branch 'main' into ms/23961-optimistic_data_iou_total_update_af…
BeeMargarida Aug 21, 2023
b7dc724
feat: change message values of report when IOU amount changes
BeeMargarida Aug 22, 2023
7c9706e
fix: typo
BeeMargarida Aug 22, 2023
c7a2d06
fix: re-render when transaction for LHN changes
BeeMargarida Aug 23, 2023
4cbd6d5
fix: simplify total calculation and add missing proptype
BeeMargarida Aug 24, 2023
db3cf8a
fix: small fix
BeeMargarida Aug 24, 2023
ea2926f
feat: improve prop composition
BeeMargarida Aug 24, 2023
4399aa1
fix: reset chatReport data on failure
koko57 Aug 28, 2023
3e98419
Merge branch 'main' into ms/23961-optimistic_data_iou_total_update_af…
BeeMargarida Sep 11, 2023
e89644c
fix: prettier fix
BeeMargarida Sep 11, 2023
529180d
fix: update header when description changes
BeeMargarida Sep 12, 2023
2590bdc
fix: console error
BeeMargarida Sep 13, 2023
58346d1
fix: do not use onyx subscription composition
BeeMargarida Sep 14, 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
9 changes: 8 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHNData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ function OptionRowLHNData({
comment,
policies,
parentReportActions,
transactions,
...propsToForward
}) {
const reportID = propsToForward.reportID;
Expand All @@ -86,6 +87,8 @@ function OptionRowLHNData({
const policy = lodashGet(policies, [`${ONYXKEYS.COLLECTION.POLICY}${fullReport.policyID}`], '');

const parentReportAction = parentReportActions[fullReport.parentReportActionID];
const transactionID = lodashGet(parentReportAction, ['originalMessage', 'IOUTransactionID'], '');
const transaction = lodashGet(transactions, [`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`], '');
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also just load this single transaction by composing multiple withOnyx.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause a lot of re-renders since we're including all the transactions. Similar optimisation can also be done for the parentReportAction above

Copy link
Contributor Author

@BeeMargarida BeeMargarida Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You mean having several withOnyx that depend on one another? Won't those also be injected as props? Or we ignore the transactions prop and only use the one transaction?

Copy link
Contributor Author

@BeeMargarida BeeMargarida Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is possible, because there's no way to access the parentReportAction by key. And this parentReportAction is necessary to access the transaction. So we can't do this in withOnyx.
Or am I missing something? 🤔

The only thing that I think would work is make a withReportActions where we could pass transformValue that selected the report action taking into account the fullreport.

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 you can use selector function from onyx. No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, because in the selector we don't have access to the props, so we couldn't use the fullReport to access the correct report action

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm...
Can't you add the following here:

withOnyx({
            transaction: {
                key: ({parentReportActions, fullReport}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${lodashGet(parentReportActions[fullReport.parentReportActionID], ['originalMessage', 'IOUTransactionID'], '')}`,
                canEvict: false,
            },
        }),

Copy link
Contributor Author

@BeeMargarida BeeMargarida Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yap, I think I can do that, but I can't make the same refactor to parentReportActions.
Well, I can, but I need to make a withReportActions HOC (allows to use the props in transformValue so we can only get the report action that we want). I've developed this and it seems to work fine. Do you think this optimization is worth it?
Note: the optimization could also be applied to the policies prop.

Copy link
Contributor

@allroundexperts allroundexperts Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If you're the owner of this code, and you think its not worth it, then I'm totally fine with this as well!

@mountiny Do you agree?


const optionItemRef = useRef();
const optionItem = useMemo(() => {
Expand All @@ -97,8 +100,9 @@ function OptionRowLHNData({
optionItemRef.current = item;
return item;
// Listen parentReportAction to update title of thread report when parentReportAction changed
// Listen to transaction to update title of transaction report when transaction changed
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [fullReport, reportActions, personalDetails, preferredLocale, policy, parentReportAction]);
}, [fullReport, reportActions, personalDetails, preferredLocale, policy, parentReportAction, transaction]);

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
Expand Down Expand Up @@ -179,6 +183,9 @@ export default React.memo(
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
},
transactions: {
key: ONYXKEYS.COLLECTION.TRANSACTION,
},
BeeMargarida marked this conversation as resolved.
Show resolved Hide resolved
}),
withOnyx({
parentReportActions: {
Expand Down
69 changes: 68 additions & 1 deletion src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,12 +949,64 @@ function editMoneyRequest(transactionID, transactionThreadReportID, transactionC
const transactionThread = allReports[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`];
const transaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
const iouReport = allReports[`${ONYXKEYS.COLLECTION.REPORT}${transactionThread.parentReportID}`];
const chatReport = allReports[`${ONYXKEYS.COLLECTION.REPORT}${iouReport.chatReportID}`];
const isFromExpenseReport = ReportUtils.isExpenseReport(iouReport);

// STEP 2: Build new modified expense report action.
const updatedReportAction = ReportUtils.buildOptimisticModifiedExpenseReportAction(transactionThread, transaction, transactionChanges, isFromExpenseReport);
const updatedTransaction = TransactionUtils.getUpdatedTransaction(transaction, transactionChanges, isFromExpenseReport);

// STEP 3: Compute the IOU total and update the report preview message so LHN amount owed is correct
// Should only update if the transaction matches the currency of the report, else we wait for the update
// from the server with the currency conversion
let updatedIouReport = null;
BeeMargarida marked this conversation as resolved.
Show resolved Hide resolved
const updatedChatReport = {...chatReport};
mountiny marked this conversation as resolved.
Show resolved Hide resolved
if (updatedTransaction.currency === iouReport.currency && updatedTransaction.modifiedAmount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@BeeMargarida @rezkiy37 run into an issue because the modifiedAmount is true after oneamount update, the modified amount is always there, I think we have to look at the amount diff here, are you able to make a follow up PR?

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'll make a PR with that fix

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The regression fix PR: #27451 (comment)

if (ReportUtils.isExpenseReport(iouReport)) {
updatedIouReport = {...iouReport};
updatedIouReport.total -= TransactionUtils.getAmount(transaction, true);
updatedIouReport.total += TransactionUtils.getAmount(updatedTransaction, true);
} else {
updatedIouReport = IOUUtils.updateIOUOwnerAndTotal(
iouReport,
updatedReportAction.actorAccountID,
TransactionUtils.getAmount(transaction, false),
TransactionUtils.getCurrency(transaction),
true,
);
updatedIouReport = IOUUtils.updateIOUOwnerAndTotal(
updatedIouReport,
updatedReportAction.actorAccountID,
TransactionUtils.getAmount(updatedTransaction, false),
TransactionUtils.getCurrency(updatedTransaction),
false,
);
BeeMargarida marked this conversation as resolved.
Show resolved Hide resolved
}

updatedIouReport.cachedTotal = CurrencyUtils.convertToDisplayString(updatedIouReport.total, updatedTransaction.currency);

// Update the last message of the IOU report
const lastMessage = ReportUtils.getIOUReportActionMessage(
iouReport.reportID,
CONST.IOU.REPORT_ACTION_TYPE.CREATE,
BeeMargarida marked this conversation as resolved.
Show resolved Hide resolved
updatedIouReport.total,
'',
updatedTransaction.currency,
'',
false,
);
updatedIouReport.lastMessageText = lastMessage[0].text;
updatedIouReport.lastMessageHtml = lastMessage[0].html;

// Update the last message of the IOU chat report
BeeMargarida marked this conversation as resolved.
Show resolved Hide resolved
const messageText = Localize.translateLocal('iou.payerOwesAmount', {
payer: updatedIouReport.managerEmail,
amount: CurrencyUtils.convertToDisplayString(updatedIouReport.total, updatedIouReport.currency),
BeeMargarida marked this conversation as resolved.
Show resolved Hide resolved
});
updatedChatReport.lastMessageText = messageText;
updatedChatReport.lastMessageHtml = messageText;
}

// STEP 4: Compose the optimistic data
const optimisticData = [
{
Expand All @@ -969,6 +1021,16 @@ function editMoneyRequest(transactionID, transactionThreadReportID, transactionC
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
value: updatedTransaction,
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
value: updatedIouReport,
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.chatReportID}`,
value: updatedChatReport,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have set lastReadTime and lastVisibleActionCreated of transaction thread report in optimistic data.
Without this, unread indicator didn't show even after marking report action as unread after changing date offline.
Issue: #27159

];

const successData = [
Expand All @@ -991,6 +1053,11 @@ function editMoneyRequest(transactionID, transactionThreadReportID, transactionC
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
value: {pendingAction: null},
},
];

const failureData = [
Expand All @@ -1008,7 +1075,7 @@ function editMoneyRequest(transactionID, transactionThreadReportID, transactionC
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.report}`,
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
value: iouReport,
},
mountiny marked this conversation as resolved.
Show resolved Hide resolved
];
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/steps/NewRequestAmountPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function NewRequestAmountPage({route, iou, report}) {
<View style={[styles.flex1, safeAreaPaddingBottomStyle]}>
<HeaderWithBackButton
title={translate('iou.amount')}
onBackButonBackButtonPress={navigateBack}
onBackButtonPress={navigateBack}
/>
{content}
</View>
Expand Down