[HOLD for payment 2024-07-24] [$250] [Search v1] Search shows User A submits to User B when User B submits to User A in the same report

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.86-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4647855
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. [User A] Submit an expense to User B
3. [User B] Submit an expense to User A
4. [User A or User B] Go to Search > Shared
5. Look for the expense submitted by User B

Expected Result:

From and To field for the expense submitted by User B will show User B in From field and User A in To field

Actual Result:

From and To field for the expense submitted by User B will show User A in From field and User B in To field, when it is submitted from User B and User A
The Total field also does not make sense either. The amount sent by User A should be indicated by a negative or better indication so that the total amount correctly reflects the sum of both expenses

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6519453_1718891782532.bandicam_2024-06-20_21-49-16-902.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bc65eaf7cfc97061
• Upwork Job ID: 1805340472257661981
• Last Price Increase: 2024-06-24
• Automatic offers:
• brunovjk | Reviewer | 102915805
• c3024 | Contributor | 102915807

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[c3024]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search shows incorrect same from and to for all expenses between a set of users though some of the expenses have from and to the other way.

What is the root cause of that problem?

The following portion till "Outdated ends" is no more applicable because it was fixed with a backend change as mentioned here.

---

Outdated

_If the sum of all the IOUs between two users A and B comes to be "User A owes X", all the transactions have same from 'B' and to 'A' for all transactions passed here

App/src/components/SelectionList/Search/ReportListItem.tsx

Line 188 in b021195

{reportItem.transactions.map((transaction) => (

so from 'B' and to 'A' is same for all transactions.

What changes do you think we should make in order to solve the problem?

We should swap the from and to based on the amount in the transaction.


something like this

const participantFrom = item.amount > 0 ? item.from : item.to;
    const participantTo = item.amount > 0 ? item.to : item.from;
    const participantFromDisplayName = item.amount > 0 ? item.formattedFrom : item.formattedTo;
    const participantToDisplayName = item.amount > 0 ? item.formattedTo : item.formattedFrom;

in TransactionListItemRow and pass these here

App/src/components/SelectionList/Search/TransactionListItemRow.tsx

Lines 313 to 321 in b021195

	<UserInfoCell
	participant={item.from}
	displayName={item.formattedFrom}
	/>
	</View>
	<View style={[StyleUtils.getSearchTableColumnStyles(CONST.SEARCH_TABLE_COLUMNS.FROM)]}>
	<UserInfoCell
	participant={item.to}
	displayName={item.formattedTo}

and here

Outdated ends

---

Now there is a little frontend change necessary because for the narrower screen we show this

App/src/components/SelectionList/Search/TransactionListItemRow.tsx

Lines 228 to 232 in b021195

	<ExpenseItemHeaderNarrow
	participantFrom={item.from}
	participantFromDisplayName={item.formattedFrom}
	participantTo={item.to}
	participantToDisplayName={item.formattedTo}

and the from and to values for the whole report is taken as the from and to of the first transaction.

App/src/components/SelectionList/Search/ReportListItem.tsx

Lines 89 to 95 in 9d88c63

	const participantFrom = reportItem.transactions[0].from;
	const participantTo = reportItem.transactions[0].to;
	// These values should come as part of the item via SearchUtils.getSections() but ReportListItem is not yet 100% handled
	// This will be simplified in future once sorting of ReportListItem is done
	const participantFromDisplayName = participantFrom?.name ?? participantFrom?.displayName ?? participantFrom?.login ?? '';
	const participantToDisplayName = participantTo?.name ?? participantTo?.displayName ?? participantTo?.login ?? '';

This need not always be true because the first transaction accountID and managerID need not be the same as the accountID and managerID for the whole report.
Now that the report sent from backend has managerID and accountIDs we can get the participantFrom value from the from or to of the first transaction by comparing the accountID of the report with managerID or accountID of the transaction. We can get participantTo similarly.

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01bc65eaf7cfc97061

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @brunovjk (External)

[brunovjk]

Thanks for the proposal @c3024. Sorry but, your root cause is not very clear to me:

If the sum of all the IOUs between two users A and B comes to be "User A owes X", all the transactions have the same from 'B' and to 'A'

I can reproduce and see this, but why does it happen? Can we fix this?

[c3024]

That is how the backend sends the response for Search page @brunovjk

[brunovjk]

That is how the backend sends the response for Search page @brunovjk

I see, @c3024 can/should we fix it there?

---

1. I tested your solution and works:

screenshot_1

Sorry, but it seems like a workaround, don't you think?

---

2. When we reduce the screen, the FROM and TO columns are replaced by a title:

screenshot_2

This title also changes depending on "who owes less". Do you think we should update this as well? Maybe always show the name of the logged in user first, and in the total amount indicate "who owes who", as stated in the issue description.

---

3. Lastly, in the issue description it says:
''The Total field also does not make sense either. The amount sent by User A should be indicated by a negative or better indication so that the total amount correctly reflects the sum of both expenses"
Have you thought anything about this?

Thanks.

[c3024]

can/should we fix it there?

I think a backend fix needs to be coupled with some frontend changes as well because for the narrow layout we are just taking the first transactions from and to as the user who submitted the more expense value and the other user respectively.

App/src/components/SelectionList/Search/ReportListItem.tsx

Lines 89 to 90 in 67f92c6

	const participantFrom = reportItem.transactions[0].from;
	const participantTo = reportItem.transactions[0].to;

If we were to send the transactions from and to differently for different transactions between a set of users the first transaction need not be the user who submitted the more expense value and this needs to be checked.

I don't think the solution I suggested is a workaround. 😆

When we reduce the screen, the FROM and TO columns are replaced by a title:

I think this is consistent with individual rows in the wider screen so in my opinion this is fine. I don't think negative values would look good here. You might want to tag the design team if you think swapping the user positions would be better.

The Total field also does not make sense either. The amount sent by User A should be indicated by a negative or better indication so that the total amount correctly reflects the sum of both expenses

I think this is clear enough. The Total field is the net amount (with the one who submitted more expense value or who owes less) on the left or from side. Once we correctly show the from and to as suggested in my fix this confusion should go away.

You can make a comment with the ribbon eyes ribbon without approving a proposal to get an internal engineer assigned to this issue and they can provide their input or you can start a discussion on Slack if you need more input.

[melvin-bot]

Triggered auto assignment to @cristipaval, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[brunovjk]

@cristipaval please take a look at my last comment. I tagged you to confirm a few things before approving the proposal.

---

We need to fix:

1. "From and To field for the expense submitted by User B will show User A in From field and User B in To field, when it is submitted from User B and User A"
2. "The Total field also does not make sense either. The amount sent by User A should be indicated by a negative or better indication so that the total amount correctly reflects the sum of both expenses"

For the first point @c3024 's proposal works well, still, I wonder if a backend/frontend solution might better resolve the root cause.
Could you please confirm the expected outcome for the second point above?

Thank you!

[cristipaval]

@luacmartins I think you have more knowledge around Search functionality. Could you have a look at this one or maybe take it over? 👀

[luacmartins]

I agree that we need to implement some backend fixes for this. I can work on that.

[c3024]

Will you change the individual transactions' from and to to actual expense submitter and submitted respectively with amount always positive in a transaction?

[luacmartins]

I'm flipping the managerID and accountID if the report is an IOU report and the amount is negative. I wasn't gonna change the amount since that seems to be displayed correctly to the user

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.5-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• correct logic of from and to for narrow layout in Shared in Search #44658

If no regressions arise, payment will be issued on 2024-07-17. 🎊

For reference, here are some details about the assignees on this issue:

• @brunovjk requires payment automatic offer (Reviewer)
• @c3024 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@brunovjk / @c3024] The PR that introduced the bug has been identified. Link to the PR:
• [@brunovjk / @c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@brunovjk / @c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@brunovjk / @c3024] Determine if we should create a regression test for this bug.
• [@brunovjk / @c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[johncschuster]

Acknowledged! I will pay this out after the regression threshold is crossed 👍

@brunovjk / @c3024, can you complete the BZ Checklist before then? Thank you!

[brunovjk]

@luacmartins should we wait to fix #45013 to proceed with payment/closing here? Thanks.

[luacmartins]

Sure, let's do that

[johncschuster]

Hey, @brunovjk / @luacmartins, can you help me clarify the path forward for payment? It sounds like we're going to wait to pay out this issue (#44093) until #45013 is completed. Have I understood that correctly?

[brunovjk]

@luacmartins would you mind clarifying this? Thank you.

[luacmartins]

@johncschuster correct, we're waiting for this PR to hit production and start the countdown on the 7 day regression period. We can update the payment date once that PR is deployed to prod. I also left a comment on this issue so we don't double pay.

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.6-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• correct logic of from and to for narrow layout in Shared in Search #44658

If no regressions arise, payment will be issued on 2024-07-22. 🎊

For reference, here are some details about the assignees on this issue:

• @brunovjk requires payment automatic offer (Reviewer)
• @c3024 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@brunovjk / @c3024] The PR that introduced the bug has been identified. Link to the PR:
• [@brunovjk / @c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@brunovjk / @c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@brunovjk / @c3024] Determine if we should create a regression test for this bug.
• [@brunovjk / @c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[johncschuster]

Awesome. Thanks for clarifying, @luacmartins! I'll be standing by for payment 👍

[johncschuster]

Looks like the linked PR just hit prod. Payment should be issued on July 24 assuming no regressions.

[johncschuster]

No regressions so far. We're still on track to pay this out on the 24th 👍

[brunovjk]

• [@brunovjk] The PR that introduced the bug has been identified. Link to the PR: N/A
• [@brunovjk] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• [@brunovjk] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• [@brunovjk] Determine if we should create a regression test for this bug. Yes
• [@brunovjk] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

• Launch New Expensify app.
• Create a new workspace.
• [User A] Submit an expense for amount 'X' (e.g., 10 units) to [User B].
• [User B] Submit an expense for amount 'Y' (e.g., 20 units) to [User A].
• [User A] Submit an expense to the workspace.
• Go to Search > Shared.
• Verify that:
  • The From and To fields are correctly displayed for each expense (e.g., User B in From field and User A in To field for the expense from User B).
  • The total amount accurately reflects the sum of both user expenses with correct indications.
  • The workspace expense is grouped correctly and shows that it was submitted to the workspace.

Do we agree 👍 or 👎

[johncschuster]

Payment has been issued! Thanks for your contributions!