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.

Sign up for GitHub

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

Separate report name and icon configuration from personal details #7367

Merged
merged 4 commits into from
Jan 25, 2022
Merged

Separate report name and icon configuration from personal details #7367

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df12d41
Separate report name and icon configuration from personal details
sobitneupane Jan 24, 2022
57f3264
Resolve race condition in fetchChatReportsByIDs
sobitneupane Jan 24, 2022
753ad92
Make syntatic change in use of Promise
sobitneupane Jan 25, 2022
8e19bf3
Maintain consistent interface
sobitneupane Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 3 additions & 32 deletions src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import CONST from '../../CONST';
import NetworkConnection from '../NetworkConnection';
import * as API from '../API';
import NameValuePair from './NameValuePair';
import * as ReportUtils from '../reportUtils';
import * as OptionsListUtils from '../OptionsListUtils';
import Growl from '../Growl';
import * as Localize from '../Localize';
Expand Down Expand Up @@ -146,6 +145,7 @@ function fetchPersonalDetails() {
* Get personal details from report participants.
*
* @param {Object} reports
* @returns {Promise}
*/
function getFromReportParticipants(reports) {
const participantEmails = _.chain(reports)
Expand All @@ -158,7 +158,7 @@ function getFromReportParticipants(reports) {
return;
}

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

Expand All @@ -173,36 +173,7 @@ function getFromReportParticipants(reports) {

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);
return details;
});
}

Expand Down
39 changes: 38 additions & 1 deletion src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import Timers from '../Timers';
import * as ReportActions from './ReportActions';
import Growl from '../Growl';
import * as Localize from '../Localize';
import * as OptionsListUtils from '../OptionsListUtils';

let currentUserEmail;
let currentUserAccountID;
Expand Down Expand Up @@ -319,6 +320,39 @@ function fetchIOUReportID(debtorEmail) {
});
}

function configureReportNameAndIcon(reports, details) {
// 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 = ReportUtils.isChatRoom(report) ? (['']) : OptionsListUtils.getReportIcons(report, details);
const reportName = ReportUtils.isChatRoom(report)
? report.reportName
: _.chain(report.participants)
.filter(participant => participant !== currentUserEmail)
.map(participant => lodashGet(
details,
[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);
}

/**
* Fetches chat reports when provided a list of chat report IDs.
* If the shouldRedirectIfInaccessible flag is set, we redirect to the Concierge chat
Expand Down Expand Up @@ -400,7 +434,10 @@ function fetchChatReportsByIDs(chatList, shouldRedirectIfInaccessible = false) {
Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, simplifiedReports);

// Fetch the personal details if there are any
PersonalDetails.getFromReportParticipants(_.values(simplifiedReports));
Promise.resolve(PersonalDetails.getFromReportParticipants(_.values(simplifiedReports))).then(
details => configureReportNameAndIcon(_.values(simplifiedReports), details),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Promise.resolve(PersonalDetails.getFromReportParticipants(_.values(simplifiedReports))).then(
details => configureReportNameAndIcon(_.values(simplifiedReports), details),
);
const simplifiedReportsList = _.values(simplifiedReports);
PersonalDetails.getFromReportParticipants(simplifiedReportsList)
.then(details => configureReportNameAndIcon(simplifiedReportsList, details));

Copy link
Contributor Author

@sobitneupane sobitneupane Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we should also return promise in case particpantEmails.length === 0 from PersonalDetails.getFromReportParticipants()

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

@parasharrajat Any Suggestion?

Copy link
Member

@parasharrajat parasharrajat Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is fine. the interface should be consistent.

  if (participantEmails.length === 0) {
      return Promise.resolve([]); // if details is array then empty array.
  }


return fetchedReports;
})
.catch((err) => {
Expand Down