-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow split actions to be edited and implement IOU action completeSplitBill #29064
Conversation
Deploying with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youssef-lr lint is not happy
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.isRequired and 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 |
comment: transactionDescription, | ||
merchant: transactionMerchant, | ||
created: transactionCreated, | ||
} = draftTransaction ? ReportUtils.getTransactionDetails(draftTransaction) : ReportUtils.getTransactionDetails(transaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always return true because we add a default {}
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youssef-lr This was not added to the prop type definition for this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should keep the name consistent and use isTypeSplit
.
defaultAmount={transactionAmount} | ||
reportID={reportID} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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