-
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
Update welcome note #38023
Update welcome note #38023
Conversation
@situchan can you give us an ETA on reviewing this PR please? |
ETA end of day |
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.
@dukenv0307 Spanish copy is missing. Please request on slack
And at least one platform screenshot with Spanish tested. And also screenshot of invite email |
@situchan we don't actually need for the welcome messages if they are being sent from Concierge/a guide, etc. @dukenv0307 if you need other screens translated, you can ask me and I'll get them. |
@situchan @danielrvidal I just updated Spanish and updated the screenshot in |
Sorry @dukenv0307, I think you might have misunderstood Daniel. Messages in the product that come from "Expensify" in a support capacity like Concierge, an account manager or a setup specialist etc are not translated into Spanish because we don't provide support in Spanish. |
@trjExpensify we have Spanish message in production. So now this should be unlocalized? Lines 2003 to 2004 in 0cb701c
|
@dukenv0307 do you mind showing what the email looks like when:
This screenshot looks to be incorrect, but I think it's because you are inputting information into the invite message: This is what the email should be:
And the bottom two lines (for invitation/workspace description) are dependent on 1) if they are present and 2) what the text of those inputs are. If you don't mind showing a mock-up/video up what it looks like with the suggestions above I can definitively tell you if there is an issue in the coding or if it was just because of what was input in the invitation message. |
@danielrvidal this is my error, thank you for your explanation. I will update the PR soon. |
@situchan @trjExpensify If we decide to remove welcomeNote, should we remove all welcomeNotes from the en and es files and then move welcomeNote to the const file? |
@danielrvidal can you give me some link examples or link formats here? |
|
I don't mind putting this directly in view component like copy on onboarding modal
Maybe it would be good to group concierge messages all together but out of scope |
@dukenv0307 here is an example of a workspace chat link: https://new.expensify.com/r/4656517140007919 It would just direct me to my workspace chat: Obviously for a new user, theirs would not be populated with any requests yet but it would drop them into their workspace chat. |
@situchan @dukenv0307 any update on this one? Let me know if you need anything else. |
@danielrvidal #38023 (comment) Can you verify my question here here? |
Sorry, what is BE in this reference? I'll be sure to get it answered asap. |
@danielrvidal I think @dukenv0307 is asking if we're getting the intended subject line for this message from the backend or not?
|
Yep, that is the email expected for existing users. It is really weird you are not getting the email for new users, though, as I seem to get it without any issues. You are not getting it regardless of the contents of the message and/or name of the workspace? I'm trying to figure out if any special characters may be causing issues |
@dukenv0307 @rayane-djouah can you follow those steps exactly, please? Let's get videos of going through that whole flow to verify that's we're all on the same page. Thanks! |
@trjExpensify My step here, I only receive the email about welcome to Expensify for new account. welcomeNote-1.mov |
@dukenv0307, after a bunch of testing, we have seen some inconsistent behavior on the staging backend when sending custom subject emails. I got them fine, Tom got them in some domains, and not in others... At this point, we are unsure of the cause, but it is unrelated to this issue, so we can move ahead with it, as it tests well for me. @situchan, can you check if it works for you too/approve and we go ahead and merge this? Alternatively, if you have access to emails in other domains, you can try inviting those 🤷 |
@Gonals, I tried with several temporary email providers to test different domains, and I removed any special characters from the workspace name, but I'm not receiving the email regardless of the contents of the message and/or name of the workspace. Recording.2024-04-12.151208.mp4 |
@dukenv0307, as we are now adding the email subject only in this PR, and we didn't change the default welcome message, lets update the test steps in the author checklist. Thanks! |
@Gonals, the code is LGTM, but I'm not able to test it. LMK if I should skip testing and approve anyway. in this case, we should add [No QA] to skip QA testing also. |
Let's just go ahead and approve this. I can QA it, so no need for the [No QA] label |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@Gonals, Could you please add |
✋ 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/Gonals in version: 1.4.63-0 🚀
|
This PR is failing because of issue #40445 The issue is reproducible in: all environments Bug6453359_1713429052596.Screen_Recording_2024-04-18_at_11.26.01_in_the_morning.1.mp4 |
@lanitochka17 this PR require a internal QA, @Gonals will QA it, see #38023 (comment) |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
@Gonals @dukenv0307 I tried to generate invite emails in the new format and couldn't get them to work. Are you able to get it to work?
Could we diagnose what is happening ASAP? If we're not sending invite emails for anyone being added, that is not great, and we should prioritize a fix ASAP. Here is a video of my flow: |
@danielrvidal Base on @Gonals 's comment here #38023 (comment) I can't test this. @Gonals Can you please take a look at this comment above? Thanks. |
Hey @dukenv0307, it looks like me not getting the email was from another issue, so this is all good. Thank you! |
Details
Fixed Issues
$ #37152
PROPOSAL: #37152 (comment)
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 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 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.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web.mov
web-es.mov
MacOS: Desktop
desktop.mov