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

Personal details list migration: use accountID (number) in setContactMethodAsDefault #20792

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
2 changes: 1 addition & 1 deletion src/components/Reactions/ReactionTooltipContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const propTypes = {
/**
* A list of account IDs to display in the tooltip.
*/
accountIDs: PropTypes.arrayOf(PropTypes.string).isRequired,
accountIDs: PropTypes.arrayOf(PropTypes.number).isRequired,

...withCurrentUserPersonalDetailsPropTypes,
};
Expand Down
4 changes: 2 additions & 2 deletions src/components/Reactions/ReportActionItemReactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function ReportActionItemReactions(props) {
>
{_.map(reactionsWithCount, (reaction) => {
const reactionCount = reaction.users.length;
const reactionUsers = _.map(reaction.users, (sender) => sender.accountID.toString());
const reactionUsers = _.map(reaction.users, (sender) => sender.accountID);
const emoji = _.find(emojis, (e) => e.name === reaction.emoji);
const emojiCodes = EmojiUtils.getUniqueEmojiCodes(emoji, reaction.users);
const hasUserReacted = Report.hasAccountIDReacted(props.currentUserPersonalDetails.accountID, reactionUsers);
Expand All @@ -80,7 +80,7 @@ function ReportActionItemReactions(props) {
currentUserPersonalDetails={props.currentUserPersonalDetails}
/>
)}
renderTooltipContentKey={[...reactionUsers, ...emojiCodes]}
renderTooltipContentKey={[..._.map(reactionUsers, (user) => user.toString()), ...emojiCodes]}
key={reaction.emoji}
>
<View>
Expand Down
6 changes: 3 additions & 3 deletions src/libs/PersonalDetailsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ function getDisplayNameOrDefault(passedPersonalDetails, pathToDisplayName, defau
}

/**
* Given a list of account IDs (as string) it will return an array of personal details objects.
* @param {Array<string>} accountIDs - Array of accountIDs
* Given a list of account IDs (as number) it will return an array of personal details objects.
* @param {Array<number>} accountIDs - Array of accountIDs
* @param {Number} currentUserAccountID
* @param {Boolean} shouldChangeUserDisplayName - It will replace the current user's personal detail object's displayName with 'You'.
* @returns {Array} - Array of personal detail objects
Expand All @@ -35,7 +35,7 @@ function getPersonalDetailsByIDs(accountIDs, currentUserAccountID, shouldChangeU
_.each(
_.filter(personalDetails, (detail) => accountIDs.includes(detail.accountID)),
(detail) => {
if (shouldChangeUserDisplayName && currentUserAccountID.toString() === detail.accountID) {
if (shouldChangeUserDisplayName && currentUserAccountID === detail.accountID) {
result.push({
...detail,
displayName: Localize.translateLocal('common.you'),
Expand Down
10 changes: 5 additions & 5 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -1451,8 +1451,8 @@ function getOptimisticDataForReportActionUpdate(originalReportAction, message, r

/**
* Returns true if the accountID has reacted to the report action (with the given skin tone).
* @param {String} accountID
* @param {Array<Object | String | number>} users
* @param {Number} accountID
* @param {Array<Object | Number>} users
* @param {Number} [skinTone]
* @returns {boolean}
*/
Expand All @@ -1461,12 +1461,12 @@ function hasAccountIDReacted(accountID, users, skinTone) {
_.find(users, (user) => {
let userAccountID;
if (typeof user === 'object') {
userAccountID = `${user.accountID}`;
userAccountID = user.accountID;
} else {
userAccountID = `${user}`;
userAccountID = user;
}

return userAccountID === `${accountID}` && (skinTone == null ? true : user.skinTone === skinTone);
return userAccountID === accountID && (skinTone == null ? true : user.skinTone === skinTone);
}) !== undefined
);
}
Expand Down
7 changes: 2 additions & 5 deletions src/libs/actions/User.js
Original file line number Diff line number Diff line change
Expand Up @@ -787,12 +787,10 @@ function setContactMethodAsDefault(newDefaultContactMethod) {
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: {
[newDefaultContactMethod]: {
...myPersonalDetails,
[currentUserAccountID]: {
login: newDefaultContactMethod,
displayName: PersonalDetails.getDisplayName(newDefaultContactMethod, myPersonalDetails),
},
[oldDefaultContactMethod]: null,
},
},
];
Expand Down Expand Up @@ -835,8 +833,7 @@ function setContactMethodAsDefault(newDefaultContactMethod) {
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
value: {
[newDefaultContactMethod]: null,
[oldDefaultContactMethod]: {...myPersonalDetails},
[currentUserAccountID]: {...myPersonalDetails},
},
},
];
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ReactionList/PopoverReactionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ class PopoverReactionList extends React.Component {
};
}
const emojiCount = selectedReaction.users.length;
const reactionUsers = _.map(selectedReaction.users, (sender) => sender.accountID.toString());
const reactionUsers = _.map(selectedReaction.users, (sender) => sender.accountID);
const emoji = _.find(emojis, (e) => e.name === selectedReaction.emoji);
const emojiCodes = EmojiUtils.getUniqueEmojiCodes(emoji, selectedReaction.users);
const hasUserReacted = Report.hasAccountIDReacted(this.props.currentUserPersonalDetails.accountID, reactionUsers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oofda - Report.hasAccountIDReacted is checking that accountIDs equal each other by converting accountIDs to strings (probably why we had .toString() above before - can we update that function to treat userAccountIDs as numbers? 🙏

Unless you think that has the potential to cause bigger problems...

Copy link
Contributor Author

@BeeMargarida BeeMargarida Jun 15, 2023

Choose a reason for hiding this comment

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

Done, I think this also fixed the tooltips content

Expand Down