Allow split actions to be edited and implement IOU action completeSplitBill #29064

youssef-lr · 2023-10-09T04:25:34Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/322303

Tests

Verify that no errors appear in the JS console

From Global Create

Start a split bill from Global Create by using the Scan tab and attaching a receipt, choose a new user.
Tap on the split action to reveal the split details page.
Make sure the page is editable.
Enter the amount, date, and merchant, make sure the amount is updated next to the 'Split' button.
Click 'Split', and confirm a report preview displaying the split amount.
Make sure an IOU report has been created, which should contain an expense with the same receipt attached to the original split action.

From Teachers Unite workspace

Sign up as a teacher using https://staging.new.expensify.com/teachersunite
Split a bill from the workspace chat created, and complete using the same test steps as above.
Make sure an expense report is created with half the amount of the total created.

Testing errors

Start a split bill
One the split details page, click on Split without entering the details
Make sure errors appear next to the missing field
Fill in those missing fields one by one, make sure the errors disappear, as well as the form error.
Complete the split bill and make sure child reports are created succesfully.

Offline tests

The same test steps from above, but go offline before completing a split bill, then back online once you click on 'Split'.

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

This was tested thoroughly and evidence can be seen in the comments of this PR as well as in internal Slack channels where videos have been shared

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

cloudflare-workers-and-pages · 2023-10-09T04:28:27Z

Deploying with Cloudflare Pages

Latest commit:	`69c0df9`
Status:	✅ Deploy successful!
Preview URL:	https://c0140b05.helpdot.pages.dev
Branch Preview URL:	https://youssef-completesplitbill.helpdot.pages.dev

View logs

mountiny

@youssef-lr lint is not happy

src/libs/TransactionUtils.ts

Co-authored-by: Vit Horacek <[email protected]>

youssef-lr · 2023-10-10T18:04:30Z

src/libs/TransactionUtils.ts

@@ -143,7 +143,13 @@ function getUpdatedTransaction(transaction: Transaction, transactionChanges: Tra
        updatedTransaction.tag = transactionChanges.tag;
    }

-    if (shouldStopSmartscan && transaction?.receipt && Object.keys(transaction.receipt).length > 0 && transaction?.receipt?.state !== CONST.IOU.RECEIPT_STATE.OPEN) {
+    if (
+        shouldUpdateReceiptState &&


I'm not sure why this function handles updating a receipt, the function name is getUpdatedTransaction, but seems like the function has a side effect.

s77rt · 2023-10-11T21:20:40Z

Strange how the error is The prop transaction.isRequired is marked as required instead of The prop transaction is marked as required.

s77rt · 2023-10-11T21:40:31Z

Okay got it. It should be

transaction: transactionPropTypes.isRequired

and not

transaction: PropTypes.shape(transactionPropTypes).isRequired

transactionPropTypes already is a shape. Same fix needed for draftTransaction in SplitBillDetailsPage and EditSplitBillPage

mountiny · 2023-10-11T23:08:26Z

@s77rt this is ready, can you pleaase complete the checklist so we can get this PR to staging today. Thanks!

s77rt · 2023-10-11T23:38:34Z

Conflicts

s77rt · 2023-10-11T23:45:01Z

@mountiny Can you please fix this prop type for draftTransaction as well

s77rt · 2023-10-12T00:03:51Z

Is the mweb bug supposed to be fixed later?

luacmartins · 2023-10-12T00:46:53Z

src/pages/EditSplitBillPage.js

+        comment: transactionDescription,
+        merchant: transactionMerchant,
+        created: transactionCreated,
+    } = draftTransaction ? ReportUtils.getTransactionDetails(draftTransaction) : ReportUtils.getTransactionDetails(transaction);


This will always return true because we add a default {}, no?

Will make it undefined in the follow up PR

luacmartins · 2023-10-12T01:07:10Z

Note: we merged this PR as is and will address the remaining issues as a followup tomorrow @youssef-lr

OSBotify · 2023-10-12T01:13:25Z

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

youssef-lr · 2023-10-12T05:08:57Z

Thanks folks!!!

tgolen · 2023-10-12T17:08:33Z

src/components/MoneyRequestConfirmationList.js

    const {translate, toLocaleDigit} = useLocalize();
+    const transaction = props.isEditingSplitBill ? props.draftTransaction || props.transaction : props.transaction;


@youssef-lr This was not added to the prop type definition for this component.

Working on a follow up PR to clean things up, we wanted this merged quickly so we can test 🙏🏼

OSBotify · 2023-10-13T21:55:11Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

OSBotify · 2023-10-13T22:07:14Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

OSBotify · 2023-10-13T22:23:27Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀

platform	result
🤖 android 🤖	skipped 🚫
🖥 desktop 🖥	skipped 🚫
🍎 iOS 🍎	skipped 🚫
🕸 web 🕸	skipped 🚫

OSBotify · 2023-10-13T22:58:51Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀

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

tgolen · 2023-10-12T17:18:36Z

src/components/MoneyRequestConfirmationList.js


    const isTypeRequest = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST;
+    const isSplitBill = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT;


Should keep the name consistent and use isTypeSplit.

tgolen · 2023-11-03T19:21:30Z

src/pages/EditSplitBillPage.js

+                defaultAmount={transactionAmount}
+                reportID={reportID}


FYI, this component doesn't use either of these props. I'm going to remove them in my PR and I'll ping you on it.

s77rt · 2024-02-05T02:19:26Z

src/components/MoneyRequestConfirmationList.js

+                            error={
+                                shouldDisplayFieldError && (transaction.modifiedMerchant === '' || transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT)
+                                    ? translate('common.error.enterMerchant')
+                                    : ''
+                            }


We should show the merchant error only if the merchant is required i.e. the report is linked to a workspace.

youssef-lr added 6 commits October 8, 2023 20:29

Make SplitBillDetailsPage editable when receipt is scanning or SS failed

8c8d3ff

Only show SmartScan fields splitting manually or editting a split

12b9043

Create EditSplitBillPage

7fba50a

Create routes for EditSplitBillPage

bd7c4b2

Configure inputs on split details page and link them with routes

4b95154

Save split transaction edits to a draft transaction

8f3f426

youssef-lr changed the title ~~Youssef complete split bill~~ Allow split actions to be edited and implement IOU action completeSplitBill Oct 9, 2023

youssef-lr changed the base branch from main to youssef_startSplitBill October 9, 2023 04:27

youssef-lr added 5 commits October 9, 2023 15:27

Cleanup

60bca0e

Add useCallback dep

914985f

Add proptype

b3c4a3e

Fix code removed by mistake

d62883c

Bug fix and lint

08d7577

mountiny assigned youssef-lr Oct 9, 2023

youssef-lr added 2 commits October 10, 2023 00:11

wip

f516e3b

Merge branch 'youssef_startSplitBill' into youssef_completeSplitBill

5812464

mountiny reviewed Oct 9, 2023

View reviewed changes

src/libs/TransactionUtils.ts Outdated Show resolved Hide resolved

youssef-lr mentioned this pull request Oct 10, 2023

Add Scan to Split Bill and implement IOU action startSplitBill #28710

Merged

57 tasks

youssef-lr added 6 commits October 10, 2023 17:48

Merge branch 'youssef_startSplitBill' into youssef_completeSplitBill

7950ce7

Remove deprecated method and use withOnyx

4096fe2

Add completeSplitBill action

dd69105

Send necessary params to completeSplitBill

46b6398

Show right icon next to all fields in split details

f134e3f

Fix a few bugs

01d37b7

Base automatically changed from youssef_startSplitBill to main October 10, 2023 16:58

youssef-lr and others added 3 commits October 11, 2023 01:10

Merge branch 'main' into youssef_completeSplitBill

f495a29

Cleanup

5ad7a0a

Update src/libs/TransactionUtils.ts

322e20f

Co-authored-by: Vit Horacek <[email protected]>

youssef-lr commented Oct 10, 2023

View reviewed changes

mountiny added 2 commits October 11, 2023 23:56

Merge branch 'main' into youssef_completeSplitBill

ddc2214

Fix console warning

1a51105

mountiny requested a review from s77rt October 11, 2023 23:04

mountiny previously approved these changes Oct 11, 2023

View reviewed changes

resolve conflicts

69c0df9

luacmartins dismissed mountiny’s stale review via 69c0df9 October 12, 2023 00:42

luacmartins approved these changes Oct 12, 2023

View reviewed changes

marcochavezf approved these changes Oct 12, 2023

View reviewed changes

luacmartins merged commit 828b8b3 into main Oct 12, 2023
15 checks passed

luacmartins deleted the youssef_completeSplitBill branch October 12, 2023 01:06

mountiny mentioned this pull request Oct 12, 2023

Deploy Checklist: New Expensify 2023-10-12 #29417

Closed

97 tasks

tgolen reviewed Oct 12, 2023

View reviewed changes

lanitochka17 mentioned this pull request Oct 12, 2023

Scan - Split bill - Empty merchant field does not show error message in offline mode #29495

Closed

6 tasks

tgolen reviewed Nov 3, 2023

View reviewed changes

DylanDylann mentioned this pull request Jan 29, 2024

Fix/34609: Remove old description page #35137

Merged

50 tasks

s77rt reviewed Feb 5, 2024

View reviewed changes

s77rt mentioned this pull request Feb 5, 2024

[HOLD for payment 2024-02-09] [$125] Split - Error message for merchant required is displayed on failed split in group DM #34914

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow split actions to be edited and implement IOU action completeSplitBill #29064

Allow split actions to be edited and implement IOU action completeSplitBill #29064

youssef-lr commented Oct 9, 2023 •

edited by mountiny

Loading

cloudflare-workers-and-pages bot commented Oct 9, 2023 •

edited

Loading

mountiny left a comment

youssef-lr Oct 10, 2023

s77rt commented Oct 11, 2023

s77rt commented Oct 11, 2023

mountiny commented Oct 11, 2023

s77rt commented Oct 11, 2023

s77rt commented Oct 11, 2023

s77rt commented Oct 12, 2023

luacmartins Oct 12, 2023

youssef-lr Oct 12, 2023

luacmartins commented Oct 12, 2023

OSBotify commented Oct 12, 2023

youssef-lr commented Oct 12, 2023

tgolen Oct 12, 2023

youssef-lr Oct 12, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

tgolen Oct 12, 2023

tgolen Nov 3, 2023

s77rt Feb 5, 2024

		const {translate, toLocaleDigit} = useLocalize();
		const transaction = props.isEditingSplitBill ? props.draftTransaction \|\| props.transaction : props.transaction;


		const isTypeRequest = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST;
		const isSplitBill = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT;

Allow split actions to be edited and implement IOU action completeSplitBill #29064

Allow split actions to be edited and implement IOU action completeSplitBill #29064

Conversation

youssef-lr commented Oct 9, 2023 • edited by mountiny Loading

Details

Fixed Issues

Tests

From Global Create

From Teachers Unite workspace

Testing errors

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

cloudflare-workers-and-pages bot commented Oct 9, 2023 • edited Loading

Deploying with Cloudflare Pages

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Oct 11, 2023

s77rt commented Oct 11, 2023

mountiny commented Oct 11, 2023

s77rt commented Oct 11, 2023

s77rt commented Oct 11, 2023

s77rt commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins commented Oct 12, 2023

OSBotify commented Oct 12, 2023

youssef-lr commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

OSBotify commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youssef-lr commented Oct 9, 2023 •

edited by mountiny

Loading

cloudflare-workers-and-pages bot commented Oct 9, 2023 •

edited

Loading