-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix/34611: Remove old merchant page #35641
Conversation
@hoangzinh 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] |
Btw, please fix conflicts as well. |
@hoangzinh Updated |
Oops, This PR has some conflicts :( |
@hoangzinh Resolved conflict. All yours |
@DylanDylann last thing. For test steps, I think we can split into 2 test cases. Something like this Test case 1: Request money
Test case 2: Split bill
|
@DylanDylann I found a bug when editing a request money, the merchant value is not loaded correctly. Screen.Recording.2024-02-06.at.22.39.09.mov |
@cead22 I updated it, please help to review it again |
@DylanDylann can you update to resolve conflicts and ping again, and I'll review? thanks |
@cead22 Updated. Please help to review again |
const isEditingSplitBill = iouType === CONST.IOU.TYPE.SPLIT && isEditing; | ||
const {merchant} = ReportUtils.getTransactionDetails(isEditingSplitBill && !lodashIsEmpty(splitDraftTransaction) ? splitDraftTransaction : transaction); | ||
const isEmptyMerchant = merchant === '' || merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT; | ||
const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat)); |
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.
Why are we passing participants in transaction, and why do we have isPolicyExpenseChat
in participant? Sorry if this is how we've been doing this and I just didn't know, it just seemed odd to me
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.
@cead22 It is old logic
const isMerchantRequired = _.some(participants, (participant) => Boolean(participant.isPolicyExpenseChat)); |
I only change the order of definition
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 understand. Do you know why we do it this way, and whether it makes sense to update this?
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.
It seems we only require a merchant field if the participant is a policy expense chat. I think we need to confirm which cases we need to require merchant before updating
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 think I understand better now, and transaction
isn't a Transaction like in the database, it just a holder of some data, and in this case it sounds like it can be data related to Money Request view state, rather than the underlying Money Request data
So what's the type of transaction
here?
- The props define it as
transactionPropTypes
which afaict doesn't haveparticipants
- But I see ReportUtils.getTransactionDetails takes
OnyxEntry<Transaction>
and theTransaction
type does have participants
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.
@cead22 Thanks for pointing out that, I will add participant type to transactionPropTypes as a temporary fix because in the future when this component is migrated to typescript, we will use OnyxEntry type
Conflicts |
@cead22 All yours |
@hoangzinh I saw a warning on your android video, is that getting fixed somewhere else? |
@@ -67,6 +67,17 @@ export default PropTypes.shape({ | |||
}), | |||
), | |||
|
|||
/** Selected participants */ | |||
participants: PropTypes.arrayOf( |
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 this be required? And should the property isPolicyExpenseChat also be required?
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.
@cead22 in the new TS type
App/src/types/onyx/Transaction.ts
Line 136 in cc0e82d
participants?: Participant[]; |
Line 9 in cc0e82d
isPolicyExpenseChat?: boolean; |
These fields are not required. participants
exists after the user selects participant so why do you think these fields should be required?
What do you think about adding a fallback value for transaction.participant
here
const isMerchantRequired = _.some(transaction.participants, (participant) => Boolean(participant.isPolicyExpenseChat));
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.
why do you think these fields should be required?
What you said makes sense, I only thought maybe they could be required since we're using them.
why do you think these fields should be required?
We can, and it would make it more obvious that isPolicyExpenseChat
isn't a required prop, but at the same time, the result is the same because participant.isPolicyExpenseChat
will be cast to bool even without the explicit cast, so I don't feel strongly either way
@cead22 Looks like it has been fixed somewhere, I tried to test again on this PR but couldn't reproduce that warning android.mov |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.42-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.42-5 🚀
|
Details
Fixed Issues
$ #34611
PROPOSAL: #34611 (comment)
Tests
Test case 1: Request Money Flow
Test case 2: Split Bill Flow
Offline tests
Same above
QA Steps
Same above
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
Android: Native
Can't run android because of the error on latest main
Android: mWeb Chrome
c.mp4
iOS: Native
i.mp4
iOS: mWeb Safari
s.mp4
MacOS: Chrome / Safari
w.mp4
MacOS: Desktop
d.mp4