-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA][TS migration] Migrate 'UserUtils.js' lib to TypeScript #27778
Merged
aldo-expensify
merged 16 commits into
Expensify:main
from
software-mansion-labs:ts-migration/UserUtils
Oct 2, 2023
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
2327a44
[TS migration] Migrate 'UserUtils.js' lib to TypeScript
blazejkustra adaca7d
Fix tests
blazejkustra 04cc1cb
Remove file from another migration
blazejkustra ef5a4e5
Merge branch 'main' into ts-migration/UserUtils
blazejkustra c03f7e8
Make param optional
blazejkustra f9ccb69
Small improvements after review
blazejkustra 88bad87
Remove ^ (Compatible with version) for typescript (TS doesn't follow …
blazejkustra a6f5d24
Revert "Remove ^ (Compatible with version) for typescript (TS doesn't…
blazejkustra faf29b7
Fix type error
blazejkustra ff6b622
Revert "Fix type error"
blazejkustra f5f1d1d
Another try
blazejkustra 729f5c5
Merge branch 'main' into ts-migration/UserUtils
blazejkustra 42ae0f5
Fix error
blazejkustra 84175dc
Remove unused import
blazejkustra 50e1e79
Rerun tests
blazejkustra d6b6fe2
Merge branch 'main' into ts-migration/UserUtils
blazejkustra File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,14 @@ | ||
import _ from 'underscore'; | ||
import lodashGet from 'lodash/get'; | ||
import {SvgProps} from 'react-native-svg'; | ||
import {ValueOf} from 'type-fest'; | ||
import CONST from '../CONST'; | ||
import hashCode from './hashCode'; | ||
import * as Expensicons from '../components/Icon/Expensicons'; | ||
import {ConciergeAvatar, FallbackAvatar} from '../components/Icon/Expensicons'; | ||
import * as defaultAvatars from '../components/Icon/DefaultAvatars'; | ||
import Login from '../types/onyx/Login'; | ||
|
||
type AvatarRange = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24; | ||
|
||
type LoginListIndicator = ValueOf<typeof CONST.BRICK_ROAD_INDICATOR_STATUS> | ''; | ||
|
||
/** | ||
* Searches through given loginList for any contact method / login with an error. | ||
|
@@ -25,36 +30,26 @@ import * as defaultAvatars from '../components/Icon/DefaultAvatars'; | |
* } | ||
* } | ||
* }} | ||
* | ||
* @param {Object} loginList | ||
* @param {Object} loginList.errorFields | ||
* @returns {Boolean} | ||
*/ | ||
function hasLoginListError(loginList) { | ||
return _.some(loginList, (login) => _.some(lodashGet(login, 'errorFields', {}), (field) => !_.isEmpty(field))); | ||
function hasLoginListError(loginList: Login): boolean { | ||
const errorFields = loginList?.errorFields ?? {}; | ||
return Object.values(errorFields).some((field) => Object.keys(field ?? {}).length > 0); | ||
} | ||
|
||
/** | ||
* Searches through given loginList for any contact method / login that requires | ||
* an Info brick road status indicator. Currently this only applies if the user | ||
* has an unvalidated contact method. | ||
* | ||
* @param {Object} loginList | ||
* @param {String} loginList.validatedDate | ||
* @returns {Boolean} | ||
*/ | ||
function hasLoginListInfo(loginList) { | ||
return _.some(loginList, (login) => _.isEmpty(login.validatedDate)); | ||
function hasLoginListInfo(loginList: Login): boolean { | ||
return !loginList.validatedDate; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change caused this regression: #28643 |
||
/** | ||
* Gets the appropriate brick road indicator status for a given loginList. | ||
* Error status is higher priority, so we check for that first. | ||
* | ||
* @param {Object} loginList | ||
* @returns {String} | ||
*/ | ||
function getLoginListBrickRoadIndicator(loginList) { | ||
function getLoginListBrickRoadIndicator(loginList: Login): LoginListIndicator { | ||
if (hasLoginListError(loginList)) { | ||
return CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR; | ||
} | ||
|
@@ -66,42 +61,35 @@ function getLoginListBrickRoadIndicator(loginList) { | |
|
||
/** | ||
* Hashes provided string and returns a value between [0, range) | ||
* @param {String} text | ||
* @param {Number} range | ||
* @returns {Number} | ||
*/ | ||
function hashText(text, range) { | ||
function hashText(text: string, range: number): number { | ||
return Math.abs(hashCode(text.toLowerCase())) % range; | ||
} | ||
|
||
/** | ||
* Helper method to return the default avatar associated with the given accountID | ||
* @param {Number} [accountID] | ||
* @returns {String} | ||
* @param [accountID] | ||
* @returns | ||
*/ | ||
function getDefaultAvatar(accountID = -1) { | ||
function getDefaultAvatar(accountID = -1): React.FC<SvgProps> { | ||
if (accountID <= 0) { | ||
return Expensicons.FallbackAvatar; | ||
return FallbackAvatar; | ||
} | ||
if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) { | ||
return Expensicons.ConciergeAvatar; | ||
return ConciergeAvatar; | ||
} | ||
|
||
// There are 24 possible default avatars, so we choose which one this user has based | ||
// on a simple modulo operation of their login number. Note that Avatar count starts at 1. | ||
const accountIDHashBucket = (accountID % CONST.DEFAULT_AVATAR_COUNT) + 1; | ||
const accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange; | ||
|
||
return defaultAvatars[`Avatar${accountIDHashBucket}`]; | ||
} | ||
|
||
/** | ||
* Helper method to return default avatar URL associated with login | ||
* | ||
* @param {Number} [accountID] | ||
* @param {Boolean} [isNewDot] | ||
* @returns {String} | ||
*/ | ||
function getDefaultAvatarURL(accountID = '', isNewDot = false) { | ||
function getDefaultAvatarURL(accountID: string | number = '', isNewDot = false): string { | ||
if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) { | ||
return CONST.CONCIERGE_ICON_URL; | ||
} | ||
|
@@ -115,64 +103,57 @@ function getDefaultAvatarURL(accountID = '', isNewDot = false) { | |
|
||
/** | ||
* Given a user's avatar path, returns true if user doesn't have an avatar or if URL points to a default avatar | ||
* @param {String} [avatarURL] - the avatar source from user's personalDetails | ||
* @returns {Boolean} | ||
* @param [avatarURL] - the avatar source from user's personalDetails | ||
*/ | ||
function isDefaultAvatar(avatarURL) { | ||
if ( | ||
_.isString(avatarURL) && | ||
(avatarURL.includes('images/avatars/avatar_') || avatarURL.includes('images/avatars/default-avatar_') || avatarURL.includes('images/avatars/user/default')) | ||
) { | ||
return true; | ||
} | ||
|
||
// We use a hardcoded "default" Concierge avatar | ||
if (_.isString(avatarURL) && (avatarURL === CONST.CONCIERGE_ICON_URL_2021 || avatarURL === CONST.CONCIERGE_ICON_URL)) { | ||
return true; | ||
function isDefaultAvatar(avatarURL?: string): boolean { | ||
if (typeof avatarURL === 'string') { | ||
if (avatarURL.includes('images/avatars/avatar_') || avatarURL.includes('images/avatars/default-avatar_') || avatarURL.includes('images/avatars/user/default')) { | ||
return true; | ||
} | ||
|
||
// We use a hardcoded "default" Concierge avatar | ||
if (avatarURL === CONST.CONCIERGE_ICON_URL_2021 || avatarURL === CONST.CONCIERGE_ICON_URL) { | ||
return true; | ||
} | ||
} | ||
|
||
if (!avatarURL) { | ||
// If null URL, we should also use a default avatar | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/** | ||
* Provided a source URL, if source is a default avatar, return the associated SVG. | ||
* Otherwise, return the URL pointing to a user-uploaded avatar. | ||
* | ||
* @param {String} avatarURL - the avatar source from user's personalDetails | ||
* @param {Number} accountID - the accountID of the user | ||
* @returns {String|Function} | ||
* @param avatarURL - the avatar source from user's personalDetails | ||
* @param accountID - the accountID of the user | ||
*/ | ||
function getAvatar(avatarURL, accountID) { | ||
function getAvatar(avatarURL: string, accountID: number): React.FC<SvgProps> | string { | ||
return isDefaultAvatar(avatarURL) ? getDefaultAvatar(accountID) : avatarURL; | ||
} | ||
|
||
/** | ||
* Provided an avatar URL, if avatar is a default avatar, return NewDot default avatar URL. | ||
* Otherwise, return the URL pointing to a user-uploaded avatar. | ||
* | ||
* @param {String} avatarURL - the avatar source from user's personalDetails | ||
* @param {Number} accountID - the accountID of the user | ||
* @returns {String} | ||
* @param avatarURL - the avatar source from user's personalDetails | ||
* @param accountID - the accountID of the user | ||
*/ | ||
function getAvatarUrl(avatarURL, accountID) { | ||
function getAvatarUrl(avatarURL: string, accountID: number): string { | ||
return isDefaultAvatar(avatarURL) ? getDefaultAvatarURL(accountID, true) : avatarURL; | ||
} | ||
|
||
/** | ||
* Avatars uploaded by users will have a _128 appended so that the asset server returns a small version. | ||
* This removes that part of the URL so the full version of the image can load. | ||
* | ||
* @param {String} [avatarURL] | ||
* @param {Number} [accountID] | ||
* @returns {String|Function} | ||
*/ | ||
function getFullSizeAvatar(avatarURL, accountID) { | ||
function getFullSizeAvatar(avatarURL: string, accountID: number): React.FC<SvgProps> | string { | ||
const source = getAvatar(avatarURL, accountID); | ||
if (!_.isString(source)) { | ||
if (typeof source !== 'string') { | ||
return source; | ||
aldo-expensify marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return source.replace('_128', ''); | ||
|
@@ -181,14 +162,10 @@ function getFullSizeAvatar(avatarURL, accountID) { | |
/** | ||
* Small sized avatars end with _128.<file-type>. This adds the _128 at the end of the | ||
* source URL (before the file type) if it doesn't exist there already. | ||
* | ||
* @param {String} avatarURL | ||
* @param {Number} accountID | ||
* @returns {String|Function} | ||
*/ | ||
function getSmallSizeAvatar(avatarURL, accountID) { | ||
function getSmallSizeAvatar(avatarURL: string, accountID: number): React.FC<SvgProps> | string { | ||
const source = getAvatar(avatarURL, accountID); | ||
if (!_.isString(source)) { | ||
if (typeof source !== 'string') { | ||
return source; | ||
} | ||
|
||
|
@@ -207,10 +184,8 @@ function getSmallSizeAvatar(avatarURL, accountID) { | |
|
||
/** | ||
* Generate a random accountID base on searchValue. | ||
* @param {String} searchValue | ||
* @returns {Number} | ||
*/ | ||
function generateAccountID(searchValue) { | ||
function generateAccountID(searchValue: string): number { | ||
return hashText(searchValue, 2 ** 32); | ||
} | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the type
loginList: LoginList
correct? if it is, we wouldn't need the question mark inloginList?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's correct, the thing is that this function is still used in some JS files and wrong values are passed and tests were failing. Once we get to these files that use this function the underlying issue will be fixed.
Either way, it's okay to make code safe and use
nullish coalescing
andoptional chaining
even if types says it's not necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this, I expect to be able to trust our types, but I understand that this may be acceptable while in a transitional period
I'm not sure why we didn't prefer
loginList: Login | undefined | null
if we had cases where we are passingundefined
ornull
🤷