Separate report name and icon configuration from personal details #7367
Conversation
MelvinBot
requested review from
parasharrajat and
tgolen
and removed request for
a team
src/libs/actions/Report.js
Outdated
| PersonalDetails.getFromReportParticipants(_.values(simplifiedReports)); | ||
|
|
||
| // Configure Report Name and Report Icon | ||
| configureReportNameAndIcon(_.values(simplifiedReports)); |
There was a problem hiding this comment.
There could be race conditions here personal details may have not been stored in Onyx when this is called.
There was a problem hiding this comment.
Promise.resolve(PersonalDetails.getFromReportParticipants(_.values(simplifiedReports))).then(
details => configureReportNameAndIcon(_.values(simplifiedReports), details),
);
@parasharrajat It will solve the issue. Any suggestion?
There was a problem hiding this comment.
It won't be needed when PersonalDetails.getFromReportParticipants will return a promise.
src/libs/actions/Report.js
Outdated
| Onyx.connect({ | ||
| key: ONYXKEYS.PERSONAL_DETAILS, | ||
| callback: (formattedPersonalDetails) => { | ||
| const reportsToUpdate = {}; |
There was a problem hiding this comment.
Where are you canceling the subscription?
There was a problem hiding this comment.
I didn't find anything about cancelling the subscription in the code as well as documentation.
Do you mean?
Onyx.connect({
key: ONYXKEYS.PERSONAL_DETAILS,
callback: (formattedPersonalDetails) => {
+ if (!formattedPersonalDetails) {
+ return;
+ }
const reportsToUpdate = {};I am planing not to connect to onyx here. Instead use the returned value from PersonalDetails.getFromReportParticipants(_.values(simplifiedReports))
There was a problem hiding this comment.
Yeah, that is a good idea.
Action methods should only have return values (data or a promise) if they are called by other actions. This is done to encourage that action methods can be called in parallel with no dependency on other methods (see discussion above).
|
Could you please also update the template? |
src/libs/actions/Report.js
Outdated
| Promise.resolve(PersonalDetails.getFromReportParticipants(_.values(simplifiedReports))).then( | ||
| details => configureReportNameAndIcon(_.values(simplifiedReports), details), | ||
| ); |
There was a problem hiding this comment.
| Promise.resolve(PersonalDetails.getFromReportParticipants(_.values(simplifiedReports))).then( | |
| details => configureReportNameAndIcon(_.values(simplifiedReports), details), | |
| ); | |
| const simplifiedReportsList = _.values(simplifiedReports); | |
| PersonalDetails.getFromReportParticipants(simplifiedReportsList) | |
| .then(details => configureReportNameAndIcon(simplifiedReportsList, details)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, that is fine. the interface should be consistent.
if (participantEmails.length === 0) {
return Promise.resolve([]); // if details is array then empty array.
}
|
Still.... space needed between $ and url Please follow the template. It is necessary for automation. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Heyo, this is causing #7417 so this will be reverted. |
|
@parasharrajat I have updated the branch which should probably solve the issue. It solves the issue in my side. Could you please verify? Should I create a new PR? |
|
Yeah, please create a new PR |
|
🚀 Deployed to production by @roryabraham in version: 1.1.33-3 🚀
|
Details
Solve the issue of report names and icons.
Fixed Issues
$ #7261
Tests
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android