[P2P Distance] Enable split distance requests

[neil-marcellini]

Enable split distance requests following this plan

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017fd731ba8df719c5
• Upwork Job ID: 1760074110878371840
• Last Price Increase: 2024-02-20

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @jjcoffee (Internal)

[melvin-bot[bot]]

@jjcoffee Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot[bot]]

@jjcoffee Still overdue 6 days?! Let's take care of this!

[jjcoffee]

Chill, Melv :v:

[melvin-bot[bot]]

@jjcoffee Huh... This is 4 days overdue. Who can take care of this?

[neil-marcellini]

I'm starting on this now. I will split up the plan into multiple PRs to keep each one small and focused. I will start with moving split participant normalization to Auth.

Actually, now that I look more closely, it makes sense to move the creation of an individual payer account to Auth first. Then we can re-use that Auth function to create all split accounts as needed. Therefore I'll HOLD this issue on the issue to implement that part [P2P Distance] Move creating the payer account for money requests from Web-E to Auth

[neil-marcellini]

On hold for a bit, moving to weekly

[neil-marcellini]

On hold

[neil-marcellini]

still holding

[neil-marcellini]

Yep, still on hold. Backend refactors are slowing things down.

[neil-marcellini]

Holding. I'm hoping to start on the blocking issue next week.

[neil-marcellini]

I'm planning to get started today

[neil-marcellini]

I got a draft Web-E PR up, processing splits data and passing it to Auth. Next time I'll get started on the Auth PR. After that's done I'll have to go back to Web-E to make sure the rest of the command finishes up properly.

[neil-marcellini]

Yesterday I got a little Auth draft PR up. Lot's more work to do there, so it will be my top focus today.

[neil-marcellini]

I made a little bit of progress today. It's pretty tricky because the doc was written before the strict 1:1 requirement, so I keep having to look into stuff related to that. For example I found out that pushing the personal details is hard to move to Auth due to phone number formatting stuff, and so I think it should stay in Web-E for now. I may need to formally ask for an exception, since it is existing code, but added to a new api command.

Next time I'll keep working on sending split data in Web-E. I should try to follow the design doc as closely as possible, but diverge from it where needed. Breaking it up into smaller functions will help readability too.

[neil-marcellini]

There was still a bunch to do in Auth today, so I worked there and got CreateDistanceRequest and CreateIOUSplit to use the same util functions to create split requests. I haven't tested any of it yet, so I'm planning to do a lot of testing both manual and automated after finishing sending data back from Web-E.

[neil-marcellini]

I didn't work on this as much as I would have like to today because other stuff popped up, but I did sort out the error handling a bit more, and finish sending data back from Web-E. There is some error handing in ReportAPI::splitBill which handles the case where a split participant email or accountID is invalid. The code is very odd to me and I think I've found several errors in it so far, but ultimately I decided not to bother with it because the only way that flow can occur is if the frontend is quite broken. So it's not worth it.

I want to manually test manually across the full stack next, so I'm working to enable splits from App. I suspect I will find many errors while testing, and I plan to slowly build up automated tests as I learn form manual testing.

[neil-marcellini]

I'm able to send split requests from App now, and I got a simple automated test written for Web-E so I can start iterating fast there. It's where I'm finding most of the errors right now.

Next up I need to use the chatType in computeRouteAndCustomUnit to determine whether the request should use a p2p rate or not.

[melvin-bot[bot]]

@neil-marcellini Whoops! This issue is 2 days overdue. Let's get this updated quick!

[neil-marcellini]

I've been working on this a lot today. I fixed a bunch of errors today in Auth and got a basic test hitting the assertions finally. One thing I find odd is that looking at the existing code from CompleteSplitBill, the transaction data seems to be saved on the parent split transaction, and then also saved on the individual transactions with each split participant. That data duplication bothers me a bit, but I'll assume we did that for a reason and do the same for distance splits.

[neil-marcellini]

I'm still chugging away at the automated Auth tests

[melvin-bot[bot]]

@neil-marcellini Eep! 4 days overdue now. Issues have feelings too...

[neil-marcellini]

I worked on this for an hour this morning. Company priorities have shifted towards reliability again, and so I'm going to move away from this as my top focus. I will finish what I started, but at a weekly cadence.

[neil-marcellini]

I've been focused on other things this week, but planning to focus on this tomorrow

[neil-marcellini]

Picking this back up for a couple hours now

[neil-marcellini]

I worked on this quite a bit yesterday and this morning. Distance splits are being successfully created. This morning I fixed copying receipts to the child transactions and I'm working on splitting the distance for each child transaction instead of showing a split amount and the full distance.

There's an annoying bug in App on dev main right now where receipt images load but keep showing the loading spinner. I think I'll create an external issue for that because it's making things hard to test.

[neil-marcellini]

I think the feature is working very well at this point. I'm encountering a handful of bugs that I believe come from the frontend and are present on main, but I will have to go through and verify that for each bug. Going to work on this today.

[neil-marcellini]

I fixed the style and some tests on the backend after updating my branches with main. I determined that one bug is also present on main. I think I fixed a bug where the route wasn't showing optimistically for the distance splits, but there's now another bug with the split shares so I need to fix that before I can properly test.

I'll put in another concerted effort on the fronted, and then hopefully I can get all the PRs up for review this week.

[neil-marcellini]

I've been dealing with the following problem for a long time today.

Problem: Splitting a distance expense with new accounts doesn't navigate to a new group chat with the split participants. The following flow takes place.

• On the confirmation page, call IOU.createDistanceRequest passing the current report which has the value {"reportName": "Chat Report"}
• Within that function, after creating the request, navigate to the new chat report which is the activeReportID. Since the report is not a money request report, we use the parameters.chatReportID. That comes from the splitData here
• Above createSplitsAndOnyxData is called to get the splitData
• splitData.chatReportID comes from the splitChatReport, which comes from the call to getOrCreateOptimisticSplitChatReport. The existingSplitChatReportID will default to '', because it comes from report.reportID with createDistanceRequest and that is undefined
• Within getOrCreateOptimisticSplitChatReport the existingChatReportID is set to undefined because the empty string is falsey and the first participant doesn't have a reportID
• 💥 When we get the value at the key report_undefined from all reports we get a result back, wtf? Why?
• After carefully checking to see when this key gets set I noticed that it happens when selecting a waypoint. That's because the reportID is undefined when getting the route to navigate to a waypoint editor page. Side note, this is the only route which allows an optional reportID, e.g. reportID?: string. We should probably fix that.

Solution: So with all that, what's the solution? To fix the particular problem for group chat distance splits, I wrote this commit which ensure that we default to an empty reportID instead of undefined, which avoids the problem for now since we don't seem to have any code opening a report with an empty id.

However, it still seems problematic that withWritableReportOrNotFound called from the IOURequestStepDistance page doesn't really seem to be working correctly. I guess I'm not really sure what the intention of this higher order component is. If we have an invalid reportID shouldn't we show the not found page? If you navigate to https://dev.new.expensify.com:8082/r/undefined you get a not found page, but report_undefined is permanently stored in Onyx, because we return a not found error on the report instead of removing it from Onyx as we used to. If you navigate to /r/0 then you get a not found page, but that report also stick around in Onyx forever.

[neil-marcellini]

I worked on this a lot today and fixed several thorny frontend bugs. It's going to take a good bit more time of testing and debugging on the frontend before this is ready. The next one to fix is creating a split distance request in the same group chat report.

[neil-marcellini]

I found that the frontend is sending optimistic one on one chat reportIDs for each split participants when creating a split with an existing group. It's happening after a fresh sign in because getChatByParticipants tries to find the report in the allReports cache, but that only contains a few reports returned by OpenApp in focus mode, and it doesn't contain the existing 1:1 DMs. I think this will need a backend fix to use the existing chat without throwing an error. I'll also likely have to make sure the optimistic data is clean up properly.

[neil-marcellini]

I worked on this for a couple hours this morning. I fixed an Auth test and a bunch of phan/psalm errors on Web-E.

I tested on app main and the new group chat won't load report actions for a bit. You have to sign out and back in. It remains to be seen whether that's a problem on main. I also noticed that the 1:1 transaction was put on HOLD. I need to figure out why the Auth branch is causing Web-E tests to fail on main.

My new goal is to get the mainline cases working, document any existing bugs on main, and make sure it doesn't break the existing flows.