-
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
Fix: Missing translation for server errors #30073
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
@0xmiroslav 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] |
@0xmiroslav For the issue with |
This cannot be tested externally. App/src/libs/Localize/index.js Line 93 in 7bbcda6
Not all but test as much as possible |
@tienifr also test this case - https://expensify.slack.com/archives/C049HHMV9SM/p1697926650688359 |
Thanks for all. I'm investigating and found a lot of places where the phrase key is not passed properly. 25 components so far. |
@tienifr is ready for review? |
Yes, it's ready for review. |
Please fix conflict |
@tienifr bump |
Still conflicts |
@situchan I have added detailed test steps. Please continue reviewing. |
Just found this PR... |
I am interpreting this question as - we should stop showing If so, I agree with that 100% and I am very confused about why we ever did this in the first place. But assuming for now that the purpose of this PR is to fix whatever ones are showing up as broken in staging. I asked a similar question here And this was the response: #30073 (comment) Which makes it sound like this PR will fix it so that we won't show |
Looking at @iwiznia's comment and how much of a struggle this PR has been. Should we maybe pause? If we can get rid of the It sounds like we implemented a very convoluted solution to handle translated server error messages (I thought we decided to not translate those for now - but unsure when that changed). I really did not anticipate these changes would touch so many files or take this long to fix 😄 So, maybe we should cut our losses on this one and raise a PR that does this instead:
? @blazejkustra @mountiny since you all implemented the |
I wasn't involved in this actually, I believe I only migrated Localize file to typescript. PR that introduced "MISSING TRANSLATION", cc @robertKozik @dukenv0307 |
@marcaaron The proposed plan sounds good to me.
The core idea behind this was mainly stemming from going through the Violations doc, where we wanted to also translate the violations in the production. We never wanted to translate in server, just to be clear. But this could raise a problem where we are sending the translation key from the server for some violation and it might not exist in the app. Then showing the MISSING TRANSLATION would allow us internally catch any of these issues way earlier. However, what we did not expect is that there was already a myriad of places where the translate method has been used wrong (passing a full error message to translate method where we expect a key for example) so instead of this showing rarely such that we can address each instance easily, this ended up showing to internal users in many cases leading to confusion. So I would say your proposed solution to Log an issue is good and we should also try to translate anything we can at its "root". So we are always passing already translated string (if it can be translated) and no component then has to guess if the incoming string is key for translation or already translated phrase. |
Thanks for the feedback here (@blazejkustra sorry for that distraction 😄). Let's proceed as originally planned for now then. @tienifr Can you fix the conflicts? Let's get this one merged and do a follow up to see the best way to remove |
Conflicts resolved. |
What's the next step to get this merged? |
@tienifr still many cases are missing in Tests step. What I pointed out was just hard to test. Please also add remaining what I didn't mention. |
There's one case that I'm not sure of: App/src/pages/LogInWithShortLivedAuthTokenPage.tsx Lines 47 to 50 in 9f37d3e
What would this error look like? Could you give an example? |
@situchan Added remaining page test steps and screenshots. The others which are not listed are only type-changed (from |
@tienifr please check slack discussion in case you missed. |
Sure, adding screenshots for other platforms, though I don't think that's neccessary because those changes are not platfomr-dependent. |
@situchan I added screenshots for all platforms. |
Sorry, that's not what the checklist is asking for. We expect everyone to test on all platforms unless the change is platform-dependent e.g. something that only affects Android is an exception and can be tested on Android exclusively. |
✋ 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/marcaaron in version: 1.4.40-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.40-5 🚀
|
Details
Server errors are already translated so App can't find the translation key and displays
Missing translation
for@expensify.com
emails. This PR modifes to pass translation key directly toerrorText
and addisTranslated: true
to already translated texts.Fixed Issues
$ #29712
PROPOSAL: #29712 (comment)
Tests
For every error message below, try to switch language from another device and verify the message changes accordingly.
Please enter your magic code
error appearsIncorrect magic code
error appearsIncorrect or invalid magic code. Please try again or request a new code.
error appearsAddressForm
Zip / Postcode
shows the correct Zip example for your localeAvatarWithImagePicker
pdf
)Profile picture must be one of the following types: jpg, jpeg, png, gif, bmp, svg.
The selected image exceeds the maximum upload size of 6MB.
Please upload an image larger than 80x80 pixels and smaller than 4096x4096 pixels.
DistanceRequest
&IOURequestStepDistance
Please enter at least two different addresses
MoneyRequestConfirmationList
&MoneyTemporaryForRefactorRequestConfirmationList
Invalid amount
Please enter a correct merchant.
BaseOptionsSelector
Character limit exceeded (515/500)
PDFPasswordForm
Please enter a password
Incorrect password. Please try again.
TimePicker
&StatusClearAfterPage
Please choose a time at least one minute ahead.
NewChatPage
&SearchPage
&MoneyTemporaryForRefactorRequestParticipantsSelector
You appear to be offline. Search results are limited.
CompanyStep
PO boxes and mail drop addresses are not allowed
Please enter a valid legal business name
Incorrect zip code format. Acceptable format: 12345, 12345-1234, 12345 1234
REQUESTOR
stepPlease enter valid last 4 digits of SSN
MoneyRequestAmountForm
Please enter a valid amount before continuing.
TwoFactorAuthForm
Please copy or download codes before continuing.
Please enter your two-factor authentication code
Invalid Two Factor Authentication code. Please re-enter the code from your authentication app.
SignIn
The email entered is invalid. Please fix the format and try again.
Incorrect magic code. Please try again or request a new code.
Account closed successfully
Task
Please enter a title
Please select with whom you want to share.
You appear to be offline. Search results are limited.
WorkspaceMembersPage
Read here: #23702 (comment).
ExpensifyCardPage
/settings/wallet/card/Expensify
An error occurred while loading the card details. Please check your internet connection and try again.
Your card is temporarily locked while our team reviews your company's account.
ReportCardLostPage
Reason is required
TransferBalancePage
shouldShow={false}
isDisabled={false}
settings/wallet/transfer-balance
Auth TransferWalletBalance returned an error
UnlinkLoginForm
Link Sent!
appears and translates dynamically with language preference in the footerOnfidoPrivacy
const currentStep = CONST.WALLET.STEP.ONFIDO
settings/wallet/enable-payments
Sorry we're unable to verify your identity. Please fix the errors before continuing. There's an issue with your selfie/video. Please upload a new selfie/video in real time.
Offline tests
NA
QA Steps
Please enter your magic code
error appearsIncorrect magic code
error appearsIncorrect or invalid magic code. Please try again or request a new code.
error appearsPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
translation-web-compressed.mov
MacOS: Desktop