-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Do not show optimistic user to invite if he is already included in th… #22131
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@MonilBhavsar 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] |
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.
Seems like you have some Lint errors.
You also need to check all the fields in the checklist, even if they are not relevant to this PR
@yakubbaev Could you check all the author checklists? The PR should test on all platforms, so please complete the recordings on all platforms too. Also, please don't feel you need to comment on the reviewer checklist. Another note is that you need to link the issue correctly and follow the PR template, so the bot will pull the correct reviewer, the C+ first. Do this $ #20930 Don't do this |
As I mentioned in the original issue, I went on vacation on Monday and came back today. I'm going to finish off all the tasks this week. Done:
Plan:
Thank you guys for the follow-up, this is my first PR and I'll do my best not to repeat these mistakes in the future. |
Done:
|
Screen.Recording.2023-07-08.at.19.58.40.mp4@yakubbaev The user listed after searching for the phone number must be corrected. Like in the video, it should be listed user with the display name Luthfi. |
It seems to be consistent with the search on Home Screen. |
For the search page, it's a different issue. We don't share the same functionality in the |
Got it. Thanks for elaboration @mollfpr! Updated PR - now I'm hiding userToInvite instead of personalDetails when duplicated. |
@yakubbaev Could you resolve the conflict too? Thanks! |
Conflicts: src/pages/workspace/WorkspaceInvitePage.js
@mollfpr conflicts are resolved. Thanks for guiding! |
After selecting the user with the phone number, another option added. Screen.Recording.2023-07-11.at.14.47.55.mov |
const hideUserToInvite = | ||
this.state.userToInvite && | ||
_.find(personalDetailsWithoutSelected, ({login}) => { | ||
const phone = `${this.state.userToInvite.login}${CONST.SMS.DOMAIN}`; |
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.
Do you mean only to prevent this on phone numbers? Could we use OptionsListUtils.addSMSDomainIfPhoneNumber
instead?
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.
To fix duplicate when selecting user with phone number I switched to personalDetails instead of personalDetailsWithoutSelected.
Initially this was an issue with phone numbers only.
Reused OptionsListUtils.addSMSDomainIfPhoneNumber.
Thanks!
The test looks good, but I doubt whether the solution is fixing the root cause.
When there's already personal detail in the account, the
It's already worked with searching an email that in the I believe the root cause is in the |
App/src/libs/OptionsListUtils.js Line 775 in d6b2d07
This is the culprit. We check the |
@mollfpr that's right! Reverted fixing consequences and added fix to the generation site. |
@yakubbaev No worries! You still deserve it; We never get to fix the root cause without your RCA 😁 |
Screenshots/VideosWeb22131.Web.mp4Mobile Web - Chrome22131.mWeb-Chrome.mp4Mobile Web - Safari22131.mWeb-Safari.mp4Desktop22131.Desktop.mp4iOS22131.iOS.mp4Android22131.Android.mp4 |
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.
LGTM and tests well 👍
@yakubbaev Let's update the test case step with precondition that the account has chatting with an phone number user. Also, at the end of the step we should verify that the issue is not reproducible.
@mollfpr updated tests section with a precondition and verify steps 👍 |
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
✋ 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 staging by https://github.com/Gonals in version: 1.3.41-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.41-3 🚀
|
🚀 Deployed to staging by https://github.com/Gonals in version: 1.3.42-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.41-3 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀
|
@mollfpr @mallenexpensify @Gonals
Details
Enhance hasUnselectedUserToInvite with shouldIncludeUserToInvite that checks whether userToInvite is already included in the list.
Fixed Issues
$ #20930
PROPOSAL: #20930 (comment)
Tests
Precondition:
Account (user A) has been chatting with a phone number user (user B)
Offline tests
N/A
QA Steps
Same as tests section above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
mac-web.mov
Mobile Web - Chrome
Screen.Recording.2023-07-07.at.07.52.35.mov
Mobile Web - Safari
ios-web.mp4
Desktop
desktop.mov
iOS
ios.mp4
Android
android.mov