-
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
[$250] Workspace - The new WS names appear in English when Spanish is set up #53599
Comments
Triggered auto assignment to @stephanieelliott ( |
Edited by proposal-police: This proposal was edited at 2024-12-05 01:16:03 UTC. ProposalPlease 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?
App/src/libs/actions/Policy/Policy.ts Lines 1458 to 1494 in c049ac7
What changes do you think we should make in order to solve the problem?
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)ResultMonosnap.screencast.2024-12-19.11-20-42.mp4 |
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 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 macOS.Sequoia.15.1.1._.Chrome.mp4 |
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 |
Ok seems like this is a unique issue, adding labels so we can get this fixed. |
Job added to Upwork: https://www.upwork.com/jobs/~021867717521858784433 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease 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 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 What alternative solutions did you explore? (Optional)NA |
@Krishna2323's proposal is looking good! 🎀👀🎀 C+ reviewed! |
Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@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 |
@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! |
@eVoloshchak, @stephanieelliott, @MonilBhavsar 12 days overdue now... This issue's end is nigh! |
@eVoloshchak @stephanieelliott @MonilBhavsar this issue is now 4 weeks old, please consider:
Thanks! |
This issue has not been updated in over 14 days. @eVoloshchak, @stephanieelliott, @MonilBhavsar eroding to Weekly issue. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Bump on this @eVoloshchak, can you please weigh in on the above? |
Hey @eVoloshchak waiting for your input here 🙏 |
I still think @Krishna2323's proposal is valid.
I don't think that's enough of a differentiator to change the proposal we're implementing However, @shubham1206agra is technically right. @Krishna2323's proposal is technically invalid, |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@MonilBhavsar, please this comment when you get a chance. Thanks! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Hey @MonilBhavsar, bump on this comment please. My .02 -- I think we’re getting hung up on semantics here. If @eVoloshchak believes that @Krishna2323's proposal is the best path forward, then I suggest we proceed with that and focus on moving this issue forward. Regarding the Slack post, it does not explicitly state that tests are a firm requirement. Instead, it suggests tests but leaves the decision to the developer’s judgment on whether or not they are appropriate for that specific proposal. If tests are needed, let's add them -- it doesn't make sense to nullify the whole proposal over it |
Agree with these comments - #53599 (comment) #53599 (comment) |
📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Hey @Krishna2323 can you give an ETA for when you will have a PR up? |
@stephanieelliott, sorry for the delay. I'm on leave until today and will start working on the PR from tomorrow. I thought I had left a comment about my leave—sorry for that again. |
@eVoloshchak, the PR is ready for review. |
Bump on this @eVoloshchak -- PR is waiting for your review |
PR is under review |
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:
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:
Screenshots/Videos
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @eVoloshchakThe text was updated successfully, but these errors were encountered: