Private Room - Chair icon not displayed for private room created

[kbecciv]

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

---

Action Performed:

1. Launch the app and login
2. Click the '+' icon and select New room
3. Create a private room

Expected Result:

Chair Icon displayed for private room

Actual Result:

Chair icon not displayed for private room but displayed for restricted room

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.30

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

Issue was found when executing: #7197

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @kadiealexander (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[sobitneupane]

Reason
After storing report in Onyx, PersonalDetails.getFromReportParticipants(_.values(simplifiedReports)); is called from fetchChatReportsByIDs function which adds icons to the report. The same function adds icon to the restricted rooms as well. But in case of private chatRooms, there are no participants (in newly created room). Since, participantEmails length is equal to 0, PersonalDetails.getFromReportParticipants(_.values(simplifiedReports)); returns without performing any operation. I don't think intended function of PersonalDetails.getFromReportParticipants(_.values(simplifiedReports)); is to add icons incase of rooms. So, for chatroom I propose to add icons in report by adding following code.

Proposal
I propose to add icons to the report if the report is policyRoom by adding following code

if(ReportUtils.isChatRoom(simplifiedReport)){
    simplifiedReport.icons = [""]
}

in

App/src/libs/actions/Report.js

Lines 377 to 380 in 7dfe96f

	_.each(fetchedReports, (report) => {
	const simplifiedReport = getSimplifiedReportObject(report);
	simplifiedReports[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`] = simplifiedReport;
	});

[tgolen]

Hi @sobitneupane and thanks for the proposal! I really like your thinking here:

I don't think intended function of PersonalDetails.getFromReportParticipants(_.values(simplifiedReports)); is to add icons incase of rooms

I 100% agree with you. What would you think about doing a small refactoring here so that PersonalDetails.getFromReportParticipants() has the sole purpose of getting the personal details. Then in the Report action, there is a specific method that we would add like configureReportIcons() which has the sole purpose of setting the report icons for the different types of reports?

[sobitneupane]

Sure.
I propose the following solution:
Let's remove the code that sets reportName and report Icon from PersonalDetails.getFromReportParticipants().

function getFromReportParticipants(reports) {
    const participantEmails = _.chain(reports)
        .pluck('participants')
        .flatten()
        .unique()
        .value();

    if (participantEmails.length === 0) {
        return;
    }

    API.PersonalDetails_GetForEmails({emailList: participantEmails.join(',')})
        .then((data) => {
            const existingDetails = _.pick(data, participantEmails);

            // Fallback to add logins that don't appear in the response
            const details = _.chain(participantEmails)
                .filter(login => !data[login])
                .reduce((previousDetails, login) => ({
                    ...previousDetails,
                    [login]: {}, // Simply just need the key to exist
                }), existingDetails)
                .value();

            const formattedPersonalDetails = formatPersonalDetails(details);
            Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, formattedPersonalDetails);

-            // The personalDetails of the participants contain their avatar images. Here we'll go over each
-            // report and based on the participants we'll link up their avatars to report icons. This will
-            // skip over default rooms which aren't named by participants.
-            const reportsToUpdate = {};
-            _.each(reports, (report) => {
-                if (report.participants.length <= 0 && !ReportUtils.isChatRoom(report)) {
-                    return;
-                }
-
-                const avatars = OptionsListUtils.getReportIcons(report, details);
-                const reportName = ReportUtils.isChatRoom(report)
-                    ? report.reportName
-                    : _.chain(report.participants)
-                        .filter(participant => participant !== currentUserEmail)
-                        .map(participant => lodashGet(
-                            formattedPersonalDetails,
-                            [participant, 'displayName'],
-                            participant,
-                        ))
-                        .value()
-                        .join(', ');
-
-                reportsToUpdate[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`] = {icons: avatars, reportName};
-            });
-
-            // We use mergeCollection such that it updates ONYXKEYS.COLLECTION.REPORT in one go.
-            // Any withOnyx subscribers to this key will also receive the complete updated props just once
-            // than updating props for each report and re-rendering had merge been used.
-            Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, reportsToUpdate);
        });
}

And Add configureReportDetails() function in src/libs/actions/Report.js for setting report icon and report name.

function configureReportDetails(reports) {
    // The personalDetails of the participants contain their avatar images. Here we'll go over each
    // report and based on the participants we'll link up their avatars to report icons. This will
    // skip over default rooms which aren't named by participants.
    
    let formattedPersonalDetails;
    Onyx.connect({
        key: ONYXKEYS.PERSONAL_DETAILS,
        callback: (val) => {
            formattedPersonalDetails = val
            const reportsToUpdate = {};
            _.each(reports, (report) => {
                if (report.participants.length <= 0 && !ReportUtils.isChatRoom(report)) {
                    return;
                }

                const avatars = ReportUtils.isChatRoom(report) ? (['']) : OptionsListUtils.getReportIcons(report, formattedPersonalDetails);
                const reportName = ReportUtils.isChatRoom(report)
                    ? report.reportName
                    : _.chain(report.participants)
                        .filter(participant => participant !== currentUserEmail)
                        .map(participant => lodashGet(
                            formattedPersonalDetails,
                            [participant, 'displayName'],
                            participant,
                        ))
                        .value()
                        .join(', ');

                reportsToUpdate[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`] = {icons: avatars, reportName};
            });

            // We use mergeCollection such that it updates ONYXKEYS.COLLECTION.REPORT in one go.
            // Any withOnyx subscribers to this key will also receive the complete updated props just once
            // than updating props for each report and re-rendering had merge been used.
            Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, reportsToUpdate);
            },
    });
}

[tgolen]

I like it! Change the name of the method to configureReportNameAndIcon and you've got me sold :D 🟢 on that proposal.

@kadiealexander can you please move forward with hiring them for this and posting the job on upwork?

[kadiealexander]

Sorry team, was OOO for a couple days! Here's the Upwork link: https://www.upwork.com/ab/applicants/1484387572985319424/

@sobitneupane I've invited you to the job, could you please accept?

[MelvinBot]

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

[MelvinBot]

Current assignee @tgolen is eligible for the Exported assigner, not assigning anyone new.

[sobitneupane]

@sobitneupane I've invited you to the job, could you please accept?

Accepted.

[parasharrajat]

I can see a good refined proposal but it has some issues/challenges. I am sure that we can overcome those on PR review.

Overall, I like @sobitneupane 's proposal as well. I am anticipating that it will fix one more issue (can't find atm).

🎀 👀 🎀 C+ reviewed

[MelvinBot]

📣 @sobitneupane You have been assigned to this job by @kadiealexander!
Please apply to this job in Upwork 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 📖

[kadiealexander]

Don't worry about the above @sobitneupane, it's already done. :)

[johnmlee101]

This caused a staging deploy blocker so the PR is being reverted #7417

[sobitneupane]

Hello @parasharrajat , While working on this issue, I have found another issue that the icons for new chats and new group chats are not visible in report body in mobile app. Should I report the issue on slack? Should I solve the bug as part of this issue? The solution to the bug is different from that of this issue.

[parasharrajat]

If that is not caused by your PR and not fixed by the changes for this PR then you should report that on Slack.

[sobitneupane]

Okay. Thanks for the suggestion. It is not caused by my PR. It is already reverted. The issue exists in current codebase.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[botify]

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

• Separate report name and icon configuration from personal details #7367

If no regressions arise, payment will be issued on 2022-02-08. 🎊

[botify]

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

• Separate report name and icon configuration from personal details #7453

If no regressions arise, payment will be issued on 2022-02-14. 🎊

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[tgolen]

Unfortunately, it looks like this has led to another regression: #7630 so I am reverting the PR again for now.

[tgolen]

Hi! OK, coming back here... The revert of this PR helped me to discover a big performance improvement that I was able to make in #7649. As @luacmartins and I were debugging this, we found out that even with the new optimization fix, the code in this PR still results in a near-infinite loop happening that we were not able to track down. I suspect it has something to do with the addition of the Promise, since the rest of the refactoring seems straightforward. This is what I would like to recommend:

1. @sobitneupane, if you can give me your expensify email account address, I want to add you to a large policy with tons of members. This will hopefully allow you to reproduce the problem with the infinite loop
2. See if the solution using the Promise can still work without leading to the infinite loop
3. If that doesn't work, I guess we can just skip the refactoring that we tried to do and implement the icon fix inside the code where it currently sits

[sobitneupane]

Hi! OK, coming back here... The revert of this PR helped me to discover a big performance improvement that I was able to make in #7649. As @luacmartins and I were debugging this, we found out that even with the new optimization fix, the code in this PR still results in a near-infinite loop happening that we were not able to track down. I suspect it has something to do with the addition of the Promise, since the rest of the refactoring seems straightforward. This is what I would like to recommend:

1. @sobitneupane, if you can give me your expensify email account address, I want to add you to a large policy with tons of members. This will hopefully allow you to reproduce the problem with the infinite loop
2. See if the solution using the Promise can still work without leading to the infinite loop
3. If that doesn't work, I guess we can just skip the refactoring that we tried to do and implement the icon fix inside the code where it currently sits

Okay.
My Expensify email account address is [email protected].

[tgolen]

OK, I've added you! Please don't do anything on that policy in expensify.com, but you should now see a ton of logins on your personal details.

[mvtglobally]

Issue not reproducible during KI retests. (Third week)
@tgolen This particular issue is not longer repro, but looks like it lead to other issues. Should we keep this open or open a new issue for the specific scenario?

[tgolen]

Wow, that's SUPER weird that you cannot reproduce the original problem, especially considering that all the code for this was reverted. What is "the specific scenario" that's still happening?

[mvtglobally]

@tgolen I might've misunderstood the discussion. I thought the main issue is fixed and there is some additional case which still needs to be resolved.
Original issue is still not repro - Fourth week. Maybe another PR fixed the problem. Should we close this then?

[tgolen]

I think at this point, we should just close the issue. There is nothing more to do. Thanks for your help!