Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Unverified Commit b910ba25 authored by Sunik Kupfer's avatar Sunik Kupfer Committed by GitHub
Browse files

Use ID to match DB collections with content provider collections (#1274)



* Update ical4android

* Match DB collections with content provider collections via ID

* Minor renaming and KDoc

* Move string constant to companion object

* Update KDoc

* Use getOrDefault to be more explicit

* Remove exception throw on missing collection ID

* Rewrite LocalAddressBookStoreTest

* Minor changes
- remove unused param
- make companion methods internal

---------

Co-authored-by: default avatarRicki Hirner <hirner@bitfire.at>
parent 2a542210
Loading
Loading
Loading
Loading
+69 −82
Original line number Diff line number Diff line
@@ -9,26 +9,18 @@ import android.accounts.AccountManager
import android.content.ContentProviderClient
import android.content.Context
import at.bitfire.davdroid.R
import at.bitfire.davdroid.db.AppDatabase
import at.bitfire.davdroid.db.Collection
import at.bitfire.davdroid.db.Service
import at.bitfire.davdroid.repository.DavCollectionRepository
import at.bitfire.davdroid.repository.DavServiceRepository
import at.bitfire.davdroid.settings.SettingsManager
import at.bitfire.davdroid.sync.account.SystemAccountUtils
import at.bitfire.davdroid.sync.account.TestAccount
import dagger.hilt.android.qualifiers.ApplicationContext
import dagger.hilt.android.testing.HiltAndroidRule
import dagger.hilt.android.testing.HiltAndroidTest
import io.mockk.MockKAnnotations
import io.mockk.every
import io.mockk.impl.annotations.InjectMockKs
import io.mockk.impl.annotations.RelaxedMockK
import io.mockk.impl.annotations.SpyK
import io.mockk.just
import io.mockk.junit4.MockKRule
import io.mockk.mockk
import io.mockk.mockkObject
import io.mockk.runs
import io.mockk.unmockkAll
import io.mockk.verify
import okhttp3.HttpUrl.Companion.toHttpUrl
import org.junit.After
import org.junit.Assert.assertEquals
@@ -37,71 +29,59 @@ import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import java.util.logging.Logger
import javax.inject.Inject

@HiltAndroidTest
class LocalAddressBookStoreTest {

    @get:Rule
    val hiltRule = HiltAndroidRule(this)

    @Inject
    @ApplicationContext
    @SpyK
    @Inject @ApplicationContext
    lateinit var context: Context

    val account by lazy { Account("Test Account", context.getString(R.string.account_type)) }
    val addressBookAccount by lazy { Account("MrRobert@example.com", context.getString(R.string.account_type_address_book)) }

    val provider = mockk<ContentProviderClient>(relaxed = true)
    val addressBook: LocalAddressBook = mockk(relaxed = true) {
        every { account } answers { this@LocalAddressBookStoreTest.account }
        every { updateSyncFrameworkSettings() } just runs
        every { addressBookAccount } answers { this@LocalAddressBookStoreTest.addressBookAccount }
        every { settings } returns LocalAddressBookStore.contactsProviderSettings
    }

    @Suppress("unused")     // used by @InjectMockKs LocalAddressBookStore
    @RelaxedMockK
    lateinit var collectionRepository: DavCollectionRepository

    @Suppress("unused")     // used by @InjectMockKs LocalAddressBookStore
    val localAddressBookFactory = mockk<LocalAddressBook.Factory> {
        every { create(any(), any(), provider) } returns addressBook
    }
    @Inject
    lateinit var db: AppDatabase

    @Inject
    @SpyK
    lateinit var logger: Logger
    lateinit var localAddressBookStore: LocalAddressBookStore

    @RelaxedMockK
    lateinit var settingsManager: SettingsManager
    lateinit var provider: ContentProviderClient

    @Suppress("unused")     // used by @InjectMockKs LocalAddressBookStore
    val serviceRepository = mockk<DavServiceRepository>(relaxed = true) {
        every { get(any<Long>()) } returns null
        every { get(200) } returns mockk<Service> {
            every { accountName } returns "MrRobert@example.com"
        }
    }
    @get:Rule
    val hiltRule = HiltAndroidRule(this)

    @InjectMockKs
    @SpyK
    lateinit var localAddressBookStore: LocalAddressBookStore
    @get:Rule
    val mockkRule = MockKRule(this)

    lateinit var addressBookAccountType: String

    lateinit var addressBookAccount: Account
    lateinit var account: Account
    lateinit var service: Service

    @Before
    fun setUp() {
        hiltRule.inject()

        // initialize global mocks
        MockKAnnotations.init(this)
        addressBookAccountType = context.getString(R.string.account_type_address_book)
        removeAddressBooks()

        account = TestAccount.create()
        service = Service(
            id = 200,
            accountName = account.name,
            type = Service.Companion.TYPE_CARDDAV,
            principal = null
        )
        db.serviceDao().insertOrReplace(service)
        addressBookAccount = Account(
            "MrRobert@example.com",
            addressBookAccountType
        )
    }

    @After
    fun tearDown() {
        unmockkAll()
        TestAccount.remove(account)
    }


@@ -111,7 +91,7 @@ class LocalAddressBookStoreTest {
            every { id } returns 42
            every { url } returns "https://example.com/addressbook/funnyfriends".toHttpUrl()
            every { displayName } returns null
            every { serviceId } returns 404
            every { serviceId } returns 404     // missing service
        }
        assertEquals("funnyfriends #42", localAddressBookStore.accountName(collection))
    }
@@ -122,15 +102,15 @@ class LocalAddressBookStoreTest {
            every { id } returns 42
            every { url } returns "https://example.com/addressbook/funnyfriends".toHttpUrl()
            every { displayName } returns null
            every { serviceId } returns 200
            every { serviceId } returns service.id
        }
        val accountName = localAddressBookStore.accountName(collection)
        assertEquals("funnyfriends (MrRobert@example.com) #42", accountName)
        assertEquals("funnyfriends (${account.name}) #42", accountName)
    }

    @Test
    fun test_accountName_missingDisplayNameAndService() {
        val collection = mockk<Collection>(relaxed = true) {
        val collection = mockk<Collection> {
            every { id } returns 1
            every { url } returns "https://example.com/addressbook/funnyfriends".toHttpUrl()
            every { displayName } returns null
@@ -143,45 +123,42 @@ class LocalAddressBookStoreTest {
    @Test
    fun test_create_createAccountReturnsNull() {
        val collection = mockk<Collection>(relaxed = true) {
            every { serviceId } returns 200
            every { serviceId } returns service.id
            every { id } returns 1
            every { url } returns "https://example.com/addressbook/funnyfriends".toHttpUrl()
        }
        every { localAddressBookStore.createAddressBookAccount(any(), any(), any(), any()) } returns null

        mockkObject(localAddressBookStore)
        every { localAddressBookStore.createAddressBookAccount(any(), any(), any()) } returns null

        assertEquals(null, localAddressBookStore.create(provider, collection))
    }

    @Test
    fun test_create_createAccountReturnsAccount() {
    fun test_create_ReadOnly() {
        val collection = mockk<Collection>(relaxed = true) {
            every { serviceId } returns 200
            every { serviceId } returns service.id
            every { id } returns 1
            every { url } returns "https://example.com/addressbook/funnyfriends".toHttpUrl()
            every { readOnly() } returns true
        }
        every { localAddressBookStore.createAddressBookAccount(any(), any(), any(), any()) } returns addressBookAccount
        every { addressBook.readOnly } returns true
        val addrBook = localAddressBookStore.create(provider, collection)!!

        verify(exactly = 1) { addressBook.updateSyncFrameworkSettings() }
        assertEquals(addressBookAccount, addrBook.addressBookAccount)
        assertEquals(LocalAddressBookStore.contactsProviderSettings, addrBook.settings)
        assertEquals(true, addrBook.readOnly)

        every { addressBook.readOnly } returns false
        val addrBook2 = localAddressBookStore.create(provider, collection)!!
        assertEquals(false, addrBook2.readOnly)
        assertEquals(Account("funnyfriends (Test Account) #1", addressBookAccountType), addrBook.addressBookAccount)
        assertTrue(addrBook.readOnly)
    }

    @Test
    fun test_createAccount_succeeds() {
        mockkObject(SystemAccountUtils)
        every { SystemAccountUtils.createAccount(any(), any(), any()) } returns true
        val result: Account = localAddressBookStore.createAddressBookAccount(
            account, "MrRobert@example.com", 42, "https://example.com/addressbook/funnyfriends"
        )!!
        verify(exactly = 1) { SystemAccountUtils.createAccount(any(), any(), any()) }
        assertEquals("MrRobert@example.com", result.name)
        assertEquals(context.getString(R.string.account_type_address_book), result.type)
    fun test_create_ReadWrite() {
        val collection = mockk<Collection>(relaxed = true) {
            every { serviceId } returns service.id
            every { id } returns 1
            every { url } returns "https://example.com/addressbook/funnyfriends".toHttpUrl()
            every { readOnly() } returns false
        }

        val addrBook = localAddressBookStore.create(provider, collection)!!
        assertEquals(Account("funnyfriends (Test Account) #1", addressBookAccountType), addrBook.addressBookAccount)
        assertFalse(addrBook.readOnly)
    }


@@ -223,4 +200,14 @@ class LocalAddressBookStoreTest {
        assertTrue(LocalAddressBookStore.shouldBeReadOnly(collectionNotReadOnly, true))
    }


    // helpers

    private fun removeAddressBooks() {
        val accountManager = AccountManager.get(context)
        accountManager.getAccountsByType(addressBookAccountType).forEach {
            accountManager.removeAccount(it, null, null)
        }
    }

}
 No newline at end of file
+3 −2
Original line number Diff line number Diff line
@@ -53,6 +53,7 @@ class AccountSettingsMigration17Test {

    @Test
    fun testMigrate_OldAddressBook_CollectionInDB() {
        val localAddressBookUserDataUrl = "url"
        TestAccount.provide(version = 16) { account ->
            val accountManager = AccountManager.get(context)
            val addressBookAccountType = context.getString(R.string.account_type_address_book)
@@ -63,7 +64,7 @@ class AccountSettingsMigration17Test {
                // address book has account + URL
                val url = "https://example.com/address-book"
                accountManager.setAndVerifyUserData(addressBookAccount, "real_account_name", account.name)
                accountManager.setAndVerifyUserData(addressBookAccount, LocalAddressBook.USER_DATA_URL, url)
                accountManager.setAndVerifyUserData(addressBookAccount, localAddressBookUserDataUrl, url)

                // and is known in database
                db.serviceDao().insertOrReplace(
@@ -86,7 +87,7 @@ class AccountSettingsMigration17Test {

                // migration renames address book, update account
                addressBookAccount = accountManager.getAccountsByType(addressBookAccountType).filter {
                    accountManager.getUserData(it, LocalAddressBook.USER_DATA_URL) == url
                    accountManager.getUserData(it, localAddressBookUserDataUrl) == url
                }.first()
                assertEquals("Some Address Book (${account.name}) #100", addressBookAccount.name)

+1 −1
Original line number Diff line number Diff line
@@ -8,7 +8,7 @@ import at.bitfire.davdroid.db.SyncState
import at.bitfire.davdroid.resource.LocalCollection

class LocalTestCollection(
    override val collectionUrl: String = "http://example.com/test/"
    override val dbCollectionId: Long = 0L
): LocalCollection<LocalTestResource> {

    override val tag = "LocalTestCollection"
+22 −20
Original line number Diff line number Diff line
@@ -17,7 +17,6 @@ import io.mockk.just
import io.mockk.mockk
import io.mockk.runs
import io.mockk.verify
import okhttp3.HttpUrl.Companion.toHttpUrl
import org.junit.Assert.assertArrayEquals
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
@@ -66,9 +65,10 @@ class SyncerTest {

    @Test
    fun testUpdateCollections_deletesCollection() {
        val localCollection = mockk<LocalTestCollection>()
        every { localCollection.collectionUrl } returns "http://delete.the/collection"
        every { localCollection.title } returns "Collection to be deleted locally"
        val localCollection = mockk<LocalTestCollection> {
            every { dbCollectionId } returns 0L
            every { title } returns "Collection to be deleted locally"
        }

        // Should delete the localCollection if dbCollection (remote) does not exist
        val localCollections = mutableListOf(localCollection)
@@ -81,12 +81,14 @@ class SyncerTest {

    @Test
    fun testUpdateCollections_updatesCollection() {
        val localCollection = mockk<LocalTestCollection>()
        val dbCollection = mockk<Collection>()
        val dbCollections = mapOf("http://update.the/collection".toHttpUrl() to dbCollection)
        every { dbCollection.url } returns "http://update.the/collection".toHttpUrl()
        every { localCollection.collectionUrl } returns "http://update.the/collection"
        every { localCollection.title } returns "The Local Collection"
        val localCollection = mockk<LocalTestCollection> {
            every { dbCollectionId } returns 0L
            every { title } returns "The Local Collection"
        }
        val dbCollection = mockk<Collection> {
            every { id } returns 0L
        }
        val dbCollections = mapOf(0L to dbCollection)

        // Should update the localCollection if it exists
        val result = syncer.updateCollections(provider, listOf(localCollection), dbCollections)
@@ -99,13 +101,13 @@ class SyncerTest {
    @Test
    fun testUpdateCollections_findsNewCollection() {
        val dbCollection = mockk<Collection> {
            every { url } returns "http://newly.found/collection".toHttpUrl()
            every { id } returns 0L
        }
        val localCollections = listOf(mockk<LocalTestCollection> {
            every { collectionUrl } returns "http://newly.found/collection"
            every { dbCollectionId } returns 0L
        })
        val dbCollections = listOf(dbCollection)
        val dbCollectionsMap = mapOf(dbCollection.url to dbCollection)
        val dbCollectionsMap = mapOf(dbCollection.id to dbCollection)
        every { syncer.createLocalCollections(provider, dbCollections) } returns localCollections

        // Should return the new collection, because it was not updated
@@ -113,7 +115,7 @@ class SyncerTest {

        // Updated local collection list contain new entry
        assertEquals(1, result.size)
        assertEquals(dbCollection.url.toString(), result[0].collectionUrl)
        assertEquals(dbCollection.id, result[0].dbCollectionId)
    }


@@ -134,14 +136,14 @@ class SyncerTest {
        val dbCollection1 = mockk<Collection>()
        val dbCollection2 = mockk<Collection>()
        val dbCollections = mapOf(
            "http://newly.found/collection1".toHttpUrl() to dbCollection1,
            "http://newly.found/collection2".toHttpUrl() to dbCollection2
            0L to dbCollection1,
            1L to dbCollection2
        )
        val localCollection1 = mockk<LocalTestCollection>()
        val localCollection2 = mockk<LocalTestCollection>()
        val localCollection1 = mockk<LocalTestCollection> { every { dbCollectionId } returns 0L }
        val localCollection2 = mockk<LocalTestCollection> { every { dbCollectionId } returns 1L }
        val localCollections = listOf(localCollection1, localCollection2)
        every { localCollection1.collectionUrl } returns "http://newly.found/collection1"
        every { localCollection2.collectionUrl } returns "http://newly.found/collection2"
        every { localCollection1.dbCollectionId } returns 0L
        every { localCollection2.dbCollectionId } returns 1L
        every { syncer.syncCollection(provider, any(), any()) } just runs

        // Should call the collection content sync on both collections
+12 −20
Original line number Diff line number Diff line
@@ -40,11 +40,11 @@ import java.util.logging.Level
import java.util.logging.Logger

/**
 * A local address book. Requires an own Android account, because Android manages contacts per
 * A local address book. Requires its own Android account, because Android manages contacts per
 * account and there is no such thing as "address books". So, DAVx5 creates a "DAVx5
 * address book" account for every CardDAV address book.
 *
 * @param account             TODO
 * @param account             DAVx5 account which "owns" this address book
 * @param _addressBookAccount Address book account (not: DAVx5 account) storing the actual Android
 * contacts. This is the initial value of [addressBookAccount]. However when the address book is renamed,
 * the new name will only be available in [addressBookAccount], so usually that one should be used.
@@ -79,6 +79,8 @@ open class LocalAddressBook @AssistedInject constructor(
    override val title
        get() = addressBookAccount.name

    private val accountManager by lazy { AccountManager.get(context) }

    /**
     * Whether contact groups ([LocalGroup]) are included in query results
     * and are affected by updates/deletes on generic members.
@@ -87,8 +89,7 @@ open class LocalAddressBook @AssistedInject constructor(
     * but if it is enabled, [findDirty] will find dirty [LocalContact]s and [LocalGroup]s.
     */
    open val groupMethod: GroupMethod by lazy {
        val manager = AccountManager.get(context)
        val account = manager.getUserData(addressBookAccount, USER_DATA_COLLECTION_ID)?.toLongOrNull()?.let { collectionId ->
        val account = accountManager.getUserData(addressBookAccount, USER_DATA_COLLECTION_ID)?.toLongOrNull()?.let { collectionId ->
            collectionRepository.get(collectionId)?.let { collection ->
                serviceRepository.get(collection.serviceId)?.let { service ->
                    Account(service.accountName, context.getString(R.string.account_type))
@@ -103,11 +104,11 @@ open class LocalAddressBook @AssistedInject constructor(
    val includeGroups
        get() = groupMethod == GroupMethod.GROUP_VCARDS

    @Deprecated("Local collection should be identified by ID, not by URL")
    override var collectionUrl: String
        get() = AccountManager.get(context).getUserData(addressBookAccount, USER_DATA_URL)
                ?: throw IllegalStateException("Address book has no URL")
        set(url) = AccountManager.get(context).setAndVerifyUserData(addressBookAccount, USER_DATA_URL, url)
    override var dbCollectionId: Long?
        get() = accountManager.getUserData(addressBookAccount, USER_DATA_COLLECTION_ID)?.toLongOrNull()
        set(id) {
            accountManager.setAndVerifyUserData(addressBookAccount, USER_DATA_COLLECTION_ID, id.toString())
        }

    /**
     * Read-only flag for the address book itself.
@@ -122,10 +123,10 @@ open class LocalAddressBook @AssistedInject constructor(
     * Reading this flag returns the stored value from [USER_DATA_READ_ONLY].
     */
    override var readOnly: Boolean
        get() = AccountManager.get(context).getUserData(addressBookAccount, USER_DATA_READ_ONLY) != null
        get() = accountManager.getUserData(addressBookAccount, USER_DATA_READ_ONLY) != null
        set(readOnly) {
            // set read-only flag for address book itself
            AccountManager.get(context).setAndVerifyUserData(addressBookAccount, USER_DATA_READ_ONLY, if (readOnly) "1" else null)
            accountManager.setAndVerifyUserData(addressBookAccount, USER_DATA_READ_ONLY, if (readOnly) "1" else null)

            // update raw contacts
            val rawContactValues = contentValuesOf(RawContacts.RAW_CONTACT_IS_READ_ONLY to if (readOnly) 1 else 0)
@@ -213,7 +214,6 @@ open class LocalAddressBook @AssistedInject constructor(
        addressBookAccount = newAccount

        // delete old account
        val accountManager = AccountManager.get(context)
        accountManager.removeAccountExplicitly(oldAccount)

        return true
@@ -336,14 +336,6 @@ open class LocalAddressBook @AssistedInject constructor(
        const val USER_DATA_ACCOUNT_NAME = "account_name"
        const val USER_DATA_ACCOUNT_TYPE = "account_type"

        /**
         * URL of the corresponding CardDAV address book.
         *
         * User data of the address book account (String).
         */
        @Deprecated("Use the URL of the DB collection instead")
        const val USER_DATA_URL = "url"

        /**
         * ID of the corresponding database [at.bitfire.davdroid.db.Collection].
         *
Loading