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

[$250] Android/IOS - Chat - App is crashed after tap on avatar for non-existing account #9182

Closed
kbecciv opened this issue May 26, 2022 · 37 comments
Assignees
Labels
Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented May 26, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Launch the app
  2. Log in with any account
  3. Select Search
  4. Type any non-existing account
  5. Tap on Avatar for non-existing account

Expected Result:

App is not crashed after tap on avatar for non-existing account

Actual Result:

App is crashed after tap on avatar for non-existing account

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android

Version Number: 1.1.67.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

Screen_Recording_20220526-093147_New.Expensify.mp4
Image.from.iOS.29.MP4

Expensify/Expensify Issue URL:

Issue reported by: @adeel0202 Applause - Internal Team

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1649374016010969

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented May 26, 2022

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

@kavimuru kavimuru changed the title Android/IOS - App is crashed after tap on avatar for non-existing account Android/IOS - Chat - App is crashed after tap on avatar for non-existing account May 26, 2022
@danieldoglas
Copy link
Contributor

Confirmed it is a bug and it's not on API.

@danieldoglas danieldoglas added Improvement Item broken or needs improvement. External Added to denote the issue can be worked on by a contributor labels May 26, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2022

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

@danieldoglas danieldoglas removed their assignment May 26, 2022
@bfitzexpensify
Copy link
Contributor

Upwork job here

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels May 27, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 27, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 27, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 27, 2022

Triggered auto assignment to @aldo-expensify (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title Android/IOS - Chat - App is crashed after tap on avatar for non-existing account [$250] Android/IOS - Chat - App is crashed after tap on avatar for non-existing account May 27, 2022
@imran-ranglerz
Copy link

we can handle this exception by using ? mark . as you say the account is not exist so the code surly pick the data and it did not find data so we can use ? mark.
if we did not have data of non existing account we will not face crash any more with ? mark

@adeel0202
Copy link
Contributor

I reported this bug 2 months ago but its ticket wasn't created. The thread is here.

@mollfpr
Copy link
Contributor

mollfpr commented May 27, 2022

Proposal

Why?

The error showing up because there's no personalDetails data with the report id from DetailsPage component. personalDetails is come from Onyx, it needs to be fetched first and it fetches from ReportActionsView loaded.

if (!this.props.report.reportID) {
Report.fetchChatReportsByIDs([this.props.reportID], true);
}

The problem is there's a different time between Web and iOS/Android when loading the report data. I attach a screenshot of the different report data showing up on ReportScreen component and ReportActionsView component.
ReportActionsView${this.props.report.reportID} this value is show on componentDidMount and ReportScreen${this.props.report.reportID} is show inside render() function.

On Web, the report data is loaded after ReportActionsView component so it will fetch the personalDetails data with Report.fetchChatReportsByIDs([this.props.reportID], true);.

Screen Shot 2022-05-27 at 18 23 10

On the iOS/Android, the report data come in first before the ReportActionsView component is loaded. So it will not fetch the personalDetails data cause the condition is not true if (!this.props.report.reportID).

Screen Shot 2022-05-27 at 18 22 17


How?

So we need to fetch the personalDetails data for the report id to be able to show the user detail data.

if (!this.props.report.reportID) {
Report.fetchChatReportsByIDs([this.props.reportID], true);
}

We can extend the condition to check if the personalDetails data is provided or not to run the Report.fetchChatReportsByIDs function.

        if (!this.props.report.reportID || !this.props.personalDetails[this.props.report.reportID]) {
            Report.fetchChatReportsByIDs([this.props.reportID], true);
        }

Result

Screen.Recording.2022-05-27.at.18.46.15.mov
Screen.Recording.2022-05-27.at.18.45.49.mov

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels May 27, 2022
@dhruv-silicon
Copy link

We have checked the source code and tried to run the app.

We have identified that the non-existing account, it is not able to get the below example data:

{"avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_8.png", "avatarHighResolution": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_8.png", "displayName": "sd@yopmail.com", "firstName": "", "lastName": "", "login": "sd@yopmail.com", "payPalMeAddress": "", "phoneNumber": "", "pronouns": "", "timezone": {"automatic": true, "selected": "America/Los_Angeles"}}

That data you are getting from the DetailsPage.js file:

const details = props.personalDetails[props.route.params.login];
const isSMSLogin = Str.isSMSLogin(details.login);
const shouldShowLocalTime = !ReportUtils.hasExpensifyEmails([details.login]);
let pronouns = details.pronouns;

Also, we are facing this issue for the one time only, for the second time onwards, we are not getting any crashes.

Thanks & Regards.

Screen.Recording.2022-05-30.at.5.12.59.PM.mov

@melvin-bot
Copy link

melvin-bot bot commented May 30, 2022

@mananjadhav, @bfitzexpensify, @aldo-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@aldo-expensify aldo-expensify removed the Weekly KSv2 label Jun 9, 2022
@aldo-expensify
Copy link
Contributor

Taking this internally as the proposed solution doesn't work well, and I think a better approach is to just show a loading indicator while the personal details are not available:

PR up: #9385

I'm changing this to Daily because it crashes the app and it doesn't look too hard for it to happen.

@aldo-expensify aldo-expensify added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jun 9, 2022
@aldo-expensify
Copy link
Contributor

After digging more and testing in Android, I realized that what I was proposing here (just adding a spinner) doesn't fix it for Android/IOS.

There are two different problems that end up with the same crash. I mistakenly though they were the same.

  • Problem 1 (this GH issue): only happens in Android/IOS and is not related to tapping fast on the avatar. It happens always with accounts that doesn't exist.
  • Problem 2 (here): the app crashes if you tap too fast in the other users avatar, before the request PersonalDetails_GetForEmails finishes.

@mollfpr proposed solution indeed fixes the Problem 1, but the crash can still happen if you click the avatar fast enough before the request PersonalDetails_GetForEmails finishes.

On the other hand, what I was proposing (just add a spinner) solves Problem 2. My fix without @mollfpr solution causes the spinner to appear and spin forever in Android/IOS.

Having said that, I'll put everything together in this PR: #9385, and @mollfpr should still be compensated because we are using his solution.

@aldo-expensify aldo-expensify added the Reviewing Has a PR in review label Jun 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 2022

@mananjadhav, @bfitzexpensify, @aldo-expensify Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot
Copy link

melvin-bot bot commented Jun 20, 2022

@mananjadhav, @bfitzexpensify, @aldo-expensify Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Jun 22, 2022

@mananjadhav, @bfitzexpensify, @aldo-expensify Still overdue 6 days?! Let's take care of this!

@aldo-expensify
Copy link
Contributor

Melvin, PR is in review.

@bfitzexpensify bfitzexpensify added Reviewing Has a PR in review and removed Reviewing Has a PR in review labels Jun 27, 2022
@melvin-bot melvin-bot bot added the Overdue label Jun 27, 2022
@bfitzexpensify
Copy link
Contributor

Not overdue, in review

@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2022

@mananjadhav, @bfitzexpensify, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Jul 6, 2022

@mananjadhav, @bfitzexpensify, @aldo-expensify Eep! 4 days overdue now. Issues have feelings too...

@aldo-expensify
Copy link
Contributor

No overdue, the PR was merged, but has not been deployed yet.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

📣 @mollfpr You have been assigned to this job by @aldo-expensify!
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 📖

@melvin-bot
Copy link

melvin-bot bot commented Jul 14, 2022

@mananjadhav, @mollfpr, @bfitzexpensify, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Jul 18, 2022

@mananjadhav, @mollfpr, @bfitzexpensify, @aldo-expensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@aldo-expensify
Copy link
Contributor

No overdue. The PR has been deployed to production

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@bfitzexpensify
Copy link
Contributor

OK, reposting the Upwork job here, priced at $250 for the following contributors:

@adeel0202 (reporting)
@mollfpr (proposal was used in internal solution)
@mananjadhav (C+ review of proposal)

Please apply and I will pay out once that's done - thanks!

@bfitzexpensify
Copy link
Contributor

All contracts paid out and ended - thanks for the work here everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests