-
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
[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
@getusha That pesky console error keeps popping back up and is on the back end. Let's not hold on that. |
Not getting emails for this flow anymore ^, i got one the first time which didn't look right @deetergp Edit: same with rooms, i tried with different account as well |
Hm, it looks a bit weird. @danielrvidal I'll defer to you though. |
@getusha For DM & Group Chats, did you add a comment in either chat? |
@getusha good find. I think we should remove the "Hey ✋" at the beginning of the admin copy and just start with "As an admin..." Let's please make that update. |
@getusha To elaborate on my previous comment, for DMs & Group Chats, you might need to add a comment in the chat as the inviting user after inviting the new user, then wait for the normal "notify offline users of activity" time to elapse, which is about 10 minutes. They should get sent immediately, but we have a regression that is preventing that from happening, but it shouldn't hold this PR up. If you do the above and still do not get an invitation, then that's a new regression… But they should send. |
@getusha Could you finalize Reviewer Checklist? I think we are good to merge this 😄 |
@getusha I just went through testing DMs & Group Chat invites and in both cases, I had to add a comment after inviting the new user and then wait ten minutes in order for it to send a notification, but in both cases it sent. In the case of the DM, the link worked and the onboarding messages were created. In the case of Group Chat the link took me to a "Magic code expired" page, which is a regression, but if I signed in again as the user, requesting a new magic code, the onboarding messages were sent. The regression should not hold us up and will be dealt with separately, let's get this merged! |
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.
LGTM, thanks!
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.
Woohoo! LGTM 🎉
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/deetergp in version: 9.0.62-0 🚀
|
@IuliiaHerets These are known issues, check the comment above: #46052 (comment) @deetergp I'm guessing both are backend issues, so please handle it internally. If you need help on the FE side, let me know 🙇 |
@IuliiaHerets Both issues are known, though I don't think we had GHs for them till now, so thanks for that. We will definitely want to add regression tests for both of them. But for now, use the workarounds from the comment @blazejkustra mentioned to verify that the onboarding messages are sent. |
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.62-4 🚀
|
|
||
const isInviteOnboardingComplete = introSelected?.isInviteOnboardingComplete ?? false; | ||
|
||
if (introSelected && !isInviteOnboardingComplete) { |
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.
We should had also checked if the onboarding has been completed, this missed implementation caused the messages to be duplicated. This caused #52543
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