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
feat: add employee and accounting page to onboarding flow #49161
feat: add employee and accounting page to onboarding flow #49161
Changes from 1 commit
accc8a1
419f7cc
88dda39
6bdf7ed
e0bc748
de74800
5f75726
a37009d
d72dec2
2200305
646fc6d
b7b016f
6964f34
def4389
b923dab
b1e8c24
f8ff1f5
a773766
a50ce47
c9b6a0b
0d6adac
13f4da9
944b3d3
1a67914
497a35e
ced0874
41de3e1
d417b6a
c6c8cbc
11f3a4a
465ffbc
d3121d6
f72f134
076d4af
ccfa4e1
a1201d2
91bbbaf
63a5932
34f74ad
fb70a3f
bdf3a08
d0be895
5e61daf
d8b71b0
0057cb1
939c169
97e41e8
556b83f
fe9d9b5
9a1c414
8706f60
74631ed
4516b4d
aab3ad8
9985e88
7bba26c
7fb12f0
433261c
f806fde
3584504
bd6b524
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 3693 in src/libs/actions/Report.ts
GitHub Actions / typecheck
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 should be optional.
(To recheck: the BE seems to complain about the empty names)
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 also update the default value of this data in
compleOnboarding
function with the default value as empty string?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.
Make it optional in completeOnboarding
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.
AFAICT it is optional. Lmk what you are seeing. Yes I would update this to be optional.
I am also pretty confused about why it's being passed as an object if we can fix that while we are here. Maybe someone thought it better to parameterize the arguments? But the next person disagreed 😄
Looks inconsistent to me. Let's make 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.
@marcaaron
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.
Sigh... lol 😄
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.
@marcaaron Should we add default value for
firstName
andlastName
incompleteOnboarding
because in the typeCompleteGuidedSetupParams
,firstName
andlastName
is string.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.
Yes let's do it. We will need it in some cases, but not for the "manage team expense" choice so it is appropriate to make optional.
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 updated.
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.
Since we are not going to use the policy in between steps is there any reason that we create the workspace at that early step? Why not create it just before calling
completeOnboarding
in the accounting step?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.
Interesting. Did you have any concern about that or opinion on why it would be better to do one over the other?
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.
No strong preference it's just a little questionable that we create the workspace when not really needed yet. Now I think about it more I think we may have a bug:
onboardingPolicyID
is not on onyxcc @nkdengineer Can you please confirm if you can reproduce this bug?
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.
Oh hmm that's an interesting edge case for sure - I'm not sure how we would completely solve that without designating a specific policy as the "onboarding policy". Though we could potentially check if a user has an existing policy.
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.
Fixing the problem highlighted in #50759 may fix this as I think we will have to send the policy id from the backend