fix: Remove GBR from the account settings on initial signup.#49004
Merged
techievivek merged 11 commits intoExpensify:mainfrom Oct 30, 2024
Merged
fix: Remove GBR from the account settings on initial signup.#49004techievivek merged 11 commits intoExpensify:mainfrom
techievivek merged 11 commits intoExpensify:mainfrom
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@marcochavezf 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Contributor
Author
|
Sorry @marcochavezf for the bump, I mistakenly added wrong url in in the fixed issue section. @ntdiary will be the reviewer here. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Contributor
|
I'll review this PR in about 12 hours, as there is another higher-priority pr that needs to be reviewed first. :) |
ntdiary
reviewed
Sep 13, 2024
| const loginNames = Object.keys(loginList ?? {}); | ||
| const navigateBackTo = route?.params?.backTo; | ||
|
|
||
| const filteredLoginNames = loginNames.filter((loginName) => { |
Contributor
There was a problem hiding this comment.
Why is filter needed here?
Contributor
Author
There was a problem hiding this comment.
We need to filter out the temporary validation email to avoid showing the GBR when there is only one contact method. For example, when logging in with a phone number and adding a new contact method, the default contact method is added without the SMS domain.
Monosnap.screencast.2024-09-15.17-47-33.mp4
Contributor
There was a problem hiding this comment.
Thank you for your detailed explanation! I just asked in another PR whether this behavior is normal. :)
Contributor
Author
There was a problem hiding this comment.
@ntdiary, can you please re-test these, I have made the necessary changes.
Contributor
There was a problem hiding this comment.
Extra key in the loginList is no longer being added now. :)
src/libs/UserUtils.ts
Outdated
| return login.partnerUserID || pendingAction; | ||
| }); | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| return !Object.values(filteredLoginList ?? {}).every((field) => field.validatedDate || currentUserLogin === field.partnerUserID); |
Contributor
There was a problem hiding this comment.
If the above filter is necessary, then filteredLoginList will be an array, so Object.values isn't needed. :)
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
techievivek
requested review from
techievivek
and removed request for
marcochavezf
Contributor
|
Hi, @Krishna2323 @ntdiary any updates here? Are we blocked on something here? |
Contributor
@techievivek, not a major issue. The current PR adds a filter function compared to the original proposal, because when a user adds a new contact method, another PR accidentally added an extra entry to the |
Contributor
Contributor
|
@ntdiary @Krishna2323, can we look into this now? Given the other PR is merged. |
Contributor
Author
|
Thanks for the update @techievivek, I will provide updates on this today. |
Contributor
|
More conflicts 🏃 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
ntdiary
suggested changes
Oct 22, 2024
Contributor
ntdiary
left a comment
ntdiary
left a comment
There was a problem hiding this comment.
some minor suggestions. :)
| } else if (!login?.validatedDate) { | ||
| } else if (!login?.validatedDate && !isDefaultContactMethod) { | ||
| indicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO; | ||
| } else if (!login?.validatedDate && isDefaultContactMethod && filteredLoginNames.length > 1) { |
Contributor
There was a problem hiding this comment.
If I remember correctly, if the user actively adds a second contact method, we can display GBR on the unverified primary login method. So, I think using loginNames.length here should suffice, and we can remove filteredLoginNames.
BTW, so far, to add a second contact method, we need to enter the verification code sent to our primary contact method, I personally think this effectively verifies the primary login method. However, it's clear that the backend doesn't have this logic yet, mabe we can discuss it in a separate issue. :)
cc @techievivek
Contributor
There was a problem hiding this comment.
If I remember correctly, if the user actively adds a second contact method, we can display GBR on the unverified primary login method. So, I think using
loginNames.lengthhere should suffice, and we can removefilteredLoginNames.
@Krishna2323, any different thoughts about this suggestion? 😄
Contributor
There was a problem hiding this comment.
BTW, so far, to add a second contact method, we need to enter the verification code sent to our primary contact method, I personally think this effectively verifies the primary login method. However, it's clear that the backend doesn't have this logic yet, mabe we can discuss it in a separate issue. :)
Aaah, good call, I think all this are showing up after we introduce unvalidated signups to our platform. I agree we an improve here. I will create an internal GH to work on this, thanks.
Contributor
Author
There was a problem hiding this comment.
any different thoughts about this suggestion? 😄
No, I agree with that, will update that today.
Contributor
Author
There was a problem hiding this comment.
@ntdiary, sorry for delay, I have updated the code to show GRB on the default contact when we have more than 2 logins in the list.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: Native49004-android-native.mp4Android: mWeb Chrome49004-android-web.mp4iOS: Native49004-ios-native.mp4iOS: mWeb Safari49004-ios-safari.mp4MacOS: Chrome / Safari49004-web.mp4MacOS: Desktop49004-desktop.mp4 |
Contributor
|
BTW, @Krishna2323, I encountered a strange problem on the Android web (1 time), the contact methods page didn’t display the primary contact method, but I couldn’t reproduce it in subsequent new tests, not sure if you can reproduce it. 😂 49004-android-web-problem.mp4 |
Contributor
Just to add, I checked the |
techievivek
approved these changes
Oct 30, 2024
Contributor
techievivek
left a comment
techievivek
left a comment
There was a problem hiding this comment.
Thanks for the changes.
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. |
github-actions
bot
added
the
DeployBlockerCash
Contributor
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
This PR won't impact performance. |
Contributor
|
Yeah, I can't see any correlation on why it would impact |
techievivek
removed
the
DeployBlockerCash
Contributor
|
🚀 Deployed to staging by https://github.com/techievivek in version: 9.0.56-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.56-9 🚀
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #47863
PROPOSAL: #47863 (comment)
Tests
Profiletab in the Account section/settings/profile> Verify GBR is not shown onContact methodfield in thepublic profiledetail sectionContact method> Verify GBR is not shown on the default contact methodNew contact method> Enter email > ClickAddcontact-methodspage and verify GBR is shown on the default contact methodProfiletab in the Account section &Contact methodfield in thepublic profiledetail section.Offline tests
Profiletab in the Account section/settings/profile> Verify GBR is not shown onContact methodfield in thepublic profiledetail sectionContact method> Verify GBR is not shown on the default contact methodNew contact method> Enter email > ClickAddcontact-methodspage and verify GBR is shown on the default contact methodProfiletab in the Account section &Contact methodfield in thepublic profiledetail section.QA Steps
Profiletab in the Account section/settings/profile> Verify GBR is not shown onContact methodfield in thepublic profiledetail sectionContact method> Verify GBR is not shown on the default contact methodNew contact method> Enter email > ClickAddcontact-methodspage and verify GBR is shown on the default contact methodProfiletab in the Account section &Contact methodfield in thepublic profiledetail section.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4