App icon indicating copy to clipboard operation
App copied to clipboard

Workspace - The new WS names appear in English when Spanish is set up

Open vincdargento opened this issue 1 year ago β€’ 5 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.71-0 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5298436 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Open the app or go to: https://staging.expensify.com/
  2. Loggin
  3. Click on 'Settings' > 'Preferences' > 'Language' > 'Spanish'
  4. Click on 'Espacios de trabajo' > 'Nuevo espacio de trabajo'

Expected Result:

When a new WS is created, the name should be in Spanish.

Actual Result:

When a new WS is created, the name is in English.

Workaround:

Unknown

Platforms:

  • [ ] Android: Standalone
  • [ ] Android: HybridApp
  • [ ] Android: mWeb Chrome
  • [x] iOS: Standalone
  • [ ] iOS: HybridApp
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

bug

View all open jobs on GitHub

vincdargento avatar Dec 04 '24 20:12 vincdargento

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] avatar Dec 04 '24 20:12 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-12-05 01:16:03 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - The new WS names appear in English when Spanish is set up

What is the root cause of that problem?

  • We are directly returning the new workspace name string in english.

https://github.com/Expensify/App/blob/c049ac7ec616db1d936c021b08b17a35eb418fd3/src/libs/actions/Policy/Policy.ts#L1458-L1494

What changes do you think we should make in order to solve the problem?

  • We should create a translation in both languages which will receive the user name and return the workspace name accordingly. We would also need to update the regex a bit to get the last workspace number in both languages. The regex can be modified in the PR phase if needed. We might want to include username in the numberRegEx.
  • We also need to create translation for My Group Workspace.
EN: workspaceName: (userName: string, workspaceNumber?: number) => `${userName}'s Workspace${workspaceNumber ? ' ' + workspaceNumber : ''}`,
ES: workspaceName: (userName: string, workspaceNumber?: number) => `El espacio de trabajo${workspaceNumber ? ' ' + workspaceNumber : ''} de ${userName}`,

EN: myGroupWorkspace: 'My Group Workspace',
ES: myGroupWorkspace:"Mi Espacio de Trabajo en Grupo"
Pseudocode
/**
 * Generate a policy name based on an email and policy list.
 * @param [email] the email to base the workspace name on. If not passed, will use the logged-in user's email instead
 */
function generateDefaultWorkspaceName(email = ''): string {
    const emailParts = email ? email.split('@') : sessionEmail.split('@');
    if (!emailParts || emailParts.length !== 2) {
        return '';
    }

    const username = emailParts.at(0) ?? '';
    const domain = emailParts.at(1)?.toLowerCase() ?? '';
    const userDetails = PersonalDetailsUtils.getPersonalDetailByEmail(sessionEmail);
    const displayName = userDetails?.displayName?.trim();
    let displayNameForWorkspace = '';

    if (!PUBLIC_DOMAINS.some((publicDomain) => publicDomain === domain)) {
        displayNameForWorkspace = Str.UCFirst(domain.split('.').at(0) ?? '');
    } else if (displayName) {
        displayNameForWorkspace = Str.UCFirst(displayName);
    } else if (PUBLIC_DOMAINS.some((publicDomain) => publicDomain === domain)) {
        displayNameForWorkspace = Str.UCFirst(username);
    } else {
        displayNameForWorkspace = userDetails?.phoneNumber ?? '';
    }

    if (`@${domain}` === CONST.SMS.DOMAIN) {
        return Localize.translateLocal('workspace.new.myGroupWorkspace');
    }

    if (isEmptyObject(allPolicies)) {
        return Localize.translateLocal('workspace.new.workspaceName', displayNameForWorkspace);
    }

    // Dynamically include the username in the regex
    const escapedName = displayNameForWorkspace.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
    const englishRegex = new RegExp(`${escapedName}'s Workspace(?:\\s(\\d+))?$`, 'i');
    const spanishRegex = new RegExp(`El espacio de trabajo(?:\\s(\\d+))? de ${escapedName}$`, 'i');

    const workspaceNumbers = Object.values(allPolicies)
        .map((policy) => englishRegex.exec(policy?.name ?? '') || spanishRegex.exec(policy?.name ?? ''))
        .filter(Boolean) // Remove null matches
        .map((match) => Number(match?.[1] ?? '1'));

    const lastWorkspaceNumber = workspaceNumbers.length > 0 ? Math.max(...workspaceNumbers) : -Infinity;

    return lastWorkspaceNumber !== -Infinity
        ? Localize.translateLocal('workspace.new.workspaceName', displayNameForWorkspace, lastWorkspaceNumber + 1)
        : Localize.translateLocal('workspace.new.workspaceName', displayNameForWorkspace);
}ceNumber) : defaultWorkspaceName;
}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?


What alternative solutions did you explore? (Optional)

Result

https://github.com/user-attachments/assets/80dda7d9-4a10-40d5-876e-111cd52310cb

Krishna2323 avatar Dec 04 '24 21:12 Krishna2323

I think this is expected behavior, once a workspace is named it should remain static unless manually edited. I don't think changing the language should update the naming for existing workspaces. I am gonna close this as I don't think we should do anything here.

stephanieelliott avatar Dec 06 '24 02:12 stephanieelliott

@stephanieelliott Actually the issue is happening with new workspaces. When the language is set up as Spanish, new workspaces have the name in English, tester can still reproduce. Opening in case you change your mind with this knowledge

https://github.com/user-attachments/assets/7d7fc919-b879-4bec-930f-64640e241446

vincdargento avatar Dec 10 '24 18:12 vincdargento

Thanks @vincdargento! There is a similar issue being worked on here, asking to see if we're addressing WS names there or if we will need a separate PR

stephanieelliott avatar Dec 11 '24 00:12 stephanieelliott

Ok seems like this is a unique issue, adding labels so we can get this fixed.

stephanieelliott avatar Dec 13 '24 23:12 stephanieelliott

Job added to Upwork: https://www.upwork.com/jobs/~021867717521858784433

melvin-bot[bot] avatar Dec 13 '24 23:12 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

melvin-bot[bot] avatar Dec 13 '24 23:12 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - The new WS names appear in English when Spanish is set up

What is the root cause of that problem?

The function generateDefaultWorkspaceName does not support localized language. That's why all new workspace are in English only.

What changes do you think we should make in order to solve the problem?

Modify the function generateDefaultWorkspaceName to add the localized translated names using Localize.translateLocal.

Copies will be provided later by Internal team

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Add tests for generateDefaultWorkspaceName to test different scenarios.

What alternative solutions did you explore? (Optional)

NA

shubham1206agra avatar Dec 14 '24 04:12 shubham1206agra

@Krishna2323's proposal is looking good!

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed!

eVoloshchak avatar Dec 16 '24 21:12 eVoloshchak

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Dec 16 '24 21:12 melvin-bot[bot]

@eVoloshchak @MonilBhavsar I don't think the choice is correct here since the selected proposal did not fill the tests section, i.e., the proposal is incomplete.

See https://expensify.slack.com/archives/C01GTK53T8Q/p1733148961659549

shubham1206agra avatar Dec 17 '24 02:12 shubham1206agra

@eVoloshchak @stephanieelliott @MonilBhavsar this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Dec 18 '24 09:12 melvin-bot[bot]

@eVoloshchak thoughts on the above comment? I do agree we should include automated tests

MonilBhavsar avatar Dec 18 '24 18:12 MonilBhavsar

@MonilBhavsar, it's mostly translations addition, I don't think we ever add automated tests for translations. Additionally, the other proposal does not even talk about the regex update which is a must for adding the workspace number and it's totally up to us to add the translation, nothing special added in the other proposal. It just states:

Do you really think that's a great addition to the proposal or very essential?

Add tests for generateDefaultWorkspaceName to test different scenarios.

Krishna2323 avatar Dec 18 '24 18:12 Krishna2323

cc @MonilBhavsar

nothing special added in the other proposal. It just states:

Do you really think that's a great addition to the proposal or very essential?

Add tests for generateDefaultWorkspaceName to test different scenarios.

@Krishna2323 Well, your proposal is incomplete. I just completed my proposal.

Additionally, the other proposal does not even talk about the regex update which is a must for adding the workspace number

We would also need to update the regex a bit to get the last workspace number in both languages. The regex can be modified in the PR phase if needed. We might want to include username in the numberRegEx.

Are you talking about this? What kind of addition did you make in the proposal? What I see in your pseudocode is that you might have broke the numbering system. It will take into account for workspaces that are not created by the user. Plus, the logic was to avoid conflicts in naming. So, updating it is not necessary at all.

it's totally up to us to add the translation

What?? Isn't this the issue is about?

it's mostly translations addition, I don't think we ever add automated tests for translations

Quick search, and you can see a number of tests are present.

shubham1206agra avatar Dec 19 '24 02:12 shubham1206agra

Are you talking about this? What kind of addition did you make in the proposal? What I see in your pseudocode is that you might have broke the numbering system. It will take into account for workspaces that are not created by the user. Plus, the logic was to avoid conflicts in naming. So, updating it is not necessary at all.

FYI, I clearly mentioned that "We might want to include the username in the numberRegEx," and "avoiding conflicts in naming" means that the new workspace number should be one greater than the previous one. If we don't modify the regex, the number won't increment in Spanish. Please see the video below.

https://github.com/user-attachments/assets/8ffba550-a31c-447c-9f6a-6eea018432b2

@eVoloshchak, you can verify my proposal again. I have updated the regex slightly, as I clearly mentioned that we would need to update it in the PR phase, and I also added a result video.

Krishna2323 avatar Dec 19 '24 05:12 Krishna2323

Are you talking about this? What kind of addition did you make in the proposal? What I see in your pseudocode is that you might have broke the numbering system. It will take into account for workspaces that are not created by the user. Plus, the logic was to avoid conflicts in naming. So, updating it is not necessary at all.

FYI, I clearly mentioned that "We might want to include the username in the numberRegEx," and "avoiding conflicts in naming" means that the new workspace number should be one greater than the previous one. If we don't modify the regex, the number won't increment in Spanish. Please see the video below.

This is dependent on what translations we settle. Maybe we settle on different convention.

shubham1206agra avatar Dec 19 '24 06:12 shubham1206agra

This is dependent on what translations we settle. Maybe we settle on different convention.

It could be Espacio de trabajo ${number} de ${username} or Espacio espacio de trabajo ${number} de ${username}, which is why I mentioned making minor tweaks to the regex during the PR phase. For the word "workspace" in Spanish, we use 'Espacio de trabajo'. The current regex won't work in either case.

Krishna2323 avatar Dec 19 '24 11:12 Krishna2323

@eVoloshchak, @stephanieelliott, @MonilBhavsar Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Dec 20 '24 09:12 melvin-bot[bot]

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Dec 20 '24 16:12 melvin-bot[bot]

@eVoloshchak, @stephanieelliott, @MonilBhavsar 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] avatar Dec 24 '24 09:12 melvin-bot[bot]

@eVoloshchak, @stephanieelliott, @MonilBhavsar 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

melvin-bot[bot] avatar Dec 26 '24 09:12 melvin-bot[bot]

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Dec 27 '24 16:12 melvin-bot[bot]

@eVoloshchak any thoughts here?

MonilBhavsar avatar Dec 27 '24 21:12 MonilBhavsar

@eVoloshchak, @stephanieelliott, @MonilBhavsar 12 days overdue now... This issue's end is nigh!

melvin-bot[bot] avatar Dec 30 '24 09:12 melvin-bot[bot]

@eVoloshchak @stephanieelliott @MonilBhavsar this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] avatar Jan 01 '25 09:01 melvin-bot[bot]

This issue has not been updated in over 14 days. @eVoloshchak, @stephanieelliott, @MonilBhavsar eroding to Weekly issue.

melvin-bot[bot] avatar Jan 02 '25 09:01 melvin-bot[bot]

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] avatar Jan 03 '25 16:01 melvin-bot[bot]

Bump on this @eVoloshchak, can you please weigh in on the above?

stephanieelliott avatar Jan 06 '25 23:01 stephanieelliott