[HOLD for payment 2024-03-14] [$500] [P2P Distance] Enable P2P/Splits in App

[neil-marcellini]

Enable P2P/Splits in App following this plan

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0165a75cb62ec4b3a6
• Upwork Job ID: 1760135587374198784
• Last Price Increase: 2024-02-21
• Automatic offers:
• alitoshmatov | Reviewer | 0
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @alitoshmatov

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[tienifr]

Proposal

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

Enable P2P/Splits in App following this plan

What is the root cause of that problem?

This is new feature

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

1. Edit startMoneyRequest_temporaryForRefactor to save comment.customUnit.customUnitRateID in transactionDraft

• When starting the transaction flow in startMoneyRequest_temporaryForRefactor, set the comment.customUnit.customUnitRateID
• If isPolicyExpenseChat, use the ID from NVP.LAST_SELECTED_DISTANCE_RATES if present
• Otherwise, fallback to Default policy rate ID
• For P2P requests set the constant in src/CONST.ts under CUSTOM_UNITS.FAKE_P2P_ID (FAKE_P2P_ID can be defined to a value like __fake_p2p_id__)

2. Enable splits by changing below conditions (for now enable for beta ‘p2pDistanceRequests’)
   • Define a canUseP2pDistanceRequests method

     return !!betas?.includes(CONST.BETAS.P2P_DISTANCE_REQUESTS) || canUseAllBetas(betas);

   • Change this param and this to canUseP2pDistanceRequests() || iouRequestType !== CONST.IOU.REQUEST_TYPE.DISTANCE to prevent filtering out 1:1 chats
   • Change this to below so it will always be true if p2pDistanceRequests is enabled

     const isAllowedToSplit = canUseP2pDistanceRequests() || iouRequestType !== CONST.IOU.REQUEST_TYPE.DISTANCE;

3. Enable distance field for splits (for now for beta)
   • Enable it for splits by changing the check for isTypeRequest in the disabled prop to

     disabled={didConfirm || (canUseP2pDistanceRequests() || !isTypeRequest)}

• Change the iouMerchant we’re displaying now for the converted distance the distance is currently a value in meters so it’s necessary to convert it to proper units and display unit string create another util function: DistanceRequestUtils.getDistanceForDisplay that returns converted distance with unit string (km/miles) the field already navigates to the distance page

4. After the above changes, the Distance field in the confirmation list will show Route pending... because the rate logic is not built yet, the rate logic is already covered in the scope of New Rate Field part so I think we're ok with that UX here.

5. The back-end needs to be updated to support the P2P distance request as well, because currently the API is returning "Distance requests can only be sent in a workspace chat" error in this case

Above should be the core changes required, this is a quite large feature so of course there'll be things we need to further polish during the PR.

What alternative solutions did you explore? (Optional)

NA

[melvin-bot[bot]]

📣 @alitoshmatov 🎉 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[bot]]

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

[neil-marcellini]

Fast tracking this because we don't really need proposals given that I trust @tienifr to implement the plan well.

[koko57]

Commenting to help with the review.

[neil-marcellini]

@tienifr ETA for the PR?

[tienifr]

This is a new feature so it requires careful testing. Also I need to review the design doc very carefully. I'll open the PR tomorrow.

[tienifr]

@neil-marcellini Can I take a look at the NVP.LAST_SELECTED_DISTANCE_RATES structure? We need to define the type and new onyx key for it.

[tienifr]

@neil-marcellini Distance for P2P is not working yet, we should hold for the implementation of Rate field https://github.com/Expensify/App/issues/36985:

Distance for workspace is ok:

[koko57]

@tienifr I think you should revert formattedDistanceAmount to iouMerchant for the distance field for now and open the PR. The distance field value changes were meant to be done in the Phase 3 with the rate field. Now we just want to enable the field to be able to change the distance (it should work with the current logic with the iouMerchant)

cc @neil-marcellini

[tienifr]

Can I take a look at the NVP.LAST_SELECTED_DISTANCE_RATES structure? We need to define the type and new onyx key for it.

There's one thing blocked. @koko57 Do you have any hint?

[tienifr]

Let's discuss more in the PR so we won't delay opening PR https://github.com/Expensify/App/pull/37185.

[koko57]

Can I take a look at the NVP.LAST_SELECTED_DISTANCE_RATES structure? We need to define the type and new onyx key for it.

There's one thing blocked. @koko57 Do you have any hint?

Let's store the last selected rate ID here - so the type will be string and the onyx key could be NVP_LAST_SELECTED_DISTANCE_RATE (I believe we want it singular, not plural)

[tienifr]

so the type will be string

But the rates are different per workspace, aren't they? Should it be Record<policyID, customUnitRateID>?

Let's store the last selected rate ID here

I see that this is part of Phase 3 and out of scope of this issue. For now I think we only need to define the key, type and retrieve it in initMoneyRequest.

[neil-marcellini]

❗❗❗ Note to QA/bug reporters ❗❗❗

Please refrain from creating deploy blockers related to this PR/issue. The feature is under a beta that is only available to expensify accounts while we build the feature. It's expected to be incomplete right now.

[neil-marcellini]

Pr was merged 🚀

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.48-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/37185

If no regressions arise, payment will be issued on 2024-03-14. :confetti_ball:

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

• @koko57 does not require payment (Contractor)
• @alitoshmatov requires payment automatic offer (Reviewer)
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot[bot]]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@alitoshmatov] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [ ] [@adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[neil-marcellini]

@adelekennedy we don't need to do any payment right now since I implemented the PR and we did not use a C+ for review. I'm going to keep this open until we enable splits. I have only enabled P2P for distance so far.

[tienifr]

@neil-marcellini Seems like you mistook this with another issue 😅 because I and @alitoshmatov worked on this.

[tienifr]

Bumping @neil-marcellini and @adelekennedy to check my comment above ^ and process next steps because PR was on production 7 days ago.

[adelekennedy]

Yep - it looks like payment is due here for @tienifr and @alitoshmatov (checking the PR and the proposal process here)

[neil-marcellini]

@neil-marcellini Seems like you mistook this with another issue 😅 because I and @alitoshmatov worked on this.

Oh woops yes sorry!

[tienifr]

@adelekennedy I think we're good for payment 🙏

[neil-marcellini]

Gentle bump @adelekennedy, can we get this closed out please?

[adelekennedy]

payments have already been made as of the 14th - Last step outstanding is the regression test!

[adelekennedy]

[alitoshmatov]

I think new regression test from me is not needed since plan for this feature contains manual tests and states this:

All of the tests should be added to TestRail as new cases. Our QA team can decide where they should go within the test sections.

[adelekennedy]

perfect then we are good to close!