Expensify
[$500] [HOLD 36985] [P2P Distance] Create UpdateMoneyRequestRate in App
Create UpdateMoneyRequestRate following this plan. Please be sure to read and understand the relevant sections of the entire plan as well.
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01fde353d38f5fdf1b
- Upwork Job ID: 1760136022052638720
- Last Price Increase: 2024-02-21
- Automatic offers:
- paultsimura | Contributor | 0
Job added to Upwork: https://www.upwork.com/jobs/~01fde353d38f5fdf1b
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)
Triggered auto assignment to @stephanieelliott (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Once the Distance and Rate are added as separate fields of a Distance request (not stored in a merchant as currently), we want to introduce the possibility to edit the rate once the request is created.
What is the root cause of that problem?
This is a new feature request
What changes do you think we should make in order to solve the problem?
This should be done after the Rate field is added to the Money Requests.
First, we should add the EditRequestRatePage similar to other pages that modify IOU fields (IOURequestStepDescription/IOURequestStepTag) using the WorkspaceRatePage as a UI example.
We need to add the function that will call the BE UpdateMoneyRequestRate API when the page form is submitted. It should use the corresponding parameters that the Endpoint requires, as well as build optimistic data.
What alternative solutions did you explore? (Optional)
@paultsimura's proposal looks good. Sounds like we will hold on #36985, so updating the title for that.
📣 @paultsimura 🎉 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 📖
Still held, but there has been recent progress on https://github.com/Expensify/App/issues/36985,
Still held on https://github.com/Expensify/App/issues/36985, EOW ETA for PR
Hey @paultsimura what do you think about getting started on this soon, branching off of the PR that's currently in progress [App] Feat/36985 create new rate field?
@neil-marcellini just to confirm:
- Since for P2P distance requests we are using the default const rate values, the "edit rate" functionality should be enabled only for workspace distance requests?
- Is it OK to use the field name simply
rate(e.g. in the editing route:/edit/request/rate/{id}and in const:EDIT_REQUEST_FIELD.RATE), or should we lean to specifying that it's amileageRate? - What should the
MODIFIEDEXPENSEsay in case of the modified rate (considering a rate can have a custom name & the rate itself)?
- Since for P2P distance requests we are using the default const rate values, the "edit rate" functionality should be enabled only for workspace distance requests?
Yes
2. Is it OK to use the field name simply
rate(e.g. in the editing route:/edit/request/rate/{id}and in const:EDIT_REQUEST_FIELD.RATE), or should we lean to specifying that it's amileageRate?
I prefer mileageRate or distanceRate because we have other rates, such as tax.
3. What should the
MODIFIEDEXPENSEsay in case of the modified rate (considering a rate can have a custom name & the rate itself)?
To make this easy for me to answer, here is the backend logic in PHP. I'm sure it's ok to share since we should probably implement the same thing in App for the optimistic data.
Backed logic for distance modified expense message
private function getNewDotCommentForDistanceRequest(string $newMerchant, string $oldMerchant, string $newAmount, string $oldAmount): string
{
// The format of the merchant in a distance request from NewDot comes in the format:
// <quantity> <unit> @ <formatted rate with currency> / <unit>
// Example: 1 mile @ $5 / mi
// For more examples, see ModifiedExpenseTest.php's testGetNewDotComment
$distanceMerchantRegex = '/^[0-9.]+ \w+ @ (-|-\()?(\p{Sc}|\w){1,3} ?[0-9.]+\)? \/ \w+$/';
$changedField = 'distance';
// If the new & old merchants match the new distance request format, check if the distance or rate changed.
// Otherwise, just display that the distance changed.
if (preg_match($distanceMerchantRegex, $newMerchant) && preg_match($distanceMerchantRegex, $oldMerchant)) {
// Extract distance and rate from merchant. These values are separated by ' @ '.
$oldValues = explode('@', $oldMerchant);
$oldDistance = trim($oldValues[0] ?? '');
$oldRate = trim($oldValues[1] ?? '');
$newValues = explode('@', $newMerchant);
$newDistance = trim($newValues[0] ?? '');
$newRate = trim($newValues[1] ?? '');
// Determine if this action is for a rate modification.
if ($oldDistance === $newDistance && $oldRate !== $newRate) {
$changedField = 'rate';
}
} else {
Log::hmmm("Distance request merchant doesn't match NewDot format. Defaulting to showing as distance changed.", ['newMerchant' => $newMerchant, 'oldMerchant' => $oldMerchant]);
}
if (!strlen($oldMerchant)) {
return "set the $changedField to $newMerchant, which set the amount to $newAmount";
}
return "changed the $changedField to $newMerchant (previously $oldMerchant), which updated the amount to $newAmount (previously $oldAmount)";
}
@neil-marcellini maybe it makes sense then to rename the API operation to UpdateMoneyRequestDistanceRate instead of just UpdateMoneyRequestRate?
@paultsimura the new rate field PR has been merged. Agata is working on some follow ups, but that should be done soon [CP Staging] Feat/36985 create new rate field followups. What's your ETA to raise the PR for this? Would you please link it here as soon as it's ready? I usually like to review before the C+ approves.
Triggered auto assignment to @miljakljajic (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.
Reapplying the New Feature label to get another BZ member on this while I am OOO til May 2. Thanks @miljakljajic -- to catch you up on where we are, we are waiting for @paultsimura to raise a PR. I'll grab this back from you when I return!
@neil-marcellini from what I see, the BE part is not ready yet. What is the plan with it? I don't think putting a PR out for review before the endpoint is ready is a good idea as it'll definitely lead to discrepancies in optimistic data and cause follow-ups.
Ok yeah good point. I forgot how slowly that's coming along on the backend 😅. I'll let you know when it's ready, and I'll put it on HOLD for now.
Thank you. Just an FYI, I'll be OOO April 26 - May 13, so if the BE part gets resolved in that timeframe and the PR becomes urgent – feel free to reassign.
I'm back from OOO, thanks for watching this while I was out @miljakljajic!
Still held on https://github.com/Expensify/App/issues/36969, the PR blocking that one is actively being worked on.