Referral Notifications - Background Sync #2680

avazirna · 2023-07-17T07:27:36Z

Summary

This is the second PR related with the implementation of the Referral Notification feature. This part is responsible for the background sync. Background syncs are triggered after a successful delivery of a FCM message and when there is an active user session. The main goal of the sync is to allow the user access to newly created or updated referral cases without any action from the user.

Product Description

The way the background sync was planned is to be the least disruptive to the users, so there are aspects before, during and after a sync.

Before a sync is triggered
- The first check is whether there is an active session, if not, a blocking sync is triggered in the next login;
- The second task is to ensure that the user is in a sync-safe feature, at this point the main issue is Form Entries. So when a new request comes in and the user is in the middle of a form entry, a dialog is shown to the user informing of a pending sync (see image below). This sync will be triggered after the form submission and once the user is in a safe place.

During a sync:
- A notification with a progress bar will be presented to the user showing about the state of the sync;
- The user is blocked from initiating any sync-unsafe operations, such as triggering another sync, logging out and opening a form (others can be added to the list).

After a successful sync - CommCare checks the current state of the app and refreshes the UI accordingly (see videos below). Main places here are around EntitySelectActivity, EntityDetailActivity and MenuActivity (this list also increase).

post_sync_entityselect_case_update.mov	post_sync_entitydetail_case_update.mov	post_sync_entitydetail_case_closed.mov

Safety Assurance

If the PR is high risk, "High Risk" label is set
I have confidence that this PR will not introduce a regression for the reasons below
Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly

Notes for reviewers

Overall, the safety of the feature lies on preventing any sync from being triggered when the user is on a sync-unsafe operation, this is an area I would like particular attention to make sure that all such places are covered.
Currently, the user is not blocked during the post background sync phase, I can see a few potential risks there as the user is free to navigate to any place in the app. An approach could be to have a spinner and block the UI.
The background sync is currently skipping fixture and schema blocks of the restore file. I wonder if it makes sense to have a flag preventing these from being in the restore file.

Cross-request: dimagi/commcare-core#1310

shubham1g5 · 2023-07-17T15:29:55Z

@avazirna Thanks for Pring this. Some initial feedback without going in code -

Can we rebase this PR over Push notifications PR and make the base branch on this PR fcm_push_notifications so that it's easier to review this PR and separate out the reviews for 2 PRs

Currently, the user is not blocked during the post background sync phase, I can see a few potential risks there as the user is free to navigate to any place in the app. An approach could be to have a spinner and block the UI.

what does post background sync phase mean here ? Is this when the sync actually gets applied to change the case data or something else.

The background sync is currently skipping fixture and schema blocks of the restore file. I wonder if it makes sense to have a flag preventing these from being in the restore file.

Does the next manual sync restores the skipped data to the correct state ? Asking because HQ thinks it already send it to mobile so it might skip it on next manual sync. Othwerise I agree that we should have a API flag to skip this data as it doesn't make sense to download data if we are not going to use it.

avazirna · 2023-07-17T21:48:18Z

Thanks for the notes @shubham1g5

Can we rebase this PR over Push notifications PR and make the base branch on this PR fcm_push_notifications so that it's easier to review this PR and separate out the reviews for 2 PRs

Parking here a few notes from our chat, I'm going to focus on merging the Push notifications PR and then change the base of this PR.

Currently, the user is not blocked during the post background sync phase, I can see a few potential risks there as the user is free to navigate to any place in the app. An approach could be to have a spinner and block the UI.

what does post background sync phase mean here ? Is this when the sync actually gets applied to change the case data or something else.

The post background sync phase is basically around triggering the necessary UI updates depending on the activity that is on the foreground, i.e. at this point the sync is finished and the user sandbox has been updated

The background sync is currently skipping fixture and schema blocks of the restore file. I wonder if it makes sense to have a flag preventing these from being in the restore file.

Does the next manual sync restores the skipped data to the correct state ? Asking because HQ thinks it already send it to mobile so it might skip it on next manual sync. Othwerise I agree that we should have a API flag to skip this data as it doesn't make sense to download data if we are not going to use it.

Yes, only the background sync skips these elements.

avazirna · 2023-07-18T09:37:32Z

@damagatchi retest this please

shubham1g5 · 2023-07-19T19:56:49Z

I wonder if it makes sense to have a flag preventing these from being in the restore file.

Looks like there is already a flag that FP uses called skip_fixtures for restores, so maybe we can utilise that here as well.

This includes Logouts, Syncs and Form Entries

shubham1g5 · 2023-08-28T18:03:24Z

app/src/org/commcare/activities/CommCareActivity.java

@@ -130,6 +130,7 @@ public abstract class CommCareActivity<R> extends AppCompatActivity
    private boolean isMainScreenBlocked;

    DataSyncCompleteBroadcastReceiver dataSyncCompleteBroadcastReceiver = new DataSyncCompleteBroadcastReceiver();
+    private boolean dataSyncCompleteBroadcastReceiverRegistered = false;


I don't think this flag is needed.

Only activities that use this broadcast should register this broadcast instead of registering it for every CommCareActivity

we can instead do -

if (isBackgroundSyncEnabled() and shouldListenToSyncComplete()) { dataSyncCompleteBroadcastReceiver = new DataSyncCompleteBroadcastReceiver() registerReceiver(dataSyncCompleteBroadcastReceiver, new IntentFilter(COMMCARE_DATA_UPDATE_ACTION)); } // Activities that needs to refresh after sync should override this to return true public boolean shouldListenToSyncComplete() { return false; }

and

if(dataSyncCompleteBroadcastReceiver!=null){ unregisterReceiver(dataSyncCompleteBroadcastReceiver); }

shubham1g5 · 2023-08-28T19:59:41Z

app/src/org/commcare/preferences/HiddenPreferences.java

@@ -565,30 +583,28 @@ public static void markRawMediaCleanUpComplete() {
                .edit().putBoolean(RAW_MEDIA_CLEANUP_COMPLETE, true).apply();
    }

-    public static boolean isPendingSyncRequestFromServer() {
+    public static boolean isPendingSyncRequestFromServerForUser() {


this method should ideally take username as a method parameter so that we don't cause session exceptions in here.

Confirming that loggedusername is not the full username like - test@domain.commcarehq.org ?

This is to check if there is a pending sync for the current logged in user, it's triggered right after a successful login. I can rename it to make this more clear.

Confirmed.

I understand, although my suggestion is to avoid hidden session exception if in future we call this method from other places. Supplying username to this method will ensure that caller of this method will handle implications of session not being available at that time.

That's a good point

shubham1g5 · 2023-08-28T20:04:18Z

app/src/org/commcare/sync/FirebaseMessagingDataSyncer.java

+                return;
+            }
+            // Attempt to trigger the sync if the user is currently in a sync safe activity
+            if (isCurrentActivitySyncSafe()){


this check actually needs to happen for restore and not form submission.

shubham1g5 · 2023-08-28T20:05:41Z

app/src/org/commcare/sync/FirebaseMessagingDataSyncer.java

+    }
+
+    private boolean isCurrentActivitySyncSafe() {
+        return !syncUnsafeActivities.contains(CommCareApplication.currentActivityName);


Instead of setting the currentActivityName in CommCareApplication, you can directly have a variable to set backgroundSyncSafe that you can update from FormEntryActivity . This way every activity is deciding whether it can handle background sync instead of having to maintain a centralised list of classes in this class.

shubham1g5 · 2023-08-28T20:08:18Z

app/src/org/commcare/preferences/HiddenPreferences.java


    private static final String ENABLE_ANDROID_WINDOW_SECURE_FLAG = "cc-enable-android-window-secure-flag";

+    private static final String PENDING_SYNC_REQUEST_FROM_SERVER_TIME = "pending-sync-request-from-server-time";


shubham1g5 · 2023-08-28T20:11:03Z

app/src/org/commcare/preferences/HiddenPreferences.java

@@ -96,11 +100,22 @@ public class HiddenPreferences {
    private static final long NO_OF_HOURS_TO_WAIT_TO_RESUME_BACKGROUND_WORK = 36;

    // This is to be used by CommCareFirebaseMessagingService to schedule a sync after the next Login
-    public final static String PENDING_SYNC_REQUEST_FROM_SERVER = "pending-sync-request-from-server";
+    public final static String PENDING_SYNC_REQUEST_FROM_SERVER = "pending-sync-request-from-server-";


nit: can just call it this BACKGROUND_SYNC_PENDING and accordingly rename the related methods that access the preference.

shubham1g5 · 2023-08-28T20:16:19Z

app/src/org/commcare/activities/FormEntryActivity.java

+    @Override
+    public void alertPendingSync(FCMMessageData fcmMessageData) {
+        if (!HiddenPreferences.isPostFormSubmissionSyncNeeded()) {
+            HiddenPreferences.setPostFormSubmissionSyncNeeded(true);


Wondering whether we need a separate preference to track this like why can't we use the PENDING_SYNC_REQUEST_FROM_SERVER here as well ?

Yeah, I thought about this too, the thing is that the now BACKGROUND_SYNC_PENDING shared preference is concatenated with the username, to ensure that we trigger the sync for the right user. Consolidating both would mean to make sure that this continues to work on devices shared by different users.

Consolidating both would mean to make sure that this continues to work on devices shared by different users.

That's a good thing right ?

shubham1g5 · 2023-08-28T20:19:53Z

app/src/org/commcare/activities/HomeScreenBaseActivity.java

@@ -168,6 +171,19 @@ public abstract class HomeScreenBaseActivity<T> extends SyncCapableCommCareActiv
    protected boolean showCommCareUpdateMenu = false;
    private static final int MAX_CC_UPDATE_CANCELLATION = 3;

+    // This is to trigger a background sync after a form submission,
+    public static boolean shouldTriggerBackgroundSync = true;


should be private boolean shouldTriggerBackgroundSync

shubham1g5 · 2023-08-28T20:30:36Z

app/src/org/commcare/activities/HomeScreenBaseActivity.java

@@ -1249,9 +1299,15 @@ public void onResumeSessionSafe() {
            attemptDispatchHomeScreen();
        }

+        // In case a Sync was blocked because of a form entry, trigger now if it's safe
+        if (HiddenPreferences.isPostFormSubmissionSyncNeeded() && shouldTriggerBackgroundSync) {


Won't shouldTriggerBackgroundSync will always be false here when coming back from From Entry ? If so, how does the sync gets initiated after form submission ?

@shubham1g5 Not necessarily, when we open a form onActivityResultSessionSafe from HomeScreenBaseActivity in called, this then triggers the logic to start a new Form Entry session, and here we set shouldTriggerBackgroundSync to false, so by the time onResumeSessionSafe is called, shouldTriggerBackgroundSync is false and the sync is not triggered. Also, during onResumeSessionSafe, shouldTriggerBackgroundSync is reset to true, which means that when onResumeSessionSafe is called again, the sync can be triggered (if necessary), unless a new Form Entry Session is starts.

which means that when onResumeSessionSafe is called again, the sync can be triggered (if necessary), unless a new Form Entry Session is starts.

But that means that sync won't get triggered immediately after user completes the form but only when user comes back to home screen one more time.

@shubham1g5 I think we are supposed to go back to HomeScreenBaseActivity after a form submission to determine the next step. Besides, we start a FormEntry activity for result in HomeScreenBaseActivity. Are there other scenarios that I'm missing here?

I am considering the scenario where a user go through this navigation -

Home Screen -> Start -> Module -> Form Entry

Sync request from server

Form Entry Complete

Home Screen (no eof navigation) but no sync

User goes out and back into the home screen -> Sync gets triggered.

Is my interpretaion of sync behaviour in step 4 and 5 right based on the current code ?

Why wouldn't there be a sync in number 4? After a form submission, we would navigate through HomeSreenBaseActivity and shouldTriggerBackgroundSync would be true, so a sync would be triggered

I see, My understanding is that HomeSreenBaseActivity doesn't reinitialise when onActivityResult gets called on the activity as there might already be a live instance of that activity in the background. But I might be wrong here.

Yeah, I think so too, but why does it need to be reinitialized for the sync to occur?

shouldTriggerBackgroundSync is set to false in startFormEntry and will remain false without HomeSreenBaseActivity getting reinitialised after step 3 above.

shubham1g5 · 2023-08-28T20:44:58Z

app/src/org/commcare/services/DataSyncCompleteBroadcastReceiver.java

 import org.commcare.activities.HomeScreenBaseActivity;
 import org.commcare.models.AndroidSessionWrapper;
-import org.commcare.session.CommCareSession;
-import org.commcare.session.SessionDescriptorUtil;
 import org.commcare.session.SessionFrame;

 public class DataSyncCompleteBroadcastReceiver extends BroadcastReceiver {

    @Override
    public void onReceive(Context context, Intent intent) {


this whole class can be simplified to -

@Override public void onReceive(Context context, Intent intent) { AndroidSessionWrapper asw = CommCareApplication.instance().getCurrentSessionWrapper(); Activity activity = (Activity)context; asw.cleanVolatiles(); Intent returnIntent = new Intent(); setSelectedEntity(activity, returnIntent); activity.setResult(RESULT_RESTART, returnIntent); activity.finish(); } private void setSelectedEntity(Activity activity, Intent returnIntent) { if (activity instanceof EntityDetailActivity && activity.getIntent().hasExtra(SessionFrame.STATE_DATUM_VAL)) { String selectedEntity = activity.getIntent().getStringExtra(SessionFrame.STATE_DATUM_VAL); returnIntent.putExtra(EXTRA_ENTITY_KEY, selectedEntity); } }

And then you can set the selectedEntityPostSync in onActivityResult of HomeScreenBaseActivity and make it a private String selectedEntityPostSync variable of activity. public static variables in an activity should be avoided and they are mostly should only be used for constants.

shubham1g5 · 2023-08-31T23:23:22Z

app/src/org/commcare/CommCareApplication.java

@@ -196,7 +196,7 @@ public class CommCareApplication extends MultiDexApplication {
    private boolean invalidateCacheOnRestore;
    private CommCareNoficationManager noficationManager;

-    public static String currentActivityName;
+    public static boolean backgroundSyncSafe;


this doesn't need to be static, you can set it as CommCareApplication.instance().setIsBackgroundSyncSafe(false)

true, on it

shubham1g5 · 2023-08-31T23:27:05Z

app/src/org/commcare/preferences/HiddenPreferences.java

@@ -586,18 +584,18 @@ public static void markRawMediaCleanUpComplete() {
    public static boolean isPendingSyncRequestFromServerForUser() {
        String loggedUsername = CommCareApplication.instance().getSession().getLoggedInUser().getUsername();
        return PreferenceManager.getDefaultSharedPreferences(CommCareApplication.instance())
-                .contains(PENDING_SYNC_REQUEST_FROM_SERVER + loggedUsername + "@"+ getUserDomainWithoutServerUrl());
+                .contains(BACKGROUND_SYNC_PENDING + loggedUsername + "@"+ getUserDomainWithoutServerUrl());
    }
    public static void setPendingSyncRequestFromServerForUser(FCMMessageData fcmMessageData) {


nit: can rename all methods for BACKGROUND_SYNC_PENDING as well - setBackgroundSyncPending(), isBackgroundSyncPending() and clearBackgroundSyncPending()

shubham1g5 · 2023-08-31T23:38:07Z

app/src/org/commcare/activities/FormEntryActivity.java

+    @Override
+    public void alertPendingSync(FCMMessageData fcmMessageData) {
+        if (!HiddenPreferences.isPostFormSubmissionSyncNeeded()) {
+            HiddenPreferences.setPostFormSubmissionSyncNeeded(true);


Consolidating both would mean to make sure that this continues to work on devices shared by different users.

That's a good thing right ?

shubham1g5 · 2023-08-31T23:40:34Z

app/src/org/commcare/services/FCMMessageData.java

+        try {
+            return new DateTime(timeInISO8601);
+        } catch (Exception e) {
+            return new DateTime();


we should log the exception to sumo here so that we get to know about these issues

shubham1g5 · 2023-09-01T14:13:33Z

app/src/org/commcare/activities/HomeScreenBaseActivity.java

@@ -1249,9 +1299,15 @@ public void onResumeSessionSafe() {
            attemptDispatchHomeScreen();
        }

+        // In case a Sync was blocked because of a form entry, trigger now if it's safe
+        if (HiddenPreferences.isPostFormSubmissionSyncNeeded() && shouldTriggerBackgroundSync) {


shouldTriggerBackgroundSync is set to false in startFormEntry and will remain false without HomeSreenBaseActivity getting reinitialised after step 3 above.

…c-pending

shubham1g5

The new changes looks good, we need to resolve conflicts and ensure build passes on it before merge.

avazirna added QA Note High Risk If the PR introduce high risk changes that has high probability of introducing breaking changes labels Jul 17, 2023

avazirna requested a review from shubham1g5 July 17, 2023 10:23

avazirna changed the title ~~[WIP] Referral Notifications - Background Sync~~ Referral Notifications - Background Sync Jul 17, 2023

avazirna mentioned this pull request Jul 17, 2023

Referral Notifications - Background Sync dimagi/commcare-core#1310

Merged

avazirna mentioned this pull request Jul 18, 2023

Referral Notifications - FCM Push Notifications #2667

Merged

1 task

avazirna added 16 commits July 27, 2023 13:05

Add FCM Data Syncer

f865524

Assess current state of the app

8fc36d8

Alert user about pending sync

2500bb5

Block unsafe events when a background sync is underway

13c1ca4

This includes Logouts, Syncs and Form Entries

Trigger post login pending sync

491e08d

Publish background sync updates

b363687

Disable pending sync

8652d7b

Cancel unnecessary sync

076adaa

Finalize background sync

1080733

Skip certain restore file elements from being parsed

e5317c7

Refactor

7f137fa

Make FCM Message data model externalizable

13cf532

Store FCM Message data for post form submission sync

d72ed7c

Trigger sync post form submission

1b529f3

Trigger background sync

106b465

Skip fixtures

11516b7

avazirna force-pushed the fcm_background_sync branch from 6da99e5 to f2d12d0 Compare July 27, 2023 17:02

avazirna changed the base branch from master to fcm_push_notifications July 27, 2023 18:19

Remove skip parsing logic

1194304

avazirna force-pushed the fcm_background_sync branch from f2d12d0 to 1194304 Compare July 27, 2023 18:33

shubham1g5 requested review from OrangeAndGreen and ctsims August 24, 2023 14:31

shubham1g5 requested changes Aug 28, 2023

View reviewed changes

avazirna added 8 commits August 30, 2023 23:24

Revert

cd27032

Nit

b9525ed

Refactor

68e05d0

Set activities that should restart after sync

415bc8b

Refactor

f6416f3

Refactor

bb99c16

Handle malformed dates

30e2de6

Refactor

e1daf72

shubham1g5 reviewed Sep 1, 2023

View reviewed changes

avazirna added 2 commits September 4, 2023 15:19

Refactor

ff10165

Merge shared preferences post-form-submission-sync and background-syn…

be12087

…c-pending

avazirna force-pushed the fcm_background_sync branch from dd63a18 to be12087 Compare September 4, 2023 14:43

Log exception

edfce31

avazirna requested a review from shubham1g5 September 7, 2023 15:13

Base automatically changed from fcm_push_notifications to master September 7, 2023 15:32

shubham1g5 previously approved these changes Sep 7, 2023

View reviewed changes

avazirna added this to the 2.54 milestone Sep 14, 2023

avazirna added the Release Note label Sep 27, 2023

avazirna dismissed shubham1g5’s stale review via ad754e0 October 3, 2023 09:27

Resolve merge conflicts

65cc28e

avazirna force-pushed the fcm_background_sync branch from ad754e0 to 65cc28e Compare October 3, 2023 12:08

avazirna added 2 commits October 3, 2023 14:43

Fix unit test

f1fbb2a

Android 13 notification change

d96060d

avazirna requested a review from shubham1g5 October 3, 2023 13:50

shubham1g5 approved these changes Oct 3, 2023

View reviewed changes

avazirna merged commit fa55951 into master Oct 3, 2023
4 of 9 checks passed

avazirna deleted the fcm_background_sync branch October 3, 2023 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Referral Notifications - Background Sync #2680

Referral Notifications - Background Sync #2680

avazirna commented Jul 17, 2023 •

edited

Loading

shubham1g5 commented Jul 17, 2023

avazirna commented Jul 17, 2023

avazirna commented Jul 18, 2023

shubham1g5 commented Jul 19, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 28, 2023

avazirna Aug 30, 2023

shubham1g5 Aug 31, 2023

avazirna Aug 31, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 28, 2023

avazirna Aug 31, 2023

shubham1g5 Aug 31, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 28, 2023

avazirna Aug 30, 2023

shubham1g5 Aug 31, 2023

avazirna Aug 31, 2023

shubham1g5 Aug 31, 2023

avazirna Aug 31, 2023

shubham1g5 Aug 31, 2023 •

edited

Loading

avazirna Sep 1, 2023

shubham1g5 Sep 1, 2023

shubham1g5 Aug 28, 2023

shubham1g5 Aug 31, 2023

avazirna Sep 1, 2023

shubham1g5 Aug 31, 2023

shubham1g5 Aug 31, 2023

shubham1g5 Aug 31, 2023

shubham1g5 Sep 1, 2023

shubham1g5 left a comment


		private static final String ENABLE_ANDROID_WINDOW_SECURE_FLAG = "cc-enable-android-window-secure-flag";

		private static final String PENDING_SYNC_REQUEST_FROM_SERVER_TIME = "pending-sync-request-from-server-time";

Referral Notifications - Background Sync #2680

Referral Notifications - Background Sync #2680

Conversation

avazirna commented Jul 17, 2023 • edited Loading

Summary

Product Description

Safety Assurance

Notes for reviewers

shubham1g5 commented Jul 17, 2023

avazirna commented Jul 17, 2023

avazirna commented Jul 18, 2023

shubham1g5 commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 left a comment

Choose a reason for hiding this comment

avazirna commented Jul 17, 2023 •

edited

Loading

shubham1g5 Aug 31, 2023 •

edited

Loading