From 32925dc18be1bf4c12b6c629c4f127353889184f Mon Sep 17 00:00:00 2001 From: Sunik Kupfer Date: Thu, 7 Nov 2024 11:45:27 +0100 Subject: [PATCH] Set or update read-only flag when address books are renamed (#1124) * Always evaluate read only * Extract read only evaluation to companion method * Extract read only evaluation to companion method * Add test * Always pass forceReadOnly flag * Minor KDoc changes --------- Co-authored-by: Ricki Hirner --- .../davdroid/resource/LocalAddressBookTest.kt | 17 ++++++ .../davdroid/resource/LocalAddressBook.kt | 60 +++++++++++-------- .../settings/AccountSettingsMigrations.kt | 4 +- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt index a6651b311..e47cda020 100644 --- a/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt +++ b/app/src/androidTest/kotlin/at/bitfire/davdroid/resource/LocalAddressBookTest.kt @@ -12,6 +12,7 @@ import android.content.Context import android.provider.ContactsContract import androidx.test.platform.app.InstrumentationRegistry import androidx.test.rule.GrantPermissionRule +import at.bitfire.davdroid.db.Collection import at.bitfire.vcard4android.Contact import at.bitfire.vcard4android.GroupMethod import at.bitfire.vcard4android.LabeledProperty @@ -19,6 +20,8 @@ import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.android.testing.HiltAndroidRule import dagger.hilt.android.testing.HiltAndroidTest import ezvcard.property.Telephone +import io.mockk.every +import io.mockk.mockk import java.util.LinkedList import javax.inject.Inject import org.junit.After @@ -26,6 +29,7 @@ import org.junit.AfterClass import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.BeforeClass import org.junit.ClassRule @@ -123,6 +127,19 @@ class LocalAddressBookTest { assertEquals("Test Group", group.displayName) } + /** + * Tests the calculation of read only state is correct + */ + @Test + fun test_shouldBeReadOnly() { + val collectionReadOnly = mockk { every { readOnly() } returns true } + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, false)) + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionReadOnly, true)) + + val collectionNotReadOnly = mockk { every { readOnly() } returns false } + assertFalse(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, false)) + assertTrue(LocalAddressBook.shouldBeReadOnly(collectionNotReadOnly, true)) + } companion object { diff --git a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt index b8579a16a..e1ca08239 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/resource/LocalAddressBook.kt @@ -155,9 +155,8 @@ open class LocalAddressBook @AssistedInject constructor( * @param info collection where to take the settings from * @param forceReadOnly `true`: set the address book to "force read-only"; * `false`: determine read-only flag from [info]; - * `null`: don't change the existing value */ - fun update(info: Collection, forceReadOnly: Boolean? = null) { + fun update(info: Collection, forceReadOnly: Boolean) { logger.log(Level.INFO, "Updating local address book $addressBookAccount with collection $info") val accountManager = AccountManager.get(context) @@ -175,31 +174,30 @@ open class LocalAddressBook @AssistedInject constructor( settings = contactsProviderSettings // Update force read only - if (forceReadOnly != null) { - val nowReadOnly = forceReadOnly || !info.privWriteContent || info.forceReadOnly - if (nowReadOnly != readOnly) { - logger.info("Address book now read-only = $nowReadOnly, updating contacts") - - // update address book itself - readOnly = nowReadOnly - - // update raw contacts - val rawContactValues = ContentValues(1) - rawContactValues.put(RawContacts.RAW_CONTACT_IS_READ_ONLY, if (nowReadOnly) 1 else 0) - provider!!.update(rawContactsSyncUri(), rawContactValues, null, null) - - // update data rows - val dataValues = ContentValues(1) - dataValues.put(ContactsContract.Data.IS_READ_ONLY, if (nowReadOnly) 1 else 0) - provider!!.update(syncAdapterURI(ContactsContract.Data.CONTENT_URI), dataValues, null, null) - - // update group rows - val groupValues = ContentValues(1) - groupValues.put(Groups.GROUP_IS_READ_ONLY, if (nowReadOnly) 1 else 0) - provider!!.update(groupsSyncUri(), groupValues, null, null) - } + val nowReadOnly = shouldBeReadOnly(info, forceReadOnly) + if (nowReadOnly != readOnly) { + logger.info("Address book now read-only = $nowReadOnly, updating contacts") + + // update address book itself + readOnly = nowReadOnly + + // update raw contacts + val rawContactValues = ContentValues(1) + rawContactValues.put(RawContacts.RAW_CONTACT_IS_READ_ONLY, if (nowReadOnly) 1 else 0) + provider!!.update(rawContactsSyncUri(), rawContactValues, null, null) + + // update data rows + val dataValues = ContentValues(1) + dataValues.put(ContactsContract.Data.IS_READ_ONLY, if (nowReadOnly) 1 else 0) + provider!!.update(syncAdapterURI(ContactsContract.Data.CONTENT_URI), dataValues, null, null) + + // update group rows + val groupValues = ContentValues(1) + groupValues.put(Groups.GROUP_IS_READ_ONLY, if (nowReadOnly) 1 else 0) + provider!!.update(groupsSyncUri(), groupValues, null, null) } + // make sure it will still be synchronized when contacts are updated updateSyncFrameworkSettings() } @@ -432,11 +430,21 @@ open class LocalAddressBook @AssistedInject constructor( addressBook.updateSyncFrameworkSettings() addressBook.settings = contactsProviderSettings - addressBook.readOnly = forceReadOnly || !info.privWriteContent || info.forceReadOnly + addressBook.readOnly = shouldBeReadOnly(info, forceReadOnly) return addressBook } + /** + * Determines whether the address book should be set to read-only. + * + * @param forceReadOnly Whether (usually managed, app-wide) setting should overwrite local read-only information + * @param info Collection data to determine read-only status from (either user-set read-only flag or missing write privilege) + */ + @VisibleForTesting + internal fun shouldBeReadOnly(info: Collection, forceReadOnly: Boolean): Boolean = + info.readOnly() || forceReadOnly + /** * Finds a [LocalAddressBook] based on its corresponding collection. * diff --git a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt index f2a7d46df..91558b94f 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettingsMigrations.kt @@ -21,7 +21,6 @@ import androidx.work.WorkManager import at.bitfire.davdroid.R import at.bitfire.davdroid.db.AppDatabase import at.bitfire.davdroid.db.Service -import at.bitfire.davdroid.repository.AccountRepository import at.bitfire.davdroid.repository.DavCollectionRepository import at.bitfire.davdroid.repository.DavServiceRepository import at.bitfire.davdroid.resource.LocalAddressBook @@ -52,7 +51,6 @@ class AccountSettingsMigrations @AssistedInject constructor( @Assisted val account: Account, @Assisted val accountSettings: AccountSettings, @ApplicationContext val context: Context, - private val accountRepository: AccountRepository, private val collectionRepository: DavCollectionRepository, private val db: AppDatabase, private val localAddressBookFactory: LocalAddressBook.Factory, @@ -103,7 +101,7 @@ class AccountSettingsMigrations @AssistedInject constructor( collectionRepository.getByServiceAndUrl(service.id, url)?.let { collection -> // Set collection ID and rename the account val localAddressBook = localAddressBookFactory.create(oldAddressBookAccount, provider) - localAddressBook.update(collection) + localAddressBook.update(collection, /* read-only flag will be updated at next sync */ forceReadOnly = false) } } }