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-07-10] [$1000] Web - Tooltip is not displayed if hovered the users which are down the list #21099

Closed
1 of 6 tasks
kbecciv opened this issue Jun 20, 2023 · 50 comments
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

@kbecciv
Copy link

kbecciv commented Jun 20, 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:

Action Performed:

  1. Go to staging dot on web chrome
  2. Go to search
  3. Hover over the first few profile avatar of the users and notice that tooltipis displayed
  4. Scroll down the list up to bottom and try to hover on the profile avatar (not on the display name/email) and notice that tooltip is not displayed

Expected Result:

Tooltip should be displayed if hovered the users which are down the list

Actual Result:

Tooltip is not displayed if hovered the users which are down the list

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.29.6

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

error-2023-06-13_10.04.37.1.mp4
Recording.777.mp4

Expensify/Expensify Issue URL:

Issue reported by: @priya-zha

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018ca8b2c30604ca26
  • Upwork Job ID: 1671246480159539200
  • Last Price Increase: 2023-06-20
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 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

@isabelastisser
Copy link
Contributor

I can reproduce this!

@isabelastisser isabelastisser added the External Added to denote the issue can be worked on by a contributor label Jun 20, 2023
@melvin-bot melvin-bot bot changed the title Web - Tooltip is not displayed if hovered the users which are down the list [$1000] Web - Tooltip is not displayed if hovered the users which are down the list Jun 20, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 2023

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

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

melvin-bot bot commented Jun 20, 2023

Current assignee @isabelastisser is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 2023

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

@daordonez11
Copy link
Contributor

daordonez11 commented Jun 21, 2023

Proposal

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

For some elements in the search (personalDetails) the tooltip is not displayed if hovering the image

What is the root cause of that problem?

The searchPage has a list of OptionRows, each element uses a component called MultipleAvatars to render de image, even though the prop of shouldOptionShowTooltip is calculated right this line calculates the tooltip content:
const tooltipTexts = props.shouldShowTooltip ? _.pluck(props.icons, 'name') : []; using the property name in the icons array.

For the section of personalDetails in the searchPage the icons are calculated using the method getIcons in ReportUtils.js

Since there are no reports the method only returns the source of the image.

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

Update getIcons and include a new parameter called defaultName

function getIcons(report, personalDetails, defaultIcon = null, isPayer = false, defaultName = '') {
    const result = {
        source: '',
        type: CONST.ICON_TYPE_AVATAR,
        name: '',
    };

    if (_.isEmpty(report)) {
        result.source = defaultIcon || Expensicons.FallbackAvatar;
        result.name = defaultName || '';
        return [result];
    }

So it can be sent in this line

result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail.avatar, personalDetail.login),false,personalDetail.login);

Image of solution working in personalDetails section:

image

Video of solution working in all list:

Tooltip.working.in.whole.search.results.webm

What alternative solutions did you explore? (Optional)

At the beginning, I thought this was a backend issue but after seeing Onyx I found that it was about handling the data stored.

Other solution might work is in OptionRow component preprocess the data of the row and force the inclusion of name on each icon using the information that arrives in props. Something like including this.props.option.text as the name in each element of this.props.icons. It just seems like a lot of single resp code instead of tackling directly in the root.

@BhuvaneshPatil
Copy link
Contributor

BhuvaneshPatil commented Jun 21, 2023

Proposal

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

Web - Tooltip is not displayed if hovered the users which are down the list

What is the root cause of that problem?

Easy step to reproduce this bug is - Create a workspace invite non-existent user, after that try creating the group ( the new user will appear) and it will not have tooltip.
The root cause of this problem is that some rows doesn't have UserDetailTooltip wrapped around them.
This mainly happens because when we send option prop to OptionRow it has icons field, and in that - name is empty.
While calculating the icons -

result.text = reportName;
result.searchText = getSearchText(report, reportName, personalDetailList, result.isChatRoom || result.isPolicyExpenseChat, result.isThread);
result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID));
result.subtitle = subtitle;
return result;
}

We call getIcons() method - And if the user hasn't take a part in any conversation the report is null or probably policyExpenseChart

App/src/libs/ReportUtils.js

Lines 715 to 815 in 0b7455f

function getIcons(report, personalDetails, defaultIcon = null, isPayer = false) {
const result = {
source: '',
type: CONST.ICON_TYPE_AVATAR,
name: '',
};
if (_.isEmpty(report)) {
result.source = defaultIcon || Expensicons.FallbackAvatar;
return [result];
}
if (isArchivedRoom(report)) {
result.source = Expensicons.DeletedRoomAvatar;
return [result];
}
if (isThread(report)) {
const parentReportAction = ReportActionsUtils.getParentReportAction(report);
const actorEmail = lodashGet(parentReportAction, 'actorEmail', '');
const actorAccountID = lodashGet(parentReportAction, 'actorAccountID', '');
const actorIcon = {
id: actorAccountID,
source: UserUtils.getAvatar(lodashGet(personalDetails, [actorAccountID, 'avatar']), actorAccountID),
name: actorEmail,
type: CONST.ICON_TYPE_AVATAR,
};
return [actorIcon];
}
if (isTaskReport(report)) {
const ownerEmail = report.ownerEmail || '';
const ownerIcon = {
id: report.ownerAccountID,
source: UserUtils.getAvatar(lodashGet(personalDetails, [report.ownerAccountID, 'avatar']), report.ownerAccountID),
name: ownerEmail,
type: CONST.ICON_TYPE_AVATAR,
};
return [ownerIcon];
}
if (isDomainRoom(report)) {
result.source = Expensicons.DomainRoomAvatar;
return [result];
}
if (isAdminRoom(report)) {
result.source = Expensicons.AdminRoomAvatar;
return [result];
}
if (isAnnounceRoom(report)) {
result.source = Expensicons.AnnounceRoomAvatar;
return [result];
}
if (isChatRoom(report)) {
result.source = Expensicons.ActiveRoomAvatar;
return [result];
}
if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
const workspaceName = lodashGet(allPolicies, [`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`, 'name']);
const policyExpenseChatAvatarSource = lodashGet(allPolicies, [`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`, 'avatar']) || getDefaultWorkspaceAvatar(workspaceName);
// Return the workspace avatar if the user is the owner of the policy expense chat
if (report.isOwnPolicyExpenseChat && !isExpenseReport(report)) {
result.source = policyExpenseChatAvatarSource;
result.type = CONST.ICON_TYPE_WORKSPACE;
result.name = workspaceName;
return [result];
}
const adminIcon = {
id: report.ownerAccountID,
source: UserUtils.getAvatar(lodashGet(personalDetails, [report.ownerAccountID, 'avatar']), report.ownerAccountID),
name: report.ownerEmail,
type: CONST.ICON_TYPE_AVATAR,
};
const workspaceIcon = {
source: policyExpenseChatAvatarSource,
type: CONST.ICON_TYPE_WORKSPACE,
name: workspaceName,
};
// If the user is an admin, return avatar source of the other participant of the report
// (their workspace chat) and the avatar source of the workspace
return [adminIcon, workspaceIcon];
}
if (isIOUReport(report)) {
const email = isPayer ? report.managerEmail : report.ownerEmail;
const accountID = isPayer ? report.managerID : report.ownerAccountID;
return [
{
id: accountID,
source: UserUtils.getAvatar(lodashGet(personalDetails, [accountID, 'avatar']), accountID),
name: email,
type: CONST.ICON_TYPE_AVATAR,
},
];
}
return getIconsForParticipants(report.participantAccountIDs, personalDetails);
}

We return -

App/src/libs/ReportUtils.js

Lines 716 to 720 in 0b7455f

const result = {
source: '',
type: CONST.ICON_TYPE_AVATAR,
name: '',
};

as it never reaches the -

return getIconsForParticipants(report.participantAccountIDs, personalDetails);

That's why name for icon being empty - we don't wrap it with UserDetailTooltip.

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

We need to handle the case for getting the icons when the report is empty, using result.participantsList, we have a method getIconsForParticipants() we can pass result.participantList (it will be array of accountID) and personalDetails and get the icons.
This will ensure that all the users have the correct set of icons.
Example -

result.icons = report ? <get icons by given way> : <use geticonsForParticipants()>

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jun 22, 2023
@isabelastisser
Copy link
Contributor

@sobitneupane did you have the chance to review the proposals above? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Jun 22, 2023
@sobitneupane
Copy link
Contributor

sobitneupane commented Jun 23, 2023

I like @daordonez11's proposal. We are already using defaultIcon and he is proposing to add defaultName.

@sobitneupane
Copy link
Contributor

Proposal from @daordonez11 looks good to me.

🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

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

@daordonez11
Copy link
Contributor

@MelvinBot wrong assignation 🤣

@sobitneupane
Copy link
Contributor

sobitneupane commented Jun 23, 2023

@daordonez11 Not a wrong assignment. The proposal will be reviewed by Carlos and if Carlos approves it and assign you the task, you can start with your PR.

@daordonez11
Copy link
Contributor

Oh my bad. 😂 thanks for the explanation @sobitneupane I thought you were already doing the assignation.

@0xmiros
Copy link
Contributor

0xmiros commented Jun 23, 2023

I suggest to put this on hold for #20512 which updates avatar patterns and tooltip. ReportUtils.getIcons function is being refactored there.

@daordonez11
Copy link
Contributor

daordonez11 commented Jun 23, 2023

@0xmiroslav mi proposal is about the first if when no report arrives, which hasn't been touched in the PR.
image
This is more like a minor improvement rather than a bugfix:

@daordonez11
Copy link
Contributor

I mean @grgia since you are already focused on this topic you could just implement the solution proposed it seems it is just a border case for personalDetailsSeciton in searchpage but might fix the issue in other points

@isabelastisser
Copy link
Contributor

I will be OOO until July 5, so assigning another team member. SO for reference.

@isabelastisser isabelastisser removed their assignment Jun 23, 2023
@daordonez11
Copy link
Contributor

NVM already tested high traffic everything working fine!

@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

  • when @daordonez11 got assigned: 2023-06-26 16:17:25 Z
  • when the PR got merged: 2023-06-29 16:54:33 UTC
  • days elapsed: 3

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jul 3, 2023
@melvin-bot melvin-bot bot changed the title [$1000] Web - Tooltip is not displayed if hovered the users which are down the list [HOLD for payment 2023-07-10] [$1000] Web - Tooltip is not displayed if hovered the users which are down the list Jul 3, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.35-5 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-07-10. 🎊

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

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 Jul 3, 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:

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

@daordonez11
Copy link
Contributor

Hey @sobitneupane @luacmartins is the rule of the #urgency bonus specifically 72 hours? This PR was closed on the 3rd business day after assignation so I want to know if the bonus does apply on this issue. And it was due to a variable name change 😅

@luacmartins
Copy link
Contributor

@daordonez11 yes, it's within 72h.

@daordonez11
Copy link
Contributor

Noted thanks for the info! @luacmartins

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 9, 2023
@sobitneupane
Copy link
Contributor

sobitneupane commented Jul 10, 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:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

Probably this PR or might have caused by some PR involving accountID migration.

  • [@sobitneupane] 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:

https://expensify.slack.com/archives/C049HHMV9SM/p1688977644296659

  • [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

  • [@sobitneupane] 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.

#21099 (comment)

@sobitneupane
Copy link
Contributor

Regression Test Proposal:

  1. Login with a high traffic account
  2. Go to search
  3. Hover over the profile picture of the user without previous chat history in the search list
  4. Verify that the tooltip is displayed.

Do we agree 👍 or 👎

@sobitneupane
Copy link
Contributor

Payment Requested on Expensify.

@isabelastisser
Copy link
Contributor

@sobitneupane I will review and process the payment by EOD.

@isabelastisser
Copy link
Contributor

@daordonez11 - what's your profile in Upwork? I can't find you there.
@priyeshshah11 - what's your profile in Upwork? I can't find you there.
@sobitneupane - I sent you an offer in Upwork!

@priyeshshah11
Copy link
Contributor

priyeshshah11 commented Jul 11, 2023

@daordonez11 - what's your profile in Upwork? I can't find you there.

@priya-zha - what's your profile in Upwork? I can't find you there.

@sobitneupane - I sent you an offer in Upwork!

I think you might have wrongly tagged me instead of @priya-zha

@sobitneupane
Copy link
Contributor

@isabelastisser I have requested payment and will be paid through Expensify App.

@isabelastisser
Copy link
Contributor

@priya-zha, I sent you an offer in Upwork.

@sobitneupane, I don't see a payment request from you in Upwork, please accept the offer I sent you today, and I will process the payment thorough Upwork.

@isabelastisser
Copy link
Contributor

[@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

https://github.com/Expensify/Expensify/issues/299030

@daordonez11
Copy link
Contributor

@anmurali
Copy link

Paid @sobitneupane

@isabelastisser
Copy link
Contributor

All set!

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

10 participants