Skip to content

don't show user while searching #4647

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

Merged
merged 5 commits into from
Aug 19, 2021

Conversation

aman-atg
Copy link
Contributor

@aman-atg aman-atg commented Aug 13, 2021

Details

#4325 (comment)

Fixed Issues

$ #4325

Tests || QA Steps

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

Tested On

  • Web
  • Mobile Web
  • Android
  • Please test on these two
  • Desktop
  • iOS

Screenshots

Web

image

Mobile Web

mWeb

Desktop

iOS

Android

mobile

@aman-atg aman-atg requested a review from a team as a code owner August 13, 2021 17:43
@MelvinBot MelvinBot requested review from Julesssss and removed request for a team August 13, 2021 17:43
@aman-atg
Copy link
Contributor Author

@Julesssss
Ready for review 👀

Julesssss
Julesssss previously approved these changes Aug 16, 2021
@Julesssss
Copy link
Contributor

@aman-atg looks like a test failure still

@aman-atg
Copy link
Contributor Author

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

const {loginList} = currentUser;

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

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

image

@Julesssss
Copy link
Contributor

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

@aman-atg
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

  • The function ordering was somehow important

I think this could be the reason.

Testing locally to find out the root cause.

@aman-atg
Copy link
Contributor Author

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

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

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.

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

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

Root cause must be this. #4647 (comment)

@Julesssss
Copy link
Contributor

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

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

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)

Comment on lines +300 to +307
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@aman-atg
Copy link
Contributor Author

aman-atg commented Aug 17, 2021

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
Copy link
Contributor Author

Hi @Julesssss, so what should we do here?

@Julesssss
Copy link
Contributor

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
Copy link
Contributor

Confirmed on iOS & Desktop

@Julesssss Julesssss merged commit 064e582 into Expensify:main Aug 19, 2021
@OSBotify
Copy link
Contributor

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

@aman-atg aman-atg deleted the aman-atg-dontShowUserOnSearch branch August 27, 2021 14:40
@OSBotify
Copy link
Contributor

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

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

@botify
Copy link

botify commented Sep 1, 2021

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. 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants