Merge pull request #9646 from nextcloud/run-contacts-backup-only-when-network-is-connected

Run contacts backup job only when network is connected

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) {