Skip to content
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

[$250] [P2P Distance] Split - Incorrect amount & distance behavior when changing distance on confirm page offline #47048

Closed
6 tasks done
IuliiaHerets opened this issue Aug 8, 2024 · 43 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 8, 2024

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: v9.0.18-1
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to group chat.
  3. Click + > Split expense > Distance.
  4. Enter waypoints > Next.
  5. On confirmation page, go offline.
  6. Click Distance.
  7. Change distance by swapping position and save it.

Expected Result:

  • Amount field will be empty.
  • Distance field will change to Pending.
  • Split input will be 0.00.

Actual Result:

Amount, Distance and split inputs change to a very big value.

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

Bug6565228_1723089231018.20240808_114732.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016482e5c32e2edfad
  • Upwork Job ID: 1823019084308686776
  • Last Price Increase: 2024-09-02
Issue OwnerCurrent Issue Owner: @carlosmiceli
@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

Triggered auto assignment to @Julesssss (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Aug 8, 2024

Triggered auto assignment to @greg-schroeder (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.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 8, 2024
@IuliiaHerets
Copy link
Author

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

Copy link
Contributor

github-actions bot commented Aug 8, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@bernhardoj
Copy link
Contributor

Proposal

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

The distance, amount, and individual split share changes to a very big value.

What is the root cause of that problem?

From the expected result, the 1st (Amount field will be empty) and 2nd problem (Distance field will change to Pending) is the same root cause as #46952 and is currently being handled in #46964.

For the 3rd problem (Split input will be 0.00), even after we fix the 1st and 2nd, the split amount still won't be 0 when the distance amount is 0 because we only update the individual split amount if the distance amount (amount) is truth (> 0).

const participantAccountIDs: number[] = selectedParticipantsProp.map((participant) => participant.accountID ?? -1);
if (isTypeSplit && !isPolicyExpenseChat && amount && transaction?.currency) {
IOU.setSplitShares(transaction, amount, currency, participantAccountIDs);
}

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

If we want to update the individual split amount to 0 when the distance amount is 0, then we need to remove the amount check here.

const participantAccountIDs: number[] = selectedParticipantsProp.map((participant) => participant.accountID ?? -1);
if (isTypeSplit && !isPolicyExpenseChat && amount && transaction?.currency) {
IOU.setSplitShares(transaction, amount, currency, participantAccountIDs);
}

However, I notice that when we create the split distance while offline with the pending distance, the request will always fail and also, the participant is only the current user because we don't include participants with 0 split amount.

const splits: Split[] = [{email: currentUserEmailForIOUSplit, accountID: currentUserAccountID, amount: currentUserAmount, taxAmount: currentUserTaxAmount}];

let splitParticipants = selectedParticipants;
// Filter out participants with an amount equal to O
if (iouType === CONST.IOU.TYPE.SPLIT && transaction?.splitShares) {
const participantsWithAmount = Object.keys(transaction.splitShares ?? {})
.filter((accountID: string): boolean => (transaction?.splitShares?.[Number(accountID)]?.amount ?? 0) > 0)
.map((accountID) => Number(accountID));
splitParticipants = selectedParticipants.filter((participant) =>
participantsWithAmount.includes(participant.isPolicyExpenseChat ? participant?.ownerAccountID ?? -1 : participant.accountID ?? -1),
);
}

App/src/libs/actions/IOU.ts

Lines 4062 to 4066 in 9c7c4a1

// To exclude someone from a split, the amount can be 0. The scenario for this is when creating a split from a group chat, we have remove the option to deselect users to exclude them.
// We can input '0' next to someone we want to exclude.
if (splitAmount === 0) {
return;
}

I tried to always include the participant even when the amount is 0, but it still fails.

Maybe we can disable the confirmation button if the distance amount is 0.

@Julesssss
Copy link
Contributor

Not treating this as a blocker. While annoying it should be rare and has an obvious workaround.

@Julesssss Julesssss added Daily KSv2 Improvement Item broken or needs improvement. and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Aug 8, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@Julesssss Julesssss added the External Added to denote the issue can be worked on by a contributor label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

Job added to Upwork: https://www.upwork.com/jobs/~016482e5c32e2edfad

@melvin-bot melvin-bot bot changed the title Split - Incorrect amount & distance behavior when changing distance on confirm page offline [$250] Split - Incorrect amount & distance behavior when changing distance on confirm page offline Aug 12, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@carlosmiceli carlosmiceli added Daily KSv2 and removed Weekly KSv2 labels Sep 11, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@neil-marcellini
Copy link
Contributor

@mvtglobally why do you say it's not reproducible? I just found that it was reproducible yesterday. Did it get fixed in a day? I doubt it. Would you please include a video of your test?

@greg-schroeder
Copy link
Contributor

Bump @mvtglobally on the above!

@carlosmiceli
Copy link
Contributor

Should get started on it today.

Copy link

melvin-bot bot commented Sep 17, 2024

@carlosmiceli, @greg-schroeder, @fedirjh Eep! 4 days overdue now. Issues have feelings too...

@greg-schroeder
Copy link
Contributor

Thanks for the update @carlosmiceli!

@carlosmiceli
Copy link
Contributor

@neil-marcellini How do I get location working? I already gave access.

Screenshot 2024-09-18 at 4 02 46 PM

@melvin-bot melvin-bot bot added the Overdue label Sep 18, 2024
@carlosmiceli carlosmiceli added Daily KSv2 and removed Daily KSv2 labels Sep 18, 2024
@melvin-bot melvin-bot bot removed the Overdue label Sep 18, 2024
@neil-marcellini
Copy link
Contributor

@neil-marcellini How do I get location working? I already gave access.

Screenshot 2024-09-18 at 4 02 46 PM

That's odd, might be a separate bug. Try reloading the page, signing out and back in, etc. If you still have trouble you can start looking into the MapView and how it's loaded, how the token is fetched.

I just tried and found out that GetMapboxAccessToken is failing because we removed the token and haven't sent an action required email yet. I explained the solution here.

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

@carlosmiceli, @greg-schroeder, @fedirjh Huh... This is 4 days overdue. Who can take care of this?

@carlosmiceli
Copy link
Contributor

Was OOO, will try to work on it this week after I deal with some backlog.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Sep 25, 2024
@greg-schroeder
Copy link
Contributor

Okay Thanks @carlosmiceli!

@carlosmiceli carlosmiceli added Weekly KSv2 and removed Daily KSv2 labels Sep 30, 2024
@carlosmiceli
Copy link
Contributor

Got carried away with wrapping homepage phase 2 issues, should be able to start this week 🙏

@melvin-bot melvin-bot bot removed the Overdue label Sep 30, 2024
@greg-schroeder
Copy link
Contributor

Closing as #vip-split is now closed/archived/paused - let me know if you disagree @carlosmiceli

context: https://expensify.slack.com/archives/C05RECHFBEW/p1727723167802139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants