Skip to content

Fix LHN display issue for money request via scan #25906

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
luacmartins merged 30 commits into from
Sep 5, 2023
Merged

Fix LHN display issue for money request via scan #25906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8eb68aa
Fix LHN display issue for money request via scan
ygshbht Aug 24, 2023
958b1ca
Linting fixes
ygshbht Aug 24, 2023
c735418
Eslint fixes
ygshbht Aug 24, 2023
07e3ad8
Merge branch 'main' into fix-LHN-scan-money-request-issue3
ygshbht Aug 29, 2023
cc6965e
Fix LHN subtitle display issue for money request via scan
ygshbht Aug 30, 2023
84ce15f
Merge https://github.com/Expensify/App into fix-LHN-scan-money-reques…
ygshbht Aug 30, 2023
1c6bb4d
Add lastIOUReportActions dependency to OptionRowLHNData
ygshbht Aug 30, 2023
69b1700
Add ability to show receipt being scanned LHN subtitle for report wit…
ygshbht Aug 31, 2023
455d19e
Merge https://github.com/Expensify/App into fix-LHN-scan-money-reques…
ygshbht Aug 31, 2023
df24a10
Merge https://github.com/Expensify/App into fix-LHN-scan-money-reques…
ygshbht Sep 1, 2023
2e8059f
Display "Receipt is scanning..." only if its the first reportAction
ygshbht Sep 4, 2023
ac7d58a
Merge https://github.com/Expensify/App into fix-LHN-scan-money-reques…
ygshbht Sep 4, 2023
4a4fbcd
Check if thereAreFollowingManualIOURequests before showing report nam…
ygshbht Sep 4, 2023
a772a35
Fix pretteir diff
ygshbht Sep 4, 2023
bca8116
Don't show "Receipt being scanned..." for Workspace chat view LHN sub…
ygshbht Sep 4, 2023
f018b21
Don't show "Report is being scanned..." for Exprense Report View's LH…
ygshbht Sep 4, 2023
1392317
Don't show "Receipt is being scanned..." in Request View LHN subtitle
ygshbht Sep 4, 2023
59630d6
OptionRowLHNData Dependency cleanup for bca81161f1d41d683c2def7129c2f…
ygshbht Sep 4, 2023
73bdde1
set getReportPreviewMessage's shouldConsiderReceiptBeingScanned param…
ygshbht Sep 4, 2023
03e4b3f
Linting fixes
ygshbht Sep 4, 2023
2b99c3c
Refactoring - LHN subtitle report scan
ygshbht Sep 5, 2023
b3f15ba
Refactoring
ygshbht Sep 5, 2023
41e35b6
Bugfix - properly access iouReportID instead of reportID
ygshbht Sep 5, 2023
d9e57a7
Revert LHN subtitle "Report scan in progress..." to use previous meth…
ygshbht Sep 5, 2023
cb294a2
Refactoring LHN subtitle 'Receipt scan in progress...'
ygshbht Sep 5, 2023
5568850
Refactoring
ygshbht Sep 5, 2023
5546081
Remove unnecessary comment
ygshbht Sep 5, 2023
ebad276
Remove unnecessary comment
ygshbht Sep 5, 2023
f652ef5
Prettier linting
ygshbht Sep 5, 2023
4811977
rename lastTransaction to linkedTransaction in OptionRowLHNData
ygshbht Sep 5, 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
19 changes: 18 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 @@ -12,6 +12,9 @@ import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDe
import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN';
import * as Report from '../../libs/actions/Report';
import * as UserUtils from '../../libs/UserUtils';
import * as ReportActionsUtils from '../../libs/ReportActionsUtils';
import * as TransactionUtils from '../../libs/TransactionUtils';

import participantPropTypes from '../participantPropTypes';
import CONST from '../../CONST';
import reportActionPropTypes from '../../pages/home/report/reportActionPropTypes';
Expand Down Expand Up @@ -75,6 +78,7 @@ function OptionRowLHNData({
preferredLocale,
comment,
policies,
receiptTransactions,
parentReportActions,
...propsToForward
}) {
Expand All @@ -88,6 +92,14 @@ function OptionRowLHNData({
const parentReportAction = parentReportActions[fullReport.parentReportActionID];

const optionItemRef = useRef();

const linkedTransaction = useMemo(() => {
const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(reportActions);
const lastReportAction = _.first(sortedReportActions);
return TransactionUtils.getLinkedTransaction(lastReportAction);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [fullReport.reportID, receiptTransactions, reportActions]);
Copy link
Contributor

@allroundexperts allroundexperts Sep 5, 2023

Choose a reason for hiding this comment

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

Why is receiptTransactions is needed as a dependency?

Copy link
Contributor Author

@ygshbht ygshbht Sep 5, 2023

Choose a reason for hiding this comment

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

So that when the status of transaction updates (from scanning to scanned), the LHN is updated too

Copy link
Contributor Author

@ygshbht ygshbht Sep 5, 2023

Choose a reason for hiding this comment

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

Did you mean to ask this in this context? #25906 (comment)

Copy link
Contributor Author

@ygshbht ygshbht Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it depends on the size of the object you are deep comparing and the component that has to be re-rendered. If the deep comparison object is small, like in this case, I think it's a negligible performance issue.

Copy link
Contributor

@allroundexperts allroundexperts Sep 5, 2023

Choose a reason for hiding this comment

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

Sorry yes. Let's take this there!


const optionItem = useMemo(() => {
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData(fullReport, reportActions, personalDetails, preferredLocale, policy);
Expand All @@ -98,7 +110,7 @@ function OptionRowLHNData({
return item;
// Listen parentReportAction to update title of thread report when parentReportAction changed
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [fullReport, reportActions, personalDetails, preferredLocale, policy, parentReportAction]);
}, [fullReport, linkedTransaction, reportActions, personalDetails, preferredLocale, policy, parentReportAction]);

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
Expand Down Expand Up @@ -186,6 +198,11 @@ export default React.memo(
key: ({fullReport}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${fullReport.parentReportID}`,
canEvict: false,
},
// Ideally, we aim to access only the last transaction for the current report by listening to changes in reportActions.
// In some scenarios, a transaction might be created after reportActions have been modified.
// This can lead to situations where `lastTransaction` doesn't update and retains the previous value.
// However, performance overhead of this is minimized by using memos inside the component.
receiptTransactions: {key: ONYXKEYS.COLLECTION.TRANSACTION},
Copy link
Contributor

@tgolen tgolen Oct 11, 2023

Choose a reason for hiding this comment

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

I'm not sure that it was fully realized, but this means that every option in the LHN is connected to the entire transaction collection. That means that when any transaction is updated, every single option in the LHN is re-rendered. That seems like it's bad for performance.

Copy link
Contributor

@tgolen tgolen Oct 13, 2023

Choose a reason for hiding this comment

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

bump @ygshbht and @luacmartins

Copy link
Contributor Author

@ygshbht ygshbht Oct 13, 2023

Choose a reason for hiding this comment

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

@tgolen Yes, that results in additional renders. However, I don't perceive it as a significant performance issue as transaction updates are infrequent and memoization is implemented within the component. But indeed, there's room for improvement. What specific optimizations do you propose?

Copy link
Contributor Author

@ygshbht ygshbht Oct 13, 2023

Choose a reason for hiding this comment

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

I initally tried accessing only the transcation attached to last reportAction but that doesn't work as I explain in the comment
image

Copy link
Contributor

@tgolen tgolen Oct 13, 2023

Choose a reason for hiding this comment

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

OK, I don't really fully understand that comment. Some of the questions I have are:

  • "In some scenarios" what scenarios exactly?
  • Where does lastTransaction come from and why doesn't it update?

It seems like if the root problem for that was fixed, then there wouldn't be a need for this workaround.

Copy link
Contributor Author

@ygshbht ygshbht Oct 13, 2023

Choose a reason for hiding this comment

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

Something like this https://github.com/Expensify/App/pull/25906/files/4811977a3acb278ca1e210601eb521584ba8db95#diff-020f2493b0feaf1b3762bed3d5d7ad6c28b5ef52b9e9c0a9fc8eb9673d270478
image

Copy link
Contributor

@tgolen tgolen Oct 13, 2023

Choose a reason for hiding this comment

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

getTransaction(lastReportAction);

OK, so it's getting the transactionID like this transactionID = reportAction.originalMessage?.IOUTransactionID ?? ''; which is fine.

The transcaction seems to be created sometime after the transactionID of reportAction is generated. So you don't have access to transcation when the reportAction gets the transactionID

I don't think that matters. As long as you have the transactionID, you can connect to Onyx with it and you'll get any updates to the object on that key.

I imagine the code should look more like this:

receiptTransaction: {
    key: ({ reportActions }) => {
        const lastReportAction = _.last(reportActions);
        if (!lastReportAction || !lastReportAction.name || lastReportAction.name !== CONST.REPORT.ACTIONS.TYPE.IOU || !lastReportAction.originalMessage || !lastReportAction.originalMessage.IOUTransactionID) {
            return `${ONYXKEYS.COLLECTION.TRANSACTION}0`; // This will always return `null` for these cases
        }

        return `${ONYXKEYS.COLLECTION.TRANSACTION}${lastReportAction.originalMessage.IOUTransactionID}`;
    }
}

Copy link
Contributor Author

@ygshbht ygshbht Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@tgolen What you say makes sense. But I'm not sure how Onyx works in detail. From what i remember this approach didn't work for me. If it does for you, the change can be implemented. If you test, please also check if the LHN is updated when the status of transaction is completed (i.e to scanned or scan failed). If i recall correclty, many times, the lastTranscation that i had was actually the second last one

Copy link
Contributor

@tgolen tgolen Oct 13, 2023

Choose a reason for hiding this comment

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

There isn't anything magical about how Onyx works (I know because I wrote a lot of it and there are many many engineers better than me out there).

So, I think it's just running with something like what I've posted and debugging it until it works. Or also, maybe you uncover some bugs, which is great too! We all learn through this. That's better than leaving a workaround like this in the code which could have a big performance impact.

When it's all said and done though, it sounds like we're having a discussion about completely refactoring this component to not have any withOnyx() bindings, but I don't know how far away that is or when it is coming.

Copy link
Contributor Author

@ygshbht ygshbht Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. Honestly, I'm very curious to know what the actual issue is here. My apologies if it appears to be a workaround. I did not see it as such; it was merely the best I could do and provided the explanation for the same

}),
)(OptionRowLHNData),
);
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ function getLastMessageTextForReport(report) {
if (ReportUtils.isReportMessageAttachment({text: report.lastMessageText, html: report.lastMessageHtml, translationKey: report.lastMessageTranslationKey})) {
lastMessageTextFromReport = `[${Localize.translateLocal(report.lastMessageTranslationKey || 'common.attachment')}]`;
} else if (ReportActionUtils.isMoneyRequestAction(lastReportAction)) {
lastMessageTextFromReport = ReportUtils.getReportPreviewMessage(report, lastReportAction);
lastMessageTextFromReport = ReportUtils.getReportPreviewMessage(report, lastReportAction, true);
} else if (ReportActionUtils.isReportPreviewAction(lastReportAction)) {
const iouReport = ReportUtils.getReport(ReportActionUtils.getIOUReportIDFromReportActionPreview(lastReportAction));
lastMessageTextFromReport = ReportUtils.getReportPreviewMessage(iouReport, lastReportAction);
Expand Down
11 changes: 10 additions & 1 deletion src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,9 +1418,10 @@ function getTransactionReportName(reportAction) {
*
* @param {Object} report
* @param {Object} [reportAction={}]
* @param {Boolean} [shouldConsiderReceiptBeingScanned=false]
* @returns {String}
*/
function getReportPreviewMessage(report, reportAction = {}) {
function getReportPreviewMessage(report, reportAction = {}, shouldConsiderReceiptBeingScanned = false) {
const reportActionMessage = lodashGet(reportAction, 'message[0].html', '');

if (_.isEmpty(report) || !report.reportID) {
Expand All @@ -1437,6 +1438,14 @@ function getReportPreviewMessage(report, reportAction = {}) {
return `approved ${formattedAmount}`;
}

if (shouldConsiderReceiptBeingScanned && ReportActionsUtils.isMoneyRequestAction(reportAction)) {
const linkedTransaction = TransactionUtils.getLinkedTransaction(reportAction);

if (!_.isEmpty(linkedTransaction) && TransactionUtils.hasReceipt(linkedTransaction) && TransactionUtils.isReceiptBeingScanned(linkedTransaction)) {
return Localize.translateLocal('iou.receiptScanning');
}
}

if (isSettled(report.reportID)) {
// A settled report preview message can come in three formats "paid ... using Paypal.me", "paid ... elsewhere" or "paid ... using Expensify"
let translatePhraseKey = 'iou.paidElsewhereWithAmount';
Expand Down
9 changes: 6 additions & 3 deletions src/pages/home/report/ReportActionItemSingle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,18 +140,21 @@ function ReportActionItemSingle(props) {
]
: props.action.person;

const reportID = props.report && props.report.reportID;
const iouReportID = props.iouReport && props.iouReport.reportID;

const showActorDetails = useCallback(() => {
if (isWorkspaceActor) {
showWorkspaceDetails(props.report.reportID);
showWorkspaceDetails(reportID);
} else {
// Show participants page IOU report preview
if (displayAllActors) {
Navigation.navigate(ROUTES.getReportParticipantsRoute(props.iouReport.reportID));
Navigation.navigate(ROUTES.getReportParticipantsRoute(iouReportID));
return;
}
showUserDetails(props.action.delegateAccountID ? props.action.delegateAccountID : actorAccountID);
}
}, [isWorkspaceActor, props.report.reportID, actorAccountID, props.action.delegateAccountID, props.iouReport, displayAllActors]);
}, [isWorkspaceActor, reportID, actorAccountID, props.action.delegateAccountID, iouReportID, displayAllActors]);

const shouldDisableDetailPage = useMemo(
() => !isWorkspaceActor && ReportUtils.isOptimisticPersonalDetail(props.action.delegateAccountID ? props.action.delegateAccountID : actorAccountID),
Expand Down