Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[LOW] [Splits] [$500] Individual split amount is inconsistent for manual and scan split bill #35549

Closed lanitochka17 closed 7 months ago

lanitochka17 commented 9 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.35-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/4254803 Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. Create a group with another two users
  2. Create a scan split with a $6.50 receipt
  3. Create a manual $6.50 split
  4. After scan split is complete, navigate to 1:1 DM with one of the split members

Expected Result:

Since the split amount for both manual and split scan requests is the same, which is $6.50, the individual split amount should be the same for both types of request

Actual Result:

The individual split amount for Scan split request is $2.16, while it is $2.17 for Manual split request

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/8e5bd4a0-1107-48da-841b-c00faf025aa3

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01680f4e130a8667a2
  • Upwork Job ID: 1753045981654507520
  • Last Price Increase: 2024-02-16
melvin-bot[bot] commented 9 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01680f4e130a8667a2

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

lanitochka17 commented 9 months ago

We think that this bug might be related to #vip-split-p2p-chat-groups

allgandalf commented 9 months ago

Proposal

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

Individual split amount is inconsistent for manual and scan split bill

Inconsistency in duplicate distance error in distance creation and edit flow

What is the root cause of that problem?

In MoneyRequestPreview, we wrap the total amount in a convertToDisplayString

https://github.com/Expensify/App/blob/b6363778127003ce5ecb7375c30bb5d05d77b7d9/src/components/ReportActionItem/MoneyRequestPreview.js#L342-L343

And hence as you can see in the video attached, there we see both the manual as well as scan split amount same, but in individual page view, we don't do so with scanned split bill

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

Wrap the individual split bill amount in convertToDisplayString as well

What alternative solutions did you explore? (Optional)

N/A

alitoshmatov commented 9 months ago

@GandalfGwaihir I don't think that is related

alitoshmatov commented 9 months ago

When opening a 1:1 DM and money request there, backend is sending two different values for scan and manual:

@CortneyOfstad Can we get an internal engineer to take a look at this

Screenshot 2024-02-05 at 21 43 01
CortneyOfstad commented 9 months ago

Sounds good!

melvin-bot[bot] commented 9 months ago

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

melvin-bot[bot] commented 9 months ago

Current assignee @alitoshmatov is eligible for the Internal assigner, not assigning anyone new.

CortneyOfstad commented 9 months ago

Not overdue β€” waiting on an engineer to take a look

alitoshmatov commented 9 months ago

Looks like no engineer assigned to this issue

CortneyOfstad commented 9 months ago

@alitoshmatov No worries, that is what the hot pick tag is for. It allows engineers to know that this one is available if they have any free time to snag it πŸ‘

CortneyOfstad commented 9 months ago

Still waiting on an engineering πŸ‘

melvin-bot[bot] commented 9 months ago

@CortneyOfstad @alitoshmatov this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 9 months ago

Current assignee @alitoshmatov is eligible for the External assigner, not assigning anyone new.

CortneyOfstad commented 9 months ago

Alright, checked with the team and this can be external, so going to correct the labels. Hopefully we'll get someone to bite, but if not, I'll reach out to CallStack if their team is interested πŸ‘

brandonhenry commented 9 months ago

Proposal

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

The individual split amount is inconsistent between manual and scan split bills, resulting in a discrepancy of $0.01 in the split amount for a $6.50 bill.

What is the root cause of that problem?

We need to review the code responsible for calculating the individual split amounts in both manual and scan split scenarios. The issue likely lies in the rounding logic or the way the total amount is divided among participants when communicating between the frontend and backend.

1. Rounding Logic in iouUtils's calculateAmount:

https://github.com/Expensify/App/blob/c8826dddb35a401faf35598a9ebc77c760c55335/src/libs/IOUUtils.ts#L33

2. Currency Conversion Logic in CurrencyUtils:

https://github.com/Expensify/App/blob/c8826dddb35a401faf35598a9ebc77c760c55335/src/libs/CurrencyUtils.ts#L81 https://github.com/Expensify/App/blob/c8826dddb35a401faf35598a9ebc77c760c55335/src/libs/CurrencyUtils.ts#L90

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

1. Review and Update calculateAmount Function:

2. Ensure Consistent Conversion Logic:

CortneyOfstad commented 9 months ago

@alitoshmatov thoughts on the proposal above? I have a suspicion that the rounding logic is also affecting this, especially when off by a single cent.

mvtglobally commented 9 months ago

Issue not reproducible during KI retests. (First week)

alitoshmatov commented 8 months ago

@brandonhenry Thank you for your proposal but you are not suggesting any meaningful solution.

alitoshmatov commented 8 months ago

As of my finding this is definitely related to backend and cannot be handled by external engineers.

cc: @CortneyOfstad

melvin-bot[bot] commented 8 months ago

@CortneyOfstad @alitoshmatov this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

CortneyOfstad commented 8 months ago

Thanks @alitoshmatov! Going to adjust this to internal πŸ‘

melvin-bot[bot] commented 8 months ago

Current assignee @alitoshmatov is eligible for the Internal assigner, not assigning anyone new.

CortneyOfstad commented 8 months ago

Adjusted to show a "unassigned" β€” hopefully that will get eyes on this πŸ‘

CortneyOfstad commented 8 months ago

This is considered "Low" Priority, so going to adjust this to Weekly πŸ‘

melvin-bot[bot] commented 8 months ago

@CortneyOfstad @alitoshmatov this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @miljakljajic (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

CortneyOfstad commented 8 months ago

Hi @miljakljajic! I am heading OoO and will be back March 11th, so reassigned to have someone keep this moving until I am back. At this stage, we're waiting for internal engineers to snag this. Is pretty low priority but wanted to have someone watching just in case. Thanks!

CortneyOfstad commented 8 months ago

Back from OoO β€” thanks for holding down the fort @miljakljajic!

mvtglobally commented 8 months ago

Issue not reproducible during KI retests. (First week)

CortneyOfstad commented 8 months ago

I am also having trouble recreating this in testing. @alitoshmatov @lanitochka17 is anyone else able to recreate this consistently?

CortneyOfstad commented 7 months ago

@alitoshmatov @lanitochka17 bump on the comment above ^^^

CortneyOfstad commented 7 months ago

@alitoshmatov @lanitochka17 bump again β€” please provide update by EOD today, thank you!

alitoshmatov commented 7 months ago

Sorry @CortneyOfstad, I was having hard time scanning receipt other days.

I tried now and yes the issue is not reproducible

Screenshot 2024-04-04 at 1 52 23β€―AM Screenshot 2024-04-04 at 1 52 18β€―AM Screenshot 2024-04-04 at 1 52 08β€―AM
CortneyOfstad commented 7 months ago

No worries @alitoshmatov! Going to close as this is not reproducible πŸ‘