-
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
add tasks for new user first workspace #55302
add tasks for new user first workspace #55302
Conversation
@shubham1206agra 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] |
@shubham1206agra please ignore this one @mananjadhav will review this |
Reassigning C+ for review, this is coming from #55017. Thanks |
Checked the code, did one pass and most of it looks good. @ishpaul777 Can you complete the author checklist?
Asking the same question here #55017 (comment). @francoisl Should we ignore the rule for this particular instance? |
Looking good - a few things:
|
will address this soon..
yes i'll do it right right after ^ |
there is also one more bug that this message "Here are some important tasks to help get your team’s expenses under control." goes away after createworkspace api command is success, can you please check that one as well please it looks like BE issue |
my best guess is that this because of |
For some reason the video and message come back if I revert this commit. |
@@ -424,7 +424,7 @@ function createWorkspaceWithPolicyDraftAndNavigateToIt( | |||
* @param [file] Optional, avatar file for workspace | |||
*/ | |||
function savePolicyDraftByNewWorkspace(policyID?: string, policyName?: string, policyOwnerEmail = '', makeMeAdmin = false, currency = '', file?: File) { | |||
createWorkspace(policyOwnerEmail, makeMeAdmin, policyName, policyID, '', currency, file); | |||
createWorkspace(policyOwnerEmail, makeMeAdmin, policyName, policyID, CONST.ONBOARDING_CHOICES.MANAGE_TEAM, currency, file); |
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.
@ishpaul777 @francoisl do you see a need for adding a comment on why we're passing MANAGE_TEAM ?
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 feelings, but it feel self explanatory if we don't typescript will yell
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 feelings either, though I think it might be more beneficial if we add an explanation in the doc of the function definition, in Policy.ts
. Right now it says @param [engagementChoice] Purpose of using application selected by user in guided setup flow
- we can add a note saying that the default is MANAGE_TEAM
and you can change for specific policy creation flows
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.
Reviewed the code changes, will do a final review once more and then start testing this.
@ishpaul777 I had raised a comment earlier regarding the markdown text. Can you please take a look? The whole text starting from ![]() Also I completed the Invite Accountant flow, I don't see the tasks on the Concierge chat. ![]() |
@mananjadhav does your test account includes a |
I did check the #admin room. Let me try with an email with |
Sorry I think I messed up the branch or something. The test works fine. @ishpaul777 I am working on the checklist, can you please resolve the conflicts? |
I am having issues with my iOS Mobile Web Safari. The signup API is giving 429 errors abruptly. Finishing the checklist without that, will uploaded it may be later. |
Reviewer Checklist
|
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.
Waiting on uploading the mobile web safari screenshot and resolving merge conflicts. Meanwhile @francoisl All yours.
I am on it! @mananjadhav |
it seems like we removed logic for adding the onboarding videos in this issue |
🚧 @francoisl has triggered a test build. You can view the workflow run here. |
👍 I had no idea. Things change too fast 😂 |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Ok, tests well for me locally! Going to try to give it a quick run on Android once the ad-hoc build finishes, but so far I think it's ready to merge. |
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 well for me too, let's ship this 🚀
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Hi 👋🏼 Due to an unfortunate mistake, this code has been deleted from main, and this PR needs to be re-created from scratch. There are some additional details in slack here. Feel free to tag me in slack if you have questions. Apologies for the inconvenience 🙇🏼 |
here's the new PR #56141 @francoisl @mananjadhav |
@@ -3681,6 +3687,20 @@ function prepareOnboardingOptimisticData( | |||
if (['addAccountingIntegration', 'setupCategoriesAndTags'].includes(task.type) && !userReportedIntegration) { | |||
return false; | |||
} | |||
type SkipViewTourOnboardingChoices = 'newDotSubmit' | 'newDotSplitChat' | 'newDotPersonalSpend' | 'newDotEmployer'; |
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.
@ishpaul777 @mananjadhav I don't think this is a good place to declare type.
Explanation of Change
Fixed Issues
$ #53509
PROPOSAL:
Tests
Test 2
Offline tests
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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
Screen.Recording.2025-01-17.at.11.43.00.PM.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2025-01-18.at.3.24.19.AM.mov
iOS: mWeb Safari
Screen.Recording.2025-01-18.at.3.41.30.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2025-01-17.at.3.53.34.AM.mov
MacOS: Desktop
Screen.Recording.2025-01-30.at.1.26.58.AM.mov
Screen.Recording.2025-01-30.at.1.31.24.AM.mov