[P2P Distance] Create UpdateMoneyRequestDistanceRate

[neil-marcellini]

Create UpdateMoneyRequestDistanceRate API command in Web-E and Auth following this plan

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0116944cbb002d044c
• Upwork Job ID: 1760074735350415360
• Last Price Increase: 2024-02-20

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@mollfpr Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[mollfpr]

Not overdue

[melvin-bot[bot]]

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

[melvin-bot[bot]]

6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot[bot]]

8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot[bot]]

12 days overdue. Walking. Toward. The. Light...

[Beamanator]

Hey hey I'm happy to take this one one - do we have a deadline for implementing this bad boi?

[Beamanator]

This step is giving me a bit of trouble:

Use computeDistanceCustomUnit in updateMoneyRequestRate, passing the $distance param from the existing transaction

I'm trying to figure out how to get an existing $distance amount from an existing transaction - in computeRouteAndCustomUnit we get the distance from the transaction's routes

• $route = MapboxAPI::getRoutes...
• $distance = TransactionUtils::convertDistance($route['distance'], $unit);

But I'm wondering if that's overkill for this since we're updating a rate, not distance 🤔

[Beamanator]

Ok Auth PR should be ready for review (cc @neil-marcellini if you're interested in helping review) - hoping to have Web-E PR ready for review tomorrow 🙏

[neil-marcellini]

Woah I totally missed that you were working on this! Thanks so much! I'm happy to help review the rest of it. Please DM me when it's ready for review of if you have any questions.

[neil-marcellini]

I'm trying to figure out how to get an existing $distance amount from an existing transaction

Ah yeah that part wasn't described super clearly. You can get the existing distance from the transaction NVPs. Something like this.

$transaction->getNVPs()["customUnit"]["quantity"];

[Beamanator]

Thanks @neil-marcellini ! Feel free to post-review the Auth PR if you're interested, but I'm actually OOO tomorrow so won't be continuing the Web-E changes till Monday! Here's my WIP PR for web-e: https://github.com/Expensify/Web-Expensify/pull/41495

And thanks for explaining where to get the distance!! I haven't worked much with distance requests, and it definitely isn't immediately obvious that it's in the quantity of the custom unit, so thanks for clarifying!! 🙏 hopefully I'll have that PR ready for review on Monday 🙏

[Beamanator]

Hmm as I'm finishing up the web-e PR, I'm realizing this will only be truly completely testable via App, do we have anyone working on the front-end for this new distance request "Rate" field?

[neil-marcellini]

Yep, check out [$500] [P2P Distance] Create a new Rate field. There's a PR up now which might be complete enough for testing [App] Feat/36985 create new rate field.

[Beamanator]

aah nice, it also looks like the "Edit rate" functionality will be added in [$500] [HOLD 36985] [P2P Distance] Create UpdateMoneyRequestRate in App, so I mayyyy wait to test with the changes made there?

[paultsimura]

Hey @neil-marcellini @Beamanator, could you please share the payload you expect for this endpoint?

[Beamanator]

Hey @paultsimura ! Yes good question, will do shortly

[Beamanator]

@paultsimura the payload params we're expecting on UpdateDistanceRequestRate are as follows:

(string $authToken, string $transactionID, string $customUnitRateID, int $optimisticReportActionID)

[paultsimura]

@Beamanator in other endpoints (e.g. UpdateMoneyRequestDate, UpdateMoneyRequestDistance) those are transactionID and reportActionID, not optimisticReportActionID.

Could we change it to keep the types consistent?

[Beamanator]

Aah that's totally fine if we name the report action ID param reportActionID, in our backend I'll just quickly rename it to optimistic* so it's clear we're in optimistic land :D (since that's what we already do, at least with UpdateMoneyRequestDate)

Also completely unrelated but interesting note @neil-marcellini : UpdateMoneyRequestDistance looks like we don't actually accept the reportActionID param in our api.php 🤔 😅 weird? Bug? Maybe?

[neil-marcellini]

Also completely unrelated but interesting note @neil-marcellini : UpdateMoneyRequestDistance looks like we don't actually accept the reportActionID param in our api.php 🤔 😅 weird? Bug? Maybe?

Certainly possible haha. I'm working on some changes to that command for tracking distance expenses so I could include the fix there.

[Beamanator]

Nice, sounds good - let me know if you need help but feel free to work on that fix in your PR 👍 🙏

[Beamanator]

Eek, I'm moving so slow here b/c it's a bit confusing how to break a new computeDistanceCustomUnit function out of computeRouteAndCustomUnit in the most efficient way possible 😅

[Beamanator]

@neil-marcellini question for you, which I asked in another issue, here: https://github.com/Expensify/App/issues/31673#issuecomment-2051719369

This command UpdateMoneyRequestRate will be used for P2P AND for money requests in workspaces, right? If so, I might have to deal with tax updates for workspace money requests, eh?

[Beamanator]

FYI I couldn't quite get this one ready by my EOD - I had to run out a bit early, I'll either be back late this evening or over the weekend for a few hours to make up for this.

@neil-marcellini (sorry to keep pinging you) - If you have time today, it would be great if you could give https://github.com/Expensify/Web-Expensify/pull/41495 a preliminary review -> Like to see if I'm going a decent direction that you were planning in your doc. It looked easyish and straightforwardish, but currently it's breaking TrackExpenseTest and there's some psalm errors which, for the life of me, i can't figure out why they don't exist in main 😅

[neil-marcellini]

@neil-marcellini question for you, which I asked in another issue, here: #31673 (comment)

This command UpdateMoneyRequestRate will be used for P2P AND for money requests in workspaces, right? If so, I might have to deal with tax updates for workspace money requests, eh?

Only workspace requests. I answered in detail over on that issue here.

[Beamanator]

Thanks @neil-marcellini 🙏

Ok so it seeeeeems like we ARE going to have to deal with tax stuff possibly, but will cross that bridge in a bit 😅

For now, I'm going to have to rewrite a few things, due to us agreeing (here) that the current implementation isn't 1:1:1. Here's my new plan of action for now:

Web-E:

1. Continue accepting authToken, transactionID, customUnitRateID, reportActionID params
2. Since we don't want to do a Read call to get the transaction ID in PHP, we can't check if the transaction is a custom unit distance, AND we can't get the linked policy chat report, AND we can't get the transaction's policy data
3. So we'll pass the params directly to Auth to validate & do all the updates
4. We also can't do TransactionUtils::pushTransactionUpdate at the end, since this reads iouReportID, iouThreadReportID, both report's actions, their NVPs, and possibly more

Auth:

1. Do all the things Web-E was going in point 2 above in Peek
2. Also in Peek, Compute the custom distance amount & merchant, and custom unit info (from TransactionAPI::getP2PDistanceRequestRate or TransactionAPI::getDistanceRequestRate)
3. Use those bits ^ in the currently existing auth.moneyRequest.updateRate function
4. Note: I'll also probably have to worry about some newish Tax updates, I've been discussing a bit with monil here
5. Figure out how to send all onyx updates via `queueOnyxUpdates(, , )

[neil-marcellini]

Thanks Alex, that plan sounds pretty good. I think you'll be happy to hear that my top focus right now is this Migrate TransactionUtils::pushTransactionUpdate function to auth. Once that's done you should be able to use it in the new UpdateMoneyRequestRate Auth command.

I'm tackling that migration in two parts. In the first part I'll move most of it to Auth, and leave a small part in PHP to send the modified expense report action and the violations, because it's a massive amount of work to move those over. I'm working on a plan to distribute that migration work across the team, which will be the final part of moving pushTransactionUpdate to Auth.

[Beamanator]

Oh siiiick, that's great that we're taking that in chunks and i won't have to think about that part 😅 I'll get started on other pieces of the plan tomorrow, taking into consideration that i won't have to deal with migrating pushTransactionUpdate 👍 🙏

[paultsimura]

Hey @neil-marcellini @Beamanator I'll duplicate this question here as well. Since we've decided to call the field distanceRate (not just rate), shouldn't we rename the API operation to UpdateMoneyRequestDistanceRate as well?

[neil-marcellini]

Great point, sure, let's call it UpdateMoneyRequestDistanceRate

[Beamanator]

I like this plan, I can also make this change in the Auth command 👍

[melvin-bot[bot]]

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

[Beamanator]

Web-e PR is up and waiting on Auth PR

Auth PR is slowly in progress, but is waiting on at least 2 other PRs most likely (Monil's PR for some Tax updates in Auth & Neil's PR for moving pushTransactionUpdate to Auth

[Beamanator]

I should be able to get back into this tomorrow, though i still believe i won't be able to complete this until the other tasks I mentioned are done - I'll try to find those WIP PRs to link here

[Beamanator]

Tax stuff: https://github.com/Expensify/Auth/pull/10517

pushTransactionUpdate stuff: https://github.com/Expensify/Auth/pull/10487

[Beamanator]

Looks like I'm ALSO going to be waiting for ForexUtils::format to be transferred to Auth (in this PR, see this slack convo)

[Beamanator]

Ooh ok some updates:

• ForexUtils::format was moved to Currency::format, which had some mega refactoring in https://github.com/Expensify/Auth/pull/10625 (merged)
• Transaction::generateTaxUDF was built in https://github.com/Expensify/Auth/pull/10517 (merged)
• Transaction::pushTransactionUpdate still being implemented in https://github.com/Expensify/Auth/pull/10487

[Beamanator]

Alright @neil-marcellini - my Auth PR https://github.com/Expensify/Auth/pull/10638 is feelin much better - I haven't added in the stuff from ^ above PRs, but it's at least doing some of the web-e stuff... Do you mind taking a quick look to give any thoughts on where it's at for now?

[Beamanator]

Thanks for the initial review! Good questions, I got a few resolved but didn't have enough time (half day today & tomorrow) so will try to move forward tomorrow or early next week

[melvin-bot[bot]]

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

[Beamanator]

WIP! Currently working on updating Currency::format to be able to format doubles as well, since currently it only accepts ints

[Beamanator]

FYI https://github.com/Expensify/Auth/pull/10809 just got merged, which unblocks https://github.com/Expensify/Auth/pull/10638 - hoping to get that one finished this week, as i believe all the other PRs it was held on are merged!

[Beamanator]

Made a bit of progress adding a few more tests today, didn't have much time w/ deploy duty

[Beamanator]

Back from vacay, didn't have time to work on this today but will try to prioritize the next few days

[Beamanator]

Ok @neil-marcellini I think I made a lot of progress on the PR today - I added auth.transaction.pushTransactionUpdate and related tests, fixed a few things, and made sure currency doubling looks good.

Mind giving it another text in the next few days? I'm probably going to have to chat with @MonilBhavsar to figure out what kind of tax stuff i may have to incorporate as well