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

[HOLD for payment 2023-10-23] [$500] Share code - App displays message menu on self profile #28591

Closed
3 of 6 tasks
izarutskaya opened this issue Oct 2, 2023 · 42 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@izarutskaya
Copy link

izarutskaya commented Oct 2, 2023

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 the app
  2. Open settings->share code and click on copy
  3. Logout and visit the copied URL
  4. Login and observe that for sometime, app displays message menu on self profile which is also clickable

Expected Result:

App should not display message menu on visiting self profile using profile link

Actual Result:

App displays message menu for sometime on login with self profile URL

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • Windows / Chrome
  • MacOS / Desktop

Version Number: v1.3.75-8

Reproducible in staging?: Y

Reproducible in production?: Y

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

windows.chrome.message.tab.on.self.profile.mp4
ios.chrome.app.displays.message.on.login.with.self.profile.URL.mov
mac.chrome.app.displays.message.on.login.with.self.profile.URL.mov
android.chrome.message.tab.on.self.profile.mp4
Recording.1680.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696098415905769

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018b9b851a1c88f1df
  • Upwork Job ID: 1708781985248755712
  • Last Price Increase: 2023-10-02
  • Automatic offers:
    • s77rt | Reviewer | 27094065
    • c3024 | Contributor | 27094066
    • dhanashree-sawant | Reporter | 27094067
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 2, 2023
@c3024
Copy link
Contributor

c3024 commented Oct 2, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Logging in with deep link of profile page shows message oneself option briefly.

What is the root cause of that problem?

We show the MenuItem with message option here

{!isCurrentUser && !Session.isAnonymousUser() && (
<MenuItem
title={`${props.translate('common.message')}${displayName}`}
titleStyle={styles.flex1}
icon={Expensicons.ChatBubble}
onPress={() => Report.navigateToAndOpenReportWithAccountIDs([accountID])}
wrapperStyle={styles.breakAll}
shouldShowRightIcon
/>

when the user is not current user and not anonymous.
and the isCurrentUser check here is done with props.loginList
const isCurrentUser = _.keys(props.loginList).includes(login);

So there is a brief period where the loginList is not loaded yet and the session user login is absent from the loginList. Then isCurrentUser is false and the MenuItem with messaging option is shown briefly.

What changes do you think we should make in order to solve the problem?

Since we moved to accountIDs I think it is better to do the check of isCurrentUser with accountID instead of finding if props.loginList contains the login.
So subscribe to session in the ProfilePage.

        session: {
            key: ONYXKEYS.SESSION,
        }

and change the isCurrentUser check to

const isCurrentUser = props.session.accountID === accountID;

I see a isCurrentUser function in OptionListUtils which takes details of a user and checks if login field of the details is part of the loginList.
But I think we should have an isCurrentUser function in Session or SessionUtils or UserUtils or ReportUtils with the comparison of session account ID with an account ID passed to the isCurrentUser function. There are cases where we want to check if a user is current user and this function makes it DRY.

What alternative solutions did you explore? (Optional)

  1. Instead of using an account ID comparison we can retain the comparison as it is for the check isCurrentUser but we can change this condition here
    {!isCurrentUser && !Session.isAnonymousUser() && (

    to
{!_.isEmpty(props.loginList) && !isCurrentUser && !Session.isAnonymousUser() && (

so that we do not show this MenuItem if the loginList was not loaded yet.
2. Another change we can do for the primary proposal is since we are subscribing to both personalDetails and session we might consider using HOC withCurrentUserPersonalDetails in place of these.

Result
Screen.Recording.2023-10-01.at.12.01.23.PM.mov

@melvin-bot melvin-bot bot changed the title Share code - App displays message menu on self profile [$500] Share code - App displays message menu on self profile Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

Job added to Upwork: https://www.upwork.com/jobs/~018b9b851a1c88f1df

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

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

@saranshbalyan-1234
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

App sis displaying display message menu on visiting self profile using profile link

What is the root cause of that problem?

We are not showing loading screen if current user details are not available. Even if current user details is still loading we are rendering the component.

{!hasMinimumDetails && isLoading && <FullScreenLoadingIndicator style={styles.flex1} />}

What changes do you think we should make in order to solve the problem?

We can add another condition over the above code like this to show the loading screen if currentuserdetails are note available

 {!hasMinimumDetails && isLoading || !login && <FullScreenLoadingIndicator style={styles.flex1} />}

What alternative solutions did you explore? (Optional)

We can add the same condition here as well

{!isCurrentUser && !Session.isAnonymousUser() && (
<MenuItem
title={`${props.translate('common.message')}${displayName}`}
titleStyle={styles.flex1}
icon={Expensicons.ChatBubble}
onPress={() => Report.navigateToAndOpenReportWithAccountIDs([accountID])}
wrapperStyle={styles.breakAll}
shouldShowRightIcon
/>
)}

 {!isCurrentUser && !Session.isAnonymousUser() &&!login && (
   <MenuItem
    title={`${props.translate('common.message')}${displayName}`}
    titleStyle={styles.flex1}
    icon={Expensicons.ChatBubble}
    onPress={() => Report.navigateToAndOpenReportWithAccountIDs([accountID])}
    wrapperStyle={styles.breakAll}
    shouldShowRightIcon
    />
)}

Adding the conditions in both the places gives the best result.

@s77rt
Copy link
Contributor

s77rt commented Oct 2, 2023

@c3024 Thanks for the proposal. Your RCA makes sense. Regarding the solution is the session onyx value populated first? (before loginList) If so can you please include the timeline when each is populated?

Also can you please confirm that we are not "inverting" the bug i.e. try deep link login with an url that points to another user account - in this case the message button should immediately be visible

@s77rt
Copy link
Contributor

s77rt commented Oct 2, 2023

@saranshbalyan-1234 Thanks for the proposal. Your RCA makes sense but I don't think the solution follows our offline pattern. If the user is just waiting to read data from the server then we should show whatever data we have in onyx (even if stale) and update it once we get fresh data. We should not block the user. As for the alternative solution, I think you meant to type login and not !login in the condition, regardless that solution is a very similar to the one proposed here.

@c3024
Copy link
Contributor

c3024 commented Oct 3, 2023

Thanks @s77rt for the review.

Here is the timeline of loading session and loginList.

Screen.Recording.2023-10-03.at.5.42.36.AM.mov
sessionAndLoginListTimeLine

If we login with deep link of another user link, the message option shows right away so it does not invert the bug.

anotherUserShareCodeLink.mov

Since session is available immediately on login, correct isCurrentUser is available right from the start. So, it does not invert the existing behavior for logging in with deep link of another user's share code link.

@dukenv0307
Copy link
Contributor

I think this a BE bug that returns the wrong data of the current user in OpenPublicProfile API

Screenshot from 2023-10-03 21-03-43

@s77rt
Copy link
Contributor

s77rt commented Oct 3, 2023

@c3024 Thanks for the confirmation. Let's get this fixed.

🎀 👀 🎀 C+ reviewed
Link to proposal

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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

@s77rt
Copy link
Contributor

s77rt commented Oct 3, 2023

@dukenv0307 I don't see how this is a BE bug. What data is the server not sending correctly? isCurrentUser is a client-side only prop and not set by the server.

@dukenv0307
Copy link
Contributor

@s77rt You can see the image, the data doesn't contain the login though this is the current user. That makes hasMinimumDetail as true and the page is displayed but isCurrentUser as false briefly before openApp return the full data and then message option is shown.

@s77rt
Copy link
Contributor

s77rt commented Oct 3, 2023

@dukenv0307 The data is not supposed to include login (email) as that field is replaced by accountID.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Oct 3, 2023

My bad, you are right.

@dukenv0307
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

The app displays message menu on self profile

What is the root cause of that problem?

As I mentioned above, OpenPublicProfilePage API doesn't return login fields. That makes hasMinimumDetails is true but isCurrentUser is false briefly before the OpenApp API is complete.

const isCurrentUser = _.keys(props.loginList).includes(login);

And then the profile is displayed unexpectedly before the full data is loaded

{hasMinimumDetails && (

What changes do you think we should make in order to solve the problem?

  1. We can get the accountID of the current user and compare with accountID instead of comparing login field.

  2. We still have a problem that is the avatar and the name is displayed before other details. So we should change the condition here to only display the profile page after the full data is loaded.

{hasMinimumDetails && (

To do this we can use isLoadingApp that is added recently to replace isLoadingReportData. This is only true when openApp API is called.

And update the condition here to hasMinimumDetails && !props.isLoadingApp

{hasMinimumDetails && (

and here to (!hasMinimumDetails || props.isLoadingApp) && isLoading

{!hasMinimumDetails && isLoading && <FullScreenLoadingIndicator style={styles.flex1} />}

We can accept that the profile page is loading whenever we reload this page or if we want to only show the loading the first time we can get the login field of the current user and check if it is empty we will not show loading page

const currentUserLogin = lodashGet(props.personalDetails, `${props.session.accountID}.login`, '');
hasMinimumDetails && (!props.isLoadingApp || currentUserLogin)
(!hasMinimumDetails || (props.isLoadingApp && !currentUserLogin)) && isLoading

What alternative solutions did you explore? (Optional)

NA

Result

result-nen.mp4

@dukenv0307
Copy link
Contributor

@s77rt @c3024 's proposal still has a problem in that is avatar and display name are displayed before other details. And if the display name of the user is the login, the Hidden name will display briefly. You also can see this problem in the result video of this proposal.

@s77rt
Copy link
Contributor

s77rt commented Oct 3, 2023

@dukenv0307 Thanks for the proposal. The second highlighted problem is not what was reported here and I don't see it as a blocker, it looks okay that the name and the avatar are shown before other info. Also it does not seem fair to go with your proposal just because it's fixing another bug. If you feel that the bug is something we should fix please report in Slack.

@dukenv0307
Copy link
Contributor

  1. Let's see the video below, with the account that hasn't changed the display name yet, the Hidden will display briefly
Screen.Recording.2023-10-04.at.05.38.53.mp4
  1. I think we current root cause of this issue is the page is displayed before all necessary data is loaded completely by the condition here. So I think the solution should be to update this condition to make sure the page is only displayed when the full data is loaded completely.

{hasMinimumDetails && (

@c3024
Copy link
Contributor

c3024 commented Oct 3, 2023

Showing loading every time we reload the profile page is not a good idea.
But in here that you suggested

const currentUserLogin = lodashGet(props.personalDetails, `${props.session.accountID}.login`, '');
hasMinimumDetails && (!props.isLoadingApp || currentUserLogin)
(!hasMinimumDetails || (props.isLoadingApp && !currentUserLogin)) && isLoading

checking currentUserLogin also for cases when we login/access another user's share code from deeplink does not appear correct.
We should rather check the accountID from route param and check if it has been loaded yet or not. We should not check if session accountID login is loaded or not loaded yet.
So, while this might work, it does not convey the correct logic when accessing another user share code link from deeplink.

Moreover, complicating existing conditions this much for such a trivial edge case does not make sense to me.

@dukenv0307
Copy link
Contributor

currentUserLogin is only used to check openApp is called the first time after logging. If the user opens another user's share code from deeplink, this page still works well because if the personal details contain the data, the page will display without loading otherwise, openPublicProfile is called and loading page will display as well

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.83-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
  • [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@s77rt] Determine if we should create a regression test for this bug.
  • [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 13, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile Oct 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.83-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 13, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
  • [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@s77rt] Determine if we should create a regression test for this bug.
  • [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
  • [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@s77rt] Determine if we should create a regression test for this bug.
  • [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@s77rt s77rt mentioned this issue Oct 16, 2023
57 tasks
@s77rt
Copy link
Contributor

s77rt commented Oct 16, 2023

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 16, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile Oct 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.84-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-23. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
  • [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@s77rt] Determine if we should create a regression test for this bug.
  • [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@s77rt
Copy link
Contributor

s77rt commented Oct 16, 2023

Melvin is going crazy with the checklists

@greg-schroeder greg-schroeder changed the title [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-23] [HOLD for payment 2023-10-20] [HOLD for payment 2023-10-20] [$500] Share code - App displays message menu on self profile [HOLD for payment 2023-10-23] [$500] Share code - App displays message menu on self profile Oct 19, 2023
@greg-schroeder
Copy link
Contributor

Man what is going on with this issue, lol

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 20, 2023
@greg-schroeder
Copy link
Contributor

Not overdue, will process payment today

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@greg-schroeder
Copy link
Contributor

Payments complete. C+ checklist completed here. closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants