Allow split actions to be edited and implement IOU action completeSplitBill #29064
Conversation
youssef-lr
changed the title
Deploying with
|
| Latest commit: |
69c0df9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c0140b05.helpdot.pages.dev |
| Branch Preview URL: | https://youssef-completesplitbill.helpdot.pages.dev |
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
| @@ -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 && | |||
There was a problem hiding this comment.
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.
|
Strange how the error is |
|
Okay got it. It should be transaction: transactionPropTypes.isRequiredand not transaction: PropTypes.shape(transactionPropTypes).isRequired
|
|
@s77rt this is ready, can you pleaase complete the checklist so we can get this PR to staging today. Thanks! |
|
Conflicts |
|
@mountiny Can you please fix this prop type for draftTransaction as well |
|
Is the mweb bug supposed to be fixed later? 
|
| comment: transactionDescription, | ||
| merchant: transactionMerchant, | ||
| created: transactionCreated, | ||
| } = draftTransaction ? ReportUtils.getTransactionDetails(draftTransaction) : ReportUtils.getTransactionDetails(transaction); |
There was a problem hiding this comment.
This will always return true because we add a default {}, no?
There was a problem hiding this comment.
Will make it undefined in the follow up PR
|
Note: we merged this PR as is and will address the remaining issues as a followup tomorrow @youssef-lr |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Thanks folks!!! |
| const {translate, toLocaleDigit} = useLocalize(); | ||
| const transaction = props.isEditingSplitBill ? props.draftTransaction || props.transaction : props.transaction; |
There was a problem hiding this comment.
@youssef-lr This was not added to the prop type definition for this component.
There was a problem hiding this comment.
Working on a follow up PR to clean things up, we wanted this merged quickly so we can test 🙏🏼
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
2 similar comments
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
|
|
||
| const isTypeRequest = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST; | ||
| const isSplitBill = props.iouType === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT; |
There was a problem hiding this comment.
Should keep the name consistent and use isTypeSplit.
| defaultAmount={transactionAmount} | ||
| reportID={reportID} |
There was a problem hiding this comment.
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.
| error={ | ||
| shouldDisplayFieldError && (transaction.modifiedMerchant === '' || transaction.modifiedMerchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT) | ||
| ? translate('common.error.enterMerchant') | ||
| : '' | ||
| } |
There was a problem hiding this comment.
We should show the merchant error only if the merchant is required i.e. the report is linked to a workspace.
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/322303
Tests
From Global Create
From Teachers Unite workspace
Testing errors
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
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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