don't show user while searching #4647

aman-atg · 2021-08-13T17:43:26Z

Details

#4325 (comment)

Fixed Issues

$ #4325

Tests || QA Steps

Login to newDot
Search for your email/phone
Your own profile will NOT appear as an option

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

src/libs/OptionsListUtils.js

aman-atg · 2021-08-16T10:23:55Z

@Julesssss
Ready for review 👀

Julesssss · 2021-08-16T11:22:56Z

@aman-atg looks like a test failure still

aman-atg · 2021-08-16T11:46:02Z

@Julesssss
Yeah, I found out that replacing this line with below code might work

App/src/libs/OptionsListUtils.js

Line 742 in 8b10c1c

const {loginList} = currentUser;

    const loginList = _.isEmpty(currentUser) ? [] : currentUser.loginList;

But, I'm not sure so please correct me if I'm wrong here.

Julesssss · 2021-08-16T13:22:52Z

I can't see why your change would cause this 😕 Any ideas?

aman-atg · 2021-08-16T13:29:46Z

I can't see why your change would cause this

Unfortunately, I had the exact same thought and I still can't find a reason behind that!

Julesssss · 2021-08-16T13:49:31Z

The only thing I can think of is:

The function ordering was somehow important
Something on main changed and broke when you merged it into this PR

aman-atg · 2021-08-16T13:55:26Z

The function ordering was somehow important

I think this could be the reason.

Testing locally to find out the root cause.

aman-atg · 2021-08-16T14:44:34Z

@Julesssss
I think now I understand why this is happening.

App/tests/unit/OptionsListUtilsTest.js

Line 216 in 8b10c1c

it('getNewChatOptions()', () => {

App/tests/unit/OptionsListUtilsTest.js

Line 287 in 8b10c1c

it('getNewGroupOptions()', () => {

These two tests are now failing cause now they are making use of isCurrentUser function through getOptions function. And Earlier we weren't using isCurrentUser inside getOptions so it was not being tested.

App/src/libs/OptionsListUtils.js

Lines 534 to 544 in 8b10c1c

    
           function getNewChatOptions( 
        
               reports, 
        
               personalDetails, 
        
               searchValue = '', 
        
               excludeConcierge, 
        
               betas, 
        
           ) { 
        
               return getOptions(reports, personalDetails, {}, 0, { 
        
                   betas, 
        
                   searchValue, 
        
                   excludeDefaultRooms: true,

App/src/libs/OptionsListUtils.js

Lines 595 to 605 in 8b10c1c

    
           function getNewGroupOptions( 
        
               reports, 
        
               personalDetails, 
        
               searchValue = '', 
        
               selectedOptions = [], 
        
               excludeConcierge, 
        
               betas, 
        
           ) { 
        
               return getOptions(reports, personalDetails, {}, 0, { 
        
                   betas, 
        
                   searchValue,

Root cause must be this. #4647 (comment)

Julesssss · 2021-08-16T15:30:17Z

Ah I see, please make the fix in that case 👍

src/libs/OptionsListUtils.js

Julesssss

Thanks @aman-atg -- it's looking good, but now that you've modified isCurrentUser I have one last suggestion:

It makes no sense to pass an object when we could simply pass login. Do you agree? There seems no reason to require an object, it's a bit confusing.

&& !isCurrentUser({login: searchValue})

could be simplified to

&& !isCurrentUser(searchValue)

Julesssss · 2021-08-17T14:19:04Z

src/libs/OptionsListUtils.js

+    // Checking userDetailsLogin against to current user login options.
+    while (index < loginList.length && !result) {
+        if (loginList[index].partnerUserID.toLowerCase() === userDetailsLogin.toLowerCase()) {
+            result = true;
+        }
+        index++;
+    }
+    return result;


The while loop is also an odd choice, but as it doesn't affect your changes we can ignore this.

aman-atg · 2021-08-17T14:30:36Z

It makes no sense to pass an object when we could simply pass login. Do you agree? There seems no reason to require an object, it's a bit confusing.
&& !isCurrentUser({login: searchValue})
could be simplified to
&& !isCurrentUser(searchValue)

Yeah, I agree that it makes sense to simplify it but changing it here will require changes in other files as well.
Maybe the original author can give some insights on this?

This is the PR that introduced this function : #4063

aman-atg · 2021-08-19T09:01:23Z

Hi @Julesssss, so what should we do here?

Julesssss · 2021-08-19T09:32:36Z

Sorry, we've been busy on internal issues and I forgot about this review.

Okay, fair point. Let's keep the changes as they are.

Julesssss · 2021-08-19T10:08:55Z

Confirmed on iOS & Desktop

OSBotify · 2021-08-19T10:09:48Z

✋ 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 · 2021-08-30T17:39:00Z

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

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

botify · 2021-09-01T01:26:00Z

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-08. 🎊

don't show user while searching

278c7ef

aman-atg requested a review from a team as a code owner August 13, 2021 17:43

MelvinBot requested review from Julesssss and removed request for a team August 13, 2021 17:43

Julesssss reviewed Aug 16, 2021

View reviewed changes

src/libs/OptionsListUtils.js Outdated Show resolved Hide resolved

aman-atg added 2 commits August 16, 2021 15:38

Merge branch 'Expensify:main' into aman-atg-dontShowUserOnSearch

550bcf0

add requested changes

09ebd0a

Julesssss previously approved these changes Aug 16, 2021

View reviewed changes

fix logic

b186790

aman-atg dismissed Julesssss’s stale review via b186790 August 16, 2021 15:36

aman-atg requested a review from Julesssss August 16, 2021 16:16

Julesssss reviewed Aug 17, 2021

View reviewed changes

src/libs/OptionsListUtils.js Outdated Show resolved Hide resolved

Julesssss reviewed Aug 17, 2021

View reviewed changes

src/libs/OptionsListUtils.js Outdated Show resolved Hide resolved

fix

1ae25f2

Julesssss reviewed Aug 17, 2021

View reviewed changes

Julesssss approved these changes Aug 19, 2021

View reviewed changes

Julesssss merged commit 064e582 into Expensify:main Aug 19, 2021

This was referenced Aug 20, 2021

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

Deploy Checklist: New Expensify 2021-08-25 #4828

Closed

aman-atg deleted the aman-atg-dontShowUserOnSearch branch August 27, 2021 14:40

don't show user while searching #4647

don't show user while searching #4647

Uh oh!

Conversation

aman-atg commented Aug 13, 2021 • edited by Julesssss Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests || QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Uh oh!

aman-atg commented Aug 16, 2021

Uh oh!

Julesssss commented Aug 16, 2021

Uh oh!

aman-atg commented Aug 16, 2021

Uh oh!

Julesssss commented Aug 16, 2021

Uh oh!

aman-atg commented Aug 16, 2021

Uh oh!

Julesssss commented Aug 16, 2021

Uh oh!

aman-atg commented Aug 16, 2021

Uh oh!

aman-atg commented Aug 16, 2021

Uh oh!

Julesssss commented Aug 16, 2021

Uh oh!

Uh oh!

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

Julesssss Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

aman-atg commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aman-atg commented Aug 19, 2021

Uh oh!

Julesssss commented Aug 19, 2021

Uh oh!

Julesssss commented Aug 19, 2021

Uh oh!

OSBotify commented Aug 19, 2021

Uh oh!

OSBotify commented Aug 30, 2021

Uh oh!

botify commented Sep 1, 2021

Uh oh!

Uh oh!

aman-atg commented Aug 13, 2021 •

edited by Julesssss

Loading

aman-atg commented Aug 17, 2021 •

edited

Loading