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] You" Excluded When Selecting account Email in Share-somewhere Section #39764

Closed
2 of 6 tasks
m-natarajan opened this issue Apr 5, 2024 · 32 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause

Comments

@m-natarajan
Copy link

m-natarajan commented Apr 5, 2024

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


Version Number: 1.4.60-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
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

Prerequisite: Please use a long display name (if the default display name is used, ensure the email address is long).
1, sign in with your Gmail account > Click on FAB and select Assign task.
2, Add a title and click next.
3, Go to share somewhere and select your account email (the email used to sign in).
4, Notice that "You" is excluded.

Expected Result:

When selecting one's account email (the email used to sign in) in Share Somewhere, "You" should not be excluded.

Actual Result:

When selecting the account email (the email used to sign in) in Share somewhere, "You" is excluded if the displayed name is long.

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

Bug6438524_1712267855352.Screen_Recording_2024-04-04_at_2.55.41_PM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011a3106b6d163f6b3
  • Upwork Job ID: 1777358786323771392
  • Last Price Increase: 2024-04-08
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

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

@m-natarajan
Copy link
Author

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@dragnoir
Copy link
Contributor

dragnoir commented Apr 7, 2024

Proposal

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

"You" excluded when selecting account with long email/name in share-somewhere section

What is the root cause of that problem?

(You) is always added to the end of the name.
and is this case, it's passed here

https://github.com/dragnoir/App/blob/595bf4075988c0b64cb0db1e37694284637fbea2/src/components/MenuItem.tsx#L495-L502

So for an example as "Testing Long name for task you with more.. (you)"

When we use numberOfLines for Text, the out of frame text wish include the (You) will be hidden

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

We need to move (You) to the beguinning of the phrase

image

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Apr 8, 2024
@Krishna2323
Copy link
Contributor

Krishna2323 commented Apr 8, 2024

Proposal

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

"You" excluded when selecting account with long email/name in share-somewhere section

What is the root cause of that problem?

The (You) postfix is always there but due to long display name it has been cut off and ellipsis is added. This doesn't seem like a issue but there is an inconsistency, we have don't show tooltip for self dm report in share somewhere display but it is shown in assignee display. The cause of this is the lines below, we use report?.participantAccountIDs to get display names but incase of self dm it will be an empty array.

const participantAccountIDs = report?.participantAccountIDs ?? [];

if (ReportUtils.isChatReport(report) && ReportUtils.isDM(report) && ReportUtils.hasSingleParticipant(report)) {

const participantAccountID = report?.participantAccountIDs?.[0] ?? -1;

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

When we have a self dm report we need to use the ownerAccountID instead of participantAccountIDs.

Pseudo-Code
/**
 * Get the share destination data
 * */
function getShareDestination(reportID: string, reports: OnyxCollection<OnyxTypes.Report>, personalDetails: OnyxEntry<OnyxTypes.PersonalDetailsList>): ShareDestination {
    const report = reports?.[`report_${reportID}`] ?? null;

    const participantAccountIDs = ReportUtils.isSelfDM(report) ? [report?.ownerAccountID ?? 0] : report?.participantAccountIDs ?? [];
    const isMultipleParticipant = participantAccountIDs.length > 1;
    const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, personalDetails), isMultipleParticipant);

    let subtitle = '';
    if ((ReportUtils.isChatReport(report) && ReportUtils.isDM(report) && ReportUtils.hasSingleParticipant(report)) || ReportUtils.isSelfDM(report)) {
        const participantAccountID = ReportUtils.isSelfDM(report) ? report?.ownerAccountID ?? -1 : report?.participantAccountIDs?.[0] ?? -1;

        const displayName = personalDetails?.[participantAccountID]?.displayName ?? '';
        const login = personalDetails?.[participantAccountID]?.login ?? '';
        subtitle = LocalePhoneNumber.formatPhoneNumber(login || displayName);
    } else {
        subtitle = ReportUtils.getChatRoomSubtitle(report) ?? '';
    }
    return {
        icons: ReportUtils.getIcons(report, personalDetails, Expensicons.FallbackAvatar),
        displayName: ReportUtils.getReportName(report),
        subtitle,
        displayNamesWithTooltips,
        shouldUseFullTitleToDisplay: ReportUtils.shouldUseFullTitleToDisplay(report),
    };
}

Also, we need to pass true for fourth parameter of getDisplayNamesWithTooltips to include (you) (shouldAddCurrentUserPostfix)

const assigneeTooltipDetails = ReportUtils.getDisplayNamesWithTooltips(
OptionsListUtils.getPersonalDetailsForAccountIDs(task?.assigneeAccountID ? [task.assigneeAccountID] : [], personalDetails),
false,
);

const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, personalDetails), isMultipleParticipant);

Result

What alternative solutions did you explore? (Optional)

If we plan to remove you post fix, then we need to make several changes.

Remove you part from these and functions and update everywhere we use these functions. Also check if we have similar functions.

function getDisplayNameForParticipant(accountID?: number, shouldUseShortForm = false, shouldFallbackToHidden = true, shouldAddCurrentUserPostfix = false): string {

function getDisplayNameOrDefault(passedPersonalDetails?: Partial<PersonalDetails> | null, defaultValue = '', shouldFallbackToHidden = true, shouldAddCurrentUserPostfix = false): string {

@Krishna2323
Copy link
Contributor

Proposal Updated

@isabelastisser isabelastisser added the External Added to denote the issue can be worked on by a contributor label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

Job added to Upwork: https://www.upwork.com/jobs/~011a3106b6d163f6b3

@melvin-bot melvin-bot bot changed the title You" Excluded When Selecting account Email in Share-somewhere Section [$250] You" Excluded When Selecting account Email in Share-somewhere Section Apr 8, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

@isabelastisser
Copy link
Contributor

@parasharrajat, please validate the issue and review the proposals above. Thanks!

@parasharrajat
Copy link
Member

@Krishna2323 I didn't understand your proposal. Why do have to change this id? What is purpose of tooltip in this issue?

@parasharrajat
Copy link
Member

@Expensify/design Should the (you) part in share somewhere selector on task confirmation page, remain at the end? Is it possible to move this as prefix?

@shawnborton
Copy link
Contributor

I think we should remain as consistent as possible, so if we are showing this in the contacts list or LHN list, it should also be on this task confirmation page.

cc @thienlnam in case you have any additional context as to why it was left out.

@Krishna2323
Copy link
Contributor

@parasharrajat, we have an inconsistency in assignee details and share somewhere details. In the assignee section, when we select "self dm," the email is displayed, and when we hover over the display name, the tooltip is also shown. However, in the "share somewhere" section, we don't have the tooltip or email displayed. Additionally, in the LHS, we also have the tooltip.

task_page_inconsistency.mp4

@shubham1206agra
Copy link
Contributor

@parasharrajat Just a heads up. (https://expensify.slack.com/archives/C066HJM2CAZ/p1712591630706319)

I am going to add a subtitle in selfDM for better clarity.
image

@shawnborton
Copy link
Contributor

Ah great reminder. If we are going to do the subtitle everywhere, then maybe we can just drop the (you) from the contact name everywhere? Curious what others think about though.

@dannymcclain
Copy link
Contributor

Ah great reminder. If we are going to do the subtitle everywhere, then maybe we can just drop the (you) from the contact name everywhere? Curious what others think about though.

I think we should keep it. I like the combo of the (you) and the Your space subheading to make it super clear this is not a "normal" contact. Not a hill I will die on, just my 2 cents.

@parasharrajat
Copy link
Member

@Krishna2323 Please post proposal update message when you make changes to your proposal with brief info of what has been changed.

@Krishna2323
Copy link
Contributor

@parasharrajat, I apologize for that. I planned to make further changes before posting proposal updated, but I haven't added any change after exploring a bit and forgot to post proposal updated.

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Alternative added.

@thienlnam thienlnam self-assigned this Apr 9, 2024
@parasharrajat
Copy link
Member

@shubham1206agra Do we have a ticket for this yet? Link does not open for me.

@shubham1206agra
Copy link
Contributor

@shubham1206agra Do we have a ticket for this yet? Link does not open for me.

@parasharrajat Not really. Since this was a minor change, I am doing it with #39956.

@parasharrajat
Copy link
Member

@shubham1206agra is going to make this change in one of its PR #39956. Nothing is required on this issue. There will be no payments @isabelastisser... We can put this on Hold for #38699.

@thienlnam thienlnam changed the title [$250] You" Excluded When Selecting account Email in Share-somewhere Section [HOLD #38699][$250] You" Excluded When Selecting account Email in Share-somewhere Section Apr 10, 2024
@thienlnam thienlnam removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 10, 2024
@isabelastisser
Copy link
Contributor

Thanks for the update, @parasharrajat!

@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
@isabelastisser
Copy link
Contributor

Not overdue. On hold.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 15, 2024
@isabelastisser isabelastisser removed the Daily KSv2 label Apr 18, 2024
@isabelastisser
Copy link
Contributor

On hold.

@isabelastisser
Copy link
Contributor

Not overdue, on hold.

@melvin-bot melvin-bot bot removed the Overdue label May 20, 2024
@shubham1206agra
Copy link
Contributor

@isabelastisser Please take it off hold.

@parasharrajat
Copy link
Member

@isabelastisser can we please get this retested?

@isabelastisser isabelastisser added the Daily KSv2 label May 20, 2024
@isabelastisser isabelastisser changed the title [HOLD #38699][$250] You" Excluded When Selecting account Email in Share-somewhere Section [$250] You" Excluded When Selecting account Email in Share-somewhere Section May 20, 2024
@isabelastisser isabelastisser removed the Monthly KSv2 label May 20, 2024
@parasharrajat
Copy link
Member

parasharrajat commented May 22, 2024

Can we get this retested please @isabelastisser? @Expensify/applause

@isabelastisser isabelastisser added the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label May 23, 2024
@isabelastisser
Copy link
Contributor

Hey @Expensify/applauseleads, can you please retest this? Thanks!

@kbecciv
Copy link

kbecciv commented May 23, 2024

Checking

@kbecciv
Copy link

kbecciv commented May 24, 2024

Issue is no longer reproducible, build 1.4.75.0

Screen.Recording.2024-05-23.at.10.16.08.PM.mp4

@isabelastisser
Copy link
Contributor

Thanks for confirming, @kbecciv ! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause
Projects
No open projects
Archived in project
Development

No branches or pull requests

10 participants