Skip to content

Fix user info missing in Crashlytics reports #3474

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

Jump to bottom
Open
avazirna wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix user info missing in Crashlytics reports #3474

avazirna wants to merge 1 commit into from

Conversation

@avazirna
Copy link
Contributor

@avazirna avazirna commented Dec 29, 2025

Product Description

This PR addresses an issue causing Crashlytics reports to not include username information, only Connect PersonalID is showing up in reports since version 2.58.

Safety Assurance

Safety story

Successfully tested locally. Here's the Crashlytics report: https://console.firebase.google.com/u/0/project/commcare-a57e4/crashlytics/app/android:org.commcare.dalvik/issues/eb1732035c3483ae925619824fd819a2

Labels and Review

  • Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
  • Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@avazirna avazirna requested a review from shubham1g5 December 29, 2025 10:07
@coderabbitai
Copy link

coderabbitai bot commented Dec 29, 2025

📝 Walkthrough

Walkthrough

The change modifies the getUserForCrashes method in ReportingUtils.java to alter its return behavior. When a non-empty user is retrieved, the method now returns that user immediately within the try block instead of allowing execution to fall through to a final null return statement. The handling of empty users and exception cases remains unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • Set Crashlytics user to ConnectID #3055: Directly related as it uses ReportingUtils.getUserForCrashes' return value as a fallback in CrashUtil.registerUserData for determining the Crashlytics user ID.

Suggested labels

product/invisible

Suggested reviewers

  • pm-dimagi
  • Jignesh-dimagi
  • shubham1g5

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description includes Product Description and Safety Assurance sections with testing evidence, but is missing Technical Summary, Feature Flag, Automated test coverage, QA Plan details, and incomplete Safety Assurance (no rationale or design decisions). Add Technical Summary with ticket link and design rationale, provide details on Automated test coverage and comprehensive QA Plan, and expand Safety Assurance with change rationale and data impact analysis.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: restoring user info to Crashlytics reports, which directly matches the primary change in the code.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-user-info-missing-in-crash-reports
📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0b18ff and 0907140.

📒 Files selected for processing (1)
  • app/src/org/commcare/android/logging/ReportingUtils.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-21T17:29:58.014Z 
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.

Applied to files:

  • app/src/org/commcare/android/logging/ReportingUtils.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Lint Code Base
🔇 Additional comments (1)
app/src/org/commcare/android/logging/ReportingUtils.java (1)

148-149: Excellent fix for the missing username bug!

The addition of return user; correctly resolves the critical issue where non-empty usernames were retrieved but never returned, causing Crashlytics reports to only show PersonalID. The method now properly returns the username when available, falls back to PersonalID when empty, and safely handles exceptions.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shubham1g5 shubham1g5 shubham1g5 approved these changes

@conroy-ricketts conroy-ricketts conroy-ricketts approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@avazirna @shubham1g5 @conroy-ricketts