Create v1 Collect workspace Bottom Up flow by mountiny · Pull Request #30582 · Expensify/App

mountiny · 2023-10-30T14:19:04Z

Details

In this PR we are implementing a basic v1 bottom up flow in which users can:

choose to pay p2p IOU report with business bank account
collect workspace is created for them as admin and the submitter as employee
the IOU report is converted to an expense report, the parent report changes to the new workspace chat, change the report to tal to negative amount as its done in case of expense reports
the transaction data is updated to match expected transaction data for a workspace (amount is changed to negative value)
We are adding a new Moved report action which is used to link the DM with the new workspace chat

Fixed Issues

Tests

For testing purposes as Expensify Engineer locally set the skipGuidesAssignent to true on this line as most likely the guide assignment might not be working well for you locally and it could cause the call to fail.

Set your location/ VPN to the USA (thats to ensure the policy currency is set to USD which is the only currency we support for the business bank accounts in NewDot right now)
Create 2 accounts, one will be used as employee and second as an admin (now they are just a normal pals, friends, nothing more than that, but we will change them to an admin and employee in this flow)
Request 1:1 money as employee from the admin (the IOU report can only have "one-way" requests, if the admin would also request some money from the employee, this flow is not allowed/accessible)
As admin, go to the 1:1 chat and click Pay With Expensify
Choose to pay with Business Bank account
Verify you have been brought to the Bank account flow
Close the bank account flow
Verify you can see a new report message in the 1:1 chat which links you to the workspace chat of the new employee
Click on the link (when developing in localhost against staging api, the link will point to staging so you will have to update it)
Verify you can see a report preview for the same amount request in the expense report
Verify you can navigate to the expense report both as admin and employee
Verify the expense report and workspace chat correctly show the default workspace avatar in the LHN

Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

src/CONST.ts

src/components/AddPaymentMethodMenu.js

src/libs/actions/Policy.js

puneetlath

Looking good! Just some minor feedback.

src/libs/ReportActionsUtils.ts

src/libs/actions/Policy.js

Julesssss · 2023-11-22T08:42:28Z

fwiw the logs suggest you did not change

I 100% did this. I was very careful to save the file given that this was the 3rd test. Maybe there is another line that I need to comment out?

akinwale · 2023-11-22T20:57:20Z

@mountiny Test results with staging server enabled. Video showing fresh accounts (left: admin, right: employee) included.

Requesting (employee) user

Added to #announce room for the created workspace
Collab. IOU report created (collaboration between user and workspace)
No workspace chat

Requestee (admin) user

Added to #admin and #announce rooms for the workspace
Collab. IOU report created
IOU report moved
No workspace chat

30582-web-3.mp4

puneetlath · 2023-11-22T21:01:41Z

I see the employee's workspace chat there for both. Isn't it these? Seems like it's working correctly to me.

puneetlath · 2023-11-22T21:02:10Z

Oh, I guess the admin's own workspace chat doesn't seem to have been added. But the employee's does for both.

akinwale · 2023-11-22T21:07:33Z

Oh, I guess the admin's own workspace chat doesn't seem to have been added. But the employee's does for both.

Yeah, I assumed we're looking for the workspace's chat room which is different from those. I opened all the reports in the video just to make sure.

mountiny · 2023-11-22T21:20:24Z

It was added optimistically but then disappeared, I think this might be something to do with the Collect policy settings. This would require a change in the backend I have created a follow up to investigate so we do not forget about this #31754

Both users have workspace chat and seems like everything worked so I am going to go ahead with merge. Thanks everyone fore testing and reviewing 🚀

Addressed

OSBotify · 2023-11-22T21:25:30Z

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

OSBotify · 2023-11-23T17:10:45Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.3-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	cancelled 🔪
🕸 web 🕸	success ✅

OSBotify · 2023-11-23T17:50:55Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.3-0 🚀

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

situchan · 2023-11-23T20:48:36Z

src/components/AddPaymentMethodMenu.js

+                          {
+                              text: translate('common.businessBankAccount'),
+                              icon: Expensicons.Building,
+                              onSelected: () => onItemSelected(CONST.PAYMENT_METHODS.BUSINESS_BANK_ACCOUNT),


This caused crash - #31797 as it fallback to throw here:

App/src/pages/settings/Wallet/WalletPage/WalletPage.js

Line 210 in 7a68a3d

throw new Error('Invalid payment method type selected');

luacmartins · 2023-11-23T21:49:17Z

src/components/KYCWall/BaseKYCWall.js

+                Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute('', policyID));
+                return;
+            }
+            Navigation.navigate(this.props.addBankAccountRoute);


I think this default might be incorrect. I think this defaults to a personal bank account since we pass it here

I think this should be correct because we pass it to the SettlementButton in both cases using this method which checks if the parent report is workspace chat or DM

App/src/components/MoneyReportHeader.js

Line 94 in e909316

const bankAccountRoute = ReportUtils.getBankAccountRoute(chatReport);

OSBotify · 2023-11-27T19:04:51Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.3-11 🚀

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

situchan · 2023-12-06T05:50:47Z

src/components/SettlementButton.js

-        if (isExpenseReport) {
-            buttonOptions.push(paymentMethods[CONST.IOU.PAYMENT_TYPE.VBBA]);
-        }
+        buttonOptions.push(paymentMethods[CONST.IOU.PAYMENT_TYPE.VBBA]);


This change caused duplicated "Pay with Expensify" option

aimane-chnaif · 2024-01-03T11:07:47Z

src/components/AddPaymentMethodMenu.js

-                    },
-                },
-                ...(Permissions.canUseWallet(props.betas)
+                ...(ReportUtils.isIOUReport(iouReport)


Send money case was missing in this condition, which caused #32228

aimane-chnaif · 2024-01-29T14:56:54Z

src/components/AddPaymentMethodMenu.js

-                    onSelected: () => {
-                        props.onItemSelected(CONST.PAYMENT_METHODS.BANK_ACCOUNT);
-                    },
-                },


This also removed personal bank account option in Wallet page. Issue: #32079

ntdiary · 2024-02-02T04:43:52Z

src/libs/actions/Policy.js

+
+    const optimisticData = [
+        {
+            onyxMethod: Onyx.METHOD.MERGE,


Coming from #31836.
As there isn't a better way at the moment to update data immediately before navigation, we have fallen back to using the SET method.

😢 This is multiple times I've seen this used and I think it is a hack and that we should really try to find some other real solution to the problem.

I did raise that concern in the linked issue, however, no other viable solution has been found in couple of weeks. Then given we already use the SET for classic policy creation (so not part of the bottom up flow), I agree to proceed with using SET.

Would you be OK with creating a GH issue with a solid problem statement and start getting some proposals for fixing it? I'd like to not just let this one fade away with others continuing to copy the same hack. I have some ideas like:

If there was just a merge() then the data should be immediately (and synchronously) available in the Onyx cache. Let's find a way to use it faster

Is the problem because subscribers aren't notified until the next-tick? If so, maybe we can add an option to Onyx to bypass for some scenarios (since it was only added as an optimization)

I have this saved, but I still did not get around to do this focusing on Ideal nav and waves in general. Noting previous comment from another PR for future.

mountiny added 5 commits October 21, 2023 00:00

Add a new button to the payment methods popover

8f961ac

Add the business bank account option

6c8bd19

Work on the Bottom Up flow command

1a46590

Merge main

e68e8a8

Merge main

34064f5

mountiny self-assigned this Oct 30, 2023

mountiny changed the title ~~Collect Bottom UP flow~~ Collect Bottom Up flow Oct 30, 2023