-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix 53003 on room invite sms shown #53240
Changes from 12 commits
4624eb4
f9927db
079e8c3
b10f5fb
bd015ed
423332c
3e28287
30912b1
b004644
845de1c
ab4b111
1c367fe
2c3415c
55e9738
e308a4b
864293c
63e30c3
f0987e0
3b39a32
6d93690
e79c907
3e216f8
de3750f
e36f20c
9c25564
b05d47b
fa48b45
21da67d
6bdeffb
953d533
b705118
135dde6
fe38203
8e43fe1
2e8a6ea
d8cf634
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -355,7 +355,7 @@ function getParticipantsOption(participant: ReportUtils.OptionData | Participant | |
const detail = getPersonalDetailsForAccountIDs([participant.accountID ?? -1], personalDetails)[participant.accountID ?? -1]; | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
const login = detail?.login || participant.login || ''; | ||
const displayName = PersonalDetailsUtils.getDisplayNameOrDefault(detail, LocalePhoneNumber.formatPhoneNumber(login) || participant.text); | ||
const displayName = LocalePhoneNumber.formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(detail, login || participant.text)); | ||
|
||
return { | ||
keyForList: String(detail?.accountID), | ||
|
@@ -405,7 +405,7 @@ function uniqFast(items: string[]): string[] { | |
function getLastActorDisplayName(lastActorDetails: Partial<PersonalDetails> | null, hasMultipleParticipants: boolean) { | ||
return hasMultipleParticipants && lastActorDetails && lastActorDetails.accountID !== currentUserAccountID | ||
? // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
lastActorDetails.firstName || PersonalDetailsUtils.getDisplayNameOrDefault(lastActorDetails) | ||
lastActorDetails.firstName || LocalePhoneNumber.formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(lastActorDetails)) | ||
: ''; | ||
} | ||
|
||
|
@@ -503,7 +503,7 @@ function getLastMessageTextForReport(report: OnyxEntry<Report>, lastActorDetails | |
case CONST.REPORT.ARCHIVE_REASON.REMOVED_FROM_POLICY: | ||
case CONST.REPORT.ARCHIVE_REASON.POLICY_DELETED: { | ||
lastMessageTextFromReport = Localize.translate(preferredLocale, `reportArchiveReasons.${archiveReason}`, { | ||
displayName: PersonalDetailsUtils.getDisplayNameOrDefault(lastActorDetails), | ||
displayName: LocalePhoneNumber.formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(lastActorDetails)), | ||
policyName: ReportUtils.getPolicyName(report, false, policy), | ||
}); | ||
break; | ||
|
@@ -1333,20 +1333,20 @@ function getShareLogOptions(options: OptionList, betas: Beta[] = []): Options { | |
* Build the IOUConfirmation options for showing the payee personalDetail | ||
*/ | ||
function getIOUConfirmationOptionsFromPayeePersonalDetail(personalDetail: OnyxEntry<PersonalDetails>, amountText?: string): PayeePersonalDetails { | ||
const formattedLogin = LocalePhoneNumber.formatPhoneNumber(personalDetail?.login ?? ''); | ||
const login = personalDetail?.login ?? ''; | ||
return { | ||
text: PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, formattedLogin), | ||
alternateText: formattedLogin || PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, '', false), | ||
text: LocalePhoneNumber.formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, login)), | ||
alternateText: LocalePhoneNumber.formatPhoneNumber(login || PersonalDetailsUtils.getDisplayNameOrDefault(personalDetail, '', false)), | ||
icons: [ | ||
{ | ||
source: personalDetail?.avatar ?? FallbackAvatar, | ||
name: personalDetail?.login ?? '', | ||
name: LocalePhoneNumber.formatPhoneNumber(personalDetail?.login ?? ''), | ||
type: CONST.ICON_TYPE_AVATAR, | ||
id: personalDetail?.accountID, | ||
}, | ||
], | ||
descriptiveText: amountText ?? '', | ||
login: personalDetail?.login ?? '', | ||
login: LocalePhoneNumber.formatPhoneNumber(personalDetail?.login ?? ''), | ||
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. Please avoid updating the login here. Instead, we will only update the 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. ok, i did a test on spliting expenses and i agree. It will be good if you can also confirm by a visual test |
||
accountID: personalDetail?.accountID ?? -1, | ||
keyForList: String(personalDetail?.accountID ?? -1), | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2156,11 +2156,11 @@ function getDisplayNameForParticipant( | |
|
||
// If the user's personal details (first name) should be hidden, make sure we return "hidden" instead of the short name | ||
if (shouldFallbackToHidden && longName === hiddenTranslation) { | ||
return longName; | ||
return LocalePhoneNumber.formatPhoneNumber(longName); | ||
} | ||
|
||
const shortName = personalDetails.firstName ? personalDetails.firstName : longName; | ||
return shouldUseShortForm ? shortName : longName; | ||
return LocalePhoneNumber.formatPhoneNumber(shouldUseShortForm ? shortName : longName); | ||
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. @Kalydosos I am trying to test this change with various users those have phone numbers, but I’m unsure why we need LocalePhoneNumber.formatPhoneNumber here. Each time I log and check, both shortName and longName are already formatted (no SMS and no country code prefix). Could you let me know if there’s a way to test this specific change? 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. |
||
} | ||
|
||
function getParticipantsAccountIDsForDisplay(report: OnyxEntry<Report>, shouldExcludeHidden = false, shouldExcludeDeleted = false, shouldForceExcludeCurrentUser = false): number[] { | ||
|
@@ -2344,7 +2344,7 @@ function getIcons( | |
const actorIcon = { | ||
id: actorAccountID, | ||
source: personalDetails?.[actorAccountID ?? -1]?.avatar ?? FallbackAvatar, | ||
name: actorDisplayName, | ||
name: LocalePhoneNumber.formatPhoneNumber(actorDisplayName), | ||
type: CONST.ICON_TYPE_AVATAR, | ||
fallbackIcon: personalDetails?.[parentReportAction?.actorAccountID ?? -1]?.fallbackIcon, | ||
}; | ||
|
@@ -3804,7 +3804,7 @@ function getInvoicePayerName(report: OnyxEntry<Report>, invoiceReceiverPolicy?: | |
const isIndividual = invoiceReceiver?.type === CONST.REPORT.INVOICE_RECEIVER_TYPE.INDIVIDUAL; | ||
|
||
if (isIndividual) { | ||
return PersonalDetailsUtils.getDisplayNameOrDefault(allPersonalDetails?.[invoiceReceiver.accountID]); | ||
return LocalePhoneNumber.formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(allPersonalDetails?.[invoiceReceiver.accountID])); | ||
} | ||
|
||
return getPolicyName(report, false, invoiceReceiverPolicy ?? allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${invoiceReceiver?.policyID}`]); | ||
|
@@ -3896,7 +3896,7 @@ function getInvoicesChatName(report: OnyxEntry<Report>, receiverPolicy: OnyxEntr | |
} | ||
|
||
if (isIndividual) { | ||
return PersonalDetailsUtils.getDisplayNameOrDefault(allPersonalDetails?.[invoiceReceiverAccountID]); | ||
return LocalePhoneNumber.formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(allPersonalDetails?.[invoiceReceiverAccountID])); | ||
} | ||
|
||
return getPolicyName(report, false, invoiceReceiverPolicy); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; | |
import * as Report from '@libs/actions/Report'; | ||
import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; | ||
import type {RoomMembersNavigatorParamList} from '@libs/Navigation/types'; | ||
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils'; | ||
import * as PolicyUtils from '@libs/PolicyUtils'; | ||
import * as ReportUtils from '@libs/ReportUtils'; | ||
import Navigation from '@navigation/Navigation'; | ||
|
@@ -33,7 +34,7 @@ type RoomMemberDetailsPagePageProps = WithReportOrNotFoundProps & PlatformStackS | |
|
||
function RoomMemberDetailsPage({report, route}: RoomMemberDetailsPagePageProps) { | ||
const styles = useThemeStyles(); | ||
const {translate} = useLocalize(); | ||
const {formatPhoneNumber, translate} = useLocalize(); | ||
const StyleUtils = useStyleUtils(); | ||
const currentUserPersonalDetails = useCurrentUserPersonalDetails(); | ||
const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); | ||
|
@@ -47,7 +48,7 @@ function RoomMemberDetailsPage({report, route}: RoomMemberDetailsPagePageProps) | |
const member = report?.participants?.[accountID]; | ||
const details = personalDetails?.[accountID] ?? ({} as PersonalDetails); | ||
const fallbackIcon = details.fallbackIcon ?? ''; | ||
const displayName = details.displayName ?? ''; | ||
const displayName = formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)); | ||
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. IMO, this is not needed here. We are already using 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. @jayeshmangwani i'll check that 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. @Kalydosos Please keep the discussion either here or on Slack. 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. ok, got it, i was trying not to pollute the PR with questions about your question, got it 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. Haha, no problem! Keeping discussion in one place makes things faster, so 😃 . 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. i will do a new merge with main 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. @jayeshmangwani i did a test again after the new merge and it's the same result as in the video above 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. @Kalydosos For me, it’s working fine both with and without using the 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. @Kalydosos Please leave the 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. @jayeshmangwani I made a test again for this one and i still have the same result., any change on your side for this ? |
||
const isSelectedMemberCurrentUser = accountID === currentUserPersonalDetails?.accountID; | ||
const isSelectedMemberOwner = accountID === report.ownerAccountID; | ||
const shouldDisableRemoveUser = (ReportUtils.isPolicyExpenseChat(report) && PolicyUtils.isUserPolicyAdmin(policy, details.login)) || isSelectedMemberCurrentUser || isSelectedMemberOwner; | ||
|
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 this change necessary here? Instead, we can simply use
LocalePhoneNumber.formatPhoneNumber
directly where theicons.name
key is being used.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.
same here, i did a test on spliting expenses and i agree. It will be good if you can also confirm by a visual test