-
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: restart the flow for another policy #49687
base: main
Are you sure you want to change the base?
fix: restart the flow for another policy #49687
Conversation
@DylanDylann 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] |
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.
@DylanDylann what is your ETA for testing on this pr?
If we rush, I can start now |
We do not rush specifically, just asking 😂 |
|
||
function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps) { | ||
const session = useSession(); | ||
const [isLoadingApp] = useOnyx(ONYXKEYS.IS_LOADING_APP); |
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 we add a fallback value as before?
Same to plaidLinkToken, onfidoToken, plaidCurrentEvent
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 don't think we need it
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.
@koko57 Why don't you think we need it? Let's see the our previous implementation
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.
@DylanDylann I don't see anywhere in the app we're using fallback values for the values from useOnyx. And here if we get undefined or null to won't break anything, the conditions will work the same and the TS is not complaining. So I don't see the reason to add these fallbacks.
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.
but ok, I see that it was implemented in the conflicting file
Co-authored-by: Vit Horacek <[email protected]> Co-authored-by: DylanDylann <[email protected]>
@mountiny @DylanDylann comments addressed. @DylanDylann I had to reverse your suggestion with parentheses, because prettier was failing. It works as it's supposed |
@DylanDylann could you please test other ways that we can get to VBBA that you may know? |
That would be namely:
|
@mountiny yes, thanks! the first one I also tested myself - it's in the video, but I haven't tested the bottom-up flow |
@koko57 Also please resolve conflict |
@DylanDylann conflicts resolved, could you please retest it? |
/** | ||
* Returns true if a VBBA exists in any state other than OPEN or LOCKED | ||
*/ | ||
function hasInProgressVBBA(): boolean { | ||
return !!achData?.bankAccountID && achData?.state !== BankAccount.STATE.OPEN && achData?.state !== BankAccount.STATE.LOCKED; | ||
return !!achData?.state && achData?.state !== BankAccount.STATE.OPEN && achData?.state !== BankAccount.STATE.LOCKED; |
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 remove bankAccountID check here? We need to check bankAccountID condition before display continue setup button (in getShouldShowContinueSetupButtonInitialValue function)
Screen.Recording.2024-10-01.at.14.25.21.mov
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.
@DylanDylann I don't remember, but ok reverting it.
Details
Fixed Issues
$ #49447
PROPOSAL:
Tests
Offline tests
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
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)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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-09-26.at.19.18.23.mp4
MacOS: Desktop