-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Guided Setup: Stage 3] Send the Onboarding Message After Sign-In #46052
base: main
Are you sure you want to change the base?
[Guided Setup: Stage 3] Send the Onboarding Message After Sign-In #46052
Conversation
merge main into @cdOut/guided-3-onboarding
merge main into @cdOut/guided-3-onboarding
👋 @getusha can you review this one today, please? Thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeWorkspace Admin screen-recording-2024-09-26-at-45054-in-the-afternoon_GAIw0CSx.mp4Workspace Non-Admin Screen.Recording.2024-09-26.at.4.58.14.in.the.afternoon.moviOS: NativeWorkspace Admin Screen.Recording.2024-09-26.at.5.11.22.in.the.afternoon.movMacOS: Chrome / SafariWorkspace Non-Admin screen-recording-2024-09-25-at-101910-in-the-morning_QRIpKXNH.mp4Workspace Admin Screen.Recording.2024-09-25.at.6.37.21.in.the.evening.movUser-created Room / DM / Group Chat: |
src/CONST.ts
Outdated
@@ -4297,7 +4332,7 @@ const CONST = { | |||
type: 'meetGuide', | |||
autoCompleted: false, | |||
title: 'Meet your setup specialist', | |||
description: ({adminsRoomLink}: {adminsRoomLink: 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.
Why is 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.
@kosmydel who originally worked on this wanted to have better typescript checks for the message types so this has been removed in favor of adding a satisfies Record<OnboardingPurposeType, OnboardingMessageType>
to the whole message CONST definition just to be clear that the values defined there are correct.
Not able to get an invitation email after inviting new member to a workspace, is it working for you? @cdOut |
@getusha I've cleaned up the code after your review. I did not encounter an issue with receiving the invitation emails, no. Did you check after by logging into the invited account if they are even being properly added to said workspace? |
@getusha let me know what email address you used to invite and invited. |
@deetergp should the automated tests for it be added here or handled separately?
Also I'm fairly sure all the cases are handled in test steps, I've separated them into three different sections. |
merge main into @cdOut/guided-3-onboarding
Sorry, attached a wrong video. this is the link.
I think it should've been something like this to work: |
This PR hasn't touched emails has it? 🤔 |
It has not, but for whatever reason we send two types of links when inviting new users. One is in the form of Still, this doesn't block testing and at best is just a slight inconvenience. My advice would be to swap to use staging backend for testing and just retry until you get a valid replacable url for local dev. |
@cdOut I signed in via an invite link on Android native, but the video isn't loading. Is this something we can fix here?
|
If you organically sign-up and choose "Manage my team's expenses" - does the video load in that flow on Android native? |
@getusha Would you be able to get the cURL and requestID so Scott could check what kinda data is being sent back over to you? |
@deetergp all flows send both types of urls, this isn't flow-dependant. @getusha could you specify if this only happens on android and check whether the specific flow you used on android works for desktop / web or does it also return broken video? Also try completing the tasks, do they complete successfully when pressing the checkbox or are they also broken? Is that just a problem with the video? |
I was also able to reproduce on iOS. i'll check on other platforms as well. |
@getusha is that happening for one specific case or just in general? What are the steps to reproduce? |
@cdOut happens for if the account role is set to admin. |
Works well on web, and it seems like it's broken it failed several times after checking the tasks off. Screen.Recording.2024-09-30.at.9.40.17.at.night.mov |
@getusha Could you retrieve the |
|
@cdOut are you not able to reproduce it? |
@getusha all the issues we've had for the backend were resolved today, I've tested it on my end and it runs properly for all cases. Ready for your hopefully final re-test! |
@getusha Please prioritize this review so we can get this bad boy merged 🙏 |
@cdOut this is the only message i am seeing from concierge Screen.Recording.2024-10-25.at.12.20.58.in.the.afternoon.mov |
Alright let me take a look at it, any specific case? |
I've asked about the issue in this thread on slack, seems like it's a backend issue once again as we don't have any way of modifying the email messages on frontend. |
Details
Added logic for sending onboarding messages through concierge after sign-in through a magic link sent via email for multiple cases.
Fixed Issues
$ #45044
PROPOSAL:
Tests
Workspace Non-admin:
Workspace Admin:
DM / Group Chat:
Start a chatis checked off & struck through.User-created Room:
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 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
MacOS: Chrome / Safari
Workspace Admin
WEB.WORKSPACE.ADMIN.low.res.mov
Workspace Non-Admin
WEB.WORKSPACE.NON-ADMIN.low.res.mov