CCCT-2070 - Connect Generic Opportunity Push Notification#3555
CCCT-2070 - Connect Generic Opportunity Push Notification#3555Jignesh-dimagi wants to merge 23 commits intomasterfrom
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts,
jaypanchal-13 and
shubham1g5
|
@damagatchi retest this please |
Jignesh-dimagi
changed the base branch from
jignesh/feat/ccct-2054
to
master
📝 WalkthroughWalkthroughThis pull request migrates the Android Connect platform from integer-based identifiers (jobId, unitId, paymentId) to UUID-based string identifiers across the entire data layer and API surface. Database models (ConnectJobRecord, ConnectDeliveryDetails, ConnectJobPaymentRecord, etc.) are updated to use UUID fields. Database helper methods (ConnectJobUtils) change to query by UUID instead of numeric IDs. Network API endpoints (ApiConnect, ApiService) are modified to accept and pass UUIDs. Activities and fragments are updated to use the new UUID-based getters and setters. A new generic opportunity notification type (CCC_GENERIC_OPPORTUNITY) is introduced with centralized action resolution utilities (getActionFromIntent, getActionFromRecord, getActionFromPayload). Database upgrade paths are added to handle schema evolution. Notification sync workers are enhanced to support the generic opportunity flow alongside existing delivery and learning progress synchronization. Sequence DiagramsequenceDiagram
participant App as Activities/Fragments
participant NotiHandler as NotificationsSyncWorkerManager
participant NotiWorker as NotificationsSyncWorker
participant JobHelper as ConnectJobHelper
participant DBUtil as ConnectJobUtils
participant API as ApiConnect/ApiService
participant DB as Database
App->>NotiHandler: Notification received (CCC_GENERIC_OPPORTUNITY + OPPORTUNITY_UUID)
NotiHandler->>NotiHandler: Identify as generic opportunity
NotiHandler->>NotiWorker: Enqueue SYNC_GENERIC_OPPORTUNITY work
NotiWorker->>NotiWorker: startAppropriateSync()
NotiWorker->>DBUtil: getCompositeJob(context, opportunityUUID)
DBUtil->>DB: Query by META_JOB_UUID
DB-->>DBUtil: Return ConnectJobRecord
DBUtil-->>NotiWorker: ConnectJobRecord
alt Job found
NotiWorker->>JobHelper: updateJobProgress(job, listener)
JobHelper->>API: Fetch delivery/learning progress
API-->>JobHelper: Updated data
JobHelper->>DB: Store updated deliveries/learnings/assessments
DB-->>JobHelper: Success
JobHelper->>NotiWorker: Progress updated
NotiWorker-->>App: Sync complete
else Job not found
NotiWorker->>NotiWorker: handleNoConnectJob()
NotiWorker-->>App: Sync failed (no job)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/org/commcare/activities/DispatchActivity.java (1)
170-178:⚠️ Potential issue | 🟡 Minor
actionTypeon Line 172 is assigned but never used.After switching the analytics call to
getActionFromIntent(pnIntent), the local variableactionType(assigned frompnIntent.getStringExtra(REDIRECT_ACTION)) is no longer referenced anywhere in this method. It should be removed to avoid confusion.🧹 Proposed fix
Intent pnIntent = checkIfAnyPNIntentPresent(); if (pnIntent != null) { - String actionType = pnIntent.getStringExtra(REDIRECT_ACTION); FirebaseAnalyticsUtil.reportNotificationEvent( AnalyticsParamValue.NOTIFICATION_EVENT_TYPE_CLICK, AnalyticsParamValue.REPORT_NOTIFICATION_CLICK_NOTIFICATION_TRAY, getActionFromIntent(pnIntent), pnIntent.getStringExtra(NOTIFICATION_ID) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/activities/DispatchActivity.java` around lines 170 - 178, Remove the unused local variable actionType assigned in DispatchActivity (from checkIfAnyPNIntentPresent() result via pnIntent.getStringExtra(REDIRECT_ACTION)); it is dead code now that FirebaseAnalyticsUtil.reportNotificationEvent uses getActionFromIntent(pnIntent). Delete the declaration/assignment of actionType in the method containing checkIfAnyPNIntentPresent(), leaving the pnIntent null-check and the call to FirebaseAnalyticsUtil.reportNotificationEvent(getActionFromIntent(pnIntent), ...) intact.app/src/org/commcare/connect/ConnectNavHelper.kt (1)
17-31:⚠️ Potential issue | 🔴 Critical
listener.connectActivityComplete(success)is now called unconditionally—verify that all callers handle both success and failure cases.The change is intentional: callers need notification when the unlock flow completes regardless of outcome. However, review found a critical issue:
- LoginActivity.java (line 544-547): The callback executes
setResult(RESULT_OK); finish();unconditionally, ignoring thesuccessparameter. If unlock fails, the activity finishes with RESULT_OK, which is incorrect.- BaseDrawerActivity.kt: Three implementations correctly guard operations on
if (success).- CommCareSetupActivity.java: Empty callback; minimal risk but should handle the result appropriately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/connect/ConnectNavHelper.kt` around lines 17 - 31, The unlockAndGoTo flow now always calls listener.connectActivityComplete(success), so update all callers to handle both success and failure; specifically modify LoginActivity's ConnectActivityCompleteListener implementation to check the success boolean and only call setResult(RESULT_OK); finish(); when success is true (otherwise set an appropriate failure result like RESULT_CANCELED and surface an error or keep the activity open), ensure CommCareSetupActivity implements a non-empty handler for failure, and verify the three BaseDrawerActivity implementations continue to guard post-unlock actions with if (success) before proceeding; locate these in the ConnectActivityCompleteListener usages and adjust their callbacks accordingly.
🧹 Nitpick comments (8)
app/src/org/commcare/android/database/connect/models/ConnectDeliveryDetails.java (1)
10-15: Missing getter forunitUUIDbreaks the accessor pattern — either add it or remove the field.The
unitUUIDfield is set in the constructor but has no corresponding getter or setter, unlike all other fields in the class.ConnectDeliveryProgressReportAdapterdoes not need it, but the inconsistency suggests incomplete refactoring. Either addgetUnitUUID()andsetUnitUUID()for consistency, or removeunitUUIDentirely if it's confirmed unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/android/database/connect/models/ConnectDeliveryDetails.java` around lines 10 - 15, The ConnectDeliveryDetails class defines unitUUID in the constructor but lacks accessors; add a public getUnitUUID() (and a public setUnitUUID(String)) to restore the accessor pattern in ConnectDeliveryDetails so it matches the other fields and avoids incomplete refactor issues—if you confirm unitUUID is truly unused across the codebase (including ConnectDeliveryProgressReportAdapter), alternatively remove the field and constructor parameter instead, but prefer adding getUnitUUID()/setUnitUUID() for consistency.app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java (1)
103-105: Consider addinggetJobUUID()for consistency with other record types.
ConnectAppRecordandConnectJobDeliveryRecordboth exposegetJobUUID(), butConnectJobAssessmentRecordonly has the setter. While the field is queryable via@MetaField(META_JOB_UUID)for database operations, adding the getter would maintain API symmetry with related record types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java` around lines 103 - 105, Add a public getter getJobUUID() to ConnectJobAssessmentRecord to match the API of ConnectAppRecord and ConnectJobDeliveryRecord; implement it to return the existing jobUUID field (annotated with `@MetaField`(META_JOB_UUID)) so callers can read the value symmetrically with setJobUUID(String jobUUID).app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java (1)
934-961: Method nameupgradePushNotificationRecordToV22is misleading — it produces V23 records.This method migrates
PushNotificationRecordV21→PushNotificationRecordV23(there is no V22 for this model). The name suggests the target is V22, which is confusing given the laterupgradeTwentyTwoTwentyThreeandupgradeTwentyThreeTwentyFoursteps. Consider renaming toupgradePushNotificationRecordToV23for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java` around lines 934 - 961, Rename the misleading method upgradePushNotificationRecordToV22 in ConnectDatabaseUpgrader to upgradePushNotificationRecordToV23 and update any internal references/callers accordingly; ensure the method signature and its usage still match the class (keep logic that adds META_PAYMENT_UUID/META_OPPORTUNITY_UUID columns and the migration loop converting PushNotificationRecordV21 to PushNotificationRecordV23 via PushNotificationRecordV23.Companion.fromV21, preserving setID and newStorage.write calls).app/src/org/commcare/activities/connect/ConnectActivity.java (1)
164-164: Misleading variable name:paymentIdholds a UUID.The variable reads from
PAYMENT_UUIDbut is namedpaymentId. This is confusing given the PR's purpose of migrating from IDs to UUIDs.Proposed rename
- String paymentId = getIntent().getStringExtra(PAYMENT_UUID); - if (!TextUtils.isEmpty(paymentId) && job!=null && job.getStatus() == ConnectJobRecord.STATUS_DELIVERING) { + String paymentUuid = getIntent().getStringExtra(PAYMENT_UUID); + if (!TextUtils.isEmpty(paymentUuid) && job!=null && job.getStatus() == ConnectJobRecord.STATUS_DELIVERING) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/activities/connect/ConnectActivity.java` at line 164, The variable name paymentId in ConnectActivity (where it reads getIntent().getStringExtra(PAYMENT_UUID)) is misleading because it actually contains a UUID; rename paymentId to paymentUuid (or paymentUUID) across the ConnectActivity class and update all local references (uses, comparisons, logs, and method parameters in ConnectActivity) to the new name to reflect that it holds a UUID rather than an integer ID; ensure any related methods or intent builders that reference the variable name are updated accordingly so compilation and behavior remain correct.app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (1)
8-13: Remove unused imports:CommCareApplication,ConnectDatabaseHelper,SqlStorage,Vector.These imports are not referenced anywhere in this file and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java` around lines 8 - 13, The file ConnectJobRecord.java contains unused imports—remove the unused symbols CommCareApplication, ConnectDatabaseHelper, SqlStorage, and Vector from the import list; keep only imports actually referenced (for example Persisted, R, Persisting) so the class ConnectJobRecord compiles without unused-import warnings and the import block is minimal.app/src/org/commcare/connect/database/JobStoreManager.java (1)
149-259: Migration runs outside the mainstoreJobstransaction — confirm idempotency guarantees.
migrateOtherModelsForJobUUIDcommits its own transaction (line 255) beforestoreJobsopens the main transaction at line 42. If the main transaction fails and rolls back, the UUID migration will have already been committed. This is acceptable because the migration is idempotent (line 157 short-circuits when the UUID is already up-to-date), but be aware that a partial failure leaves UUIDs updated without the new job data being stored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/connect/database/JobStoreManager.java` around lines 149 - 259, migrateOtherModelsForJobUUID currently starts/commits its own DB transaction (ConnectDatabaseHelper.connectDatabase.beginTransaction()/setTransactionSuccessful()/endTransaction()) which can commit UUID changes even if the caller storeJobs later rolls back; remove the internal transaction from migrateOtherModelsForJobUUID (or add a boolean parameter like useInternalTransaction) and instead perform its writes under the caller's transaction (storeJobs) so UUID updates are atomic with job storage, or explicitly call it only inside an already-open transaction; update storeJobs to call migrateOtherModelsForJobUUID inside its transaction (or pass useInternalTransaction=true when needed) and remove the standalone beginTransaction()/setTransactionSuccessful()/endTransaction() calls from migrateOtherModelsForJobUUID.app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
543-551: Inconsistent null/empty check betweengetActionFromPayloadandgetActionFromIntent.
getActionFromPayloadusespayload.containsKey(NOTIFICATION_KEY)(allows empty-string values), whilegetActionFromIntentuses!TextUtils.isEmpty(intent.getStringExtra(NOTIFICATION_KEY))(rejects empty strings). If the server sendskey: "", the payload version returns""while the intent version falls back toREDIRECT_ACTION. Consider aligning the two.Proposed alignment
public static String getActionFromPayload(Map<String, String> payload) { - return (CCC_GENERIC_OPPORTUNITY.equals(payload.get(REDIRECT_ACTION)) && - payload.containsKey(NOTIFICATION_KEY)) ? payload.get(NOTIFICATION_KEY) : payload.get(REDIRECT_ACTION); + return (CCC_GENERIC_OPPORTUNITY.equals(payload.get(REDIRECT_ACTION)) && + !TextUtils.isEmpty(payload.get(NOTIFICATION_KEY))) ? payload.get(NOTIFICATION_KEY) : payload.get(REDIRECT_ACTION); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/utils/FirebaseMessagingUtil.java` around lines 543 - 551, The two helpers getActionFromPayload and getActionFromIntent are inconsistent: getActionFromPayload uses payload.containsKey(NOTIFICATION_KEY) which allows empty strings while getActionFromIntent rejects empty values with TextUtils.isEmpty; change getActionFromPayload to the same non-empty check by replacing payload.containsKey(NOTIFICATION_KEY) with !TextUtils.isEmpty(payload.get(NOTIFICATION_KEY)) so both methods treat empty notification keys the same and safely fall back to REDIRECT_ACTION.app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt (1)
188-211: Consider using a named argument for clarity.The third argument to
ConnectJobHelper.updateJobProgressis theshowLoadingparameter (Boolean, default null). In a background worker context, passingfalseis semantically appropriate. However, using a named argument would improve readability:showLoading = falseinstead of the positionalfalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt` around lines 188 - 211, In syncLearnOrDeliveryProgress(), the call to ConnectJobHelper.updateJobProgress passes the Boolean positional argument for showLoading (currently "false"), which reduces clarity; update the call to use a named argument (showLoading = false) when invoking ConnectJobHelper.updateJobProgress inside the suspendCoroutine block (the method using ConnectActivityCompleteListener), so the intent is explicit while leaving other arguments unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/org/commcare/android/database/connect/models/ConnectLearnModuleSummaryRecord.java`:
- Around line 114-117: The class ConnectLearnModuleSummaryRecord has a public
setter setJobUUID(String) but lacks the corresponding getter; add a public
String getJobUUID() method to ConnectLearnModuleSummaryRecord that returns the
jobUUID field so the class exposes a matching public getter/setter pair
consistent with ConnectAppRecord and ConnectJobDeliveryRecord.
In
`@app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java`:
- Around line 130-132: Add a missing getter for the persisted jobUUID field in
ConnectPaymentUnitRecord to pair with the existing setJobUUID(String) method:
implement a public String getJobUUID() that returns jobUUID so callers can read
the value; update the class ConnectPaymentUnitRecord only (no removal or changes
to fromJson()/fromV21()) to maintain symmetry with setJobUUID and match other
classes like ConnectJobRecord/ConnectJobDeliveryRecord/ConnectAppRecord.
In `@app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt`:
- Around line 268-277: The method startLearnOrDeliverySyncWorker currently only
checks notificationPayload.containsKey(OPPORTUNITY_UUID) and can proceed with a
null/empty opportunityUUID; update it to mirror
startLearningSyncWorker/startDeliverySyncWorker by retrieving val
opportunityUUID = notificationPayload[OPPORTUNITY_UUID] and verifying
!TextUtils.isEmpty(opportunityUUID) (or opportunityUUID.isNullOrEmpty()
inverted) before calling startWorkRequest with
SyncAction.SYNC_GENERIC_OPPORTUNITY so you never build a malformed unique name;
keep references to startWorkRequest and SyncAction.SYNC_GENERIC_OPPORTUNITY
unchanged.
In `@app/src/org/commcare/utils/FirebaseMessagingUtil.java`:
- Around line 277-283: handleGenericPushNotification currently builds an intent
and shows a notification without validating that the payload contains
OPPORTUNITY_UUID; replicate the guard used in
handleResumeLearningOrDeliveryJobPushNotification and
handleOpportunitySummaryPagePushNotification by checking
fcmMessageData.getData().containsKey(OPPORTUNITY_UUID) (or equivalent accessor)
before calling
getConnectActivityNotification/buildNotification/showNotification, log an error
and return null when the UUID is missing (so background sync/handleNoConnectJob
won't break), and keep getConnectActivityNotification, buildNotification and
showNotification usage otherwise.
- Around line 513-531: The empty catch in getIntentForPNIfAny silently swallows
all exceptions; change it to catch Exception e and log the exception at debug
(or appropriate) level so programming errors like NPE/ClassCast are visible
while preserving current flow—use the existing logger (or add one) to log a
short message and the exception when handling intent extras and calling
handleNotification (refer to getIntentForPNIfAny, intent.replaceExtras, and
handleNotification) and then continue to return null as before.
In
`@app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java`:
- Around line 417-419: Add a per-version comment for the three new
externalizable entries and move PushNotificationRecordV21 into the existing 2.62
block: update the list so
"org.commcare.android.database.connect.models.PushNotificationRecordV21" is
grouped with the other V21 variants under the "// Added in 2.62" comment, and
add a new comment (e.g., "// Added in 2.63" or the correct release tag)
immediately above the two ConnectJobDeliveryRecordV22 and
PushNotificationRecordV23 entries to match the file's per-version sectioning
convention.
---
Outside diff comments:
In `@app/src/org/commcare/activities/DispatchActivity.java`:
- Around line 170-178: Remove the unused local variable actionType assigned in
DispatchActivity (from checkIfAnyPNIntentPresent() result via
pnIntent.getStringExtra(REDIRECT_ACTION)); it is dead code now that
FirebaseAnalyticsUtil.reportNotificationEvent uses
getActionFromIntent(pnIntent). Delete the declaration/assignment of actionType
in the method containing checkIfAnyPNIntentPresent(), leaving the pnIntent
null-check and the call to
FirebaseAnalyticsUtil.reportNotificationEvent(getActionFromIntent(pnIntent),
...) intact.
In `@app/src/org/commcare/connect/ConnectNavHelper.kt`:
- Around line 17-31: The unlockAndGoTo flow now always calls
listener.connectActivityComplete(success), so update all callers to handle both
success and failure; specifically modify LoginActivity's
ConnectActivityCompleteListener implementation to check the success boolean and
only call setResult(RESULT_OK); finish(); when success is true (otherwise set an
appropriate failure result like RESULT_CANCELED and surface an error or keep the
activity open), ensure CommCareSetupActivity implements a non-empty handler for
failure, and verify the three BaseDrawerActivity implementations continue to
guard post-unlock actions with if (success) before proceeding; locate these in
the ConnectActivityCompleteListener usages and adjust their callbacks
accordingly.
---
Nitpick comments:
In `@app/src/org/commcare/activities/connect/ConnectActivity.java`:
- Line 164: The variable name paymentId in ConnectActivity (where it reads
getIntent().getStringExtra(PAYMENT_UUID)) is misleading because it actually
contains a UUID; rename paymentId to paymentUuid (or paymentUUID) across the
ConnectActivity class and update all local references (uses, comparisons, logs,
and method parameters in ConnectActivity) to the new name to reflect that it
holds a UUID rather than an integer ID; ensure any related methods or intent
builders that reference the variable name are updated accordingly so compilation
and behavior remain correct.
In
`@app/src/org/commcare/android/database/connect/models/ConnectDeliveryDetails.java`:
- Around line 10-15: The ConnectDeliveryDetails class defines unitUUID in the
constructor but lacks accessors; add a public getUnitUUID() (and a public
setUnitUUID(String)) to restore the accessor pattern in ConnectDeliveryDetails
so it matches the other fields and avoids incomplete refactor issues—if you
confirm unitUUID is truly unused across the codebase (including
ConnectDeliveryProgressReportAdapter), alternatively remove the field and
constructor parameter instead, but prefer adding getUnitUUID()/setUnitUUID() for
consistency.
In
`@app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java`:
- Around line 103-105: Add a public getter getJobUUID() to
ConnectJobAssessmentRecord to match the API of ConnectAppRecord and
ConnectJobDeliveryRecord; implement it to return the existing jobUUID field
(annotated with `@MetaField`(META_JOB_UUID)) so callers can read the value
symmetrically with setJobUUID(String jobUUID).
In `@app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java`:
- Around line 8-13: The file ConnectJobRecord.java contains unused
imports—remove the unused symbols CommCareApplication, ConnectDatabaseHelper,
SqlStorage, and Vector from the import list; keep only imports actually
referenced (for example Persisted, R, Persisting) so the class ConnectJobRecord
compiles without unused-import warnings and the import block is minimal.
In `@app/src/org/commcare/connect/database/JobStoreManager.java`:
- Around line 149-259: migrateOtherModelsForJobUUID currently starts/commits its
own DB transaction
(ConnectDatabaseHelper.connectDatabase.beginTransaction()/setTransactionSuccessful()/endTransaction())
which can commit UUID changes even if the caller storeJobs later rolls back;
remove the internal transaction from migrateOtherModelsForJobUUID (or add a
boolean parameter like useInternalTransaction) and instead perform its writes
under the caller's transaction (storeJobs) so UUID updates are atomic with job
storage, or explicitly call it only inside an already-open transaction; update
storeJobs to call migrateOtherModelsForJobUUID inside its transaction (or pass
useInternalTransaction=true when needed) and remove the standalone
beginTransaction()/setTransactionSuccessful()/endTransaction() calls from
migrateOtherModelsForJobUUID.
In `@app/src/org/commcare/models/database/connect/ConnectDatabaseUpgrader.java`:
- Around line 934-961: Rename the misleading method
upgradePushNotificationRecordToV22 in ConnectDatabaseUpgrader to
upgradePushNotificationRecordToV23 and update any internal references/callers
accordingly; ensure the method signature and its usage still match the class
(keep logic that adds META_PAYMENT_UUID/META_OPPORTUNITY_UUID columns and the
migration loop converting PushNotificationRecordV21 to PushNotificationRecordV23
via PushNotificationRecordV23.Companion.fromV21, preserving setID and
newStorage.write calls).
In `@app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt`:
- Around line 188-211: In syncLearnOrDeliveryProgress(), the call to
ConnectJobHelper.updateJobProgress passes the Boolean positional argument for
showLoading (currently "false"), which reduces clarity; update the call to use a
named argument (showLoading = false) when invoking
ConnectJobHelper.updateJobProgress inside the suspendCoroutine block (the method
using ConnectActivityCompleteListener), so the intent is explicit while leaving
other arguments unchanged.
In `@app/src/org/commcare/utils/FirebaseMessagingUtil.java`:
- Around line 543-551: The two helpers getActionFromPayload and
getActionFromIntent are inconsistent: getActionFromPayload uses
payload.containsKey(NOTIFICATION_KEY) which allows empty strings while
getActionFromIntent rejects empty values with TextUtils.isEmpty; change
getActionFromPayload to the same non-empty check by replacing
payload.containsKey(NOTIFICATION_KEY) with
!TextUtils.isEmpty(payload.get(NOTIFICATION_KEY)) so both methods treat empty
notification keys the same and safely fall back to REDIRECT_ACTION.
|
|
||
| public void setJobUUID(String jobUUID) { | ||
| this.jobUUID = jobUUID; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the ConnectLearnModuleSummaryRecord file
find . -name "ConnectLearnModuleSummaryRecord.java" -type fRepository: dimagi/commcare-android
Length of output: 158
🏁 Script executed:
# Let's also find other Connect record files for comparison
find . -name "ConnectAppRecord.java" -o -name "ConnectDeliveryRecord.java" -type fRepository: dimagi/commcare-android
Length of output: 143
🏁 Script executed:
# Search for all jobUUID getter methods in the codebase to see the pattern
rg "getJobUUID" --type java -B 2 -A 2Repository: dimagi/commcare-android
Length of output: 19650
🏁 Script executed:
# Check for usage of getJobUUID() calls to see if they're actually needed
rg "\.getJobUUID\(\)" --type java -B 2 -A 2Repository: dimagi/commcare-android
Length of output: 18266
🏁 Script executed:
# Read the ConnectLearnModuleSummaryRecord file to verify the current state
cat -n ./app/src/org/commcare/android/database/connect/models/ConnectLearnModuleSummaryRecord.java | head -150Repository: dimagi/commcare-android
Length of output: 4791
🏁 Script executed:
# Also check if there are any getJobUUID() method definitions in ConnectLearnModuleSummaryRecord
rg "public String getJobUUID|public void setJobUUID" app/src/org/commcare/android/database/connect/models/ConnectLearnModuleSummaryRecord.javaRepository: dimagi/commcare-android
Length of output: 111
Add missing getJobUUID() getter to match setter.
The setJobUUID() method exists but the corresponding getter is missing. Other similar record classes in the same module (e.g., ConnectAppRecord, ConnectJobDeliveryRecord) expose both setter and getter for jobUUID, ensuring consistent API patterns. Add the getter to maintain parity:
Proposed addition
public void setJobUUID(String jobUUID) {
this.jobUUID = jobUUID;
}
+
+ public String getJobUUID() {
+ return jobUUID;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void setJobUUID(String jobUUID) { | |
| this.jobUUID = jobUUID; | |
| } | |
| public void setJobUUID(String jobUUID) { | |
| this.jobUUID = jobUUID; | |
| } | |
| public String getJobUUID() { | |
| return jobUUID; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/org/commcare/android/database/connect/models/ConnectLearnModuleSummaryRecord.java`
around lines 114 - 117, The class ConnectLearnModuleSummaryRecord has a public
setter setJobUUID(String) but lacks the corresponding getter; add a public
String getJobUUID() method to ConnectLearnModuleSummaryRecord that returns the
jobUUID field so the class exposes a matching public getter/setter pair
consistent with ConnectAppRecord and ConnectJobDeliveryRecord.
| public void setJobUUID(String jobUUID) { | ||
| this.jobUUID = jobUUID; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all callers of getUnitId() in the codebase
echo "=== All callers of getUnitId() ==="
rg -n 'getUnitId\(\)' --type java
echo ""
echo "=== Callers of getJobUUID() ==="
rg -n 'getJobUUID\(\)' --type java
echo ""
echo "=== ConnectPaymentUnitRecord file content (full) ==="
find . -name "ConnectPaymentUnitRecord.java" -type f | head -1 | xargs cat -nRepository: dimagi/commcare-android
Length of output: 11081
Add missing getJobUUID() getter to complete the UUID migration pattern.
The setJobUUID() setter (lines 130–132) should have a paired getter. Currently:
jobUUIDfield is persisted, written viafromJson()andfromV21(), and exposed for mutation viasetJobUUID()— but cannot be read back via getter.- Other Connect record classes (
ConnectJobRecord,ConnectJobDeliveryRecord,ConnectAppRecord) all exposegetJobUUID()for consistency. - The
unitUUIDgetter exists (lines 134–136) with no setter, reinforcing the asymmetry.
Regarding getUnitId() removal: No remaining callers of getUnitId() on ConnectPaymentUnitRecord were found. The removal is safe; the field migration in fromV21() (lines 93–94) correctly calls getUnitId() on the legacy ConnectPaymentUnitRecordV21 record, not on the new class.
Add getJobUUID() to align with the UUID identity pattern used throughout the Connect module:
public String getJobUUID() {
return jobUUID;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/org/commcare/android/database/connect/models/ConnectPaymentUnitRecord.java`
around lines 130 - 132, Add a missing getter for the persisted jobUUID field in
ConnectPaymentUnitRecord to pair with the existing setJobUUID(String) method:
implement a public String getJobUUID() that returns jobUUID so callers can read
the value; update the class ConnectPaymentUnitRecord only (no removal or changes
to fromJson()/fromV21()) to maintain symmetry with setJobUUID and match other
classes like ConnectJobRecord/ConnectJobDeliveryRecord/ConnectAppRecord.
| private fun startLearnOrDeliverySyncWorker(notificationPayload: Map<String, String>) { | ||
| if (notificationPayload.containsKey(OPPORTUNITY_UUID) && cccCheckPassed(context)) { | ||
| val opportunityUUID = notificationPayload.get(OPPORTUNITY_UUID) | ||
| startWorkRequest( | ||
| notificationPayload, | ||
| SyncAction.SYNC_GENERIC_OPPORTUNITY, | ||
| SyncAction.SYNC_GENERIC_OPPORTUNITY.toString() + "-$opportunityUUID", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent UUID validation compared to sibling methods.
startLearningSyncWorker and startDeliverySyncWorker both validate with !TextUtils.isEmpty(opportunityUUID), but startLearnOrDeliverySyncWorker only checks containsKey(OPPORTUNITY_UUID). If the key is present with an empty or null value, this method will proceed and enqueue a work request with a malformed unique name (e.g., "SYNC_GENERIC_OPPORTUNITY-" or "SYNC_GENERIC_OPPORTUNITY-null").
Proposed fix
private fun startLearnOrDeliverySyncWorker(notificationPayload: Map<String, String>) {
- if (notificationPayload.containsKey(OPPORTUNITY_UUID) && cccCheckPassed(context)) {
- val opportunityUUID = notificationPayload.get(OPPORTUNITY_UUID)
+ val opportunityUUID = notificationPayload.get(OPPORTUNITY_UUID)
+ if (!TextUtils.isEmpty(opportunityUUID) && cccCheckPassed(context)) {
startWorkRequest(
notificationPayload,
SyncAction.SYNC_GENERIC_OPPORTUNITY,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun startLearnOrDeliverySyncWorker(notificationPayload: Map<String, String>) { | |
| if (notificationPayload.containsKey(OPPORTUNITY_UUID) && cccCheckPassed(context)) { | |
| val opportunityUUID = notificationPayload.get(OPPORTUNITY_UUID) | |
| startWorkRequest( | |
| notificationPayload, | |
| SyncAction.SYNC_GENERIC_OPPORTUNITY, | |
| SyncAction.SYNC_GENERIC_OPPORTUNITY.toString() + "-$opportunityUUID", | |
| ) | |
| } | |
| } | |
| private fun startLearnOrDeliverySyncWorker(notificationPayload: Map<String, String>) { | |
| val opportunityUUID = notificationPayload.get(OPPORTUNITY_UUID) | |
| if (!TextUtils.isEmpty(opportunityUUID) && cccCheckPassed(context)) { | |
| startWorkRequest( | |
| notificationPayload, | |
| SyncAction.SYNC_GENERIC_OPPORTUNITY, | |
| SyncAction.SYNC_GENERIC_OPPORTUNITY.toString() + "-$opportunityUUID", | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/pn/workermanager/NotificationsSyncWorkerManager.kt`
around lines 268 - 277, The method startLearnOrDeliverySyncWorker currently only
checks notificationPayload.containsKey(OPPORTUNITY_UUID) and can proceed with a
null/empty opportunityUUID; update it to mirror
startLearningSyncWorker/startDeliverySyncWorker by retrieving val
opportunityUUID = notificationPayload[OPPORTUNITY_UUID] and verifying
!TextUtils.isEmpty(opportunityUUID) (or opportunityUUID.isNullOrEmpty()
inverted) before calling startWorkRequest with
SyncAction.SYNC_GENERIC_OPPORTUNITY so you never build a malformed unique name;
keep references to startWorkRequest and SyncAction.SYNC_GENERIC_OPPORTUNITY
unchanged.
| private static Intent handleGenericPushNotification(Context context, FCMMessageData fcmMessageData, boolean showNotification) { | ||
| Intent intent = getConnectActivityNotification(context, fcmMessageData); | ||
| if (showNotification) | ||
| showNotification(context, buildNotification(context, intent, fcmMessageData), | ||
| fcmMessageData); | ||
| return intent; | ||
| } |
There was a problem hiding this comment.
handleGenericPushNotification doesn't validate OPPORTUNITY_UUID presence, unlike peer handlers.
Other handlers (handleResumeLearningOrDeliveryJobPushNotification at line 265, handleOpportunitySummaryPagePushNotification at line 294) check for OPPORTUNITY_UUID and return null with an error log when absent. This handler unconditionally creates an intent and shows a notification. If the server sends a malformed payload without the UUID, the user sees a notification, but the background sync will fail (handleNoConnectJob()), and tapping the notification may lead to a broken screen.
Consider adding the same validation guard present in peer handlers.
Proposed fix
private static Intent handleGenericPushNotification(Context context, FCMMessageData fcmMessageData, boolean showNotification) {
+ if (!fcmMessageData.getPayloadData().containsKey(OPPORTUNITY_UUID)) {
+ Logger.exception("Empty push notification for action 'ccc_generic_opportunity'",
+ new Throwable("Empty notification without 'opportunity' details"));
+ return null;
+ }
Intent intent = getConnectActivityNotification(context, fcmMessageData);
if (showNotification)
showNotification(context, buildNotification(context, intent, fcmMessageData),
fcmMessageData);
return intent;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static Intent handleGenericPushNotification(Context context, FCMMessageData fcmMessageData, boolean showNotification) { | |
| Intent intent = getConnectActivityNotification(context, fcmMessageData); | |
| if (showNotification) | |
| showNotification(context, buildNotification(context, intent, fcmMessageData), | |
| fcmMessageData); | |
| return intent; | |
| } | |
| private static Intent handleGenericPushNotification(Context context, FCMMessageData fcmMessageData, boolean showNotification) { | |
| if (!fcmMessageData.getPayloadData().containsKey(OPPORTUNITY_UUID)) { | |
| Logger.exception("Empty push notification for action 'ccc_generic_opportunity'", | |
| new Throwable("Empty notification without 'opportunity' details")); | |
| return null; | |
| } | |
| Intent intent = getConnectActivityNotification(context, fcmMessageData); | |
| if (showNotification) | |
| showNotification(context, buildNotification(context, intent, fcmMessageData), | |
| fcmMessageData); | |
| return intent; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/utils/FirebaseMessagingUtil.java` around lines 277 -
283, handleGenericPushNotification currently builds an intent and shows a
notification without validating that the payload contains OPPORTUNITY_UUID;
replicate the guard used in handleResumeLearningOrDeliveryJobPushNotification
and handleOpportunitySummaryPagePushNotification by checking
fcmMessageData.getData().containsKey(OPPORTUNITY_UUID) (or equivalent accessor)
before calling
getConnectActivityNotification/buildNotification/showNotification, log an error
and return null when the UUID is missing (so background sync/handleNoConnectJob
won't break), and keep getConnectActivityNotification, buildNotification and
showNotification usage otherwise.
| public static Intent getIntentForPNIfAny(Context context, Intent intent) { | ||
| // Added try-catch to avoid any data corruption handling, and dispatch activity flow is not disturbed. | ||
| try { | ||
| if (intent != null && intent.getExtras() != null) { | ||
| Map<String, String> dataPayload = new HashMap<>(); | ||
| for (String key : intent.getExtras().keySet()) { | ||
| String value = intent.getExtras().getString(key); | ||
| dataPayload.put(key, value); | ||
| } | ||
| Intent pnIntent = handleNotification(context, dataPayload, null, false); | ||
| if (pnIntent != null) { | ||
| intent.replaceExtras(new Bundle()); // clear intents if push notification intents are present else app keeps reloading same PN intents | ||
| } | ||
| return pnIntent; | ||
| } | ||
| return pnIntent; | ||
| } catch (Exception e) { // no need to log | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Empty catch block silently swallows all exceptions, including programming errors.
The catch at line 528 discards exceptions with no logging. While the intent is to prevent dispatch flow disruption, silently swallowing NullPointerException, ClassCastException, etc. makes bugs nearly impossible to diagnose in production. At minimum, add a debug-level log.
Proposed fix
- } catch (Exception e) { // no need to log
+ } catch (Exception e) {
+ Logger.log(LogTypes.TYPE_FCM, "Error processing PN intent: " + e.getMessage());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static Intent getIntentForPNIfAny(Context context, Intent intent) { | |
| // Added try-catch to avoid any data corruption handling, and dispatch activity flow is not disturbed. | |
| try { | |
| if (intent != null && intent.getExtras() != null) { | |
| Map<String, String> dataPayload = new HashMap<>(); | |
| for (String key : intent.getExtras().keySet()) { | |
| String value = intent.getExtras().getString(key); | |
| dataPayload.put(key, value); | |
| } | |
| Intent pnIntent = handleNotification(context, dataPayload, null, false); | |
| if (pnIntent != null) { | |
| intent.replaceExtras(new Bundle()); // clear intents if push notification intents are present else app keeps reloading same PN intents | |
| } | |
| return pnIntent; | |
| } | |
| return pnIntent; | |
| } catch (Exception e) { // no need to log | |
| } | |
| return null; | |
| } | |
| public static Intent getIntentForPNIfAny(Context context, Intent intent) { | |
| // Added try-catch to avoid any data corruption handling, and dispatch activity flow is not disturbed. | |
| try { | |
| if (intent != null && intent.getExtras() != null) { | |
| Map<String, String> dataPayload = new HashMap<>(); | |
| for (String key : intent.getExtras().keySet()) { | |
| String value = intent.getExtras().getString(key); | |
| dataPayload.put(key, value); | |
| } | |
| Intent pnIntent = handleNotification(context, dataPayload, null, false); | |
| if (pnIntent != null) { | |
| intent.replaceExtras(new Bundle()); // clear intents if push notification intents are present else app keeps reloading same PN intents | |
| } | |
| return pnIntent; | |
| } | |
| } catch (Exception e) { | |
| Logger.log(LogTypes.TYPE_FCM, "Error processing PN intent: " + e.getMessage()); | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/utils/FirebaseMessagingUtil.java` around lines 513 -
531, The empty catch in getIntentForPNIfAny silently swallows all exceptions;
change it to catch Exception e and log the exception at debug (or appropriate)
level so programming errors like NPE/ClassCast are visible while preserving
current flow—use the existing logger (or add one) to log a short message and the
exception when handling intent extras and calling handleNotification (refer to
getIntentForPNIfAny, intent.replaceExtras, and handleNotification) and then
continue to return null as before.
| "org.commcare.android.database.connect.models.PushNotificationRecordV21", | ||
| "org.commcare.android.database.connect.models.ConnectJobDeliveryRecordV22", | ||
| "org.commcare.android.database.connect.models.PushNotificationRecordV23" |
There was a problem hiding this comment.
New externalizable entries are missing a version comment.
The three new entries are appended after the // Added in 2.62 block without a version label. The rest of the list uses per-version section comments. Additionally, PushNotificationRecordV21 belongs logically with the other V21 variants already in the 2.62 block — it appears it may have been missed in the prior PR.
✏️ Suggested format
+ // Added in 2.62 (missed)
+ "org.commcare.android.database.connect.models.PushNotificationRecordV21",
+
+ // Added in 2.XX
+ "org.commcare.android.database.connect.models.ConnectJobDeliveryRecordV22",
+ "org.commcare.android.database.connect.models.PushNotificationRecordV23"
- "org.commcare.android.database.connect.models.PushNotificationRecordV21",
- "org.commcare.android.database.connect.models.ConnectJobDeliveryRecordV22",
- "org.commcare.android.database.connect.models.PushNotificationRecordV23"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "org.commcare.android.database.connect.models.PushNotificationRecordV21", | |
| "org.commcare.android.database.connect.models.ConnectJobDeliveryRecordV22", | |
| "org.commcare.android.database.connect.models.PushNotificationRecordV23" | |
| // Added in 2.62 (missed) | |
| "org.commcare.android.database.connect.models.PushNotificationRecordV21", | |
| // Added in 2.XX | |
| "org.commcare.android.database.connect.models.ConnectJobDeliveryRecordV22", | |
| "org.commcare.android.database.connect.models.PushNotificationRecordV23" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java`
around lines 417 - 419, Add a per-version comment for the three new
externalizable entries and move PushNotificationRecordV21 into the existing 2.62
block: update the list so
"org.commcare.android.database.connect.models.PushNotificationRecordV21" is
grouped with the other V21 variants under the "// Added in 2.62" comment, and
add a new comment (e.g., "// Added in 2.63" or the correct release tag)
immediately above the two ConnectJobDeliveryRecordV22 and
PushNotificationRecordV23 entries to match the file's per-version sectioning
convention.
1 participant
Product Description
This will have changes in push notifications, as the app is now able to show all the Connect generic opportunity push notifications.
As some of the server changes were not available, the UUID ticket was not merged in master, but it was required for these changes, so it was merged in this ticket.
Some of the changes which are not present in the UUID ticket but are available in master are showing in diff, but this will go away whenever the UUID ticket is merged in master and this PR's base branch is changed to master.
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-2070
From now on, the application will handle all types of Connect generic opportunity push notifications. Push notification having
ccc_generic_opportunityas an action is considered a Connect generic opportunity. This type of push notification will be used whenever it's required toShow the learn/delivery progress screen
Show payment screen
The decision to show learning/delivery is taken by mobile code, and right now, no forceful action is calculated from the push notification payload.
Regarding Firebase Analytics, if the push notification is this generic, it will log the action available in its payload as `key 'e.g.'
For payment rollback, it will log
payment_rollbackas action.For the learning assessment score, it will log
scored_assessmentas the action.Safety Assurance
Safety story
I have tested the payment rollback available on staging server and its working for the mobile.
QA Plan
ccc_generic_opportunityas available for payment rollback and learning assessment score.Labels and Review