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

Fix IOU request sent to new phone number account #3651

Merged
merged 5 commits into from
Jun 18, 2021
Merged

Fix IOU request sent to new phone number account #3651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3542e8a
Add util to add SMS domain to phone num login
Beamanator Jun 17, 2021
c0ba461
Remove SMS domain where necessary
Beamanator Jun 17, 2021
250870a
Add sms domain while merging personal details for logins
Beamanator Jun 17, 2021
a107e9f
Reposition and fix up util function
Beamanator Jun 17, 2021
0973d3a
Add SMS domain to user-to-invite's login
Beamanator Jun 17, 2021
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
17 changes: 15 additions & 2 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,19 @@ let preferredLocale;
// will update when the OptionsListUtils re-loads. But will stay the same color for the life of the JS session.
const defaultAvatarForUserToInvite = getDefaultAvatar();

/**
* Adds expensify SMS domain (@expensify.sms) if login is a phone number and if it's not included yet
*
* @param {String} login
* @return {String}
*/
function addSMSDomainIfPhoneNumber(login) {
if (Str.isValidPhone(login) && !Str.isValidEmail(login)) {
return login + CONST.SMS.DOMAIN;
}
return login;
Comment on lines +35 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

We might prefer to return early, but NAB

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even make it a ternary, but I'm not sure either is more legible than this. I'd say let's keep it like it is now.

}
Comment on lines +34 to +39
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could go nicely in expensify-common, but I wanted to keep this in one PR since it's a deploy blocker


/**
* Returns the personal details for an array of logins
*
Expand All @@ -38,7 +51,7 @@ function getPersonalDetailsForLogins(logins, personalDetails) {

if (!personalDetail) {
personalDetail = {
login,
login: addSMSDomainIfPhoneNumber(login),
displayName: login,
avatar: getDefaultAvatar(login),
};
Expand Down Expand Up @@ -339,7 +352,7 @@ function getOptions(reports, personalDetails, draftComments, activeReportID, {
&& (searchValue !== CONST.EMAIL.CHRONOS || Permissions.canUseChronos(betas))
) {
// If the phone number doesn't have an international code then let's prefix it with the
// current users international code based on their IP address.
// current user's international code based on their IP address.
const login = (Str.isValidPhone(searchValue) && !searchValue.includes('+'))
? `+${countryCodeByIP}${searchValue}`
: searchValue;
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/OptionsListUtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ describe('OptionsListUtils', () => {
expect(results.recentReports.length).toBe(0);
expect(results.personalDetails.length).toBe(0);
expect(results.userToInvite).not.toBe(null);
expect(results.userToInvite.login).toBe('+15005550006');
expect(results.userToInvite.login).toBe(`+15005550006${CONST.SMS.DOMAIN}`);

// When we add a search term for which no options exist and the searchValue itself
// is a potential phone number with country code added
Expand All @@ -361,7 +361,7 @@ describe('OptionsListUtils', () => {
expect(results.recentReports.length).toBe(0);
expect(results.personalDetails.length).toBe(0);
expect(results.userToInvite).not.toBe(null);
expect(results.userToInvite.login).toBe('+15005550006');
expect(results.userToInvite.login).toBe(`+15005550006${CONST.SMS.DOMAIN}`);

// Test Concierge's existence in new group options
results = OptionsListUtils.getNewGroupOptions(REPORTS_WITH_CONCIERGE, PERSONAL_DETAILS_WITH_CONCIERGE);
Expand Down