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.36k stars 2.78k forks source link

[$1000] Primary login account is added to split bill participant list and amount also gets divided changing default contact method #19981

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!


Action Performed:

Precondition: account should have primary login account A, and secondary login account B

  1. Click on 'FAB' icon > click on 'Split bill' link > enter amount > click on 'Next' button
  2. Select two users
  3. Click on 'Split' button
  4. Click on IOU preview component & verify that total of 3 participants are displayed in the details section
  5. Click on 'Profile' icon > click on 'Contact methods' link > click on secondary login account B > click on 'Set as default' button
  6. Close RHN & refresh the page for the changes to take effect
  7. Click on the IOU preview component

    Expected Result:

    Primary login account A shouldn't be added to the split bill participant list

    Actual Result:

    Primary login account A is added to the split bill participant list amount is also divided between primary login account and secondary login account on changing default contact method.

    Workaround:

    Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.22 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/66585e83-865d-4891-93b0-b98950edfe32

https://github.com/Expensify/App/assets/43996225/165d8a92-0bfb-4fb0-8ef9-ffb955d46e55

Expensify/Expensify Issue URL: Issue reported by: @natnael-guchima Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685434411591889 https://expensify.slack.com/archives/C049HHMV9SM/p1685430101536709 View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b4c1bac53442a0dd
  • Upwork Job ID: 1664579284030091264
  • Last Price Increase: 2023-06-02
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

conorpendergrast commented 1 year ago

conorpendergrast+19981c@gmail.com created the split, then changed the default contact method to conorpendergrast+19981cc@gmail.com

Before the default contact method change:

image

After the default contact method change:

image

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

tienifr commented 1 year ago

Proposal

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

Primary login account A is added to the split bill participant list, amount is also divided between primary login account and secondary login account on changing default contact method.

What is the root cause of that problem?

There're 3 user A, B, C. User A has another contact method AA.

A splits bill between A, B and C. then A changes contact method to AA.

Let's see this line https://github.com/Expensify/App/blob/af6fb0768b65e95f8a9feb5e36626ae9df5f2097/src/pages/iou/SplitBillDetailsPage.js#L70

The participants are [AA, B, C] (Since A already changes contact to AA). The reportAction.actorEmail is A (it's the original login). So in this line, it should have filtered AA out so we have the list of participants "without payee", but in this case it doesn't since the login in participants is the new contact method, while the actorEmail is the old method.

So we'll see the payee duplicate in the list of Who was there?.

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

We need to change how we compare 2 participants/actors are the same. Currently we're comparing emails but they can change if anyone changes their contact method. So we need to:

  1. Use unique attribute to compare the participant/actor. We can use something like accountID
  2. If yes, that means it's the same participant/actor

I believe there's back-end change required to save the accountID of the actor as well as some changes to save it along with the participants list when splitting bill.

What alternative solutions did you explore? (Optional)

We can also get the list of contact methods of both sides (by their current contact method) and check if there's any contact method in common. For this we need to change the personalDetails structure to include the loginList for all people as well, then get it to compare whenever we need to check if both emails are of the same person.

wildan-m commented 1 year ago

Proposal

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

The primary login account is added to the split bill participant list and the amount also gets divided changing the default contact method

What is the root cause of that problem?

reportAction.originalMessage.participants not updated to the recent default contact method.

SCR-20230603-m5u

We started to use participants data here: https://github.com/Expensify/App/blob/0813e2ca1476635a8324cbd9f998424663a33128/src/pages/iou/SplitBillDetailsPage.js#L67

That makes users' old default contact added to split participants.

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

Update the originalMessage participants to the current default contact method in the backend.

What alternative solutions did you explore? (Optional)

N/A

madmax330 commented 1 year ago

@thesahindia have you had a chance to review the proposals?

thesahindia commented 1 year ago

@madmax330, looked into the proposal and look like this needs backend changes.

madmax330 commented 1 year ago

Okay will ask internally to see if anyone knows about this.

mountiny commented 1 year ago

@tienifr I think using the accountId makes sense and we are actively working now to make sure we are re-keying the personal details with accountIDs so I believe this should work well

melvin-bot[bot] commented 1 year ago

📣 @tienifr You have been assigned to this job by @madmax330! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

madmax330 commented 1 year ago

Actually we discussed this internally and we're refactoring personalDetails by accountID already. So let's hold on that so that we don't duplicate work

conorpendergrast commented 1 year ago

Still on hold for https://github.com/Expensify/App/issues/19007

conorpendergrast commented 1 year ago

@madmax330 I see that https://github.com/Expensify/App/issues/19007 is now on Production, so I think we can take this off hold, re-test and fix. Do you agree?

madmax330 commented 1 year ago

Yeah seems like we can take this off hold

conorpendergrast commented 1 year ago

Off-hold, ready to re-test and fix (if it still exists as an issue)

tienifr commented 1 year ago

I cannot reproduce this bug now. I found out that this issue was fixed in the PR https://github.com/Expensify/App/pull/20328 and the related issue https://github.com/Expensify/App/issues/19007 where we migrated login to accountID through the app, which was the approach mentioned in my proposal.

Just wonder if I'm eligible for a bonus here? @conorpendergrast

JmillsExpensify commented 1 year ago

Removing from the manual request project since this is a wave2 issue.

conorpendergrast commented 1 year ago

@tienifr What are you asking for as compensation? We can certainly discuss it!

tienifr commented 1 year ago

@conorpendergrast thanks for looking into this! According to previous precedents (contributors assigned but solution becomes out of date due to other changes) like here and here, compensation will be full GH issue amount ($1000).

conorpendergrast commented 1 year ago

Ok, I agree with that! I'll queue up contracts via Upwork.com now (the job post is closed so I need to do that manually)

conorpendergrast commented 1 year ago

Original Upwork job: https://www.upwork.com/jobs/~01b4c1bac53442a0dd

Natnael-Guchima commented 1 year ago

Accepted the offer. Thanks @conorpendergrast

conorpendergrast commented 1 year ago

Paid @tienifr 👍 Closing, as @thesahindia will send a Manual Request.

JmillsExpensify commented 1 year ago

$1,000 payment approved for @thesahindia based on BZ summary.