[HOLD for payment 2024-12-30] [$250] Distance- Rate added offline seen grayed out going online then back offline

[lanitochka17]

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: 9.0.53-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): applausetester+ck1024@applause.expensifail.com
Issue reported by: Applause - Internal Team

Issue found when executing PR #51159

Action Performed:

Precondition: Have both perdiem and distance rates set up in OldDot.

1. Open the ND app
2. Go to Settings >> Workspaces >> Workspace >> Distance rates
3. Go offline
4. Add a new rate
5. Delete one of the distance rate which was added previously (the rate added online in precondition)
6. Go online (Note: New rate gets bolded (active state) & other rate gets deleted)
7. Go back offline (Note: The new rate which was active gets grayed out again)

Expected Result:

The new rate (added offline) which is active/bolded after going back online should stay bolded after user go offline again

Actual Result:

The new rate added offline becomes active when going online. But seen grayed out when user go offline again after the distance rate is completely active
The same rate seen grayed out every time user go offline

Workaround:

Unknown

Platforms:

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

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6644637_1729782903129.Recording__117.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021849920398995761024
• Upwork Job ID: 1849920398995761024
• Last Price Increase: 2024-12-06
• Automatic offers:
• allgandalf | Reviewer | 105228996
• wildan-m | Contributor | 105228998

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot]

Triggered auto assignment to @lschurr (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]

@lschurr 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

[CyberAndrii]

Edited by proposal-police: This proposal was edited at 2024-10-31 14:40:48 UTC.

Proposal

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

Rate added offline seen grayed out going online then back offline

What is the root cause of that problem?

This issue occurs because all rates (even those not being deleted) are included in optimisticData and successData

App/src/libs/actions/Policy/DistanceRate.ts

Lines 488 to 504 in 476ad6b

	for (const rateID of Object.keys(currentRates)) {
	if (rateIDsToDelete.includes(rateID)) {
	optimisticRates[rateID] = {
	...currentRates[rateID],
	enabled: false,
	pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
	};
	failureRates[rateID] = {
	...currentRates[rateID],
	pendingAction: null,
	errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage'),
	};
	} else {
	optimisticRates[rateID] = currentRates[rateID];
	successRates[rateID] = currentRates[rateID];
	}
	}

Later, when successData is applied, it unintentionally overrides changes made by the CreatePolicyDistanceRate request.

The flow is as follows:

1. Rate B is created: optimisticData is applied
2. Rate A is deleted: optimisticData is applied
3. Go online
4. Rate B successData is applied: it sets the pendingAction prop on Rate B to null (correct)
5. Rate A successData is applied: it sets the pendingAction prop on Rate B back to the previous value (incorrect)

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

We should remove this else block:

App/src/libs/actions/Policy/DistanceRate.ts

Lines 500 to 503 in 476ad6b

	} else {
	optimisticRates[rateID] = currentRates[rateID];
	successRates[rateID] = currentRates[rateID];
	}

It was added in #38237 when this feature was first implemented. A few months later it was moved from Policy.ts to DistanceRate.ts to reduce the file size. There don't seem to be any discussions about it so I believe it was added by mistake and overlooked during the review.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@lschurr, @allgandalf Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[lschurr]

@allgandalf could you review the proposal?

[allgandalf]

This issue occurs because all rates (even those not being deleted) are included in optimisticData and successData.

@CyberAndrii can you dig more into the RCA? there must be a reason that we included those rates there. please try to find the PR and intension of the data added there

[CyberAndrii]

Proposal updated

• Updated RCA and solution.

It turns out that removing the spread of non-modified properties (e.g. ...currentRates[rateID]) is not required, as I thought, but it is still weird. All it needs is customUnitRateID property.

cc @allgandalf

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[lschurr]

@allgandalf could you review the updated proposal?

[lschurr]

Bumped in Slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1730735812544159

[melvin-bot]

@lschurr, @allgandalf Eep! 4 days overdue now. Issues have feelings too...

[allgandalf]

@CyberAndrii can you please provide me a test branch? I would like to test some things before moving forward with your solution (FYI, your RCA sounds correct to me)

[CyberAndrii]

@allgandalf just this 1 change

[allgandalf]

testing the proposed change ....

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@lschurr, @allgandalf Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[CyberAndrii]

@allgandalf Did you figure out why it copies an entire array and entire objects (instead of just the ID property) in the first place? In theory successData and failureData could end up overwriting changes from GetMissingOnyxUpdates. And since all these functions were written by the same person, I think it's just a mistake.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @wildan-m 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[wildan-m]

@allgandalf The PR is ready #53725. Thanks!

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[allgandalf]

♻️ Update: PR was merged

[allgandalf]

♻️ PR is on staging

[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.77-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Distance Rate added offline seen grayed out going online then back offline #53725

If no regressions arise, payment will be issued on 2024-12-30. 🎊

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

• @wildan-m requires payment automatic offer (Contributor)
• @allgandalf requires payment automatic offer (Reviewer)

[melvin-bot]

@allgandalf @lschurr @allgandalf The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[allgandalf]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: https://github.com/Expensify/App/pull/38237/files#r1896471530

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

• Policy with Enabled Distance rate feature

Test:

1. Go to Settings >> Workspaces >> Workspace >> Distance rates >Create a new distance rate
2. Go offline > Add another new rate
3. Delete one of the distance rate which was added previously (the rate added online in precondition)
4. Go online (Note: New rate gets bolded (active state) & other rate gets deleted)
5. Go back offline

Verify the new rate (added offline) which is active/bolded after going back online should stay bolded after user go offline again

Do we agree 👍 or 👎

[lschurr]

Payment summary:

• Contributor: @wildan-m $250 (paid via Upwork)
• Reviewer: @allgandalf $250 (paid via Upwork)