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

feat: create formatPhoneNumber util function #16804

Merged
merged 18 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"@react-navigation/native": "6.0.13",
"@react-navigation/stack": "6.3.1",
"@ua/react-native-airship": "^15.2.0",
"awesome-phonenumber": "^5.4.0",
"babel-plugin-transform-remove-console": "^6.9.4",
"babel-polyfill": "^6.26.0",
"dom-serializer": "^0.2.2",
Expand Down
27 changes: 8 additions & 19 deletions src/components/withLocalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import getComponentDisplayName from '../libs/getComponentDisplayName';
import ONYXKEYS from '../ONYXKEYS';
import * as Localize from '../libs/Localize';
import DateUtils from '../libs/DateUtils';
import * as LocalePhoneNumber from '../libs/LocalePhoneNumber';
import * as NumberFormatUtils from '../libs/NumberFormatUtils';
import * as LocaleDigitUtils from '../libs/LocaleDigitUtils';
import CONST from '../CONST';
import compose from '../libs/compose';
import withCurrentUserPersonalDetails from './withCurrentUserPersonalDetails';
import * as LocalePhoneNumber from '../libs/LocalePhoneNumber';

const LocaleContext = createContext(null);

Expand All @@ -29,11 +29,9 @@ const withLocalizePropTypes = {
/** Formats a datetime to local date and time string */
datetimeToCalendarTime: PropTypes.func.isRequired,

/** Returns a locally converted phone number without the country code */
toLocalPhone: PropTypes.func.isRequired,

/** Returns an internationally converted phone number with the country code */
fromLocalPhone: PropTypes.func.isRequired,
/** Returns a locally converted phone number for numbers from the same region
* and an internationally converted phone number with the country code for numbers from other regions */
formatPhoneNumber: PropTypes.func.isRequired,

/** Gets the standard digit corresponding to a locale digit */
fromLocaleDigit: PropTypes.func.isRequired,
Expand Down Expand Up @@ -77,8 +75,7 @@ class LocaleContextProvider extends React.Component {
numberFormat: this.numberFormat.bind(this),
datetimeToRelative: this.datetimeToRelative.bind(this),
datetimeToCalendarTime: this.datetimeToCalendarTime.bind(this),
fromLocalPhone: this.fromLocalPhone.bind(this),
toLocalPhone: this.toLocalPhone.bind(this),
formatPhoneNumber: this.formatPhoneNumber.bind(this),
fromLocaleDigit: this.fromLocaleDigit.bind(this),
toLocaleDigit: this.toLocaleDigit.bind(this),
preferredLocale: this.props.preferredLocale,
Expand Down Expand Up @@ -126,19 +123,11 @@ class LocaleContextProvider extends React.Component {
}

/**
* @param {Number} number
* @returns {String}
*/
toLocalPhone(number) {
return LocalePhoneNumber.toLocalPhone(this.props.preferredLocale, number);
}

/**
* @param {Number} number
* @param {String} phoneNumber
* @returns {String}
*/
fromLocalPhone(number) {
return LocalePhoneNumber.fromLocalPhone(this.props.preferredLocale, number);
formatPhoneNumber(phoneNumber) {
return LocalePhoneNumber.formatPhoneNumber(phoneNumber);
}

/**
Expand Down
96 changes: 58 additions & 38 deletions src/libs/LocalePhoneNumber.js
Original file line number Diff line number Diff line change
@@ -1,53 +1,73 @@
import lodashGet from 'lodash/get';
import lodashTrim from 'lodash/trim';
import lodashIncludes from 'lodash/includes';
import lodashStartsWith from 'lodash/startsWith';
import Onyx from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
import translations from '../languages/translations';
import {parsePhoneNumber} from 'awesome-phonenumber';
import ONYXKEYS from '../ONYXKEYS';

/**
* Returns a locally converted phone number without the country code
*
* @param {String} locale eg 'en', 'es-ES'
* @param {String} number
* @returns {String}
*/
function toLocalPhone(locale, number) {
const numString = lodashTrim(number);
const withoutPlusNum = lodashIncludes(numString, '+') ? Str.cutBefore(numString, '+') : numString;
const country = lodashGet(translations, [locale, 'phoneCountryCode']);

if (country) {
if (lodashStartsWith(withoutPlusNum, country)) {
return Str.cutBefore(withoutPlusNum, country);
let currentUserEmail;
Onyx.connect({
key: ONYXKEYS.SESSION,
callback: (val) => {
// When signed out, val is undefined
if (!val) {
return;
}
return numString;
}
return number;
}

currentUserEmail = val.email;
},
});

let currentUserPersonalDetails;
Onyx.connect({
key: ONYXKEYS.PERSONAL_DETAILS,
callback: (val) => {
currentUserPersonalDetails = lodashGet(val, currentUserEmail, {});
},
});

let countryCodeByIP;
Onyx.connect({
key: ONYXKEYS.COUNTRY_CODE,
callback: val => countryCodeByIP = val || 1,
});

/**
* Returns an internationally converted phone number with the country code
* Returns a locally converted phone number for numbers from the same region
* and an internationally converted phone number with the country code for numbers from other regions
*
* @param {String} locale eg 'en', 'es-ES'
* @param {String} number
* @returns {String}
*/
function fromLocalPhone(locale, number) {
const numString = lodashTrim(number);
const withoutPlusNum = lodashIncludes(numString, '+') ? Str.cutBefore(numString, '+') : numString;
const country = lodashGet(translations, [locale, 'phoneCountryCode']);

if (country) {
if (lodashStartsWith(withoutPlusNum, country)) {
return `+${withoutPlusNum}`;
}
return `+${country}${withoutPlusNum}`;
function formatPhoneNumber(number) {
const parsedPhoneNumber = parsePhoneNumber(Str.removeSMSDomain(number));

// return the string untouched if it's not a phone number
if (!parsedPhoneNumber.valid) {
return number;
}

let signedInUserCountryCode;

puneetlath marked this conversation as resolved.
Show resolved Hide resolved
/**
* if there is a phone number saved in the user's personal details we format the other numbers depending on
* the phone number's country code, otherwise we use country code based on the user's IP
*/
if (currentUserPersonalDetails.phoneNumber) {
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
signedInUserCountryCode = parsePhoneNumber(currentUserPersonalDetails.phoneNumber).countryCode;
} else {
signedInUserCountryCode = countryCodeByIP;
}
return number;

const regionCode = parsedPhoneNumber.countryCode;

puneetlath marked this conversation as resolved.
Show resolved Hide resolved
if (regionCode === signedInUserCountryCode) {
return parsedPhoneNumber.number.national;
}

return parsedPhoneNumber.number.international;
}

export {
toLocalPhone,
fromLocalPhone,
// eslint-disable-next-line import/prefer-default-export
formatPhoneNumber,
};
9 changes: 5 additions & 4 deletions src/libs/OptionsListUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import Permissions from './Permissions';
import * as CollectionUtils from './CollectionUtils';
import Navigation from './Navigation/Navigation';
import * as LoginUtils from './LoginUtils';
import * as LocalePhoneNumber from './LocalePhoneNumber';

/**
* OptionsListUtils is used to build a list options passed to the OptionsList component. Several different UI views can
Expand Down Expand Up @@ -161,7 +162,7 @@ function getPersonalDetailsForLogins(logins, personalDetails) {
if (!personalDetail) {
personalDetail = {
login,
displayName: Str.removeSMSDomain(login),
displayName: LocalePhoneNumber.formatPhoneNumber(login),
avatar: ReportUtils.getDefaultAvatar(login),
};
}
Expand Down Expand Up @@ -189,7 +190,7 @@ function getParticipantsOptions(report, personalDetails) {
text: details.displayName,
firstName: lodashGet(details, 'firstName', ''),
lastName: lodashGet(details, 'lastName', ''),
alternateText: Str.isSMSLogin(details.login) ? Str.removeSMSDomain(details.login) : details.login,
alternateText: Str.isSMSLogin(details.login) ? LocalePhoneNumber.formatPhoneNumber(details.login) : details.login,
icons: [{
source: ReportUtils.getAvatar(details.avatar, details.login),
name: details.login,
Expand Down Expand Up @@ -431,13 +432,13 @@ function createOption(logins, personalDetails, report, reportActions = {}, {
} else {
result.alternateText = (showChatPreviewLine && lastMessageText)
? lastMessageText
: Str.removeSMSDomain(personalDetail.login);
: LocalePhoneNumber.formatPhoneNumber(personalDetail.login);
}
reportName = ReportUtils.getReportName(report, policies);
} else {
reportName = ReportUtils.getDisplayNameForParticipant(logins[0]);
result.keyForList = personalDetail.login;
result.alternateText = Str.removeSMSDomain(personalDetail.login);
result.alternateText = LocalePhoneNumber.formatPhoneNumber(personalDetail.login);
}

result.isIOUReportOwner = ReportUtils.isIOUOwnedByCurrentUser(result, iouReports);
Expand Down
4 changes: 2 additions & 2 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import ONYXKEYS from '../ONYXKEYS';
import CONST from '../CONST';
import * as Localize from './Localize';
import * as LocalePhoneNumber from './LocalePhoneNumber';
import * as Expensicons from '../components/Icon/Expensicons';
import hashCode from './hashCode';
import Navigation from './Navigation/Navigation';
Expand All @@ -21,6 +20,7 @@ import linkingConfig from './Navigation/linkingConfig';
import * as defaultAvatars from '../components/Icon/DefaultAvatars';
import isReportMessageAttachment from './isReportMessageAttachment';
import * as defaultWorkspaceAvatars from '../components/Icon/WorkspaceDefaultAvatars';
import * as LocalePhoneNumber from './LocalePhoneNumber';

let sessionEmail;
Onyx.connect({
Expand Down Expand Up @@ -766,7 +766,7 @@ function getDisplayNameForParticipant(login, shouldUseShortForm = false) {
const loginWithoutSMSDomain = Str.removeSMSDomain(personalDetails.login);
let longName = personalDetails.displayName || loginWithoutSMSDomain;
if (longName === loginWithoutSMSDomain && Str.isSMSLogin(longName)) {
longName = LocalePhoneNumber.toLocalPhone(preferredLocale, longName);
longName = LocalePhoneNumber.formatPhoneNumber(longName);
}
const shortName = personalDetails.firstName || longName;

Expand Down
6 changes: 5 additions & 1 deletion src/libs/SidebarUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import * as Localize from './Localize';
import CONST from '../CONST';
import * as OptionsListUtils from './OptionsListUtils';
import * as CollectionUtils from './CollectionUtils';
import * as LocalePhoneNumber from './LocalePhoneNumber';

// Note: It is very important that the keys subscribed to here are the same
// keys that are connected to SidebarLinks withOnyx(). If there was a key missing from SidebarLinks and it's data was updated
Expand Down Expand Up @@ -251,6 +252,9 @@ function getOptionData(reportID) {
const hasMultipleParticipants = participantPersonalDetailList.length > 1 || result.isChatRoom || result.isPolicyExpenseChat;
const subtitle = ReportUtils.getChatRoomSubtitle(report, policies);

const login = Str.removeSMSDomain(personalDetail.login);
const formattedLogin = Str.isSMSLogin(login) ? LocalePhoneNumber.formatPhoneNumber(login) : login;

// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips((participantPersonalDetailList || []).slice(0, 10), hasMultipleParticipants);

Expand Down Expand Up @@ -302,7 +306,7 @@ function getOptionData(reportID) {
}).join(' ');
}

result.alternateText = lastMessageText || Str.removeSMSDomain(personalDetail.login);
result.alternateText = lastMessageText || formattedLogin;
}

result.isIOUReportOwner = ReportUtils.isIOUOwnedByCurrentUser(result, iouReports);
Expand Down
5 changes: 3 additions & 2 deletions src/libs/actions/PersonalDetails.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';
import * as API from '../API';
import * as ReportUtils from '../ReportUtils';
import * as LocalePhoneNumber from '../LocalePhoneNumber';
import ROUTES from '../../ROUTES';
import Navigation from '../Navigation/Navigation';

Expand All @@ -29,9 +30,9 @@ Onyx.connect({
* @returns {String}
*/
function getDisplayName(login, personalDetail) {
// If we have a number like +15857527441@expensify.sms then let's remove @expensify.sms
// If we have a number like +15857527441@expensify.sms then let's remove @expensify.sms and format it
// so that the option looks cleaner in our UI.
const userLogin = Str.removeSMSDomain(login);
const userLogin = LocalePhoneNumber.formatPhoneNumber(login);
const userDetails = personalDetail || lodashGet(personalDetails, login);

if (!userDetails) {
Expand Down
18 changes: 12 additions & 6 deletions src/pages/DetailsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ class DetailsPage extends React.PureComponent {
pronouns = this.props.translate(`pronouns.${localeKey}`);
}

const phoneNumber = getPhoneNumber(details);
const displayName = isSMSLogin ? this.props.formatPhoneNumber(phoneNumber) : details.displayName;
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this make it so that their displayName will show up as their phone number even if they have a name set. For example, if I have my primary login on my account as 1234567890 and my display name as Puneet Lath this will make it so that my display name shows as (123) 456-7890. I think we only want it to show as that if I don't have a display name set.

Copy link
Contributor Author

@koko57 koko57 Apr 11, 2023

Choose a reason for hiding this comment

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

@puneetlath It is the current behavior, I just moved the condition up and changed the formatting number function for a new one. I can ofc add a check if there is a name and display it instead of the phone number, but should it be done in this issue?

Copy link
Member

@rushatgabhane rushatgabhane Apr 11, 2023

Choose a reason for hiding this comment

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

Yeah, looks like it's the current behavior and we shouldn't do anything about it in this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok got it. Let's handle separately then.

@koko57 would you want to raise a separate PR for that fix?

Copy link
Member

@rushatgabhane rushatgabhane Apr 11, 2023

Choose a reason for hiding this comment

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

@koko57 if you don't have the time / different priorities, I can create a tracking issue and the PR.
Just let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rushatgabhane I can take it :) So if you create an issue, please assign me

Copy link
Member

@rushatgabhane rushatgabhane Apr 12, 2023

Choose a reason for hiding this comment

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

Was trying to get screenshots for the issue but I don't have an account with phone as the primary login.
I'm traveling and expensify isn't allowing me to create an account with phone number from another country

@puneetlath could you help us out 😅

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need an account you can sign in with or just one you can message? If you just need to message one, you can message my +19726585619 account. I can take care of any tests that require logging in.

Copy link
Member

@rushatgabhane rushatgabhane Apr 16, 2023

Choose a reason for hiding this comment

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

Thanks! I sent you a message and I can't see a display name.

Please confirm that you have a name set on that account, and I'll create an issue.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Just responded!

const phoneOrEmail = isSMSLogin ? getPhoneNumber(details) : details.login;

return (
<ScreenWrapper>
<FullPageNotFoundView shouldShow={_.isEmpty(login)}>
Expand All @@ -127,7 +131,7 @@ class DetailsPage extends React.PureComponent {
<ScrollView>
<View style={styles.avatarSectionWrapper}>
<AttachmentModal
headerTitle={isSMSLogin ? this.props.toLocalPhone(details.displayName) : details.displayName}
headerTitle={displayName}
source={ReportUtils.getFullSizeAvatar(details.avatar, details.login)}
isAuthTokenRequired
>
Expand All @@ -151,7 +155,7 @@ class DetailsPage extends React.PureComponent {
</AttachmentModal>
{Boolean(details.displayName) && (
<Text style={[styles.textHeadline, styles.mb6, styles.pre]} numberOfLines={1}>
{isSMSLogin ? this.props.toLocalPhone(details.displayName) : details.displayName}
{displayName}
</Text>
)}
{details.login ? (
Expand All @@ -161,11 +165,13 @@ class DetailsPage extends React.PureComponent {
? 'common.phoneNumber'
: 'common.email')}
</Text>
<CommunicationsLink value={isSMSLogin ? getPhoneNumber(details) : details.login}>
<Tooltip text={isSMSLogin ? getPhoneNumber(details) : details.login}>
<CommunicationsLink
value={phoneOrEmail}
>
<Tooltip text={phoneOrEmail}>
<Text numberOfLines={1}>
{isSMSLogin
? this.props.toLocalPhone(getPhoneNumber(details))
? this.props.formatPhoneNumber(phoneNumber)
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
: details.login}
</Text>
</Tooltip>
Expand All @@ -186,7 +192,7 @@ class DetailsPage extends React.PureComponent {
</View>
{details.login !== this.props.session.email && (
<MenuItem
title={`${this.props.translate('common.message')}${details.displayName}`}
title={`${this.props.translate('common.message')}${displayName}`}
icon={Expensicons.ChatBubble}
onPress={() => Report.navigateToAndOpenReport([details.login])}
wrapperStyle={styles.breakAll}
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ParticipantLocalTime.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class ParticipantLocalTime extends PureComponent {
render() {
const reportRecipientDisplayName = this.props.participant.firstName
|| (Str.isSMSLogin(this.props.participant.login)
? this.props.toLocalPhone(this.props.participant.displayName)
? this.props.formatPhoneNumber(this.props.participant.displayName)
: this.props.participant.displayName);

return (
Expand Down
Loading