CI-423 Categorize Demo Users In Analytics

[conroy-ricketts]

CI-423

Technical Summary

These changes complete the first step in the ticket:

Categorize users who are marked as Demo in general when looking at firebase analytics

Later, my plan is to modify our query for the user funnel based on this new property.

Safety Assurance

Safety story

I will run a test query to make sure that the new parameter for my test user shows as expected before I merge these changes in.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds demo user tracking to analytics by introducing a detection mechanism across the stack. It exposes the private isDemo field in ConnectUserRecord via a new getter, creates a utility method in ReportingUtils to safely retrieve demo user status from the current user record with error handling, defines a new analytics parameter constant IS_DEMO_USER, and integrates demo status tracking into FirebaseAnalyticsUtil.setUserProperties() for user property reporting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Files to focus on: FirebaseAnalyticsUtil.java (verify null-check logic and exception handling flow) and ReportingUtils.java (confirm error handling doesn't mask legitimate failures)

Possibly related PRs

• Add photo and isDemo fields to Connect User Record #3112: Earlier addition of the isDemo field to ConnectUserRecord that this PR now exposes via the new getter method.
• Added device_id to analytics in user properties. #3056: Similar pattern of extending CCAnalyticsParam and FirebaseAnalyticsUtil.setUserProperties() to track additional user properties.
• - Restricted demo user from sending firebase otp #3124: Parallel work modifying ConnectUserRecord with demo user handling and public accessors.

Suggested labels

skip-integration-tests

Suggested reviewers

• OrangeAndGreen
• shubham1g5
• Jignesh-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
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 (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding demo user categorization to analytics. It directly relates to the primary objective of the changeset.
Description check	✅ Passed	The description includes a ticket link, technical summary of the change, and safety assurance with planned verification. However, several template sections are missing or incomplete: Product Description, Feature Flag, Automated test coverage, and QA Plan are absent or underdeveloped.

✨ 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 bug/CI-423-categorize-demo-users-in-analytics

---

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 (2)

app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)

239-241: Accessor for demo flag looks good; consider brief documentation.

The getter cleanly exposes the persisted isDemo flag and matches its use from ReportingUtils. To align with how ConnectUserRecord fields are generally documented, you might add a short Javadoc explaining what counts as a “demo” user in this context (e.g., Personal ID demo account vs other demo states).
Based on learnings, this class benefits from field/method Javadocs.

app/src/org/commcare/android/logging/ReportingUtils.java (1)

13-13: Avoid exception-driven flow in getIsDemoUser to reduce log noise.

Right now, if there’s no Personal ID user (or getUser returns null), you’ll likely hit a NullPointerException inside the try and log an exception every time setUserProperties runs, which can be frequent on pre-login or non‑Personal‑ID flows. You already short‑circuit with isloggedIn() in getPersonalID; mirroring that here would avoid treating expected states as errors.

One possible refactor:

@@
-    public static Boolean getIsDemoUser() {
-        try {
-            CommCareApplication app = CommCareApplication.instance();
-            PersonalIdManager manager = PersonalIdManager.getInstance();
-            return manager.getUser(app).getIsDemo();
-        } catch (Exception e) {
-            String errorMessage =
-                    "Error checking if user is a demo user for Firebase analytics reporting";
-            Logger.exception(errorMessage, e);
-            return null;
-        }
-    }
+    public static Boolean getIsDemoUser() {
+        try {
+            CommCareApplication app = CommCareApplication.instance();
+            PersonalIdManager manager = PersonalIdManager.getInstance();
+
+            if (!manager.isloggedIn()) {
+                return null;
+            }
+
+            // getUser may still return null in edge cases; treat that as "unknown" demo status
+            if (manager.getUser(app) == null) {
+                return null;
+            }
+
+            return manager.getUser(app).getIsDemo();
+        } catch (Exception e) {
+            String errorMessage =
+                    "Error checking if user is a demo user for Firebase analytics reporting";
+            Logger.exception(errorMessage, e);
+            return null;
+        }
+    }

(You can cache manager.getUser(app) in a local variable if desired to avoid calling it twice.)

Also applies to: 163-174

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 484df28 and a004621.

📒 Files selected for processing (4)

• app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1 hunks)
• app/src/org/commcare/android/logging/ReportingUtils.java (2 hunks)
• app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1 hunks)
• app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-01-21T17:28:09.007Z

Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses Persisting annotations with sequential indices for field persistence, and MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.

Applied to files:

• app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java

📚 Learning: 2025-01-27T15:14:56.422Z

Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/utils/EncryptionUtils.java:167-167
Timestamp: 2025-01-27T15:14:56.422Z
Learning: MD5 hashing is acceptable for non-security purposes like analytics tracking in FirebaseAnalyticsUtil.

Applied to files:

• app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java

🧬 Code graph analysis (2)

app/src/org/commcare/android/logging/ReportingUtils.java (2)

app/src/org/commcare/CommCareApplication.java (1)

• CommCareApplication (158-1255)

app/src/org/commcare/connect/PersonalIdManager.java (1)

• PersonalIdManager (63-581)

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

app/src/org/commcare/android/logging/ReportingUtils.java (1)

• ReportingUtils (20-175)

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

• CCAnalyticsParam (7-51)

⏰ 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 (2)

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

50-50: New IS_DEMO_USER analytics param is consistent and clear.

The constant name and value follow the existing convention in CCAnalyticsParam and are appropriate for a Firebase user property key.

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

28-28: Demo user property wiring into Firebase looks correct and null‑safe.

Fetching Boolean isDemoUser from ReportingUtils and only setting IS_DEMO_USER when it’s non‑null is consistent with how other user properties are handled, and String.valueOf(isDemoUser) matches the existing pattern of stringified booleans (e.g., CCC_ENABLED). No functional issues spotted here.

Also applies to: 127-130

[conroy-ricketts]

@damagatchi retest this please