Skip to content
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

Merged
merged 69 commits into from
Feb 8, 2024
Merged

Conversation

tienifr
Copy link
Contributor

@tienifr tienifr commented Oct 20, 2023

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 to errorText and add isTranslated: 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.

  1. Go to Settings >> Profile >> Contact method
  2. Add a new contact method
  3. Press Verify in validate code page without typing anything
  4. Verify Please enter your magic code error appears
  5. Type in one digit only >> Verify
  6. Verify Incorrect magic code error appears
  7. Type in an arbitrary magic code >> Verify
  8. Verify Incorrect or invalid magic code. Please try again or request a new code. error appears
  9. Delete one digit
  10. Verify the error disappears

AddressForm

  1. Go to Settings >> Profile >> Personal details >> Address
  2. Verify the hint under Zip / Postcode shows the correct Zip example for your locale

AvatarWithImagePicker

  1. Go to Settings
  2. Press avatar >> Upload photo
  3. Upload an invalid extension file (e.g. pdf)
  4. Verify error shows Profile picture must be one of the following types: jpg, jpeg, png, gif, bmp, svg.
  5. Upload a very large image in size (> 6MB)
  6. Verify error shows The selected image exceeds the maximum upload size of 6MB.
  7. Upload a very large image in resolution (> 4096x4096px)
  8. Verify error shows Please upload an image larger than 80x80 pixels and smaller than 4096x4096 pixels.

DistanceRequest & IOURequestStepDistance

  1. Create a distance request with duplicated waypoints
  2. Verify error shows Please enter at least two different addresses
  3. Edit a distance request with new duplicated waypoints
  4. Verify step 2

MoneyRequestConfirmationList & MoneyTemporaryForRefactorRequestConfirmationList

  1. Update the rate for a workspace (Workspace >> Settings >> Reimbursements >> Track distance) to very high (e.g., 90000000)
  2. Create a distance request for that workspace
  3. Verify error shows Invalid amount
  4. Create a manual request without merchant
  5. Verify error shows Please enter a correct merchant.

BaseOptionsSelector

  1. Press FAB >> Start chat >> Chat
  2. In search input, type text with more than 500 letters
  3. Verify error shows Character limit exceeded (515/500)
  4. Remove some letters to reach 500
  5. Verify error disappears

PDFPasswordForm

  1. Send a protected PDF
  2. Open that file >> Press enter the password
  3. Do not type >> Confirm
  4. Verify error shows Please enter a password
  5. Type an incorrect password >> Confirm
  6. Verify error shows Incorrect password. Please try again.

TimePicker & StatusClearAfterPage

  1. Go to Settings >> Profile >> Status >> Clear after >> Custom >> Time
  2. Select a time in the past >> Save
  3. Verify error shows Please choose a time at least one minute ahead.

NewChatPage & SearchPage & MoneyTemporaryForRefactorRequestParticipantsSelector

  1. Toggle on Offline mode
  2. Press FAB >> Start chat
  3. Verify message shows You appear to be offline. Search results are limited.
  4. Press Search bar
  5. Verify step 3
  6. Create a manual money request
  7. Verify step 3 in participant selector step

CompanyStep

  1. Go to any Workspace settings >> Card >> Connect bank account >> Connect manually
  2. Go to Company information step
  3. Verify message under Company address field shows PO boxes and mail drop addresses are not allowed
  4. Type an emoji in Legal business name
  5. Verify error shows Please enter a valid legal business name
  6. Type an invalid Zip code
  7. Verify error shows Incorrect zip code format. Acceptable format: 12345, 12345-1234, 12345 1234
  8. Manually modify here to REQUESTOR step
  9. Type text in Last 4 digits of SSN
  10. Verify error shows Please enter valid last 4 digits of SSN

MoneyRequestAmountForm

  1. Create a money request with amount 0
  2. Verify error shows Please enter a valid amount before continuing.

TwoFactorAuthForm

  1. Go to Settings >> Security >> Two-factor authentication
  2. Without copy or download recovery code, press Next
  3. Verify error shows Please copy or download codes before continuing.
  4. Copy the code then press Next
  5. Without filling 2FA code, press Next
  6. Verfiy error shows Please enter your two-factor authentication code
  7. Fill incorrect 2FA code
  8. Verify error shows Invalid Two Factor Authentication code. Please re-enter the code from your authentication app.

SignIn

  1. Logout
  2. Type an invalid email in Phone or email
  3. Verify error shows The email entered is invalid. Please fix the format and try again.
  4. Sign in with invalid magic code
  5. Verify error shows Incorrect magic code. Please try again or request a new code.
  6. Close an account
  7. Verify message with green dot shows Account closed successfully

Task

  1. Toggle on offline mode
  2. Press FAB >> Assign task
  3. Without filling anything, press Next
  4. Verify error shows Please enter a title
  5. Fill in a title >> Next
  6. Do not select assignee
  7. Verify error shows Please select with whom you want to share.
  8. Press Share somewhere
  9. Verify message shows You appear to be offline. Search results are limited.

WorkspaceMembersPage

Read here: #23702 (comment).

ExpensifyCardPage

  1. Mock data set in console:
Onyx.set(`cardList`, {
    '2': {
        cardID: 234523452345,
        state: 3,
        bank: 'Expensify',
        availableSpend:10000,
        domainName: 'Expensify',
        lastFourPAN: '',
        isVirtual: true,
        cardholderFirstName: "Test",
        cardholderLastName: "Test",
    },
    '3': {
        cardID: 234523452345,
        state: 3,
        bank: '1000',
        availableSpend:10000,
        domainName: 'Expensify',
        lastFourPAN: '2345',
        isVirtual: false,
    },
});
  1. Go to /settings/wallet/card/Expensify
  2. Press Reveal details
  3. Verify error shows An error occurred while loading the card details. Please check your internet connection and try again.
  4. Mock fraud card
Onyx.merge(`cardList`, {
    '2': {
        fraud: 'domain',
    },
});
  1. Verify error shows Your card is temporarily locked while our team reviews your company's account.

ReportCardLostPage

  1. Press Report physical card lost / damaged
  2. Do not choose a reason >> Next
  3. Verify error shows Reason is required

TransferBalancePage

  1. Mock Onyx data:
Onyx.merge("bankAccountList", {
  '1': {
      "title": `Generic bank Checking`,
      "description": "Account ending in 5087",
      "methodID": '1',
      "key": `bankAccount-1`,
      "accountType": "bankAccount",
      "accountData": {
          "accountNumber": "XXXXX5087",
          "additionalData": {
              "bankName": `Generic bank`,
          },
          "state": "OPEN",
          "type": "BUSINESS"
      },
      "isDefault": false
  },
  '2': {
      "title": `Generic bank Checking 2`,
      "description": "Account ending in 5123",
      "methodID": '2',
      "key": `bankAccount-2`,
      "accountType": "bankAccount",
      "accountData": {
          "accountNumber": "XXXXX5087",
          "additionalData": {
              "bankName": `Generic bank`,
          },
          "state": "OPEN",
          "type": "BUSINESS"
      },
      "isDefault": false
  }
});

Onyx.merge('fundList', {
  "4": {
      "title": "Test card",
      "description": "Card ending in 4242",
      "methodID": 4,
      "key": "card-4",
      "accountType": "debitCard",
      "accountData": {
      "additionalData": {
          "isBillingCard": false,
          "isP2PDebitCard": true
      },
      "addressName": "Test cardList card",
      "addressState": "US",
      "addressStreet": "Street",
      "addressZip": 75092,
      "cardMonth": 12,
      "cardNumber": "424242XXXXXX4242",
      "cardYear": 2026,
      "created": "2023-01-13 22:27:23",
      "currency": "USD",
      "fundID": 4
      },
      "isDefault": false
  },
  "5": {
      "title": "Test card 2",
      "description": "Card ending in 0000",
      "methodID": 5,
      "key": "card-5",
      "accountType": "debitCard",
      "accountData": {
      "additionalData": {
          "isBillingCard": false,
          "isP2PDebitCard": true
      },
      "addressName": "Nowhere",
      "addressState": "US",
      "addressStreet": "Street",
      "addressZip": 75092,
      "cardMonth": 12,
      "cardNumber": "424242XXXXXX0000",
      "cardYear": 2026,
      "created": "2023-01-13 22:27:23",
      "currency": "USD",
      "fundID": 5
      },
      "isDefault": false
  }
});

Onyx.merge('walletTransfer', {
    selectedAccountID: '1',
    selectedAccountType: 'bankAccount',
    filterPaymentMethodType: 'bankAccount',
    paymentMethodType: 'bankAccount',
});
  1. Change this line to shouldShow={false}
  2. Change this line to isDisabled={false}
  3. Go to settings/wallet/transfer-balance
  4. Press Transfer
  5. Verify error shows Auth TransferWalletBalance returned an error

UnlinkLoginForm

  1. Go to Settings >> Profile >> Contact Method >> New contact method
  2. Add a new contact method B and don't verify it
  3. Login with B in a new tab
  4. Press Unlink
  5. Verify Link Sent! appears and translates dynamically with language preference in the footer

OnfidoPrivacy

  1. Mock Onyx data:
Onyx.merge('walletOnfido', {
    errors: { 1660306327299659: "Sorry we're unable to verify your identity. Please fix the errors before continuing." },
    fixableErrors: [
        "There's an issue with your selfie/video. Please upload a new selfie/video in real time."
    ]
});
  1. Modify this step here to const currentStep = CONST.WALLET.STEP.ONFIDO
  2. Go to settings/wallet/enable-payments
  3. Verify error shows 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.
  • Verify that no errors appear in the JS console

Offline tests

NA

QA Steps

  1. Go to Settings >> Profile >> Contact method
  2. Add a new contact method
  3. Press Verify in validate code page without typing anything
  4. Verify Please enter your magic code error appears
  5. Type in one digit only >> Verify
  6. Verify Incorrect magic code error appears
  7. Type in an arbitrary magic code >> Verify
  8. Verify Incorrect or invalid magic code. Please try again or request a new code. error appears
  9. Delete one digit
  10. Verify the error disappears
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
      • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Screenshot_1707370007

Screenshot_1707370021
Screenshot_1707376451
Screenshot_1707376456
Screenshot_1707376664
Screenshot_1707376667
Screenshot_1707376673
Screenshot_1707376693
Screenshot_1707376968
Screenshot_1707376987
Screenshot_1707377009
Screenshot_1707377043
Screenshot_1707377087
Screenshot_1707377097
Screenshot_1707377162
Screenshot_1707377168
Screenshot_1707377276
Screenshot_1707377285
Screenshot_1707377291
Screenshot_1707377313
Screenshot_1707377318

Android: mWeb Chrome

Screenshot_1707377686

Screenshot_1707377730
Screenshot_1707377747
Screenshot_1707377755
Screenshot_1707377777
Screenshot_1707377797
Screenshot_1707377827
Screenshot_1707377830
Screenshot_1707377851
Screenshot_1707377925
Screenshot_1707377957
Screenshot_1707377961
Screenshot_1707377997
Screenshot_1707378005
Screenshot_1707378054
Screenshot_1707378069
Screenshot_1707378077

iOS: Native

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 20 51

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 21 02

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 22 30

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 22 37

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 23 15

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 23 28

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 23 51

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 24 21

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 32 10
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 32 13
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 33 26
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 34 27
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 35 05
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 35 30
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 35 42
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 36 18
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 36 44
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 36 51
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 37 23
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 37 35
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 40 06
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 40 19
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 41 28
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 12 44 25

iOS: mWeb Safari

Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 13 50 59
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 13 51 37
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 13 51 51
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 13 52 00
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 13 52 45
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 13 53 44
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 00 00
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 00 02
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 01 10
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 01 28
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 02 19
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 02 30
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 02 48
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 03 04
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 03 12
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 04 03
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 04 41
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 04 48
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 05 13
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 05 18
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 07 17
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 07 21
Simulator Screenshot - iPhone 15 Pro Max - 2024-02-08 at 14 08 08

MacOS: Chrome / Safari
translation-web-compressed.mov
Screenshot 2024-01-31 at 18 59 27 Screenshot 2024-01-31 at 19 03 27 Screenshot 2024-02-02 at 20 05 46 Screenshot 2024-02-02 at 20 30 30 Screenshot 2024-02-02 at 20 46 35 Screenshot 2024-02-02 at 20 47 42 Screenshot 2024-02-03 at 00 12 03 Screenshot 2024-02-08 at 08 44 51 Screenshot 2024-02-08 at 08 42 38 Screenshot 2024-02-08 at 08 41 27 Screenshot 2024-02-08 at 08 37 01 Screenshot 2024-02-08 at 08 47 45 Screenshot 2024-02-08 at 09 13 59 Screenshot 2024-02-08 at 09 04 13 Screenshot 2024-02-08 at 09 02 41 Screenshot 2024-02-08 at 09 21 34 Screenshot 2024-02-08 at 09 17 02 Screenshot 2024-02-08 at 11 23 57 Screenshot 2024-02-08 at 11 21 24 Screenshot 2024-02-08 at 11 12 59 Screenshot 2024-02-08 at 11 46 29 Screenshot 2024-02-08 at 11 45 38 Screenshot 2024-02-08 at 11 44 49 Screenshot 2024-02-08 at 11 33 39 Screenshot 2024-02-08 at 11 54 27 Screenshot 2024-02-08 at 11 52 01 Screenshot 2024-02-08 at 11 50 16 Screenshot 2024-02-08 at 12 08 20 Screenshot 2024-02-08 at 11 58 43
MacOS: Desktop Screenshot 2024-01-31 at 18 59 27 Screenshot 2024-01-31 at 19 03 27 Screenshot 2024-02-02 at 20 05 46 Screenshot 2024-02-02 at 20 30 30 Screenshot 2024-02-02 at 20 46 35 Screenshot 2024-02-02 at 20 47 42 Screenshot 2024-02-03 at 00 12 03 Screenshot 2024-02-08 at 08 44 51 Screenshot 2024-02-08 at 08 42 38 Screenshot 2024-02-08 at 08 41 27 Screenshot 2024-02-08 at 08 37 01 Screenshot 2024-02-08 at 08 47 45 Screenshot 2024-02-08 at 09 13 59 Screenshot 2024-02-08 at 09 04 13 Screenshot 2024-02-08 at 09 02 41 Screenshot 2024-02-08 at 09 21 34 Screenshot 2024-02-08 at 09 17 02 Screenshot 2024-02-08 at 11 23 57 Screenshot 2024-02-08 at 11 21 24 Screenshot 2024-02-08 at 11 12 59 Screenshot 2024-02-08 at 11 46 29 Screenshot 2024-02-08 at 11 45 38 Screenshot 2024-02-08 at 11 44 49 Screenshot 2024-02-08 at 11 33 39 Screenshot 2024-02-08 at 11 54 27 Screenshot 2024-02-08 at 11 52 01 Screenshot 2024-02-08 at 11 50 16 Screenshot 2024-02-08 at 12 08 20 Screenshot 2024-02-08 at 11 58 43

@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2023

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@tienifr tienifr marked this pull request as ready for review October 23, 2023 02:17
@tienifr tienifr requested a review from a team as a code owner October 23, 2023 02:17
@melvin-bot melvin-bot bot requested review from 0xmiros and removed request for a team October 23, 2023 02:18
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

@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]

@tienifr
Copy link
Contributor Author

tienifr commented Oct 23, 2023

@0xmiroslav For the issue with @expensify.com email in the original OP, I really don't know how to reproduce. Can you give a hint of that? Moreover, do we need to test server errors for all the changed components? Because I'm not sure how to reproduce server errors for each of them.

@0xmiros
Copy link
Contributor

0xmiros commented Oct 23, 2023

This cannot be tested externally.
For dev testing, manually set this value to true:

if (userEmail.includes(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN)) {

Moreover, do we need to test server errors for all the changed components? Because I'm not sure how to reproduce server errors for each of them.

Not all but test as much as possible

@mountiny
Copy link
Contributor

Also got this when trying to send money and it failed, lets make sure these are covered too
image

@0xmiros
Copy link
Contributor

0xmiros commented Oct 25, 2023

@tienifr
Copy link
Contributor Author

tienifr commented Oct 25, 2023

Thanks for all. I'm investigating and found a lot of places where the phrase key is not passed properly. 25 components so far.

@0xmiros
Copy link
Contributor

0xmiros commented Oct 30, 2023

@tienifr is ready for review?

@tienifr
Copy link
Contributor Author

tienifr commented Oct 30, 2023

Yes, it's ready for review.

@0xmiros
Copy link
Contributor

0xmiros commented Nov 12, 2023

Please fix conflict

@0xmiros
Copy link
Contributor

0xmiros commented Nov 17, 2023

@tienifr bump

@0xmiros
Copy link
Contributor

0xmiros commented Nov 21, 2023

Still conflicts

@tienifr
Copy link
Contributor Author

tienifr commented Feb 2, 2024

@situchan I have added detailed test steps. Please continue reviewing.

@iwiznia
Copy link
Contributor

iwiznia commented Feb 2, 2024

Just found this PR...
Is this PR going to fix all issues of missing translation?
Also, did you see this thread (and more about it in https://expensify.slack.com/archives/C01GTK53T8Q/p1705512051876189?thread_ts=1705511331.469849&cid=C01GTK53T8Q)? And do you agree that we should get rid of the method translateIfPhraseKey?

@marcaaron
Copy link
Contributor

Is this PR going to fix all issues of missing translation

I am interpreting this question as - we should stop showing MISSING_TRANSLATION entirely?

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 MISSING_TRANSLATION for any server errors moving forward. But maybe @tienifr can confirm.

@marcaaron
Copy link
Contributor

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 isTranslated requirement then maybe we would end up with far fewer changes (and probably less conflicts... which seems to be at least one contributing factor to the delays here).

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:

  • Remove the translateIfPhraseKey() method
  • Use translate() directly
  • In translate() if we do not find the phrase key then:
  • Log an ensure bugbot alert if on staging or production
  • Assume the phrase is not supposed to be translated and show whatever we sent the user
  • Throw error on dev (so we can avoid this ever getting to staging)

?

@blazejkustra @mountiny since you all implemented the MISSING_TRANSLATION stuff as part of this issue can you help us understand what would it take to unwind all this complexity? Can someone either validate the above proposal or create a new one to get this back on the right track?

@blazejkustra
Copy link
Contributor

@blazejkustra @mountiny since you all implemented the MISSING_TRANSLATION stuff as part of #27759 can you help us understand what would it take to unwind all this complexity? Can someone either validate the above proposal or create a new one to get this back on the right track?

I wasn't involved in this actually, I believe I only migrated Localize file to typescript. PR that introduced "MISSING TRANSLATION", cc @robertKozik @dukenv0307

@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

@marcaaron The proposed plan sounds good to me.

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).

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.

@marcaaron
Copy link
Contributor

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 translateIfPhraseKey()? Sound good?

@marcaaron marcaaron self-requested a review February 5, 2024 19:23
@tienifr
Copy link
Contributor Author

tienifr commented Feb 6, 2024

Conflicts resolved.

@mallenexpensify
Copy link
Contributor

What's the next step to get this merged?

@situchan
Copy link
Contributor

situchan commented Feb 7, 2024

@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.

@tienifr
Copy link
Contributor Author

tienifr commented Feb 8, 2024

There's one case that I'm not sure of:

// If an error is returned as part of the route, ensure we set it in the onyxData for the account
if (error) {
Session.setAccountError(error);
}

What would this error look like? Could you give an example?
cc @NikkiWines as you worked on that PR.

@tienifr
Copy link
Contributor Author

tienifr commented Feb 8, 2024

@situchan Added remaining page test steps and screenshots. The others which are not listed are only type-changed (from string to MaybePhraseKey) so I think we don't need to add them there.

@situchan
Copy link
Contributor

situchan commented Feb 8, 2024

@tienifr please check slack discussion in case you missed.

@tienifr
Copy link
Contributor Author

tienifr commented Feb 8, 2024

Sure, adding screenshots for other platforms, though I don't think that's neccessary because those changes are not platfomr-dependent.

@tienifr
Copy link
Contributor Author

tienifr commented Feb 8, 2024

@situchan I added screenshots for all platforms.

@marcaaron
Copy link
Contributor

though I don't think that's neccessary because those changes are not platfomr-dependent

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.

@Expensify Expensify deleted a comment from 0xmiros Feb 8, 2024
@situchan
Copy link
Contributor

situchan commented Feb 8, 2024

Test Result

1-zip-hint 2-avatar-upload 3-distance-dup 4-receipt-upload 5-request-confirm 6-offline-feedback 7-search-exceed 8-pdf-password 9-time-picker 10-search-offline 11-vba1 12-vba2 13-amount-input 14-participant-selector 15-contact-methods 16-status-clear 17-tfa-code 18-debit-card 19-new-task 20-task-share 21-members-primary

@NikkiWines
Copy link
Contributor

What would this error look like? Could you give an example?

There are a number of different errors that could be returned but here's an example of one:

image

@marcaaron marcaaron merged commit 067bfa8 into Expensify:main Feb 8, 2024
30 checks passed
@OSBotify
Copy link
Contributor

OSBotify commented Feb 8, 2024

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.4.40-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 failure ❌

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.40-5 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 failure ❌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants