[HOLD for payment 2023-10-30] [$500] Details - Comma appears before username in the detail page

[kbecciv]

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.3.83.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: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697040699753639

Action Performed:

1. Navigate to Settings > Workspace > New workspace
2. Go to the workspace expense room
3. Send a message
4. Click on “Reply in thread”
5. Click on the thread header
6. Observe that a comma is displayed before the username

Expected Result:

Comma should not be displayed before the username

Actual Result:

Comma is shown before the username on the detail page.

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

Android: Native

Android: mWeb Chrome

20231011215211.mp4

iOS: Native

iOS: mWeb Safari

Screen.Recording.2023-10-11.at.9.33.24.PM.mov

MacOS: Chrome / Safari

screen-recording-2023-10-11-at-90010-pm_CJXojmu1.mp4

Recording.4967.mp4

MacOS: Desktop

Screen.Recording.2023-10-11.at.9.49.07.PM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014b6b128d6dc38a95
• Upwork Job ID: 1712480106690822144
• Last Price Increase: 2023-10-12
• Automatic offers:
• c3024 | Contributor | 27265410

[c3024]

Proposal

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

fake or empty string followed by comma and then then user is shown in display name for a thread created in workspace policy expense chat

What is the root cause of that problem?

When we create a thread for the first time in a policy expense chat the backend adds a participantID '0' to the participants and this here

App/src/pages/ReportDetailsPage.js

Line 146 in 633c94b

return ReportUtils.getDisplayNamesWithTooltips(OptionsListUtils.getPersonalDetailsForAccountIDs(participants, props.personalDetails), hasMultipleParticipants);

fetched the details for accountID '0' as well which returns fake or Hidden for the '0' accountID

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

1. We should fix the backend to not add '0' accountID as a participant
2. I think it is better if we return early here when accountID is '0' instead of adding details of it to personal details as an extra check.
   

   App/src/libs/OptionsListUtils.js

   Lines 142 to 148 in 4df6584

   	function getPersonalDetailsForAccountIDs(accountIDs, personalDetails) {
   	const personalDetailsForAccountIDs = {};
   	if (!personalDetails) {
   	return personalDetailsForAccountIDs;
   	}
   	_.each(accountIDs, (accountID) => {
   	const cleanAccountID = Number(accountID);

    _.each(accountIDs, (accountID) => {
        if (!accountID ) {         // or accountID === '0'
                 return;
        }
        const cleanAccountID = Number(accountID);

What alternative solutions did you explore? (Optional)

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014b6b128d6dc38a95

[melvin-bot]

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

[melvin-bot]

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]

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

[saranshbalyan-1234]

Proposal

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

Comma appears before username in the detail page

What is the root cause of that problem?

when we create a thread for the first time in a policy expense chat the participantID '0' is added to the participants by backend here

App/src/pages/ReportDetailsPage.js

Line 146 in 2c9c303

return ReportUtils.getDisplayNamesWithTooltips(OptionsListUtils.getPersonalDetailsForAccountIDs(participants, props.personalDetails), hasMultipleParticipants);

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

in the foreach loop, if the accountId is not present, dont add it in personalDetailsForAccountIDs variable which is returned at the end of the function
In the above proposal, as it is a forloop which would stop further processing of loop
instead wrap whatever inside the loop with an if statement which would look something like this

function getPersonalDetailsForAccountIDs(accountIDs, personalDetails) {
    const personalDetailsForAccountIDs = {};
    if (!personalDetails) {
        return personalDetailsForAccountIDs;
    }
    _.each(accountIDs, (accountID) => {
        const cleanAccountID = Number(accountID);
 if (cleanAccountID ) { 
        let personalDetail = personalDetails[accountID];
        if (!personalDetail) {
            personalDetail = {
                avatar: UserUtils.getDefaultAvatar(cleanAccountID),
            };
        }

        if (cleanAccountID === CONST.ACCOUNT_ID.CONCIERGE) {
            personalDetail.avatar = CONST.CONCIERGE_ICON_URL;
        }

        personalDetail.accountID = cleanAccountID;
        personalDetailsForAccountIDs[cleanAccountID] = personalDetail;
}
    });
    return personalDetailsForAccountIDs;
}

What alternative solutions did you explore? (Optional)

N/A

[theTrozen77]

Proposal

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

In the Details modal in the user name, we are seeing fake or comma before the actual user name content.

What is the root cause of that problem?

We are getting a falsy value i.e. 0 in our case inside the accountIDs array passed into getPersonalDetailsForAccountIDs which gets looped further down the application inside DisplayNamesWithToolTip component to give us the above mentioned issue.

https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/libs/OptionsListUtils.js#L142C1-L143C1

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

We need to clean the accountIDs array that is passed inside of getPersonalDetailsForAccountIDs so that it only contains Numbers and remove any falsy values.

Instead of checking or removing the falsy value inside of specific components, we should check and remove the falsy values inside of getPersonalDetailsForAccountIDs.

function getPersonalDetailsForAccountIDs(accountIDs, personalDetails) {
    const personalDetailsForAccountIDs = {};
    if (!personalDetails) {
        return personalDetailsForAccountIDs;
    }

    here ---> accountIDs = _.compact(accountIDs)

    _.each(accountIDs, (accountID) => {
        const cleanAccountID = Number(accountID);
        let personalDetail = personalDetails[accountID];
        if (!personalDetail) {
            personalDetail = {
                avatar: UserUtils.getDefaultAvatar(cleanAccountID),
            };
        }

        if (cleanAccountID === CONST.ACCOUNT_ID.CONCIERGE) {
            personalDetail.avatar = CONST.CONCIERGE_ICON_URL;
        }

        personalDetail.accountID = cleanAccountID;
        personalDetailsForAccountIDs[cleanAccountID] = personalDetail;
    });
    return personalDetailsForAccountIDs;
}

I have added accountIDs = _.compact(accountIDs) to make it consistent with the application coding structure and used it outside of _.each .

What alternative solutions did you explore? (Optional)

Alternate solution would be to remove it in the data that comes from the backend. But if it is not possible, this solution is well sufficient for the fix.

[melvin-bot]

📣 @theTrozen77! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[theTrozen77]

Contributor details
Your Expensify account email: prajwalpanta87@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~016a4ffa68a382833c

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

@burczu, @sonialiap Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sonialiap]

@burczu what do you think of the proposals?

[melvin-bot]

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

[melvin-bot]

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

[sonialiap]

I'm OOO Oct 16-23. Adding leave buddy

Status: looking for/choosing proposal

[burczu]

@sonialiap I'm sorry, I was busy reviewing PR's - I'll get back to checking proposals soon.

[melvin-bot]

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

📣 @ayazhussain79 We're missing your Upwork ID to automatically send you an offer for the Reporter role.
Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

[melvin-bot]

🎯 ⚡️ Woah @burczu / @c3024, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @c3024 got assigned: 2023-10-18 19:23:48 Z
• when the PR got merged: 2023-10-19 15:22:24 UTC

On to the next one 🚀

[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.3.88-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• skip getting personalDetail for 0 accountID #29930

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

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:

• @burczu does not require payment (Contractor)
• @c3024 requires payment offer (Contributor)
• @ayazhussain79 requires payment

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]

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:

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

[sonialiap]

Back from OOO. Thanks for pushing this along, Bartek!

[sonialiap]

@c3024 fix + bonus = $750 - paid ✔️
@ayazhussain79 report = $50 - paid ✔️

[ayazhussain79]

@sonialiap offer accepted, Thank you

[sonialiap]

@burczu please complete the checklist :)

[burczu]

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:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR: Main personalDetailsList & policyMembers Onyx key migrations #20473
• [@burczu] 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: Main personalDetailsList & policyMembers Onyx key migrations #20473 (comment)
• [@burczu] 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: didn't find
• [@burczu] Determine if we should create a regression test for this bug. I don't think this issues needs any regression tests - the bug wasn't impactful and didn't break any important App flow.
• [@burczu] 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.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

@burczu I'm not seeing the conclusion for the last point on the checklist, do you think we should do a regression test for this?

[burczu]

@sonialiap As I wrote in my previous comment:

I don't think this issues needs any regression tests - the bug wasn't impactful and didn't break any important App flow.

[sonialiap]

Thanks!