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

check if report.icons and report.icons[0] exists #5381

Merged
merged 3 commits into from
Sep 23, 2021
Merged

Conversation

marcochavezf
Copy link
Contributor

@marcochavezf marcochavezf commented Sep 20, 2021

Details

Context: #5232

Sometimes the avatars disappear from the LHN and I was only able to reproduce the issue after logging out and loging in again with a different account.

I found out the value of report.actions sometimes is undefined or has an array with an empty string (['']). This is because the links of the avatar thumbnails are fetched after inserting the simplifiedReports into Onyx:

Screen Shot 2021-09-20 at 16 03 26

here ^ PersonalDetails.getFromReportParticipants(Object.values(simplifiedReports)) is called to fetch the avatar thumbnails asynchronously (using API.PersonalDetails_GetForEmails), which resolves after calling
Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, simplifiedReports) which triggers a re-render synchronously and calls the function createOption before fetching the avatars:

Screen Shot 2021-09-20 at 16 17 18

My hypothesis is that the avatar won't appear if API.PersonalDetails_GetForEmails doesn't resolve because of a connection issue (i.e. internet goes offline exactly when the app is fetching the avatars). So I added more conditions to check not only if the report object exists but also the icons and icons[0] has a valid value; I found sometimes icons[0] is an empty string too: https://github.com/Expensify/App/blob/main/src/libs/OptionsListUtils.js#L760 (which is called in PersonalDetails.getFromReportParticipants).

Seems the issue was only reproducible on Desktop and Web.

Fixed Issues

$ #5232

Tests

  1. Log in
  2. Log out
  3. Log in with a different account

QA Steps

  1. Log in
  2. Log out
  3. Log in with a different account

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-09-20 at 18 35 51

Mobile Web

Desktop

Screen Shot 2021-09-20 at 18 38 32

iOS

Android

@marcochavezf marcochavezf requested a review from a team as a code owner September 20, 2021 23:08
@MelvinBot MelvinBot requested review from tgolen and removed request for a team September 20, 2021 23:08
@marcochavezf marcochavezf self-assigned this Sep 21, 2021
@@ -230,10 +230,11 @@ function createOption(personalDetailList, report, draftComments, {
? lastMessageText
: Str.removeSMSDomain(personalDetail.login);
}
const icons = (report && report.icons && report.icons[0]) ? report.icons : [personalDetail.avatar];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use Lodash.get instead for this? I think it would make this more simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@marcochavezf
Copy link
Contributor Author

Update: use lodashGet to retrieve either report.actions or [personalDetail.avatar]

@marcochavezf marcochavezf requested a review from tgolen September 23, 2021 20:27
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Ah, that looks great. Thanks!

@tgolen tgolen merged commit dcedffb into main Sep 23, 2021
@tgolen tgolen deleted the marco-missingAvatarLHN branch September 23, 2021 22:26
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @tgolen in version: 1.1.1-9 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants