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

[NoQA] Add falsey checks and _.compact() to places where we access SMS logins from personal details #18159

Merged
merged 6 commits into from
Apr 28, 2023
Merged

[NoQA] Add falsey checks and _.compact() to places where we access SMS logins from personal details #18159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd43bee
add falsey checks and compacts to places where we access SMS logins f…
jasperhuangg Apr 28, 2023
51f4e5c
return empty string
jasperhuangg Apr 28, 2023
3373f44
use value
jasperhuangg Apr 28, 2023
27b0d47
fix typo
jasperhuangg Apr 28, 2023
caa86c4
fix isSMSLogin usages
jasperhuangg Apr 28, 2023
5cb8198
use chain correctly
jasperhuangg Apr 28, 2023
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
4 changes: 4 additions & 0 deletions src/libs/LocalePhoneNumber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ Onyx.connect({
* @returns {String}
*/
function formatPhoneNumber(number) {
if (!number) {
return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good approach, but I'm unsure if it's going to be okay, in all uses, for it to be an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally had it to just return number, but my thinking was that if we're passed an incorrect falsey value (like undefined or 0 or something), it would be better to just return an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah totally. What I'm saying is what will the caller of the function do with ''? Hopefully it's fine, but it's used all over the place so how do we verify that it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least it definitely won't crash if we return '', since the caller expects a string.

}

const parsedPhoneNumber = parsePhoneNumber(Str.removeSMSDomain(number));

// return the string untouched if it's not a phone number
Expand Down
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function getParticipantsOptions(report, personalDetails) {
text: details.displayName,
firstName: lodashGet(details, 'firstName', ''),
lastName: lodashGet(details, 'lastName', ''),
alternateText: Str.isSMSLogin(details.login) ? LocalePhoneNumber.formatPhoneNumber(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
2 changes: 1 addition & 1 deletion src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ function getDisplayNameForParticipant(login, shouldUseShortForm = false) {
function getDisplayNamesWithTooltips(participants, isMultipleParticipantReport) {
return _.map(participants, (participant) => {
const displayName = getDisplayNameForParticipant(participant.login, isMultipleParticipantReport);
const tooltip = Str.removeSMSDomain(participant.login);
const tooltip = participant.login ? Str.removeSMSDomain(participant.login) : '';

let pronouns = participant.pronouns;
if (pronouns && pronouns.startsWith(CONST.PRONOUNS.PREFIX)) {
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ function extractFirstAndLastNameFromAvailableDetails({
if (firstName || lastName) {
return {firstName: firstName || '', lastName: lastName || ''};
}
if (Str.removeSMSDomain(login) === displayName) {
if (login && Str.removeSMSDomain(login) === displayName) {
return {firstName: '', lastName: ''};
}

Expand Down
4 changes: 2 additions & 2 deletions src/pages/DetailsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const getPhoneNumber = (details) => {
}

// If the user has set a displayName, get the phone number from the SMS login
return Str.removeSMSDomain(details.login);
return details.login ? Str.removeSMSDomain(details.login) : '';
};

class DetailsPage extends React.PureComponent {
Expand All @@ -97,7 +97,7 @@ class DetailsPage extends React.PureComponent {
};
}

const isSMSLogin = Str.isSMSLogin(details.login);
const isSMSLogin = details.login ? Str.isSMSLogin(details.login) : false;

// If we have a reportID param this means that we
// arrived here via the ParticipantsPage and should be allowed to navigate back to it
Expand Down
38 changes: 20 additions & 18 deletions src/pages/ReportParticipantsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,27 @@ const defaultProps = {
const getAllParticipants = (report, personalDetails) => {
const {participants} = report;

return _.map(participants, (login) => {
const userLogin = Str.removeSMSDomain(login);
const userPersonalDetail = lodashGet(personalDetails, login, {displayName: userLogin, avatar: ''});
return _.chain(participants)
.compact()
.map(participants, (login) => {
const userLogin = Str.removeSMSDomain(login);
const userPersonalDetail = lodashGet(personalDetails, login, {displayName: userLogin, avatar: ''});

return ({
alternateText: userLogin,
displayName: userPersonalDetail.displayName,
icons: [{
source: ReportUtils.getAvatar(userPersonalDetail.avatar, login),
name: login,
type: CONST.ICON_TYPE_AVATAR,
}],
keyForList: userLogin,
login,
text: userPersonalDetail.displayName,
tooltipText: userLogin,
participantsList: [{login, displayName: userPersonalDetail.displayName}],
});
});
return ({
alternateText: userLogin,
displayName: userPersonalDetail.displayName,
icons: [{
source: ReportUtils.getAvatar(userPersonalDetail.avatar, login),
name: login,
type: CONST.ICON_TYPE_AVATAR,
}],
keyForList: userLogin,
login,
text: userPersonalDetail.displayName,
tooltipText: userLogin,
participantsList: [{login, displayName: userPersonalDetail.displayName}],
});
}).value();
};

const ReportParticipantsPage = (props) => {
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ReportActionItemSingle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,11 @@ const ReportActionItemSingle = (props) => {
// Since the display name for a report action message is delivered with the report history as an array of fragments
// we'll need to take the displayName from personal details and have it be in the same format for now. Eventually,
// we should stop referring to the report history items entirely for this information.
const isSMSLogin = login ? Str.isSMSLogin(login) : false;
const personArray = displayName
? [{
type: 'TEXT',
text: Str.isSMSLogin(login) ? props.formatPhoneNumber(displayName) : displayName,
text: isSMSLogin ? props.formatPhoneNumber(displayName) : displayName,
}]
: props.action.person;

Expand Down