-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
don't show user while searching #4647
Conversation
@Julesssss |
@aman-atg looks like a test failure still |
@Julesssss App/src/libs/OptionsListUtils.js Line 742 in 8b10c1c
const loginList = _.isEmpty(currentUser) ? [] : currentUser.loginList; But, I'm not sure so please correct me if I'm wrong here. |
I can't see why your change would cause this 😕 Any ideas? |
Unfortunately, I had the exact same thought and I still can't find a reason behind that! |
The only thing I can think of is:
|
I think this could be the reason. Testing locally to find out the root cause. |
@Julesssss App/tests/unit/OptionsListUtilsTest.js Line 216 in 8b10c1c
App/tests/unit/OptionsListUtilsTest.js Line 287 in 8b10c1c
These two tests are now failing cause now they are making use of App/src/libs/OptionsListUtils.js Lines 534 to 544 in 8b10c1c
App/src/libs/OptionsListUtils.js Lines 595 to 605 in 8b10c1c
Root cause must be this. #4647 (comment) |
Ah I see, please make the fix in that case 👍 |
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.
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)
// 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; |
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.
The while loop is also an odd choice, but as it doesn't affect your changes we can ignore this.
Yeah, I agree that it makes sense to simplify it but changing it here will require changes in other files as well. This is the PR that introduced this function : #4063 |
Hi @Julesssss, so what should we do here? |
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. |
Confirmed on iOS & Desktop |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀
|
This has been deployed to production and is now subject to a 7-day regression period. |
Details
#4325 (comment)
Fixed Issues
$ #4325
Tests || QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android