fix: distance - share with accountant #51517

paultsimura · 2024-10-26T06:32:27Z

Details

This PR is intended to fix several issues with the tracked distance expenses shared with accountants.

Fixed Issues

$ #46897
PROPOSAL: #46897 (comment)

Tests

Same as QA

Offline tests

Same as QA

QA Steps

Violation flow

Create a workspace if needed
Enable distance rates in settings from more features
Add a custom rate
Edit the default rate to a different value
Open the chat with yourself
Track a Distance expense
In the Concierge prompt, click "Share it with my accountant"
Select a workspace
Verify there's an error on the rate field instructing you to choose a workspace rate
Verify you can't submit the expense with the error present
Select a workspace rate
Verify the error clears
Submit the expense
Verify it's moved to the workspace
Verify it uses with the proper rate and distance, and the receipt image is updated

Auto-select rate flow

Open the chat with yourself
Track a Distance expense
Check the mileage rate automatically set on the request
Create a workspace if needed
Enable distance rates in settings from more features
Add a custom rate with the exact same value as in the step 3
Go back to the chat with yourself
In the Concierge prompt, click "Share it with my accountant"
Select a workspace
Verify there is no error on the rate field
Click the "Rate" field
Verify the custom rate which you've added in step 6 is automatically marked as "Selected"
Submit the expense
Verify it's moved to the workspace
Verify it uses with the proper rate and distance, and the receipt image is updated

PR Author Checklist

Screenshots/Videos

Android: Native

2024-11-13.-.20.27.-.android.mp4

Android: mWeb Chrome

2024-11-13.-.20.27.-.chrome.mp4

iOS: Native

2024-11-13.-.20.20.-.Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-13.at.19.54.36.mp4

iOS: mWeb Safari

2024-11-13.-.20.20.-.Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-11-13.at.19.53.07.mp4

MacOS: Chrome / Safari

2024-11-13.-.16.08.-.Screen.Recording.2024-11-13.at.16.05.08.mp4

MacOS: Desktop

2024-11-13.-.17.01.-.Screen.Recording.2024-11-13.at.17.00.31.mp4

paultsimura · 2024-10-26T06:33:45Z

src/libs/ModifiedExpenseMessage.ts

+    if (reportActionOriginalMessage?.movedToReportID) {
+        return "Moved this expense"; // todo: copy needed
+    }


@neil-marcellini could we please request a copy for this action?
It's added as the first report action after I share my tracked expense with an accountant.

Don't we already have a message like this when moving a manual expense for example? Can we use the same copy?

@neil-marcellini no, the fallback is shown for those expenses as well:

paultsimura · 2024-11-11T22:14:03Z

Made some progress on the PR today.

paultsimura · 2024-11-13T16:03:32Z

With the recent changes, seems like the flow works fine (please see the recordings).
Still need the "Moved expense" copy though.

melvin-bot · 2024-11-15T11:46:17Z

@Pujan92 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

paultsimura · 2024-11-15T11:46:59Z

@Pujan92 I believe only the copy is pending here, therefore you can start testing the flow.

neil-marcellini · 2024-11-20T21:24:53Z

I asked for a copy check on the issue

paultsimura · 2024-11-22T16:23:23Z

@neil-marcellini since we went live with the P2P distance requests, will there be any cleanup for the canUseP2PDistanceRequests usages?

paultsimura · 2024-11-22T16:38:51Z

@neil-marcellini for the in-existing rate, would this format work (message TBD)?

When incorrect route is selected, we don't show the field error, but a form error at the bottom. I'm wondering if we should keep this behavior consistent (I just added the RBR for better clarity of which field is faulty):

neil-marcellini · 2024-11-22T17:21:07Z

@neil-marcellini since we went live with the P2P distance requests, will there be any cleanup for the canUseP2PDistanceRequests usages?

I already removed all of them on main, so please make sure that's removed here too.

neil-marcellini · 2024-11-22T17:23:03Z

@paultsimura I like what you did there with the RBR on the field and the form error message at the bottom. If you want, you could also do that for the route error, or tackled that in a separate PR.

paultsimura · 2024-11-28T22:25:35Z

Got a bit stuck on the PR – asked a couple of clarifying questions here.

paultsimura · 2024-12-05T10:54:10Z

@neil-marcellini @Pujan92 the PR is almost ready. However, I have an issue with the dependency cycle.

We've agreed on using the root level report name in the MODIFIEDEXPENSE message:
moved expense from self DM to chat with {username} or moved expense from self DM to {workspace name}.

I'd like to reuse the ReportUtils.getReportName function inside the ModifiedExpenseMessage.getForReportAction, but ReportUtils.getReportName uses ModifiedExpenseMessage.getForReportAction in one specific case:

App/src/libs/ReportUtils.ts

Lines 4015 to 4018 in c049ac7

    
           if (!isEmptyObject(parentReportAction) && ReportActionsUtils.isModifiedExpenseAction(parentReportAction)) { 
        
               const modifiedMessage = ModifiedExpenseMessage.getForReportAction(report?.reportID, parentReportAction); 
        
               return formatReportLastMessageText(modifiedMessage); 
        
           }

What would be your suggestion to avoid this cycle?
It's nearly impossible to decouple the ReportUtils.getReportName as it uses a lot of the utilitary functions, such as isGroupChat, isPolicyExpenseChat, etc.

I could potentially move all of these functions into separate files and export them all from ReportUtils/index.ts, but it will create a dozen of one-file functions.

Or I could force allow the dependency cycle.

But maybe you could suggest a more elegant approach.

neil-marcellini

Looks great to me, thanks

Pujan92 · 2024-12-10T17:37:00Z

src/libs/ModifiedExpenseMessage.ts

+    const destinationReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${destinationReportID}`];
+    const rootParentReport = getRootParentReport(destinationReport);
+
+    // The "Move report" flow only supports moving expenses to a policy expense chat or a 1:1 DM.
+    const reportName = isPolicyExpenseChat(rootParentReport) ? getPolicyExpenseChatName(rootParentReport) : buildReportNameFromParticipantNames({report: rootParentReport});
+    const policyName = getPolicyName(rootParentReport, true);


I have an issue with the dependency cycle #51517 (comment)

Can't we move this part to ReportUtils to avoid dependency cycle issue?

No. ReportUtils already imports ModifiedExpenseMessage, so the cycle will be present anyway.
The cycle will be resolved holistically after https://expensify.slack.com/archives/C01GTK53T8Q/p1733492380567339

Ah yes, it will be the same. 😓

Pujan92 · 2024-12-10T18:01:10Z

Only issue I am seeing is that there is some distance unit(km and ml) inconsistency after the rate selection and with the price in the preview.

Screen.Recording.2024-12-10.at.23.23.06.mov

paultsimura · 2024-12-10T18:05:59Z

Only issue I am seeing is that there is some distance unit(km and ml) inconsistency after the rate selection and with the price in the preview.

Screen.Recording.2024-12-10.at.23.23.06.mov

I saw that too, but it should be out of scope here

Pujan92

LGTM, Thanks!

# Conflicts: # src/libs/actions/IOU.ts

paultsimura · 2024-12-11T14:14:03Z

src/libs/actions/IOU.ts

-    const parameters = {
-        onyxData,


@neil-marcellini I had a conflict with recently merged #52221 which you reviewed as well.
Please let me know what you think about explicitly specifying the params subtypes that I've added in this PR (this change and a couple below) to avoid passing redundant data.
If it's a good practice, maybe it should be encouraged in other refactoring PRs as well.

Ah yes, that does seem like a good practice. Thanks.

neil-marcellini · 2024-12-11T17:32:39Z

@paultsimura can you please explain why the bug with the unit inconsistency should be out of scope? It seems like something we can and should fix as part of this flow.

neil-marcellini

Looks good except that one bug

neil-marcellini · 2024-12-11T17:34:54Z

src/libs/actions/IOU.ts

-    const parameters = {
-        onyxData,


Ah yes, that does seem like a good practice. Thanks.

paultsimura · 2024-12-11T19:41:02Z

@paultsimura can you please explain why the bug with the unit inconsistency should be out of scope? It seems like something we can and should fix as part of this flow.

Because it is something that already exists in prod (so not caused by my changes) and is not described in the initial issue steps and expectations. Also, the nature of the bug is different from the one we are resolving now, although it's also related to the "share expense" flow.

If we want to fix this as well, I would like to request bounty bump to $500 as it is a good chunk of extra work — we've already expanded the scope on this PR compared to the accepted proposal.

neil-marcellini

Thanks for clarifying that the bug is on prod and yes I agree that this has already been a lot of work 😅. Please report the bug, tag me, and let's work on it as a quick follow up.

OSBotify · 2024-12-11T22:36:43Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-12-12T01:58:26Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	success ✅
🍎🔄 iOS HybridApp 🍎🔄	success ✅

izarutskaya · 2024-12-12T09:06:15Z

@paultsimura QA team faund issue #53988 when validation this PR

https://platform.applause.com/services/links/v1/external/07fed6d6849dcbd2b4edb695dce1fc38808aad5a2d21e3ea58e4b987c6878c11

paultsimura · 2024-12-12T09:09:11Z

@paultsimura QA team found issue #53988 when validation this PR

Thanks @izarutskaya, we've already established that it's only been revealed by this PR, not caused by it.

Julesssss · 2024-12-12T11:00:33Z

Hey @paultsimura @Pujan92 @neil-marcellini. We have a blocker that might be related to these distance changes. Users are now unable to change distance rate because backend is not receiving the required customUnit.name data. Which seems possibly related to these changes.

If you could take a quick look that would be very helpful.

paultsimura · 2024-12-12T11:22:23Z

That issue indeed seems to be caused by this PR, although it looks BE to me. I replied in your GH, let's keep the conversation there for clarity.

github-actions · 2024-12-13T15:40:28Z

🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.75-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	failure ❌
🍎🔄 iOS HybridApp 🍎🔄	success ✅

init: distance - share with accountant

55b0924

paultsimura commented Oct 26, 2024

View reviewed changes

paultsimura and others added 2 commits October 26, 2024 10:42

lint

f98e925

Merge branch 'main' into fix/46897-distance-accountant

1543a0d

Pass waypoints and customUnitRateID as params

7c2714f

paultsimura marked this pull request as ready for review November 15, 2024 11:46

paultsimura requested a review from a team as a code owner November 15, 2024 11:46

melvin-bot bot requested review from Pujan92 and removed request for a team November 15, 2024 11:46

neil-marcellini mentioned this pull request Nov 20, 2024

[HOLD for payment 2024-12-20] [$250] Expense shared with accountant shows incorrect rate #46897

Open

6 tasks

neil-marcellini changed the title ~~fix: distance - share with accountant~~ [HOLD Web-E 44237] fix: distance - share with accountant Nov 22, 2024

paultsimura added 2 commits November 22, 2024 22:41

Merge branch 'refs/heads/main' into fix/46897-distance-accountant

95e8885

Merge branch 'refs/heads/main' into fix/46897-distance-accountant

9437393

paultsimura added 3 commits November 29, 2024 16:16

Use a dedicated copy for the "Moved expense" action.

2188485

Merge branch 'refs/heads/main' into fix/46897-distance-accountant

9922718

Reset the draft rate ID when leaving the confirmation step.

d2d90cf

paultsimura added 3 commits December 6, 2024 12:09

Change the movedFromSelfDM copy

5061029

Fix lint

b7c2d0e

Fix lint

9ede66f

neil-marcellini previously approved these changes Dec 9, 2024

View reviewed changes

Pujan92 reviewed Dec 10, 2024

View reviewed changes

Pujan92 approved these changes Dec 10, 2024

View reviewed changes

melvin-bot bot requested a review from neil-marcellini December 10, 2024 18:13

paultsimura added 2 commits December 11, 2024 14:32

Merge branch 'refs/heads/main' into fix/46897-distance-accountant

0858c35

# Conflicts: # src/libs/actions/IOU.ts

Merge fix

edfb777

paultsimura dismissed neil-marcellini’s stale review via edfb777 December 11, 2024 14:10

paultsimura commented Dec 11, 2024

View reviewed changes

neil-marcellini changed the title ~~[HOLD Web-E 44237] fix: distance - share with accountant~~ fix: distance - share with accountant Dec 11, 2024

neil-marcellini requested changes Dec 11, 2024

View reviewed changes

neil-marcellini approved these changes Dec 11, 2024

View reviewed changes

neil-marcellini merged commit 567bd21 into Expensify:main Dec 11, 2024
21 checks passed

github-actions bot mentioned this pull request Dec 12, 2024

Deploy Checklist: New Expensify 2024-12-12 #53978

Closed

55 tasks

IuliiaHerets mentioned this pull request Dec 12, 2024

[$250] Track - Amount on the expense preview does not update when track distance expense is submitted #53988

Open

8 tasks

Julesssss mentioned this pull request Dec 12, 2024

[HOLD for payment 2024-12-20] [$250] Track distance - Not here page opens after submitting track distance expense to new workspace #53995

Closed

8 tasks

luacmartins mentioned this pull request Dec 12, 2024

[CP Staging] Revert "fix: distance - share with accountant" #54076

Merged

50 tasks

fix: distance - share with accountant #51517

fix: distance - share with accountant #51517

Conversation

paultsimura commented Oct 26, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultsimura commented Nov 11, 2024

paultsimura commented Nov 13, 2024 • edited Loading

melvin-bot bot commented Nov 15, 2024

paultsimura commented Nov 15, 2024

neil-marcellini commented Nov 20, 2024

paultsimura commented Nov 22, 2024

paultsimura commented Nov 22, 2024

neil-marcellini commented Nov 22, 2024

neil-marcellini commented Nov 22, 2024

paultsimura commented Nov 28, 2024

paultsimura commented Dec 5, 2024 • edited Loading

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pujan92 commented Dec 10, 2024

paultsimura commented Dec 10, 2024

Pujan92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini commented Dec 11, 2024

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultsimura commented Dec 11, 2024 • edited Loading

neil-marcellini left a comment

Choose a reason for hiding this comment

OSBotify commented Dec 11, 2024

github-actions bot commented Dec 12, 2024

izarutskaya commented Dec 12, 2024 • edited Loading

paultsimura commented Dec 12, 2024

Julesssss commented Dec 12, 2024

paultsimura commented Dec 12, 2024

github-actions bot commented Dec 13, 2024

paultsimura commented Oct 26, 2024 •

edited

Loading

paultsimura commented Nov 13, 2024 •

edited

Loading

paultsimura commented Dec 5, 2024 •

edited

Loading

paultsimura commented Dec 11, 2024 •

edited

Loading

izarutskaya commented Dec 12, 2024 •

edited

Loading