Remove dependency on the DIRTY flag for marking voicemails as read

Previously the DIRTY flag is used to track which voicemails has local changes that need to be uploaded to the server. The DIRTY flag is updated by the content provider and is super janky. Whenever dialer writes to the calllog or the voicemail provider the dirty flag is cleared.

When the user modified a voicemail entry, an upload is issued and blindly marking all locally read or deleted items on the server. The content on the server was never check during an upload.

After this CL, the DIRTY flag is ignored. Upload and download is now always done together. The list of voicemails on hte sever will be downloaded first, and if any voicemail is read locally but not on the server then it will be uploaded.

As the full sync when the user opens the voicemail tab was implemented, this additional transaction cost should be neglectable.

Bug: 62867500
Test: Auto test ETA Christmas, 2030. Manual: mark voicemail as read/delete voicemail.
PiperOrigin-RevId: 163093305
Change-Id: I09e918f32b9b1ffdad80731f7c25f894be2826f3

9 files changed, 57 insertions, 118 deletions

diff --git a/java/com/android/voicemail/impl/ActivationTask.java b/java/com/android/voicemail/impl/ActivationTask.java
index 6e27b5015..320ea2aaa 100644
--- a/java/com/android/voicemail/impl/ActivationTask.java
+++ b/java/com/android/voicemail/impl/ActivationTask.java
@@ -36,7 +36,6 @@ import com.android.voicemail.impl.scheduling.RetryPolicy;
 import com.android.voicemail.impl.settings.VisualVoicemailSettingsUtil;
 import com.android.voicemail.impl.sms.StatusMessage;
 import com.android.voicemail.impl.sms.StatusSmsFetcher;
-import com.android.voicemail.impl.sync.OmtpVvmSyncService;
 import com.android.voicemail.impl.sync.SyncTask;
 import com.android.voicemail.impl.sync.VvmAccountManager;
 import com.android.voicemail.impl.utils.LoggerUtils;
@@ -258,7 +257,7 @@ public class ActivationTask extends BaseTask {
         VoicemailStatus.edit(context, phoneAccountHandle),
         OmtpEvents.CONFIG_REQUEST_STATUS_SUCCESS);
     clearLegacyVoicemailNotification(context, phoneAccountHandle);
-    SyncTask.start(context, phoneAccountHandle, OmtpVvmSyncService.SYNC_FULL_SYNC);
+    SyncTask.start(context, phoneAccountHandle);
   }
 
   /** Sends a broadcast to the dialer UI to clear legacy voicemail notifications if any. */
diff --git a/java/com/android/voicemail/impl/VvmPhoneStateListener.java b/java/com/android/voicemail/impl/VvmPhoneStateListener.java
index 13d399d88..00c1358ab 100644
--- a/java/com/android/voicemail/impl/VvmPhoneStateListener.java
+++ b/java/com/android/voicemail/impl/VvmPhoneStateListener.java
@@ -19,7 +19,6 @@ import android.content.Context;
 import android.telecom.PhoneAccountHandle;
 import android.telephony.PhoneStateListener;
 import android.telephony.ServiceState;
-import com.android.voicemail.impl.sync.OmtpVvmSyncService;
 import com.android.voicemail.impl.sync.SyncTask;
 import com.android.voicemail.impl.sync.VoicemailStatusQueryHelper;
 import com.android.voicemail.impl.sync.VvmAccountManager;
@@ -82,7 +81,7 @@ public class VvmPhoneStateListener extends PhoneStateListener {
         VvmLog.v(TAG, "Signal returned: requesting resync for " + mPhoneAccount);
         // If the source is already registered, run a full sync in case something was missed
         // while signal was down.
-        SyncTask.start(mContext, mPhoneAccount, OmtpVvmSyncService.SYNC_FULL_SYNC);
+        SyncTask.start(mContext, mPhoneAccount);
       } else {
         VvmLog.v(TAG, "Signal returned: reattempting activation for " + mPhoneAccount);
         // Otherwise initiate an activation because this means that an OMTP source was
diff --git a/java/com/android/voicemail/impl/sms/OmtpMessageReceiver.java b/java/com/android/voicemail/impl/sms/OmtpMessageReceiver.java
index 1cf37370c..43835276b 100644
--- a/java/com/android/voicemail/impl/sms/OmtpMessageReceiver.java
+++ b/java/com/android/voicemail/impl/sms/OmtpMessageReceiver.java
@@ -35,7 +35,6 @@ import com.android.voicemail.impl.Voicemail.Builder;
 import com.android.voicemail.impl.VvmLog;
 import com.android.voicemail.impl.protocol.VisualVoicemailProtocol;
 import com.android.voicemail.impl.settings.VisualVoicemailSettingsUtil;
-import com.android.voicemail.impl.sync.OmtpVvmSyncService;
 import com.android.voicemail.impl.sync.SyncOneTask;
 import com.android.voicemail.impl.sync.SyncTask;
 import com.android.voicemail.impl.sync.VoicemailsQueryHelper;
@@ -152,7 +151,7 @@ public class OmtpMessageReceiver extends BroadcastReceiver {
         }
         break;
       case OmtpConstants.MAILBOX_UPDATE:
-        SyncTask.start(mContext, phone, OmtpVvmSyncService.SYNC_DOWNLOAD_ONLY);
+        SyncTask.start(mContext, phone);
         break;
       case OmtpConstants.GREETINGS_UPDATE:
         // Not implemented in V1
diff --git a/java/com/android/voicemail/impl/sync/OmtpVvmSyncReceiver.java b/java/com/android/voicemail/impl/sync/OmtpVvmSyncReceiver.java
index b2ec49e9f..1b59eccfc 100644
--- a/java/com/android/voicemail/impl/sync/OmtpVvmSyncReceiver.java
+++ b/java/com/android/voicemail/impl/sync/OmtpVvmSyncReceiver.java
@@ -52,7 +52,7 @@ public class OmtpVvmSyncReceiver extends BroadcastReceiver {
           VvmLog.i(TAG, "Unactivated account " + phoneAccount + " found, activating");
           ActivationTask.start(context, phoneAccount, null);
         } else {
-          SyncTask.start(context, phoneAccount, OmtpVvmSyncService.SYNC_FULL_SYNC);
+          SyncTask.start(context, phoneAccount);
         }
       }
     }
diff --git a/java/com/android/voicemail/impl/sync/OmtpVvmSyncService.java b/java/com/android/voicemail/impl/sync/OmtpVvmSyncService.java
index 793388362..5b5d6b054 100644
--- a/java/com/android/voicemail/impl/sync/OmtpVvmSyncService.java
+++ b/java/com/android/voicemail/impl/sync/OmtpVvmSyncService.java
@@ -43,6 +43,7 @@ import com.android.voicemail.impl.sync.VvmNetworkRequest.NetworkWrapper;
 import com.android.voicemail.impl.sync.VvmNetworkRequest.RequestFailedException;
 import com.android.voicemail.impl.utils.LoggerUtils;
 import com.android.voicemail.impl.utils.VoicemailDatabaseUtil;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
@@ -50,22 +51,13 @@ import java.util.Map;
 @TargetApi(VERSION_CODES.O)
 public class OmtpVvmSyncService {
 
-  private static final String TAG = OmtpVvmSyncService.class.getSimpleName();
+  private static final String TAG = "OmtpVvmSyncService";
 
-  /** Signifies a sync with both uploading to the server and downloading from the server. */
-  public static final String SYNC_FULL_SYNC = "full_sync";
-  /** Only upload to the server. */
-  public static final String SYNC_UPLOAD_ONLY = "upload_only";
-  /** Only download from the server. */
-  public static final String SYNC_DOWNLOAD_ONLY = "download_only";
-  /** Only download single voicemail transcription. */
-  public static final String SYNC_DOWNLOAD_ONE_TRANSCRIPTION = "download_one_transcription";
   /** Threshold for whether we should archive and delete voicemails from the remote VM server. */
   private static final float AUTO_DELETE_ARCHIVE_VM_THRESHOLD = 0.75f;
 
   private final Context mContext;
-
-  private VoicemailsQueryHelper mQueryHelper;
+  private final VoicemailsQueryHelper mQueryHelper;
 
   public OmtpVvmSyncService(Context context) {
     mContext = context;
@@ -74,23 +66,21 @@ public class OmtpVvmSyncService {
 
   public void sync(
       BaseTask task,
-      String action,
       PhoneAccountHandle phoneAccount,
       Voicemail voicemail,
       VoicemailStatus.Editor status) {
     Assert.isTrue(phoneAccount != null);
-    VvmLog.v(TAG, "Sync requested: " + action + " - for account: " + phoneAccount);
-    setupAndSendRequest(task, phoneAccount, voicemail, action, status);
+    VvmLog.v(TAG, "Sync requested for account: " + phoneAccount);
+    setupAndSendRequest(task, phoneAccount, voicemail, status);
   }
 
   private void setupAndSendRequest(
       BaseTask task,
       PhoneAccountHandle phoneAccount,
       Voicemail voicemail,
-      String action,
       VoicemailStatus.Editor status) {
     if (!VisualVoicemailSettingsUtil.isEnabled(mContext, phoneAccount)) {
-      VvmLog.v(TAG, "Sync requested for disabled account");
+      VvmLog.e(TAG, "Sync requested for disabled account");
       return;
     }
     if (!VvmAccountManager.isAccountActivated(mContext, phoneAccount)) {
@@ -102,7 +92,7 @@ public class OmtpVvmSyncService {
     LoggerUtils.logImpressionOnMainThread(mContext, DialerImpression.Type.VVM_SYNC_STARTED);
     // DATA_IMAP_OPERATION_STARTED posting should not be deferred. This event clears all data
     // channel errors, which should happen when the task starts, not when it ends. It is the
-    // "Sync in progress..." status.
+    // "Sync in progress..." status, which is currently displayed to the user as no error.
     config.handleEvent(
         VoicemailStatus.edit(mContext, phoneAccount), OmtpEvents.DATA_IMAP_OPERATION_STARTED);
     try (NetworkWrapper network = VvmNetworkRequest.getNetwork(config, phoneAccount, status)) {
@@ -111,7 +101,7 @@ public class OmtpVvmSyncService {
         task.fail();
         return;
       }
-      doSync(task, network.get(), phoneAccount, voicemail, action, status);
+      doSync(task, network.get(), phoneAccount, voicemail, status);
     } catch (RequestFailedException e) {
       config.handleEvent(status, OmtpEvents.DATA_NO_CONNECTION_CELLULAR_REQUIRED);
       task.fail();
@@ -123,14 +113,13 @@ public class OmtpVvmSyncService {
       Network network,
       PhoneAccountHandle phoneAccount,
       Voicemail voicemail,
-      String action,
       VoicemailStatus.Editor status) {
     try (ImapHelper imapHelper = new ImapHelper(mContext, phoneAccount, network, status)) {
       boolean success;
       if (voicemail == null) {
-        success = syncAll(action, imapHelper, phoneAccount);
+        success = syncAll(imapHelper, phoneAccount);
       } else {
-        success = syncOne(imapHelper, voicemail, phoneAccount);
+        success = downloadOneVoicemail(imapHelper, voicemail, phoneAccount);
       }
       if (success) {
         // TODO: b/30569269 failure should interrupt all subsequent task via exceptions
@@ -219,79 +208,33 @@ public class OmtpVvmSyncService {
     }
   }
 
-  private boolean syncAll(String action, ImapHelper imapHelper, PhoneAccountHandle account) {
-    boolean uploadSuccess = true;
-    boolean downloadSuccess = true;
-
-    if (SYNC_FULL_SYNC.equals(action) || SYNC_UPLOAD_ONLY.equals(action)) {
-      uploadSuccess = upload(account, imapHelper);
-    }
-    if (SYNC_FULL_SYNC.equals(action) || SYNC_DOWNLOAD_ONLY.equals(action)) {
-      downloadSuccess = download(imapHelper, account);
-    }
+  private boolean syncAll(ImapHelper imapHelper, PhoneAccountHandle account) {
 
-    VvmLog.v(
-        TAG,
-        "upload succeeded: ["
-            + String.valueOf(uploadSuccess)
-            + "] download succeeded: ["
-            + String.valueOf(downloadSuccess)
-            + "]");
-
-    return uploadSuccess && downloadSuccess;
-  }
+    List<Voicemail> serverVoicemails = imapHelper.fetchAllVoicemails();
+    List<Voicemail> localVoicemails = mQueryHelper.getAllVoicemails(account);
+    List<Voicemail> deletedVoicemails = mQueryHelper.getDeletedVoicemails(account);
+    boolean succeeded = true;
 
-  private boolean syncOne(ImapHelper imapHelper, Voicemail voicemail, PhoneAccountHandle account) {
-    if (shouldPerformPrefetch(account, imapHelper)) {
-      VoicemailFetchedCallback callback =
-          new VoicemailFetchedCallback(mContext, voicemail.getUri(), account);
-      imapHelper.fetchVoicemailPayload(callback, voicemail.getSourceData());
+    if (localVoicemails == null || serverVoicemails == null) {
+      // Null value means the query failed.
+      VvmLog.e(TAG, "syncAll: query failed");
+      return false;
     }
 
-    return imapHelper.fetchTranscription(
-        new TranscriptionFetchedCallback(mContext, voicemail), voicemail.getSourceData());
-  }
-
-  private boolean upload(PhoneAccountHandle phoneAccountHandle, ImapHelper imapHelper) {
-    List<Voicemail> readVoicemails = mQueryHelper.getReadVoicemails(phoneAccountHandle);
-    List<Voicemail> deletedVoicemails = mQueryHelper.getDeletedVoicemails(phoneAccountHandle);
-
-    boolean success = true;
-
     if (deletedVoicemails.size() > 0) {
       if (imapHelper.markMessagesAsDeleted(deletedVoicemails)) {
-        // We want to delete selectively instead of all the voicemails for this provider
-        // in case the state changed since the IMAP query was completed.
+        // Delete only the voicemails that was deleted on the server, in case more are deleted
+        // since the IMAP query was completed.
         mQueryHelper.deleteFromDatabase(deletedVoicemails);
       } else {
-        success = false;
+        succeeded = false;
       }
     }
 
-    if (readVoicemails.size() > 0) {
-      VvmLog.i(TAG, "Marking voicemails as read");
-      if (imapHelper.markMessagesAsRead(readVoicemails)) {
-        VvmLog.i(TAG, "Marking voicemails as clean");
-        mQueryHelper.markCleanInDatabase(readVoicemails);
-      } else {
-        success = false;
-      }
-    }
-
-    return success;
-  }
-
-  private boolean download(ImapHelper imapHelper, PhoneAccountHandle account) {
-    List<Voicemail> serverVoicemails = imapHelper.fetchAllVoicemails();
-    List<Voicemail> localVoicemails = mQueryHelper.getAllVoicemails(account);
-
-    if (localVoicemails == null || serverVoicemails == null) {
-      // Null value means the query failed.
-      return false;
-    }
-
     Map<String, Voicemail> remoteMap = buildMap(serverVoicemails);
 
+    List<Voicemail> localReadVoicemails = new ArrayList<>();
+
     // Go through all the local voicemails and check if they are on the server.
     // They may be read or deleted on the server but not locally. Perform the
     // appropriate local operation if the status differs from the server. Remove
@@ -310,6 +253,8 @@ public class OmtpVvmSyncService {
       } else {
         if (remoteVoicemail.isRead() && !localVoicemail.isRead()) {
           mQueryHelper.markReadInDatabase(localVoicemail);
+        } else if (localVoicemail.isRead() && !remoteVoicemail.isRead()) {
+          localReadVoicemails.add(localVoicemail);
         }
 
         if (!TextUtils.isEmpty(remoteVoicemail.getTranscription())
@@ -321,6 +266,16 @@ public class OmtpVvmSyncService {
       }
     }
 
+    if (localReadVoicemails.size() > 0) {
+      VvmLog.i(TAG, "Marking voicemails as read");
+      if (imapHelper.markMessagesAsRead(localReadVoicemails)) {
+        VvmLog.i(TAG, "Marking voicemails as clean");
+        mQueryHelper.markCleanInDatabase(localReadVoicemails);
+      } else {
+        return false;
+      }
+    }
+
     // The leftover messages are messages that exist on the server but not locally.
     boolean prefetchEnabled = shouldPerformPrefetch(account, imapHelper);
     for (Voicemail remoteVoicemail : remoteMap.values()) {
@@ -336,7 +291,19 @@ public class OmtpVvmSyncService {
       }
     }
 
-    return true;
+    return succeeded;
+  }
+
+  private boolean downloadOneVoicemail(
+      ImapHelper imapHelper, Voicemail voicemail, PhoneAccountHandle account) {
+    if (shouldPerformPrefetch(account, imapHelper)) {
+      VoicemailFetchedCallback callback =
+          new VoicemailFetchedCallback(mContext, voicemail.getUri(), account);
+      imapHelper.fetchVoicemailPayload(callback, voicemail.getSourceData());
+    }
+
+    return imapHelper.fetchTranscription(
+        new TranscriptionFetchedCallback(mContext, voicemail), voicemail.getSourceData());
   }
 
   private boolean shouldPerformPrefetch(PhoneAccountHandle account, ImapHelper imapHelper) {
diff --git a/java/com/android/voicemail/impl/sync/SyncOneTask.java b/java/com/android/voicemail/impl/sync/SyncOneTask.java
index cd2782abb..70c6bd890 100644
--- a/java/com/android/voicemail/impl/sync/SyncOneTask.java
+++ b/java/com/android/voicemail/impl/sync/SyncOneTask.java
@@ -39,17 +39,14 @@ public class SyncOneTask extends BaseTask {
   private static final int RETRY_INTERVAL_MILLIS = 5_000;
 
   private static final String EXTRA_PHONE_ACCOUNT_HANDLE = "extra_phone_account_handle";
-  private static final String EXTRA_SYNC_TYPE = "extra_sync_type";
   private static final String EXTRA_VOICEMAIL = "extra_voicemail";
 
   private PhoneAccountHandle mPhone;
-  private String mSyncType;
   private Voicemail mVoicemail;
 
   public static void start(Context context, PhoneAccountHandle phone, Voicemail voicemail) {
     Intent intent = BaseTask.createIntent(context, SyncOneTask.class, phone);
     intent.putExtra(EXTRA_PHONE_ACCOUNT_HANDLE, phone);
-    intent.putExtra(EXTRA_SYNC_TYPE, OmtpVvmSyncService.SYNC_DOWNLOAD_ONE_TRANSCRIPTION);
     intent.putExtra(EXTRA_VOICEMAIL, voicemail);
     context.sendBroadcast(intent);
   }
@@ -63,14 +60,13 @@ public class SyncOneTask extends BaseTask {
   public void onCreate(Context context, Bundle extras) {
     super.onCreate(context, extras);
     mPhone = extras.getParcelable(EXTRA_PHONE_ACCOUNT_HANDLE);
-    mSyncType = extras.getString(EXTRA_SYNC_TYPE);
     mVoicemail = extras.getParcelable(EXTRA_VOICEMAIL);
   }
 
   @Override
   public void onExecuteInBackgroundThread() {
     OmtpVvmSyncService service = new OmtpVvmSyncService(getContext());
-    service.sync(this, mSyncType, mPhone, mVoicemail, VoicemailStatus.edit(getContext(), mPhone));
+    service.sync(this, mPhone, mVoicemail, VoicemailStatus.edit(getContext(), mPhone));
   }
 
   @Override
@@ -78,7 +74,6 @@ public class SyncOneTask extends BaseTask {
     LoggerUtils.logImpressionOnMainThread(getContext(), DialerImpression.Type.VVM_AUTO_RETRY_SYNC);
     Intent intent = super.createRestartIntent();
     intent.putExtra(EXTRA_PHONE_ACCOUNT_HANDLE, mPhone);
-    intent.putExtra(EXTRA_SYNC_TYPE, mSyncType);
     intent.putExtra(EXTRA_VOICEMAIL, mVoicemail);
     return intent;
   }
diff --git a/java/com/android/voicemail/impl/sync/SyncTask.java b/java/com/android/voicemail/impl/sync/SyncTask.java
index 0b3e090bf..68ce0122e 100644
--- a/java/com/android/voicemail/impl/sync/SyncTask.java
+++ b/java/com/android/voicemail/impl/sync/SyncTask.java
@@ -37,17 +37,14 @@ public class SyncTask extends BaseTask {
   private static final int MINIMAL_INTERVAL_MILLIS = 60_000;
 
   private static final String EXTRA_PHONE_ACCOUNT_HANDLE = "extra_phone_account_handle";
-  private static final String EXTRA_SYNC_TYPE = "extra_sync_type";
 
   private final RetryPolicy mRetryPolicy;
 
   private PhoneAccountHandle mPhone;
-  private String mSyncType;
 
-  public static void start(Context context, PhoneAccountHandle phone, String syncType) {
+  public static void start(Context context, PhoneAccountHandle phone) {
     Intent intent = BaseTask.createIntent(context, SyncTask.class, phone);
     intent.putExtra(EXTRA_PHONE_ACCOUNT_HANDLE, phone);
-    intent.putExtra(EXTRA_SYNC_TYPE, syncType);
     context.sendBroadcast(intent);
   }
 
@@ -62,13 +59,12 @@ public class SyncTask extends BaseTask {
   public void onCreate(Context context, Bundle extras) {
     super.onCreate(context, extras);
     mPhone = extras.getParcelable(EXTRA_PHONE_ACCOUNT_HANDLE);
-    mSyncType = extras.getString(EXTRA_SYNC_TYPE);
   }
 
   @Override
   public void onExecuteInBackgroundThread() {
     OmtpVvmSyncService service = new OmtpVvmSyncService(getContext());
-    service.sync(this, mSyncType, mPhone, null, mRetryPolicy.getVoicemailStatusEditor());
+    service.sync(this, mPhone, null, mRetryPolicy.getVoicemailStatusEditor());
   }
 
   @Override
@@ -76,7 +72,6 @@ public class SyncTask extends BaseTask {
     LoggerUtils.logImpressionOnMainThread(getContext(), DialerImpression.Type.VVM_AUTO_RETRY_SYNC);
     Intent intent = super.createRestartIntent();
     intent.putExtra(EXTRA_PHONE_ACCOUNT_HANDLE, mPhone);
-    intent.putExtra(EXTRA_SYNC_TYPE, mSyncType);
     return intent;
   }
 }
diff --git a/java/com/android/voicemail/impl/sync/UploadTask.java b/java/com/android/voicemail/impl/sync/UploadTask.java
index f2b2036b5..d8f06db47 100644
--- a/java/com/android/voicemail/impl/sync/UploadTask.java
+++ b/java/com/android/voicemail/impl/sync/UploadTask.java
@@ -63,10 +63,6 @@ public class UploadTask extends BaseTask {
       return;
     }
     service.sync(
-        this,
-        OmtpVvmSyncService.SYNC_UPLOAD_ONLY,
-        phoneAccountHandle,
-        null,
-        VoicemailStatus.edit(getContext(), phoneAccountHandle));
+        this, phoneAccountHandle, null, VoicemailStatus.edit(getContext(), phoneAccountHandle));
   }
 }
diff --git a/java/com/android/voicemail/impl/sync/VoicemailsQueryHelper.java b/java/com/android/voicemail/impl/sync/VoicemailsQueryHelper.java
index 9b295dbb7..316e1ca61 100644
--- a/java/com/android/voicemail/impl/sync/VoicemailsQueryHelper.java
+++ b/java/com/android/voicemail/impl/sync/VoicemailsQueryHelper.java
@@ -49,8 +49,6 @@ public class VoicemailsQueryHelper {
   public static final int DELETED = 3;
   public static final int TRANSCRIPTION = 4;
 
-  static final String READ_SELECTION =
-      Voicemails.DIRTY + "=1 AND " + Voicemails.DELETED + "!=1 AND " + Voicemails.IS_READ + "=1";
   static final String DELETED_SELECTION = Voicemails.DELETED + "=1";
   static final String ARCHIVED_SELECTION = Voicemails.ARCHIVED + "=0";
 
@@ -65,15 +63,6 @@ public class VoicemailsQueryHelper {
   }
 
   /**
-   * Get all the local read voicemails that have not been synced to the server.
-   *
-   * @return A list of read voicemails.
-   */
-  public List<Voicemail> getReadVoicemails(@NonNull PhoneAccountHandle phoneAccountHandle) {
-    return getLocalVoicemails(phoneAccountHandle, READ_SELECTION);
-  }
-
-  /**
    * Get all the locally deleted voicemails that have not been synced to the server.
    *
    * @return A list of deleted voicemails.