[HOLD for payment 2024-07-24] [$250] Distance - Start and Stop labels do not change when dragging one over another

[izarutskaya]

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.0.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/4652117
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Launch New Expensify app.
2. Go to FAB > Submit expense > Distance.
3. Drag empty Stop row over empty Start row.
4. Note that the Stop label does not change to Start and Start label does not change to stop.
5. Tap on the Stop row.
6. Select a waypoint.
7. Now Stop and Start rows overlap with one another.

Expected Result:

In Step 4, Stop label will change to Start and Start label will change to stop.

Actual Result:

In Step 4, Stop label does not change to Start and Start label does not change to stop.
In Step 7, when selecting a waypoint, Stop and Start rows overlap with one another.

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

Bug6520253_1718958048871.Screen_Recording_20240621_161639_New_Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b23211c128469d40
• Upwork Job ID: 1805046118243790605
• Last Price Increase: 2024-06-24
• Automatic offers:
• hoangzinh | Contributor | 102892776
• Krishna2323 | Contributor | 102911936

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #collect project.

[Krishna2323]

Proposal

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

Distance - Start and Stop labels do not change when dragging one over another

What is the root cause of that problem?

The draggable list requires key for correctly render the items when dragged. When we create draft transaction, there is no key for empty waypoints and due to this the list isn't updated correctly.

App/src/libs/actions/IOU.ts

Lines 339 to 344 in 9817d3c

	// Add initial empty waypoints when starting a distance expense
	if (iouRequestType === CONST.IOU.REQUEST_TYPE.DISTANCE) {
	comment.waypoints = {
	waypoint0: {},
	waypoint1: {},
	};

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

We need to make sure that we add keys for empty waypoints too, we can create a const for initial waypoints list.

        comment.waypoints = {
            waypoint0: {keyForList: 'start_waypoint'},
            waypoint1: {keyForList: 'stop_waypoint'},
        };

App/src/pages/iou/request/step/IOURequestStepDistance.tsx

Line 79 in 9817d3c

const waypoints = useMemo(() => optimisticWaypoints ?? transaction?.comment?.waypoints ?? {waypoint0: {}, waypoint1: {}}, [optimisticWaypoints, transaction]);

Note: We also need to modify the function which checks for duplicate waypoints.

    // Remove keyForList list property before using `isEmpty`.
    const nonEmptyWaypointsCount = useMemo(
        () =>
            Object.keys(waypoints).filter((key) => {
                const {keyForList, ...waypointWithoutKey} = waypoints[key];
                return !isEmpty(waypointWithoutKey);
            }).length,
        [waypoints],
    );

What alternative solutions did you explore? (Optional)

We can use math.random to generate random keys when keyForList or other field are not present, we can use isEmpty for that.

App/src/pages/iou/request/step/IOURequestStepDistance.tsx

Line 440 in 9817d3c

keyExtractor={(item) => (waypoints[item]?.keyForList ?? waypoints[item]?.address ?? '') + item}

Result

waypoint_drag_issue.mp4

[jliexpensify]

I can reproduce, but also have asked the team here if we want to lock down those points, so they can't be dragged.

[melvin-bot]

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

[melvin-bot]

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

[jliexpensify]

Ok we are good to look at proposals @abdulrahuman5196 , thanks!

[jliexpensify]

Bump @abdulrahuman5196 for a review please

[abdulrahuman5196]

Hi @jliexpensify Sorry for the delay, I have limited availability at the moment. So unassigning myself. Kindly reassign another C+ for review.

[hoangzinh]

I can help as a C+ here.

[melvin-bot]

📣 @hoangzinh 🎉 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 📖

[jliexpensify]

All yours @hoangzinh

[hoangzinh]

The draggable list requires key for correctly render the items when dragged

Hi @Krishna2323, thanks for your proposal. Can you explain a little about it? Why does draggable list require keyForList? Thank you.

[Krishna2323]

particularly in scenarios like draggable lists where item positions change, the uniqueness is vital for React to properly track changes in the list. The issue occurring specifically on Android might stem from subtle platform differences in how React Native interacts with each platform's rendering engine.

@hoangzinh, same kind of bug was solved here, we missed this case.

[hoangzinh]

Because the keyExtractor function here already adds item (which is either waypoint0 or waypoint1), therefore it won't be empty.

App/src/pages/iou/request/step/IOURequestStepDistance.tsx

Line 440 in 9817d3c

keyExtractor={(item) => (waypoints[item]?.keyForList ?? waypoints[item]?.address ?? '') + item}

[Krishna2323]

@hoangzinh, waypoint0 or waypoint1 will always be in the same sequence only the object value will be changed. Thats why we added keyForList

[hoangzinh]

Oh right, @Krishna2323's main solution looks good to me #44153 (comment)

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @Krishna2323 🎉 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 📖

[melvin-bot]

@hoangzinh, @jliexpensify, @MonilBhavsar, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Krishna2323]

Will raise PR within 2 days.

[Krishna2323]

@hoangzinh, PR is up but I can't record videos for strict mode, there is warning and I think it is because of strict mode. What should we do? I believe this warning only shows in dev mode.

Monosnap.screencast.2024-07-03.22-19-51.mp4

[Krishna2323]

Update: It will be fixed after this revert. Will update recordings soon.

[Krishna2323]

@hoangzinh, all recordings have been added, you can review the PR.

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

• fix: Distance - Start and Stop labels do not change when dragging one over another. #44748

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

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

• @hoangzinh requires payment automatic offer (Contributor)
• @Krishna2323 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:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] 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:
• [@hoangzinh] 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:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

Payment Summary

• C: @Krishna2323
• C+: @hoangzinh $250 - waiting on checklist!

Upwork job

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: Refactor the money request creation flow #28618
• 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: @ishpaul777 already commented here on his issue https://github.com/Expensify/App/pull/28618/files#r1631030961
• 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
• Determine if we should create a regression test for this bug: ✅ Yes

[hoangzinh]

Regression Test Proposal

1. Sign in to any Expensify account
2. Go to FAB > Submit expense > Distance.
3. Drag the empty Stop row over the empty Start row.
4. Verify that the Stop label changes to Start and the Start label changes to Stop.

Do we agree 👍 or 👎

[jliexpensify]

Paid and job closed.