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.

Sign up for GitHub

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 Workspace auto naming to include using First Name over the username of a public email #44109

Merged
merged 6 commits into from
Jul 16, 2024
Merged

Update Workspace auto naming to include using First Name over the username of a public email #44109

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e50bab6
change workspace naming logic
allgandalf Jun 20, 2024
e91e472
Fix type
allgandalf Jun 20, 2024
7f2896c
fix lint
allgandalf Jun 20, 2024
10cdf40
remove test file
allgandalf Jun 29, 2024
6a403d3
apply suggestions
allgandalf Jul 10, 2024
fee8c12
Apply suggestions from code review
allgandalf Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/libs/actions/Policy/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import * as ErrorUtils from '@libs/ErrorUtils';
import getIsNarrowLayout from '@libs/getIsNarrowLayout';
import Log from '@libs/Log';
import * as NumberUtils from '@libs/NumberUtils';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as PhoneNumber from '@libs/PhoneNumber';
import * as PolicyUtils from '@libs/PolicyUtils';
import {navigateWhenEnableFeature} from '@libs/PolicyUtils';
Expand Down Expand Up @@ -1355,11 +1356,17 @@ function generateDefaultWorkspaceName(email = ''): string {
}
const username = emailParts[0];
const domain = emailParts[1];
const userDetails = PersonalDetailsUtils.getPersonalDetailByEmail(sessionEmail);
const displayName = `${userDetails?.firstName ?? ''}${userDetails?.lastName ? ` ${userDetails.lastName}` : ''}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const displayName = `${userDetails?.firstName ?? ''}${userDetails?.lastName ? ` ${userDetails.lastName}` : ''}`;
const displayName = userDetails?.displayName;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that there is a displayName property ? and it would not add extra space if we do not have the last name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is 😄

and it would not add extra space if we do not have the last name?

We can trim it if that's the case!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm, makes sense, i will update this PR by EOD.


if (PUBLIC_DOMAINS.some((publicDomain) => publicDomain === domain.toLowerCase())) {
if (!PUBLIC_DOMAINS.some((publicDomain) => publicDomain === domain.toLowerCase())) {
defaultWorkspaceName = `${Str.UCFirst(domain.split('.')[0])}'s Workspace`;
} else if (displayName !== '') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (displayName !== '') {
} else if (!!displayName) {

defaultWorkspaceName = `${Str.UCFirst(displayName)}'s Workspace`;
} else if (PUBLIC_DOMAINS.some((publicDomain) => publicDomain === domain.toLowerCase())) {
defaultWorkspaceName = `${Str.UCFirst(username)}'s Workspace`;
} else {
defaultWorkspaceName = `${Str.UCFirst(domain.split('.')[0])}'s Workspace`;
defaultWorkspaceName = userDetails?.phoneNumber ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually is there any possible case where we use a phone number? there'll always be an email associated with the account.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you login via a new account with phone number, i guess at that time this case is used, I will confirm if that hypothesis is true in sometime

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets keep this code, The comment does say that we should have a phone number case so better to keep :)

}

if (`@${domain.toLowerCase()}` === CONST.SMS.DOMAIN) {
Expand Down
Loading