Skip to content
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

[$250] Email address flickering - Reported by @gadhiyamanan #11134

Closed
mvtglobally opened this issue Sep 20, 2022 · 35 comments
Closed

[$250] Email address flickering - Reported by @gadhiyamanan #11134

mvtglobally opened this issue Sep 20, 2022 · 35 comments
Assignees
Labels
Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open settings > profile
  2. remove first name and last name
  3. uncheck or check timezone radio button, click on save
  4. immediate go back to settings screen

Expected Result:

email address shows for a second and after that it's not visible

Actual Result:

email address should not show

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • Android
  • Mobile Web

Version Number:
Reproducible in staging?: need repro
Reproducible in production?: need repro
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2022-09-16.at.11.49.49.AM.mov
Screen_Recording_20220916_100434_New.Expensify.mp4
Simulator.Screen.Recording.-.iPhone.13.mini.-.2022-08-26.at.13.34.36.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661501281504419

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 Needs Reproduction Reproducible steps needed labels Sep 20, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2022

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Sep 20, 2022
@Puneet-here
Copy link
Contributor

Puneet-here commented Sep 20, 2022

Proposal

It happens when you save profile changes and there is no name in the profile this causes display name to be empty

We should remove this check as we display the session.email so we don't need to check for displayName

{this.props.currentUserPersonalDetails.displayName && (
<Text
style={[styles.textLabelSupporting, styles.mt1]}
numberOfLines={1}
>
{Str.removeSMSDomain(this.props.session.email)}
</Text>

@Christinadobrzyn
Copy link
Contributor

I can't find any other GHs that match this same issue so sending to Eng to see if it should be fixed now.

@melvin-bot
Copy link

melvin-bot bot commented Sep 20, 2022

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Christinadobrzyn Christinadobrzyn removed their assignment Sep 20, 2022
@alex-mechler
Copy link
Contributor

@mvtglobally I'm not able to reproduce this, are you?

@mvtglobally
Copy link
Author

@alex-mechler We are not able to repro as well

@gadhiyamanan
Copy link
Contributor

@Puneet-here are you able to reproduce?
it's reproducible for me.

@Puneet-here
Copy link
Contributor

Yes, it's reproducible.
Note: You have to keep the first and last name empty when saving.

@alex-mechler
Copy link
Contributor

What version(s) are you able to reproduce on? I was unable to reproduce on staging web yesterday (v1.2.3-0)

@gadhiyamanan
Copy link
Contributor

What version(s) are you able to reproduce on?

(v1.2.3-1)-production

cc: @alex-mechler

@parasharrajat
Copy link
Member

parasharrajat commented Sep 22, 2022

I don't see why this is an issue. If a user takes an action on UI and as a result, some part of UI is updated, how can that be an issue? I simply see this as an unrealistic assumption that a user will be playing around with the profile page back and forth as shown in the video.

I vote to close it.

@Puneet-here
Copy link
Contributor

@parasharrajat, check the video below:-

  1. The email is shown in below the avatar
  2. Saving changes without names makes it blank
  3. After refreshing it works fine again
Screen.Recording.2022-09-22.at.4.29.04.PM.mov

@parasharrajat
Copy link
Member

Yeah, that sounds like an issue. OP should be updated to indicate that.

@alex-mechler
Copy link
Contributor

Ahhh got it. That video with the explict refresh helped me see what was going on here! Thanks for that! Sending this one externally!

@alex-mechler alex-mechler removed their assignment Sep 22, 2022
@alex-mechler alex-mechler added the External Added to denote the issue can be worked on by a contributor label Sep 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2022

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@alex-mechler alex-mechler removed the Needs Reproduction Reproducible steps needed label Sep 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2022

Triggered auto assignment to @iwiznia (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Email address flickering - Reported by @gadhiyamanan [$250] Email address flickering - Reported by @gadhiyamanan Sep 22, 2022
@sobitneupane
Copy link
Contributor

sobitneupane commented Sep 22, 2022

Proposal:
I am assuming that we would not want to show email twice.
Here we are checking this.props.currentUserPersonalDetails.displayName assuming it will have value only if firstName or lastName are set which is not the case. After firstName and lastName are removed, this.props.currentUserPersonalDetails.displayName takes the value Str.removeSMSDomain(login);. Thus, emails are being shown in two places.

{this.props.currentUserPersonalDetails.displayName && (
<Text
style={[styles.textLabelSupporting, styles.mt1]}
numberOfLines={1}
>
{Str.removeSMSDomain(this.props.session.email)}
</Text>
)}

-                            {this.props.currentUserPersonalDetails.displayName && (
+                            {(!_.isEmpty(this.props.currentUserPersonalDetails.firstName) || !_.isEmpty(this.props.currentUserPersonalDetails.lastName)) && (
                                <Text
                                    style={[styles.textLabelSupporting, styles.mt1]}
                                    numberOfLines={1}
                                >
                                    {Str.removeSMSDomain(this.props.session.email)}
                                </Text>
                            )}

@Puneet-here
Copy link
Contributor

IMO it's useful. When the email is longer it will be in ellipsis at the upper row but in the second row the text is small so it is fully visible. Also It was like this from the starting.

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Sep 22, 2022

Proposal:

Previous proposals don't explain the correct root cause.
RCA is same as #10891 (comment)

It's ideal to fix in backend by sending correct displayName in socket event as I proposed in other GH
If it's not fixable, alternative proposal is as follows:

displayName is set to login as a fallback if first name, last name not exists.
If we should always show email (login) even though it's same as displayName (the case when first name, last name is empty), follow Solution 1.
If we don't need to show duplicated email (login), follow Solution 2.

Solution 1:

                            <Pressable style={[styles.mt1, styles.mw100]} onPress={this.openProfileSettings}>
                                <Text style={[styles.displayName]} numberOfLines={1}>
                                    {this.props.currentUserPersonalDetails.displayName
                                        ? this.props.currentUserPersonalDetails.displayName
-                                       : Str.removeSMSDomain(this.props.session.email)}
+                                       : Str.removeSMSDomain(this.props.currentUserPersonalDetails.login)}
                                </Text>
                            </Pressable>
-                           {this.props.currentUserPersonalDetails.displayName && (
                                <Text
                                    style={[styles.textLabelSupporting, styles.mt1]}
                                    numberOfLines={1}
                                >
-                                   {Str.removeSMSDomain(this.props.session.email)}
+                                   {Str.removeSMSDomain(this.props.currentUserPersonalDetails.login)}
                                </Text>
-                           )}

Solution 2:

                            <Pressable style={[styles.mt1, styles.mw100]} onPress={this.openProfileSettings}>
                                <Text style={[styles.displayName]} numberOfLines={1}>
                                    {this.props.currentUserPersonalDetails.displayName
                                        ? this.props.currentUserPersonalDetails.displayName
-                                       : Str.removeSMSDomain(this.props.session.email)}
+                                       : Str.removeSMSDomain(this.props.currentUserPersonalDetails.login)}
                                </Text>
                            </Pressable>
-                           {this.props.currentUserPersonalDetails.displayName && (
+                           {this.props.currentUserPersonalDetails.displayName && (this.props.currentUserPersonalDetails.displayName !== this.props.currentUserPersonalDetails.login) && (
                                <Text
                                    style={[styles.textLabelSupporting, styles.mt1]}
                                    numberOfLines={1}
                                >
-                                   {Str.removeSMSDomain(this.props.session.email)}
+                                   {Str.removeSMSDomain(this.props.currentUserPersonalDetails.login)}
                                </Text>
                            )}

NOTE:
In either solution, I propose to use this.props.currentUserPersonalDetails.login instead of this.props.session.email.
Because email might not exist in case user login with phone number.

@sobitneupane
Copy link
Contributor

IMO it's useful. When the email is longer it will be in ellipsis at the upper row but in the second row the text is small so it is fully visible. Also It was like this from the starting.

If that is the case, then removing the condition will be the best thing to do. But I doubt that longer email will be fully visible in smaller text as it will hold only 12-14 characters more than the display name section.

@sobitneupane
Copy link
Contributor

sobitneupane commented Sep 22, 2022

It's ideal to fix in backend by sending correct displayName in socket event as I proposed in other GH.

@aimane-chnaif
I was planning to start this conversation in slack. As you have done great work with the analysis, why don't you start the conversation in slack as well. It will have more eyes.

@parasharrajat
Copy link
Member

parasharrajat commented Sep 22, 2022

Splendid, @aimane-chnaif you saved me debugging time. I agree, it feels like an issue from the backend. Showing the email two times is completely fine. In fact, we fall back to email when no other option applies.

But it will not only email, it can be a phone number too. That's why displayName is used.

So solution 2 is not needed. Solution 1 is already being handled on the front end. But the backend is replacing that value which means it is an issue from the backend.

@iwiznia What do you think?

As #10891 is also caused by the same reason. I vote to close the other one.

🎀 👀 🎀 C+ reviewed


@iwiznia
Copy link
Contributor

iwiznia commented Sep 23, 2022

If this is a backend error, then we should fix it in the backend. But I am not sure exactly what the error is. Specifically, after calling which API command are we receiving the wrong displayName in the personalDetails onyx key?

@parasharrajat
Copy link
Member

Based on #10891 (comment), it seems like the API is UpdateProfile.

@iwiznia
Copy link
Contributor

iwiznia commented Sep 23, 2022

Ah ok thanks! Was not completely sure.

@iwiznia
Copy link
Contributor

iwiznia commented Sep 23, 2022

ok I think I got a fix, but it's in the backend.

@stephanieelliott
Copy link
Contributor

Removing Help Wanted since we will fix this internally.

@stephanieelliott stephanieelliott removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 27, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2022

@iwiznia, @parasharrajat, @stephanieelliott Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Puneet-here
Copy link
Contributor

We can close this since it's a dupe of #10891, right?

@iwiznia
Copy link
Contributor

iwiznia commented Oct 5, 2022

I don't know why this was not auto closed when this was deployed https://github.com/Expensify/Web-Expensify/pull/34940

@iwiznia iwiznia closed this as completed Oct 5, 2022
@gadhiyamanan
Copy link
Contributor

gadhiyamanan commented Oct 5, 2022

@iwiznia am i eligible for reporting?

cc: @stephanieelliott

@iwiznia
Copy link
Contributor

iwiznia commented Oct 5, 2022

I think no, since the other issue was reported earlier.

@gadhiyamanan
Copy link
Contributor

gadhiyamanan commented Oct 5, 2022

@iwiznia i think the other issue is different

@iwiznia
Copy link
Contributor

iwiznia commented Oct 5, 2022

I don't think it is. Can you reproduce it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants