-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix IOU avatar overlap when sharing manual and scanned expenses #87096
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.
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
Changes from all commits
46eb07b
721b115
497a6f0
89b14e9
1c6b417
499f455
69b9364
6769f22
6e6ff6d
e784912
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,11 +6,12 @@ import useTransactionsAndViolationsForReport from '@hooks/useTransactionsAndViol | |||||||||
| import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; | ||||||||||
| import {getAllNonDeletedTransactions} from '@libs/MoneyRequestReportUtils'; | ||||||||||
| import {getPersonalDetailByEmail} from '@libs/PersonalDetailsUtils'; | ||||||||||
| import {getOriginalMessage, isMoneyRequestAction, isSentMoneyReportAction} from '@libs/ReportActionsUtils'; | ||||||||||
| import {getIOUActionForTransactionID, getOriginalMessage, isDeletedParentAction, isMoneyRequestAction, isSentMoneyReportAction} from '@libs/ReportActionsUtils'; | ||||||||||
| import {isDM, isIOUReport} from '@libs/ReportUtils'; | ||||||||||
| import {isScanRequest} from '@libs/TransactionUtils'; | ||||||||||
| import CONST from '@src/CONST'; | ||||||||||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||||||||||
| import type {Policy, Report, ReportAction, ReportActions, Transaction} from '@src/types/onyx'; | ||||||||||
| import type {OriginalMessageIOU, Policy, Report, ReportAction, ReportActions, Transaction} from '@src/types/onyx'; | ||||||||||
|
|
||||||||||
| function getSplitAuthor(transaction: Transaction, splits?: Array<ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.IOU>>) { | ||||||||||
| const {originalTransactionID, source} = transaction.comment ?? {}; | ||||||||||
|
|
@@ -38,6 +39,42 @@ const getSplitsSelector = (actions: OnyxEntry<ReportActions>): Array<ReportActio | |||||||||
| .filter((act) => getOriginalMessage(act)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT); | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| function getTransactionDirectionSign(transaction: Transaction): number | undefined { | ||||||||||
| if (transaction.amount !== 0) { | ||||||||||
| return Math.sign(transaction.amount); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (isScanRequest(transaction)) { | ||||||||||
| const modifiedAmount = Number(transaction.modifiedAmount); | ||||||||||
|
|
||||||||||
| if (Number.isFinite(modifiedAmount) && modifiedAmount !== 0) { | ||||||||||
| return Math.sign(modifiedAmount); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return undefined; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| function isExplicitlyDeletedIOUAction(iouAction: ReportAction): boolean { | ||||||||||
| const originalMessage = getOriginalMessage(iouAction) as OriginalMessageIOU | undefined; | ||||||||||
|
|
||||||||||
| if (originalMessage?.deleted) { | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (isDeletedParentAction(iouAction)) { | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const message = iouAction.message; | ||||||||||
|
|
||||||||||
| if (Array.isArray(message)) { | ||||||||||
| return message.some((fragment) => !!fragment?.deleted); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return !!message?.deleted; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| type GetReportPreviewSenderIDParams = { | ||||||||||
| iouReport: OnyxEntry<Report>; | ||||||||||
| action: OnyxEntry<ReportAction>; | ||||||||||
|
|
@@ -51,21 +88,66 @@ type GetReportPreviewSenderIDParams = { | |||||||||
|
|
||||||||||
| function getReportPreviewSenderID({iouReport, action, chatReport, iouActions, transactions, splits, policy, currentUserAccountID}: GetReportPreviewSenderIDParams): number | undefined { | ||||||||||
| const isOptimisticReportPreview = action?.isOptimisticAction && action?.actionName === CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW && isIOUReport(iouReport); | ||||||||||
|
|
||||||||||
| if (isOptimisticReportPreview) { | ||||||||||
| return currentUserAccountID; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // 1. If all amounts have the same sign - either all amounts are positive or all amounts are negative. | ||||||||||
| // We have to do it this way because there can be a case when actions are not available | ||||||||||
| // See: https://github.com/Expensify/App/pull/64802#issuecomment-3008944401 | ||||||||||
|
Comment on lines
-59
to
-61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restore this comment as this logic is still used |
||||||||||
| const loadedTransactionCount = transactions?.length ?? 0; | ||||||||||
| const childMoneyRequestCount = action?.childMoneyRequestCount ?? 0; | ||||||||||
| const activeMoneyRequestCount = iouReport?.transactionCount ?? childMoneyRequestCount; | ||||||||||
| const activeIOUActions = | ||||||||||
| iouActions?.filter((iouAction) => { | ||||||||||
| return !isExplicitlyDeletedIOUAction(iouAction); | ||||||||||
| }) ?? []; | ||||||||||
| const uniqueIOUActionActorMap = new Map<string, number>(); | ||||||||||
|
|
||||||||||
| const areAmountsSignsTheSame = new Set(transactions?.map((tr) => Math.sign(tr.amount))).size < 2; | ||||||||||
| for (const iouAction of activeIOUActions) { | ||||||||||
| const iouTransactionID = (getOriginalMessage(iouAction) as OriginalMessageIOU | undefined)?.IOUTransactionID; | ||||||||||
|
|
||||||||||
| if (!areAmountsSignsTheSame) { | ||||||||||
| if (!iouTransactionID || iouAction.actorAccountID === undefined) { | ||||||||||
| continue; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| uniqueIOUActionActorMap.set(iouTransactionID, iouAction.actorAccountID); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const hasCompleteActionCoverage = activeMoneyRequestCount > 0 && uniqueIOUActionActorMap.size >= activeMoneyRequestCount; | ||||||||||
| const areAllActiveChildRequestsCreatedByOneActor = new Set(uniqueIOUActionActorMap.values()).size < 2; | ||||||||||
| const canInferFromIOUActionsDuringPartialHydration = loadedTransactionCount > 0 && hasCompleteActionCoverage && activeIOUActions.length > 0 && areAllActiveChildRequestsCreatedByOneActor; | ||||||||||
|
|
||||||||||
| // After refresh, the preview action can hydrate before all active child transactions. | ||||||||||
| // Avoid collapsing to one avatar unless the available IOU actions already prove the remaining | ||||||||||
| // active requests all belong to the same sender. | ||||||||||
| if (activeMoneyRequestCount > loadedTransactionCount && !canInferFromIOUActionsDuringPartialHydration) { | ||||||||||
| return undefined; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const transactionActorAccountIDs = transactions?.map((transaction) => getIOUActionForTransactionID(activeIOUActions, transaction.transactionID)?.actorAccountID); | ||||||||||
| const hasActorAccountIDForEachTransaction = | ||||||||||
| activeIOUActions.length > 0 && !!transactionActorAccountIDs && transactionActorAccountIDs.length > 0 && transactionActorAccountIDs.every((accountID) => accountID !== undefined); | ||||||||||
|
|
||||||||||
| // 1. Use actorAccountID when it is available for every transaction. Otherwise, fall back to known transaction direction only. | ||||||||||
| if (hasActorAccountIDForEachTransaction) { | ||||||||||
| const areAllTransactionsCreatedByOneActor = new Set(transactionActorAccountIDs).size < 2; | ||||||||||
|
|
||||||||||
| if (!areAllTransactionsCreatedByOneActor) { | ||||||||||
| return undefined; | ||||||||||
|
Comment on lines
+131
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new actor-based gate can hide the single-avatar state for valid single-sender previews when requests are submitted by different delegates (or by a delegate plus the owner). In those flows, Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked this case in the codebase and I don't think this concern applies here. For IOU actions, delegate/copilot identity is stored separately in Line 7291 in 46eb07b
App/src/types/onyx/ReportAction.ts Lines 241 to 242 in 46eb07b
The preview logic in
So I don’t see evidence that delegate-created IOU actions would cause different actorAccountIDs for the same visible sender in this flow.
|
||||||||||
| } | ||||||||||
| } else { | ||||||||||
| const transactionSigns = transactions?.map((transaction) => getTransactionDirectionSign(transaction)) ?? []; | ||||||||||
| const hasUnknownDirection = transactionSigns.some((sign) => sign === undefined); | ||||||||||
|
|
||||||||||
| if (hasUnknownDirection) { | ||||||||||
| return undefined; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const areAmountsSignsTheSame = new Set(transactionSigns).size < 2; | ||||||||||
|
|
||||||||||
| if (!areAmountsSignsTheSame) { | ||||||||||
| return undefined; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // 2. If there is only one attendee - we check that by counting unique emails converted to account IDs in the attendees list. | ||||||||||
| // This is a fallback added because: https://github.com/Expensify/App/pull/64802#issuecomment-3007906310 | ||||||||||
|
|
||||||||||
|
|
@@ -83,13 +165,13 @@ function getReportPreviewSenderID({iouReport, action, chatReport, iouActions, tr | |||||||||
| } | ||||||||||
|
|
||||||||||
| // If the action is a 'Send Money' flow, it will only have one transaction, but the person who sent the money is the child manager account, not the child owner account. | ||||||||||
| const isSendMoneyFlowBasedOnActions = !!iouActions && iouActions.every(isSentMoneyReportAction); | ||||||||||
| const isSendMoneyFlowBasedOnActions = activeIOUActions.length > 0 && activeIOUActions.every(isSentMoneyReportAction); | ||||||||||
| // This is used only if there are no IOU actions in the Onyx | ||||||||||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||||||||||
| const isSendMoneyFlowBasedOnTransactions = | ||||||||||
| !!action && action.childMoneyRequestCount === 0 && transactions?.length === 1 && (chatReport ? isDM(chatReport) : policy?.type === CONST.POLICY.TYPE.PERSONAL); | ||||||||||
|
|
||||||||||
| const isSendMoneyFlow = !!iouActions && iouActions?.length > 0 ? isSendMoneyFlowBasedOnActions : isSendMoneyFlowBasedOnTransactions; | ||||||||||
| const isSendMoneyFlow = activeIOUActions.length > 0 ? isSendMoneyFlowBasedOnActions : isSendMoneyFlowBasedOnTransactions; | ||||||||||
|
|
||||||||||
| const singleAvatarAccountID = isSendMoneyFlow ? action?.childManagerAccountID : action?.childOwnerAccountID; | ||||||||||
|
|
||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit test