Null-check when logging integrity analytics event

[OrangeAndGreen]

https://dimagi.atlassian.net/browse/CCCT-1808

Product Description

No user-facing change

Technical Summary

Logging a non-fatal exception when trying to send integrity analytics event with a null (or "null") responseCode.

Safety story

Simple change to log a non-fatal exception, not user-facing.

Automated test coverage

None

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds a null-check in the reportPersonalIdIntegritySubmission method of FirebaseAnalyticsUtil.java. When responseCode is null or the string "null", the method now logs an exception via Logger.exception before proceeding with the existing analytics event building and sending logic. The method signature and subsequent control flow remain unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Simple defensive null-check with logging
• Single file modification
• No changes to method signature or core logic flow
• Standard exception logging pattern

Suggested reviewers

• shubham1g5
• Jignesh-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description includes several required sections from the template: the ticket link (CCCT-1808), product description (no user-facing change), technical summary with rationale (logging non-fatal exception for null responseCode), and the safety story. However, the description is missing the QA Plan section, which is a critical part of the Safety Assurance section requiring explanation of how the PR proves regression-free behavior and linking to a QA ticket. Additionally, the Labels and Review section with its required checkboxes for QA Note, Release Note, Risk label, and reviewer verification is entirely absent. These are substantive sections specified in the template.	The author should add the missing QA Plan section describing how regression-free behavior will be verified (even if minimal for this simple logging change) and link to any QA ticket if applicable. Additionally, the author should complete the Labels and Review section by addressing each checkbox item regarding QA Note, Release Note, Risk label, and reviewer suitability. This will ensure the description fully complies with the repository's template requirements.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "Null-check when logging integrity analytics event" clearly and concisely summarizes the main change described in the raw summary. The title accurately captures both the key action (adding a null-check) and the context (integrity analytics event logging). It is specific and directly related to the core modification without being overly broad or using vague terminology.

✨ 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 integrity_null_check

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)

434-436: Null-check logic is correct, but consider whether to proceed with analytics submission.

The null-check and logging implementation correctly handles both null and the string "null". The short-circuit evaluation prevents NPE. However, the method continues to send the analytics event with the null/invalid responseCode (line 441), which may pollute analytics data and make it harder to distinguish legitimate integrity issues from data quality problems.

Consider whether the method should return early after logging instead of proceeding:

 if(responseCode == null || responseCode.equals("null")) {
     Logger.exception("Integrity Check", new Exception("Null response code in integrity check: " + responseCode));
+    return;
 }

If you intend to track these null cases in analytics for visibility, the current approach works, but you may want to verify that downstream analytics consumers can handle or filter these null values appropriately.

Minor: The exception message could be simplified since concatenating responseCode is redundant when you already know it's null or "null":

-Logger.exception("Integrity Check", new Exception("Null response code in integrity check: " + responseCode));
+Logger.exception("Integrity Check", new Exception("Null or invalid response code in integrity check"));

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6dd813 and 3ca394d.

📒 Files selected for processing (1)

• app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)

app/src/org/commcare/android/integrity/IntegrityTokenViewModel.kt (1)

• exception (128-128)

[conroy-ricketts]

LGTM