Run contacts backup job only when network is connected

Apply network constrains to prevent daily backup
job being started without network.

Backup job restart is required to apply new constraints.

Similar migration has been applied in the past, but migration
manager does not support re-applying steps again.

Migrations manager has been refactored to make migration
step re-applying easier, as more job restarts can be
required in the future.

Fixes #9632

Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

src/androidTest/java/com/nextcloud/client/migrations/MigrationsManagerTest.kt

@@ -22,6 +22,8 @@ package com.nextcloud.client.migrations
 import androidx.test.annotation.UiThreadTest
 import com.nextcloud.client.appinfo.AppInfo
 import com.nextcloud.client.core.ManualAsyncRunner
+import com.nhaarman.mockitokotlin2.any
+import com.nhaarman.mockitokotlin2.anyOrNull
 import com.nhaarman.mockitokotlin2.inOrder
 import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.never
@@ -42,9 +44,15 @@ class MigrationsManagerTest {
         const val NEW_APP_VERSION = 42
     }
 
-    lateinit var migrationStep1: Runnable
-    lateinit var migrationStep2: Runnable
-    lateinit var migrationStep3: Runnable
+    lateinit var migrationStep1Body: (Migrations.Step) -> Unit
+    lateinit var migrationStep1: Migrations.Step
+
+    lateinit var migrationStep2Body: (Migrations.Step) -> Unit
+    lateinit var migrationStep2: Migrations.Step
+
+    lateinit var migrationStep3Body: (Migrations.Step) -> Unit
+    lateinit var migrationStep3: Migrations.Step
+
     lateinit var migrations: List<Migrations.Step>
 
     @Mock
@@ -60,26 +68,17 @@ class MigrationsManagerTest {
     @Before
     fun setUp() {
         MockitoAnnotations.initMocks(this)
-        migrationStep1 = mock()
-        migrationStep2 = mock()
-        migrationStep3 = mock()
-        migrations = listOf(
-            object : Migrations.Step(0, "first migration", true) {
-                override fun run() {
-                    migrationStep1.run()
-                }
-            },
-            object : Migrations.Step(1, "second optional migration", false) {
-                override fun run() {
-                    migrationStep2.run()
-                }
-            },
-            object : Migrations.Step(2, "third migration", true) {
-                override fun run() {
-                    migrationStep3.run()
-                }
-            }
-        )
+        migrationStep1Body = mock()
+        migrationStep1 = Migrations.Step(0, "first migration", true, migrationStep1Body)
+
+        migrationStep2Body = mock()
+        migrationStep2 = Migrations.Step(1, "second optional migration", false, migrationStep2Body)
+
+        migrationStep3Body = mock()
+        migrationStep3 = Migrations.Step(2, "third migration", true, migrationStep3Body)
+
+        migrations = listOf(migrationStep1, migrationStep2, migrationStep3)
+
         asyncRunner = ManualAsyncRunner()
         migrationsDbStore = MockSharedPreferences()
         migrationsDb = MigrationsDb(migrationsDbStore)
@@ -93,14 +92,6 @@ class MigrationsManagerTest {
         )
     }
 
-    private fun assertMigrationsRun(vararg migrationSteps: Runnable) {
-        inOrder(migrationSteps).apply {
-            migrationSteps.forEach {
-                verify(it.run())
-            }
-        }
-    }
-
     @Test
     fun inital_status_is_unknown() {
         // GIVEN
@@ -146,20 +137,50 @@ class MigrationsManagerTest {
 
         // THEN
         //      total migrations count is returned
-        //      migration functions are called
+        //      migration functions are called with step as argument
+        //      migrations are invoked in order
         //      applied migrations are recorded
         //      new app version code is recorded
         assertEquals(migrations.size, count)
-        inOrder(migrationStep1, migrationStep2, migrationStep3).apply {
-            verify(migrationStep1).run()
-            verify(migrationStep2).run()
-            verify(migrationStep3).run()
+        inOrder(migrationStep1.run, migrationStep2.run, migrationStep3.run).apply {
+            verify(migrationStep1.run).invoke(migrationStep1)
+            verify(migrationStep2.run).invoke(migrationStep2)
+            verify(migrationStep3.run).invoke(migrationStep3)
         }
         val allAppliedIds = migrations.map { it.id }
         assertEquals(allAppliedIds, migrationsDb.getAppliedMigrations())
         assertEquals(NEW_APP_VERSION, migrationsDb.lastMigratedVersion)
     }
 
+    @Test
+    @UiThreadTest
+    fun previously_run_migrations_are_not_run_again() {
+        // GIVEN
+        //      some migrations were run before
+        whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION)
+        migrationsDb.lastMigratedVersion = OLD_APP_VERSION
+        migrationsDb.addAppliedMigration(migrationStep1.id, migrationStep2.id)
+
+        // WHEN
+        //      migrations are applied
+        val count = migrationsManager.startMigration()
+        assertTrue(asyncRunner.runOne())
+
+        // THEN
+        //      applied migrations count is returned
+        //      previously applied migrations are not run
+        //      required migrations are applied
+        //      applied migrations are recorded
+        //      new app version code is recorded
+        assertEquals(1, count)
+        verify(migrationStep1.run, never()).invoke(anyOrNull())
+        verify(migrationStep2.run, never()).invoke(anyOrNull())
+        verify(migrationStep3.run).invoke(migrationStep3)
+        val allAppliedIds = migrations.map { it.id }
+        assertEquals(allAppliedIds, migrationsDb.getAppliedMigrations())
+        assertEquals(NEW_APP_VERSION, migrationsDb.lastMigratedVersion)
+    }
+
     @Test
     @UiThreadTest
     fun migration_error_is_recorded() {
@@ -174,7 +195,7 @@ class MigrationsManagerTest {
         //      one migration throws
         val lastMigration = migrations.findLast { it.mandatory } ?: throw IllegalStateException("Test fixture error")
         val errorMessage = "error message"
-        whenever(lastMigration.run()).thenThrow(RuntimeException(errorMessage))
+        whenever(lastMigration.run.invoke(any())).thenThrow(RuntimeException(errorMessage))
         migrationsManager.startMigration()
         assertTrue(asyncRunner.runOne())
 
@@ -205,7 +226,7 @@ class MigrationsManagerTest {
         //      status is set to applied
         assertEquals(0, migrationCount)
         listOf(migrationStep1, migrationStep2, migrationStep3).forEach {
-            verify(it, never()).run()
+            verify(it.run, never()).invoke(any())
         }
         assertEquals(MigrationsManager.Status.APPLIED, migrationsManager.status.value)
     }
@@ -245,7 +266,7 @@ class MigrationsManagerTest {
         //      one migration is optional and fails
         assertEquals("Fixture should provide 1 optional, failing migration", 1, migrations.count { !it.mandatory })
         val optionalFailingMigration = migrations.first { !it.mandatory }
-        whenever(optionalFailingMigration.run()).thenThrow(RuntimeException())
+        whenever(optionalFailingMigration.run.invoke(any())).thenThrow(RuntimeException())
 
         // WHEN
         //      migration is started

src/main/java/com/nextcloud/client/jobs/BackgroundJobManagerImpl.kt

@@ -225,15 +225,23 @@ internal class BackgroundJobManagerImpl(
 
     override fun schedulePeriodicContactsBackup(user: User) {
         val data = Data.Builder()
-            .putString(ContactsBackupWork.ACCOUNT, user.accountName)
-            .putBoolean(ContactsBackupWork.FORCE, true)
+            .putString(ContactsBackupWork.KEY_ACCOUNT, user.accountName)
+            .putBoolean(ContactsBackupWork.KEY_FORCE, true)
             .build()
+
+        val constraints = Constraints.Builder()
+            .setRequiredNetworkType(NetworkType.CONNECTED)
+            .build()
+
         val request = periodicRequestBuilder(
             jobClass = ContactsBackupWork::class,
             jobName = JOB_PERIODIC_CONTACTS_BACKUP,
             intervalMins = PERIODIC_BACKUP_INTERVAL_MINUTES,
             user = user
-        ).setInputData(data).build()
+        )
+            .setInputData(data)
+            .setConstraints(constraints)
+            .build()
 
         workManager.enqueueUniquePeriodicWork(JOB_PERIODIC_CONTACTS_BACKUP, ExistingPeriodicWorkPolicy.KEEP, request)
     }
@@ -292,8 +300,8 @@ internal class BackgroundJobManagerImpl(
 
     override fun startImmediateContactsBackup(user: User): LiveData<JobInfo?> {
         val data = Data.Builder()
-            .putString(ContactsBackupWork.ACCOUNT, user.accountName)
-            .putBoolean(ContactsBackupWork.FORCE, true)
+            .putString(ContactsBackupWork.KEY_ACCOUNT, user.accountName)
+            .putBoolean(ContactsBackupWork.KEY_FORCE, true)
             .build()
 
         val request = oneTimeRequestBuilder(ContactsBackupWork::class, JOB_IMMEDIATE_CONTACTS_BACKUP, user)

src/main/java/com/nextcloud/client/jobs/ContactsBackupWork.kt

@@ -70,9 +70,9 @@ class ContactsBackupWork(
 
     companion object {
         val TAG = ContactsBackupWork::class.java.simpleName
-        const val ACCOUNT = "account"
-        const val FORCE = "force"
-        const val JOB_INTERVAL_MS: Long = 24 * 60 * 60 * 1000
+        const val KEY_ACCOUNT = "account"
+        const val KEY_FORCE = "force"
+        const val JOB_INTERVAL_MS: Long = 24L * 60L * 60L * 1000L
         const val BUFFER_SIZE = 1024
     }
 
@@ -81,7 +81,7 @@ class ContactsBackupWork(
 
     @Suppress("ReturnCount") // pre-existing issue
     override fun doWork(): Result {
-        val accountName = inputData.getString(ACCOUNT) ?: ""
+        val accountName = inputData.getString(KEY_ACCOUNT) ?: ""
         if (TextUtils.isEmpty(accountName)) { // no account provided
             return Result.failure()
         }
@@ -94,7 +94,7 @@ class ContactsBackupWork(
             user,
             ContactsPreferenceActivity.PREFERENCE_CONTACTS_LAST_BACKUP
         )
-        val force = inputData.getBoolean(FORCE, false)
+        val force = inputData.getBoolean(KEY_FORCE, false)
         if (force || lastExecution + JOB_INTERVAL_MS < Calendar.getInstance().timeInMillis) {
             Log_OC.d(TAG, "start contacts backup job")
             val backupFolder: String = resources.getString(R.string.contacts_backup_folder) + OCFile.PATH_SEPARATOR

src/main/java/com/nextcloud/client/migrations/Migrations.kt

@@ -55,75 +55,82 @@ class Migrations @Inject constructor(
      * @throws Exception migration logic is permitted to throw any kind of exceptions; all exceptions will be wrapped
      * into [MigrationException]
      */
-    abstract class Step(val id: Int, val description: String, val mandatory: Boolean = true) : Runnable
+    class Step(val id: Int, val description: String, val mandatory: Boolean = true, val run: (s: Step) -> Unit) {
+        override fun toString(): String {
+            return "Migration $id: $description"
+        }
+    }
+
+    /**
+     * NOP migration used to replace applied migrations that should be applied again.
+     */
+    private fun nop(s: Step) {
+        logger.i(TAG, "$s: skipped deprecated migration")
+    }
 
     /**
      * Migrate legacy accounts by adding user IDs. This migration can be re-tried until all accounts are
      * successfully migrated.
      */
-    private val migrateUserId = object : Step(0, "Migrate user id", false) {
-        override fun run() {
-            val allAccountsHaveUserId = userAccountManager.migrateUserId()
-            logger.i(TAG, "$description: success = $allAccountsHaveUserId")
-            if (!allAccountsHaveUserId) {
-                throw IllegalStateException("Failed to set user id for all accounts")
-            }
+    private fun migrateUserId(s: Step) {
+        val allAccountsHaveUserId = userAccountManager.migrateUserId()
+        logger.i(TAG, "${s.description}: success = $allAccountsHaveUserId")
+        if (!allAccountsHaveUserId) {
+            throw IllegalStateException("Failed to set user id for all accounts")
         }
     }
 
     /**
      * Content observer job must be restarted to use new scheduler abstraction.
      */
-    private val migrateContentObserverJob = object : Step(1, "Migrate content observer job", false) {
-        override fun run() {
-            val legacyWork = workManager.getWorkInfosByTag("content_sync").get()
-            legacyWork.forEach {
-                logger.i(TAG, "$description: cancelling legacy work ${it.id}")
-                workManager.cancelWorkById(it.id)
-            }
-            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
-                jobManager.scheduleContentObserverJob()
-                logger.i(TAG, "$description: enabled")
-            } else {
-                logger.i(TAG, "$description: disabled")
-            }
+    private fun migrateContentObserverJob(s: Step) {
+        val legacyWork = workManager.getWorkInfosByTag("content_sync").get()
+        legacyWork.forEach {
+            logger.i(TAG, "${s.description}: cancelling legacy work ${it.id}")
+            workManager.cancelWorkById(it.id)
+        }
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+            jobManager.scheduleContentObserverJob()
+            logger.i(TAG, "$s: enabled")
+        } else {
+            logger.i(TAG, "$s: disabled")
         }
     }
 
     /**
-     * Contacts backup job has been migrated to new job runner framework. Re-start contacts upload
-     * for all users that have it enabled.
-     *
-     * Old job is removed from source code, so we need to restart it for each user using
-     * new jobs API.
+     * Periodic contacts backup job has been changed and should be restarted.
      */
-    private val migrateContactsBackupJob = object : Step(2, "Restart contacts backup job") {
-        override fun run() {
-            val users = userAccountManager.allUsers
-            if (users.isEmpty()) {
-                logger.i(TAG, "$description: no users to migrate")
-            } else {
-                users.forEach {
-                    val backupEnabled = arbitraryDataProvider.getBooleanValue(
-                        it.accountName,
-                        ContactsPreferenceActivity.PREFERENCE_CONTACTS_AUTOMATIC_BACKUP
-                    )
-                    if (backupEnabled) {
-                        jobManager.schedulePeriodicContactsBackup(it)
-                    }
-                    logger.i(TAG, "$description: user = ${it.accountName}, backup enabled = $backupEnabled")
+    private fun restartContactsBackupJobs(s: Step) {
+        val users = userAccountManager.allUsers
+        if (users.isEmpty()) {
+            logger.i(TAG, "$s: no users to migrate")
+        } else {
+            users.forEach {
+                val backupEnabled = arbitraryDataProvider.getBooleanValue(
+                    it.accountName,
+                    ContactsPreferenceActivity.PREFERENCE_CONTACTS_AUTOMATIC_BACKUP
+                )
+                if (backupEnabled) {
+                    jobManager.schedulePeriodicContactsBackup(it)
                 }
+                logger.i(TAG, "$s: user = ${it.accountName}, backup enabled = $backupEnabled")
             }
         }
     }
 
     /**
-     * List of migration steps. Those steps will be loaded and run by [MigrationsManager]
+     * List of migration steps. Those steps will be loaded and run by [MigrationsManager].
+     *
+     * If a migration should be run again (applicable to periodic job restarts), insert
+     * the migration with new ID. To prevent accidental re-use of older IDs, replace old
+     * migration with this::nop.
      */
+    @Suppress("MagicNumber")
     val steps: List<Step> = listOf(
-        migrateUserId,
-        migrateContentObserverJob,
-        migrateContactsBackupJob
+        Step(0, "Migrate user id", false, this::migrateUserId),
+        Step(1, "Migrate content observer job", false, this::migrateContentObserverJob),
+        Step(2, "Restart contacts backup job", true, this::nop),
+        Step(3, "Restart contacts backup job", true, this::restartContactsBackupJobs)
     ).sortedBy { it.id }.apply {
         val uniqueIds = associateBy { it.id }.size
         if (uniqueIds != size) {

src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt

@@ -33,7 +33,7 @@ internal class MigrationsManagerImpl(
     private val migrations: Collection<Migrations.Step>
 ) : MigrationsManager {
 
-    override val status: LiveData<Status> = MutableLiveData<Status>(Status.UNKNOWN)
+    override val status: LiveData<Status> = MutableLiveData(Status.UNKNOWN)
 
     override val info: List<MigrationInfo> get() {
         val applied = migrationsDb.getAppliedMigrations()
@@ -77,7 +77,7 @@ internal class MigrationsManagerImpl(
         migrations.forEach {
             @Suppress("TooGenericExceptionCaught") // migration code is free to throw anything
             try {
-                it.run()
+                it.run.invoke(it)
                 migrationsDb.addAppliedMigration(it.id)
             } catch (t: Throwable) {
                 if (it.mandatory) {