[C+ Checklist Needs Completion] [$500] OpenPublicProfilePage API not called as soon as avatar is clicked

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.42-1
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
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708010767314729

Action Performed:

1. Go to a room chat
2. Click on the user avatar

Expected Result:

the OpenPublicProfilePage API should get called as soon as you open the profile page.

Actual Result:

it isn't getting called until you click on the avatar. This means that the profile data isn't getting loaded when you open the profile, even though it should be.

Workaround:

unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Screen.Recording.2024-02-15.at.10.24.18.AM.mov

Recording.2736.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01eff48078e4b5dadf
• Upwork Job ID: 1758172876106809344
• Last Price Increase: 2024-02-15
• Automatic offers:
• cubuspl42 | Reviewer | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01eff48078e4b5dadf

[melvin-bot]

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

[melvin-bot]

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

[FitseTLT]

Proposal

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

OpenPublicProfilePage API not called as soon as avatar is clicked

What is the root cause of that problem?

The api is called if it doesn't have minimum details here

App/src/pages/ProfilePage.js

Lines 141 to 142 in 1c0d126

	if (ValidationUtils.isValidAccountRoute(accountID) && !hasMinimumDetails) {
	PersonalDetails.openPublicProfilePage(accountID);

Which means if details.avatar is empty

App/src/pages/ProfilePage.js

Lines 123 to 124 in 1c0d126

	const hasMinimumDetails = !_.isEmpty(details.avatar);
	const isLoading = lodashGet(details, 'isLoading', false) || _.isEmpty(details);

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

we should make the api call if login doesn't exist so change it to

App/src/pages/ProfilePage.js

Lines 123 to 124 in 1c0d126

	const hasMinimumDetails = !_.isEmpty(details.avatar);
	const isLoading = lodashGet(details, 'isLoading', false) || _.isEmpty(details);

    const hasMinimumDetails = !_.isEmpty(details.avatar) && !_.isEmpty(details.login);

alternatively We could also make api call dependent on non-existence of the data like timezone or pronouns notificationPreference

What alternative solutions did you explore? (Optional)

[Pujan92]

As avatar url is already available in personal details list I think it should not make a call to OpenPublicProfilePage, similar case I have mentioned here (update ?? to ||)

App/src/pages/settings/Profile/ProfileAvatar.tsx

Line 31 in 2fe384c

if (!ValidationUtils.isValidAccountRoute(Number(accountID)) ?? !!avatarURL) {

[eucool]

Actually this is not a bug (The user mentioned in the video has his profile to hidden, you should not be able to see his private details until you're logged in/ talked with them.

This is linked to another issue, let me find that one and edit this comment.

[melvin-bot]

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

[anmurali]

@greg-schroeder rotated the label since I will be OOO till 29th! I can take it back when I return!

[puneetlath]

@codinggeek2023 could you clarify what you mean?

The bug is that when you open a profile we aren't calling the OpenPublicProfilePage API. We're only calling it when you click on the avatar. This is happening whether you are an anon user or you are signed in. And no matter what user's profile you are attempting to view (from my testing). I think we should just call the API whenever you open this page, so you have the latest data about that person. And we should not call it when you click the avatar from the profile page.

[ikevin127]

As avatar url is already available in personal details list I think it should not make a call to OpenPublicProfilePage, similar case I have mentioned here (update ?? to ||).

Your proposed solution would break the avatar functionality in case the avatar is changed from another device because if we apply the solution that would mean the OpenPublicProfilePage won't be called anymore when avatarURL changes.

More context / video example here #36577 (comment).

[kmbcook]

Proposal

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

OpenPublicProfilePage API not called as soon as profile page is opened.

What is the root cause of that problem?

See

• pages/ProfilePage.js
• libs/actions/PersonalDetails.ts

OpenPublicProfilePage api call made in PersonalDetails.openPublicProfilePage.

ProfilePage receives personal details in props, normally. If so, hasMinimumDetails is set to true, which prevents openPublicProfilePage from being called.

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

ProfilePage calls openPublicProfilePage regardless of value of hasMinimumDetails.

[cubuspl42]

@FitseTLT Could you elaborate on the root cause analysis? Are we discussing a corner case? Is the current (old) hasMinimumDetails logic working in any circumstances? You suggest adding two checks in the new proposed condition: details.avatar and details.login. When is one of these non-empty when the other is empty, and vice versa?

[cubuspl42]

@kmbcook I think the check is included to minimize the number of unnecessary API calls. I think it would be preferable to fix the check instead of removing it, if it's reasonably easy.

[kmbcook]

@cubuspl42 the issue itself is about the API call not being made, not about personal details not being current. Is there some reason the issue was written the way it was, some reason why the API call MUST be made when the profile page is opened?

[puneetlath]

I think we should use hasMinimumDetails in order to decide whether to show the blocking view. But no matter what we should call the API when opening the page so that we ensure we get the most up-to-date details.

[cubuspl42]

In such a case, I believe we should go with @kmbcook

C+ reviewed 🎀 👀 🎀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Always call OpenPublicProfilePage API #37223

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

• @cubuspl42 requires payment automatic offer (Reviewer)
• @kmbcook requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@cubuspl42] The PR that introduced the bug has been identified. Link to the PR:
• [@cubuspl42] 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:
• [@cubuspl42] 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:
• [@cubuspl42] Determine if we should create a regression test for this bug.
• [@cubuspl42] 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.
• [@anmurali / @greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[aimane-chnaif]

There's regression - #37986

[kmbcook]

@cubuspl42 the issue itself is about the API call not being made, not about personal details not being current. Is there some reason the issue was written the way it was, some reason why the API call MUST be made when the profile page is opened?

Given that this issue requires the API call to be made, are these two issues mutually exclusive (this one and #37986)?

[cubuspl42]

@kmbcook Not necessarily... It's a X -> Y -> Z situation. We solved problem X, and the solution uncovered a performance problem Y. Problem Y likely existed before, but it's easier to observe now.

The problem Y is likely solvable. The question is whether we handle it as a separate problem or say that it was our fault that we didn't solve X and Y in one go.

Of course, I prefer the first interpretation.

[greg-schroeder]

@stitesExpensify what do you think about the above re: regression?

[greg-schroeder]

@kmbcook can you please accept the offer I sent you so I can pay you? thanks!

[stitesExpensify]

I think we go with "not a regression". While this did technically cause the problem, the actual issue already existed, and we did not ask for the lag to be fixed here on the issue, or during testing on the PR.

[greg-schroeder]

offer sent to @kmbcook for $500
@cubuspl42 paid $500

next up is @cubuspl42 is the checklist!

[kmbcook]

@kmbcook can you please accept the offer I sent you so I can pay you? thanks!

Done

[cubuspl42]

• The PR that introduced the bug has been identified. Link to the PR:
  • This is a kind of new feature fixing an undesirable behavior in the edge cases
• 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:
  • N/A
• 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:
  • No need for additional discussion
• Determine if we should create a regression test for this bug.
  • Up to the QA team
• 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.
  • Precondition: User A and User B don't have any previous interactions
  • User A enters a public room.
  • User B enters the same public room
  • User B posts a message.
  • User B changes their Display Name.
  • Verify that, from User A's perspective, the Display Name of User B in their message is not updated automatically
    • If it updates on its own, it can mean that these users have previous interactions
  • User A clicks/taps on the avatar of User B, which leads to their profile
  • Verify that User A's view of the profile page of User B refreshes, presenting the new Display Name

[melvin-bot]

@cubuspl42 @anmurali @kmbcook @greg-schroeder @stitesExpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @cubuspl42 is eligible for the Internal assigner, not assigning anyone new.

[anmurali]

Added the regression test. @greg-schroeder - have you paid @kmbcook ? If yes, can you please close this issue? I wasn't sure if that last payment had gone out yet.

[greg-schroeder]

Yup! All done, closing.