Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create v1 Collect workspace Bottom Up flow #30582
Create v1 Collect workspace Bottom Up flow #30582
Changes from all commits
8f961ac
6c8bd19
1a46590
e68e8a8
34064f5
b1b8349
7ab658f
c814ade
cfe141b
ae33908
b6030a9
93d176c
ec8974c
b9844f6
fc25fde
4655e8f
c9a9ec4
23dda79
bcaece0
16df5ae
59e4026
95d7849
f9a2af1
93f3475
43aaa43
c599cde
2437282
733067b
6765abd
7aeeef6
2e5baf2
5a5e494
7bf4260
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 also removed personal bank account option in Wallet page. Issue: #32079
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.
Send money case was missing in this condition, which caused #32228
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 caused crash - #31797 as it fallback to throw here:
App/src/pages/settings/Wallet/WalletPage/WalletPage.js
Line 210 in 7a68a3d
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.
Is this additional check necessary? We also make the same check within the function.
I think it is because of the navigation. And it's better to check within the function instead of outside. But I just want to double-check
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 it is as we want to separate the navigation from the actions/ Policy file to separate the concerns. Given we want to take different action based on the report type, I think we need to keep it here.
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 this default might be incorrect. I think this defaults to a personal bank account since we pass it here
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 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
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 change caused duplicated "Pay with Expensify" option