Fix - Remove @expensify.sms from SMS mentions #37559

FitseTLT · 2024-02-29T19:12:11Z

Details

Fixed Issues

$ #33902
PROPOSAL: #33902 (comment)

Tests

Precondition:
Run npm i to install the recent changes on expensify-common

Go to any room and type @
Select a user with phone number login
Verify that the phone number inserted in the compose doesn't have the sms domain suffix @expensify.sms
Send message and verify that the phone number is correct mention doesn't have the suffix and hover hover it and check the tooltip doesn't have it too
Click on it and verify the profile page displays the phone number without the suffix
On the system message click on invite the user to the room and on the message that indicates the user is invited check the tooltip displays without the suffix and click on it and verify the details page displays it without the suffix

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.mp4

Android: mWeb Chrome

2024-02-29.22-09-43.mp4

iOS: Native

ios.mp4

iOS: mWeb Safari

ios.web.mp4

MacOS: Chrome / Safari

web.mp4

MacOS: Desktop

desktop.mp4

melvin-bot · 2024-02-29T19:12:16Z

@c3024 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

FitseTLT · 2024-02-29T19:20:26Z

@c3024 @jasperhuangg There are places I used formatPhoneNumber on display names this is because in cases where the user has no display name BE was populating it with the login so the sms suffix was appearing in profile page or tooltips.

I mean the disadvantage of this is if the user inputs something that resembles a phone number it would format it too but there is already an instance we have used this like here

App/src/pages/RoomMembersPage.tsx

Lines 192 to 193 in 28f2bdf

    
           text: formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)), 
        
           alternateText: details?.login ? formatPhoneNumber(details.login) : '',

otherwise the data BE returning should be fixed.

FitseTLT · 2024-02-29T19:27:17Z

The failing tests are from main

c3024 · 2024-03-01T08:17:33Z

@FitseTLT

Changes look good. 👍

Yes, we use formatNumber extensively. I think a user sending some message resembling a phone number gets formatted with this but it is something that cannot be avoided IMO.

I think we should remove @expensify.sms everywhere on the frontend. These still have the sms part.

This can be fixed by adding formatNumber for displayName here

App/src/libs/ReportUtils.ts

Line 1683 in 0c09803

displayName,

We should add formatNumber for result.alternateText at these three places

App/src/libs/SidebarUtils.ts

Line 328 in 0c09803

result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;

App/src/libs/SidebarUtils.ts

Line 330 in 0c09803

    
           result.alternateText = lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');

App/src/libs/SidebarUtils.ts

Line 357 in 0c09803

result.alternateText = lastMessageText || formattedLogin;

Add formatPhoneNumber here

App/src/components/SelectionList/UserListItem.tsx

Line 84 in 0c09803

text={item.text}

and here

App/src/components/SelectionList/UserListItem.tsx

Line 96 in 0c09803

text={item.alternateText}

Feel free to make different changes if there are better ways.

Please fix conflicts and merge main. There are no typechecks failing on main now.

FitseTLT · 2024-03-01T12:52:04Z

@c3024 Got it! The proper and centralized place would be it to apply it here

App/src/libs/PersonalDetailsUtils.ts

Lines 27 to 28 in 450d460

    
           function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails> | null, defaultValue = '', shouldFallbackToHidden = true): string { 
        
               const displayName = passedPersonalDetails?.displayName ? passedPersonalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';

@jasperhuangg I need a confirmation from U to formatPhoneNumber on the display name here

App/src/libs/PersonalDetailsUtils.ts

Lines 27 to 28 in 450d460

    
           function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails> | null, defaultValue = '', shouldFallbackToHidden = true): string { 
        
               const displayName = passedPersonalDetails?.displayName ? passedPersonalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';

This is because in some instances BE is returning the phone number with the sms domain on the display name of the persoal detail as I explained it here #37559 (comment)

FitseTLT · 2024-03-01T13:51:24Z

I suspect that the reason the BE is returning the displayName same as the login is to avoid Hidden on display b/c when the display name is empty we display Hidden
One approach I suggest is to only formatPhoneNumber or removeSMSDomain when the display name is same as login and the login isSMSLogin here

App/src/libs/PersonalDetailsUtils.ts

Lines 27 to 28 in 450d460

    
           function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails> | null, defaultValue = '', shouldFallbackToHidden = true): string { 
        
               const displayName = passedPersonalDetails?.displayName ? passedPersonalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';

jasperhuangg · 2024-03-04T18:12:15Z

I suspect that the reason the BE is returning the displayName same as the login is to avoid Hidden on display b/c when the display name is empty we display Hidden One approach I suggest is to only formatPhoneNumber or removeSMSDomain when the display name is same as login and the login isSMSLogin here

App/src/libs/PersonalDetailsUtils.ts

Lines 27 to 28 in 450d460

function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails> | null, defaultValue = '', shouldFallbackToHidden = true): string {

const displayName = passedPersonalDetails?.displayName ? passedPersonalDetails.displayName.replace(CONST.REGEX.MERGED_ACCOUNT_PREFIX, '') : '';

@FitseTLT I think this is a sound approach, thanks for proposing!

jasperhuangg · 2024-03-05T02:43:35Z

@c3024 friendly bump on the review–we'd like to avoid conflicts where possible.

c3024 · 2024-03-05T02:59:26Z

@jasperhuangg I had reviewed the PR and suggested changes here. Changes look fine but there are a few places where the @expensify.sms still remains.

ReportScreen header, LHN username and last message text, and Search results usernames and last message texts need to be fixed as mentioned in this comment.

Comparing username and login to remove the @expensify.sms works for the header detail and username details. But the last message texts should be stripped of the sms part with the formatPhoneNumber or removeSMSDomain in all cases.

cc: @FitseTLT

jasperhuangg · 2024-03-05T17:41:25Z

@c3024 Got it, sorry I had assumed that we had already addressed those fixes, appreciate you being thorough.

@FitseTLT please prioritize fixing those other parts of this PR.

FitseTLT · 2024-03-06T13:48:28Z

@c3024 U can proceed

c3024 · 2024-03-06T14:05:24Z

@FitseTLT

Still not fixed for Search

FitseTLT · 2024-03-06T16:35:10Z

@c3024 Updated

c3024 · 2024-03-07T07:56:22Z

@FitseTLT

If you use Str.removeSMSDomain in UserListItem, you should check if the text and/or alternate text exist before applying removeSMSDomain.

It is crashing.

FitseTLT · 2024-03-07T14:17:08Z

@c3024 Updated

c3024 · 2024-03-08T02:06:42Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

mentionsAndroid.mp4

Android: mWeb Chrome

mentionsAndroidChrome.mp4

iOS: Native

mentionsiOS.mp4

iOS: mWeb Safari

mentionsiOSSafari.mp4

MacOS: Chrome / Safari

mentionsChrome1.mp4

mentionsChrome2.mp4

mentionsOffline.mp4

MacOS: Desktop

mentionsDesktop.mp4

c3024

LGTM

jasperhuangg · 2024-03-11T20:15:37Z

@FitseTLT sorry, conflicts–do you mind fixing them?

FitseTLT · 2024-03-11T20:18:14Z

@FitseTLT sorry, conflicts–do you mind fixing them?

Resolved

melvin-bot · 2024-03-12T17:07:56Z

@jasperhuangg looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2024-03-12T17:14:46Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-03-12T18:49:54Z

🚀 Deployed to staging by https://github.com/jasperhuangg in version: 1.4.51-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-03-13T19:45:09Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.51-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

rayane-djouah · 2024-05-03T17:38:46Z

Hey, this PR caused a bug: #39305
It has been handled but just FYI,
This PR removed @expensify.sms from SMS mentions but missed the thread report header case:

App/src/libs/ReportUtils.ts

Line 2620 in 112c2b2

: parentReportAction?.message?.[0]?.text ?? ''

More details here - #39305 (comment)

FitseTLT added 4 commits February 29, 2024 20:16

remove sms domain from phone mention

25a0621

remove sms domain from display name

8765e9f

remove the domain from tooltip text

f7da703

format the tooltip text

28f2bdf

FitseTLT requested a review from a team as a code owner February 29, 2024 19:12

melvin-bot bot requested review from c3024 and removed request for a team February 29, 2024 19:12

FitseTLT added 2 commits February 29, 2024 22:22

format phone number on message menu

1e0e468

Merge branch 'main' into fix-remove-sms-domain-in-phone-mentions

9e86b30

Merge branch 'main' into fix-remove-sms-domain-in-phone-mentions

450d460

CortneyOfstad mentioned this pull request Mar 1, 2024

[HOLD for payment 2024-03-20] [$500] HIGH: Remove @expensify.sms from SMS mentions #33902

Closed

6 tasks

FitseTLT added 4 commits March 6, 2024 14:58

Merge branch 'main' into fix-remove-sms-domain-in-phone-mentions

246e4ff

Merge branch 'main' into fix-remove-sms-domain-in-phone-mentions

1f8899e

fix package lock json

d61c7ae

remove sms domain from display names

4298e4d

remove sms from user list item

8d4b777

add null checker

1c5ed54

jasperhuangg approved these changes Mar 7, 2024

View reviewed changes

c3024 approved these changes Mar 8, 2024

View reviewed changes

melvin-bot bot requested a review from jasperhuangg March 8, 2024 06:25

jasperhuangg previously approved these changes Mar 11, 2024

View reviewed changes

FitseTLT added 2 commits March 11, 2024 23:16

Merge branch 'main' into fix-remove-sms-domain-in-phone-mentions

edd742b

resolve conflict

112c2b2

FitseTLT dismissed jasperhuangg’s stale review via 112c2b2 March 11, 2024 20:17

jasperhuangg approved these changes Mar 12, 2024

View reviewed changes

jasperhuangg merged commit 8c5faee into Expensify:main Mar 12, 2024
18 checks passed

melvin-bot bot added the Emergency label Mar 12, 2024

github-actions bot mentioned this pull request Mar 12, 2024

Deploy Checklist: New Expensify 2024-03-12 #38162

Closed

84 tasks

situchan mentioned this pull request Mar 12, 2024

[Situ is out until May] [HOLD for payment 2024-03-22] [HOLD for payment 2024-03-20] Chat - Emoji is not showing in chat #38169

Closed

6 tasks

lanitochka17 mentioned this pull request Mar 12, 2024

[$250] Mention - @expensify.sms is copied along with phone number when copied to clipboard #38187

Closed

6 tasks

rayane-djouah mentioned this pull request May 3, 2024

[$500] Android - Thread-Mention phone number on reply thread shows expensify.sms in header #39305

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix - Remove @expensify.sms from SMS mentions #37559

Fix - Remove @expensify.sms from SMS mentions #37559

FitseTLT commented Feb 29, 2024

melvin-bot bot commented Feb 29, 2024

FitseTLT commented Feb 29, 2024

FitseTLT commented Feb 29, 2024

c3024 commented Mar 1, 2024

FitseTLT commented Mar 1, 2024 •

edited

Loading

FitseTLT commented Mar 1, 2024

jasperhuangg commented Mar 4, 2024

jasperhuangg commented Mar 5, 2024

c3024 commented Mar 5, 2024 •

edited

Loading

jasperhuangg commented Mar 5, 2024

FitseTLT commented Mar 6, 2024

c3024 commented Mar 6, 2024

FitseTLT commented Mar 6, 2024

c3024 commented Mar 7, 2024 •

edited

Loading

FitseTLT commented Mar 7, 2024

c3024 commented Mar 8, 2024 •

edited

Loading

c3024 left a comment

jasperhuangg commented Mar 11, 2024

FitseTLT commented Mar 11, 2024

melvin-bot bot commented Mar 12, 2024

OSBotify commented Mar 12, 2024

OSBotify commented Mar 12, 2024

OSBotify commented Mar 13, 2024

rayane-djouah commented May 3, 2024

Fix - Remove @expensify.sms from SMS mentions #37559

Fix - Remove @expensify.sms from SMS mentions #37559

Conversation

FitseTLT commented Feb 29, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Feb 29, 2024

FitseTLT commented Feb 29, 2024

FitseTLT commented Feb 29, 2024

c3024 commented Mar 1, 2024

FitseTLT commented Mar 1, 2024 • edited Loading

FitseTLT commented Mar 1, 2024

jasperhuangg commented Mar 4, 2024

jasperhuangg commented Mar 5, 2024

c3024 commented Mar 5, 2024 • edited Loading

jasperhuangg commented Mar 5, 2024

FitseTLT commented Mar 6, 2024

c3024 commented Mar 6, 2024

FitseTLT commented Mar 6, 2024

c3024 commented Mar 7, 2024 • edited Loading

FitseTLT commented Mar 7, 2024

c3024 commented Mar 8, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

c3024 left a comment

Choose a reason for hiding this comment

jasperhuangg commented Mar 11, 2024

FitseTLT commented Mar 11, 2024

melvin-bot bot commented Mar 12, 2024

OSBotify commented Mar 12, 2024

OSBotify commented Mar 12, 2024

OSBotify commented Mar 13, 2024

rayane-djouah commented May 3, 2024

FitseTLT commented Mar 1, 2024 •

edited

Loading

c3024 commented Mar 5, 2024 •

edited

Loading

c3024 commented Mar 7, 2024 •

edited

Loading

c3024 commented Mar 8, 2024 •

edited

Loading