From cae11233f317cd6fe91426de8371a3b6a0c161d7 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 4 Jul 2026 02:13:21 -0500 Subject: [PATCH] fix(data): account-lifecycle data integrity (non-destructive upsert, id normalization, deleteAccount cleanup) #309: AccountDao no longer uses @Insert(REPLACE). New insertIfAbsent (IGNORE) + @Update back a non-destructive upsert, and insertAtEnd updates an existing id in place (preserving its sortOrder) instead of REPLACE. Re-adding an existing account id (e.g. re-authing an Outlook account, whose id is the deterministic outlook:) therefore no longer cascade-deletes its account_settings + signatures. #305: normalizeEmailForAccountId always lowercases the domain (mail domains are case-insensitive), and the whole address for the consumer providers (Gmail, Yahoo, iCloud, AOL, Outlook). Applied at every id-derivation site (MailProvider.createAccount, Account.outlook, ManualSetupViewModel) so differently-cased addresses can't spawn duplicate accounts. The displayed email keeps the user's casing. #299: deleteAccount collects the account's message ids and draft attachment URIs while the rows still exist, deletes the rows (now including the account's drafts), then deleteRecursively()'s each message's on-disk attachment cache dir and releases the drafts' now-unreferenced persistable URI grants. Adds unit tests for id normalization + deleteAccount cleanup and DAO-level instrumented tests for the non-destructive create/update. Closes #309 Closes #305 Closes #299 Co-Authored-By: Claude Opus 4.8 --- .../libremail/data/local/AccountDaoTest.kt | 4 +- .../data/local/AccountDatabaseTest.kt | 36 ++++++++++++ .../libremail/data/local/dao/AccountDao.kt | 40 +++++++++++-- .../org/libremail/data/local/dao/DraftDao.kt | 8 +++ .../libremail/data/local/dao/MessageDao.kt | 9 +++ .../data/repository/AccountRepositoryImpl.kt | 31 +++++++++- .../org/libremail/domain/model/Account.kt | 28 ++++++++- .../libremail/domain/model/MailProvider.kt | 4 +- .../ui/accountsetup/ManualSetupViewModel.kt | 5 +- .../repository/AccountRepositoryImplTest.kt | 54 +++++++++++++++++ .../org/libremail/domain/model/AccountTest.kt | 58 +++++++++++++++++++ .../domain/model/MailProviderTest.kt | 14 ++++- .../accountsetup/ManualSetupViewModelTest.kt | 19 ++++++ 13 files changed, 297 insertions(+), 13 deletions(-) create mode 100644 app/src/test/kotlin/org/libremail/domain/model/AccountTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt index 938c457..be830a2 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDaoTest.kt @@ -67,7 +67,9 @@ class AccountDaoTest { } @Test - fun upsertReplacesAnAccountWithTheSameId() = runBlocking { + fun upsertOnAConflictingIdUpdatesTheRowInPlace() = runBlocking { + // Non-destructive by design (issue #309): a second upsert of the same id refreshes the row + // rather than delete-then-reinserting it (which would cascade-delete settings/signatures). dao.upsert(account("acct", "ada@example.org", displayName = "Ada")) dao.upsert(account("acct", "ada@example.org", displayName = "Ada Lovelace")) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt index b908d0e..7405cf5 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt @@ -110,4 +110,40 @@ class AccountDatabaseTest { db.signatureDao().observeForAccount("acct").first().isEmpty(), ) } + + @Test + fun upsertOnAnExistingIdUpdatesInPlaceWithoutCascadingSettingsOrSignatures() = runBlocking { + // Regression for issue #309: an @Insert(REPLACE) upsert deletes-then-reinserts on an id + // conflict, firing the ON DELETE CASCADE that wipes the account's settings + signatures. + db.accountDao().upsert(account("acct")) + db.accountSettingsDao().upsert(AccountSettingsEntity("acct", signature = "Keep me")) + db.signatureDao().upsert(SignatureEntity("sig-1", "acct", "Work", "

Regards

", isDefault = true)) + + db.accountDao().upsert(account("acct").copy(displayName = "Updated")) + + assertEquals("Updated", db.accountDao().getById("acct")?.displayName) + assertEquals("Keep me", db.accountSettingsDao().get("acct")?.signature) + assertEquals(1, db.signatureDao().observeForAccount("acct").first().size) + assertEquals(1, db.accountDao().getAll().size) + } + + @Test + fun insertAtEndReAddingAnExistingAccountKeepsItsSettingsSignaturesAndPosition() = runBlocking { + // The production re-add path (addOutlookAccount -> insertAtEnd -> upsert). Re-adding a + // deterministic id must not cascade-delete its settings/signatures nor move it to the end. + db.accountDao().insertAtEnd(account("a")) + db.accountDao().insertAtEnd(account("b")) + db.accountSettingsDao().upsert(AccountSettingsEntity("b", signature = "Sig B", signatureEnabled = false)) + db.signatureDao().upsert(SignatureEntity("sig-b", "b", "Work", "

Regards

", isDefault = true)) + + db.accountDao().insertAtEnd(account("b").copy(displayName = "Renamed")) + + // Updated in place, position preserved (still 1, not appended after), children intact. + assertEquals("Renamed", db.accountDao().getById("b")?.displayName) + assertEquals(1, db.accountDao().getById("b")?.sortOrder) + assertEquals("Sig B", db.accountSettingsDao().get("b")?.signature) + assertEquals(false, db.accountSettingsDao().get("b")?.signatureEnabled) + assertEquals(1, db.signatureDao().observeForAccount("b").first().size) + assertEquals(2, db.accountDao().getAll().size) + } } diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt index dba0662..0db9fd3 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/AccountDao.kt @@ -6,6 +6,7 @@ import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query import androidx.room.Transaction +import androidx.room.Update import kotlinx.coroutines.flow.Flow import org.libremail.data.local.entity.AccountEntity @@ -27,8 +28,30 @@ interface AccountDao { @Query("SELECT * FROM accounts WHERE id = :id LIMIT 1") suspend fun getById(id: String): AccountEntity? - @Insert(onConflict = OnConflictStrategy.REPLACE) - suspend fun upsert(account: AccountEntity) + /** + * Insert [account] only if its id is absent; a conflicting id is left untouched (returns -1). + * Non-destructive by design: an `@Insert(REPLACE)` would delete-then-reinsert the row on an id + * conflict, firing the `ON DELETE CASCADE` that permanently drops the account's `account_settings` + * + `signatures` (issue #309). Pair it with [update] to refresh an existing row in place instead. + */ + @Insert(onConflict = OnConflictStrategy.IGNORE) + suspend fun insertIfAbsent(account: AccountEntity): Long + + /** Refreshes an existing account's columns in place (matched by primary key); no cascade. */ + @Update + suspend fun update(account: AccountEntity) + + /** + * Insert [account], or refresh the existing row **in place** when its id is already present — + * never the delete-then-reinsert an `@Insert(REPLACE)` does, so it does NOT cascade-delete the + * account's `account_settings` + `signatures` (issue #309). Assigns no list position: new accounts + * are appended via [insertAtEnd]; a plain upsert leaves [AccountEntity.sortOrder] as supplied. + */ + @Transaction + suspend fun upsert(account: AccountEntity) { + // -1 = the IGNORE insert was skipped because the id already exists → update the row instead. + if (insertIfAbsent(account) == -1L) update(account) + } /** The sortOrder that appends a new account to the end of the list (0 when there are none yet). */ @Query("SELECT COALESCE(MAX(sortOrder), -1) + 1 FROM accounts") @@ -36,12 +59,19 @@ interface AccountDao { /** * Insert [account] at the end of the user-defined order (issue #164), stamping it with the current - * max + 1. Both steps run in one transaction so two near-simultaneous adds can't collide on a - * sortOrder. + * max + 1. Re-adding an already-present id (e.g. re-authing an Outlook account, whose id is the + * deterministic `outlook:`) instead refreshes the row in place, keeping its list position + * and — crucially — its settings + signatures, which a REPLACE would cascade-delete (issue #309). + * Runs in one transaction so the read + write can't interleave with a concurrent add. */ @Transaction suspend fun insertAtEnd(account: AccountEntity) { - upsert(account.copy(sortOrder = nextSortOrder())) + val existing = getById(account.id) + if (existing == null) { + insertIfAbsent(account.copy(sortOrder = nextSortOrder())) + } else { + update(account.copy(sortOrder = existing.sortOrder)) + } } @Query("UPDATE accounts SET sortOrder = :sortOrder WHERE id = :id") diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt index 22af30c..715f869 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/DraftDao.kt @@ -23,9 +23,17 @@ interface DraftDao { @Query("SELECT * FROM drafts") suspend fun getAll(): List + /** The drafts composed under [accountId] — enumerated to release their URI grants on account delete (#299). */ + @Query("SELECT * FROM drafts WHERE accountId = :accountId") + suspend fun getByAccount(accountId: String): List + @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun upsert(draft: DraftEntity) @Query("DELETE FROM drafts WHERE id = :id") suspend fun delete(id: String) + + /** Removes every draft composed under [accountId] (the account is being deleted, #299). */ + @Query("DELETE FROM drafts WHERE accountId = :accountId") + suspend fun deleteByAccount(accountId: String) } diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt index c6079aa..23e3db7 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt @@ -217,6 +217,15 @@ interface MessageDao { @Query("DELETE FROM messages WHERE id IN (:ids)") suspend fun deleteByIds(ids: List) + /** + * Ids of every row belonging to [accountId] (all folders; synced rows plus transient search-only + * hits). Lets [org.libremail.data.repository.AccountRepositoryImpl.deleteAccount] enumerate a + * deleted account's messages to purge their on-disk attachment cache before [deleteByAccount] + * removes the rows that name them (issue #299). + */ + @Query("SELECT id FROM messages WHERE accountId = :accountId") + suspend fun getIdsForAccount(accountId: String): List + @Query("DELETE FROM messages WHERE accountId = :accountId") suspend fun deleteByAccount(accountId: String) diff --git a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt index 5c63327..61f16be 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/AccountRepositoryImpl.kt @@ -1,15 +1,21 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.repository +import android.content.Context +import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map +import org.libremail.data.attachment.AttachmentUriGrants +import org.libremail.data.attachmentCacheDir import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.BackfillProgressDao +import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity import org.libremail.data.local.toImapParams +import org.libremail.data.local.toOutgoingAttachments import org.libremail.data.security.CredentialStore import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.sync.SyncScheduler @@ -23,15 +29,18 @@ import javax.inject.Singleton @Singleton class AccountRepositoryImpl @Inject constructor( + @ApplicationContext private val context: Context, private val accountDao: AccountDao, private val messageDao: MessageDao, private val folderDao: FolderDao, private val backfillProgressDao: BackfillProgressDao, + private val draftDao: DraftDao, private val credentialStore: CredentialStore, private val imapClient: ImapClient, private val syncScheduler: SyncScheduler, private val accountSettingsRepository: AccountSettingsRepository, private val mailNotifier: MailNotifier, + private val attachmentUriGrants: AttachmentUriGrants, ) : AccountRepository { override fun observeAccounts(): Flow> = accountDao.observeAll().map { rows -> @@ -72,14 +81,32 @@ class AccountRepositoryImpl @Inject constructor( override suspend fun reorderAccounts(orderedIds: List) = accountDao.reorder(orderedIds) override suspend fun deleteAccount(id: String) { + // Enumerate the on-disk artifacts to clean up WHILE the rows that name them still exist: once + // the message + draft rows are gone, nothing can recover those message ids / draft URIs again, + // so the files/grants would leak forever (issue #299). + val messageIds = messageDao.getIdsForAccount(id) + val draftUris = draftDao.getByAccount(id) + .flatMap { it.attachments.toOutgoingAttachments() } + .map { it.uri } + accountDao.deleteById(id) credentialStore.delete(id) mailNotifier.deleteAccountChannel(id) - // Remove the account's cached mail (attachment rows cascade via the foreign key), folders, and - // backfill progress. The account_settings row is removed automatically by its cascading FK. + // Remove the account's cached mail (attachment rows cascade via the foreign key), folders, + // backfill progress, and drafts. The account_settings + signatures rows are removed + // automatically by their cascading FK when the account row above is deleted. messageDao.deleteByAccount(id) folderDao.deleteForAccount(id) backfillProgressDao.deleteForAccount(id) + draftDao.deleteByAccount(id) + + // Now the rows are gone: delete each message's on-disk attachment cache (keyed by message id, + // the same path MailRepositoryImpl writes), and release any persistable draft-URI grant no + // remaining draft/outbox row still needs (issue #299). + messageIds.forEach { messageId -> + runCatching { attachmentCacheDir(context.cacheDir, messageId).deleteRecursively() } + } + attachmentUriGrants.releaseUnreferenced(draftUris) } override suspend fun resetBackfillProgress(accountId: String?) { diff --git a/app/src/main/kotlin/org/libremail/domain/model/Account.kt b/app/src/main/kotlin/org/libremail/domain/model/Account.kt index 3ccb80e..0c154e4 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/Account.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/Account.kt @@ -22,9 +22,13 @@ data class Account( private const val OUTLOOK_IMAP_PORT = 993 private const val OUTLOOK_SMTP_PORT = 587 - /** An Outlook/Microsoft account using the unified office365 endpoints (personal + M365). */ + /** + * An Outlook/Microsoft account using the unified office365 endpoints (personal + M365). The id + * lowercases the whole address ([normalizeEmailForAccountId] with `lowercaseLocalPart = true`) + * so a re-auth with differently-cased casing resolves to the same account (issue #305). + */ fun outlook(email: String, displayName: String = email): Account = Account( - id = "outlook:$email", + id = "outlook:${normalizeEmailForAccountId(email, lowercaseLocalPart = true)}", email = email, displayName = displayName.ifBlank { email }, authType = AuthType.OAUTH_OUTLOOK, @@ -33,3 +37,23 @@ data class Account( ) } } + +/** + * Normalizes [email] for use in an account id (issue #305): a mailbox is one identity regardless of + * how the address is cased, and the id is the primary key. The domain is always lowercased — mail + * domains are case-insensitive — so `user@Gmail.com` and `user@gmail.com` never spawn two accounts + * syncing one mailbox. The local part is lowercased only when [lowercaseLocalPart] is true: the major + * consumer providers (Gmail, Yahoo, iCloud, AOL, Outlook) treat the whole address case-insensitively, + * but an arbitrary manually-configured IMAP server's local part MAY be case-sensitive (RFC 5321 + * §2.3.11), so a generic account normalizes only the domain. Surrounding whitespace is trimmed either + * way. An address with no `@` has no domain to isolate, so it is lowercased whole (consumer) or left + * as-is (generic). + */ +fun normalizeEmailForAccountId(email: String, lowercaseLocalPart: Boolean): String { + val trimmed = email.trim() + val at = trimmed.lastIndexOf('@') + if (at < 0) return if (lowercaseLocalPart) trimmed.lowercase() else trimmed + val local = trimmed.substring(0, at) + val domain = trimmed.substring(at + 1).lowercase() + return "${if (lowercaseLocalPart) local.lowercase() else local}@$domain" +} diff --git a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt index 730c4b6..22d1400 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt @@ -116,7 +116,9 @@ enum class MailProvider( fun createAccount(email: String, displayName: String = email): Account { val address = email.trim() return Account( - id = "imap:$address", + // Consumer providers treat addresses case-insensitively, so the id lowercases the whole + // address (issue #305) while the displayed [email] keeps the user's casing. + id = "imap:${normalizeEmailForAccountId(address, lowercaseLocalPart = true)}", email = address, displayName = displayName.trim().ifBlank { address }, authType = AuthType.PASSWORD_IMAP, diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt index 01b8237..b27128a 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModel.kt @@ -13,6 +13,7 @@ import org.libremail.domain.model.Account import org.libremail.domain.model.AuthType import org.libremail.domain.model.MailSecurity import org.libremail.domain.model.ServerConfig +import org.libremail.domain.model.normalizeEmailForAccountId import org.libremail.domain.repository.AccountRepository import javax.inject.Inject @@ -65,7 +66,9 @@ class ManualSetupViewModel @Inject constructor(private val accountRepository: Ac return } val account = Account( - id = "imap:${f.email.trim()}", + // Generic IMAP: lowercase only the domain for the id (issue #305). An arbitrary server's + // local part MAY be case-sensitive (RFC 5321), so it is left as typed. + id = "imap:${normalizeEmailForAccountId(f.email, lowercaseLocalPart = false)}", email = f.email.trim(), displayName = f.email.trim(), authType = AuthType.PASSWORD_IMAP, diff --git a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt index cbac6e1..d5bac28 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/AccountRepositoryImplTest.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.repository +import android.content.Context import app.cash.turbine.test import io.mockk.Runs import io.mockk.coEvery @@ -13,11 +14,15 @@ import io.mockk.verify import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import org.junit.Test +import org.libremail.data.attachment.AttachmentUriGrants +import org.libremail.data.attachmentCacheDir import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.BackfillProgressDao +import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.entity.AccountEntity +import org.libremail.data.local.entity.DraftEntity import org.libremail.data.local.entity.ServerConfigEmbedded import org.libremail.data.security.CredentialStore import org.libremail.data.settings.AccountSettingsRepository @@ -30,6 +35,7 @@ import org.libremail.domain.model.ServerConfig import org.libremail.mail.FetchedFolder import org.libremail.mail.ImapClient import org.libremail.notifications.MailNotifier +import java.io.File import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -42,26 +48,32 @@ import kotlin.test.assertTrue */ class AccountRepositoryImplTest { + private val context = mockk(relaxed = true) private val accountDao = mockk() private val messageDao = mockk(relaxed = true) private val folderDao = mockk(relaxed = true) private val backfillProgressDao = mockk(relaxed = true) + private val draftDao = mockk(relaxed = true) private val credentialStore = mockk(relaxed = true) private val imapClient = mockk() private val syncScheduler = mockk(relaxed = true) private val accountSettingsRepository = mockk(relaxed = true) private val mailNotifier = mockk(relaxed = true) + private val attachmentUriGrants = mockk(relaxed = true) private val repository = AccountRepositoryImpl( + context = context, accountDao = accountDao, messageDao = messageDao, folderDao = folderDao, backfillProgressDao = backfillProgressDao, + draftDao = draftDao, credentialStore = credentialStore, imapClient = imapClient, syncScheduler = syncScheduler, accountSettingsRepository = accountSettingsRepository, mailNotifier = mailNotifier, + attachmentUriGrants = attachmentUriGrants, ) @Test @@ -180,6 +192,37 @@ class AccountRepositoryImplTest { coVerify { messageDao.deleteByAccount("acct") } coVerify { folderDao.deleteForAccount("acct") } coVerify { backfillProgressDao.deleteForAccount("acct") } + coVerify { draftDao.deleteByAccount("acct") } + } + + @Test + fun `deleteAccount purges on-disk attachment caches and releases draft URI grants`() = runTest { + // A real temp cacheDir with one message's attachment cache dir populated on disk. + val cacheDir = File(System.getProperty("java.io.tmpdir"), "libremail-delete-test-${System.nanoTime()}") + every { context.cacheDir } returns cacheDir + val messageId = "acct:INBOX:1" + val attachmentDir = attachmentCacheDir(cacheDir, messageId).apply { mkdirs() } + File(attachmentDir, "0/report.pdf").apply { parentFile?.mkdirs() }.writeText("bytes") + + // The account owns that message and a draft carrying a picked attachment URI. + coEvery { accountDao.deleteById("acct") } just Runs + coEvery { messageDao.getIdsForAccount("acct") } returns listOf(messageId) + coEvery { draftDao.getByAccount("acct") } returns listOf( + draftEntity(id = "d1", attachments = """[{"uri":"content://pick/1","name":"report.pdf"}]"""), + ) + val released = slot>() + coEvery { attachmentUriGrants.releaseUnreferenced(capture(released)) } just Runs + + repository.deleteAccount("acct") + + // The message ids must be read BEFORE the rows are deleted, and the on-disk cache purged. + coVerify { messageDao.getIdsForAccount("acct") } + assertFalse(attachmentDir.exists(), "attachment cache dir must be deleted with the account") + // The deleted draft's picked-URI grant is released. + coVerify { draftDao.deleteByAccount("acct") } + assertEquals(listOf("content://pick/1"), released.captured.toList()) + + cacheDir.deleteRecursively() } @Test @@ -218,6 +261,17 @@ class AccountRepositoryImplTest { smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"), ) + private fun draftEntity(id: String, attachments: String) = DraftEntity( + id = id, + accountId = "acct", + toAddresses = "", + ccAddresses = "", + subject = "", + body = "", + updatedAt = 0L, + attachments = attachments, + ) + private val params = ImapConnectionParams( host = "imap.example.org", port = 993, diff --git a/app/src/test/kotlin/org/libremail/domain/model/AccountTest.kt b/app/src/test/kotlin/org/libremail/domain/model/AccountTest.kt new file mode 100644 index 0000000..723adae --- /dev/null +++ b/app/src/test/kotlin/org/libremail/domain/model/AccountTest.kt @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.domain.model + +import org.junit.Test +import kotlin.test.assertEquals + +/** + * Locks down account-id normalization (issue #305): the id is the primary key, so casing that varies + * between adds must not spawn a second account syncing the same mailbox. [normalizeEmailForAccountId] + * always lowercases the domain and — for consumer providers — the whole address; the generic path + * leaves the local part as typed. [Account.outlook] derives a deterministic id from the address. + */ +class AccountTest { + + @Test + fun `normalize always lowercases the domain, keeping the local part when not a consumer provider`() { + assertEquals("User@example.org", normalizeEmailForAccountId("User@Example.ORG", lowercaseLocalPart = false)) + assertEquals("ADA@gmail.com", normalizeEmailForAccountId("ADA@GMAIL.COM", lowercaseLocalPart = false)) + } + + @Test + fun `normalize lowercases the whole address for consumer providers`() { + assertEquals("user@example.org", normalizeEmailForAccountId("User@Example.ORG", lowercaseLocalPart = true)) + assertEquals("ada@gmail.com", normalizeEmailForAccountId("ADA@GMAIL.COM", lowercaseLocalPart = true)) + } + + @Test + fun `normalize trims surrounding whitespace either way`() { + assertEquals("user@example.org", normalizeEmailForAccountId(" user@Example.org ", lowercaseLocalPart = false)) + assertEquals("user@example.org", normalizeEmailForAccountId(" User@Example.org ", lowercaseLocalPart = true)) + } + + @Test + fun `normalize maps different casings of one address to the same value`() { + assertEquals( + normalizeEmailForAccountId("User@Gmail.com", lowercaseLocalPart = true), + normalizeEmailForAccountId("user@gmail.com", lowercaseLocalPart = true), + ) + } + + @Test + fun `normalize handles an address with no at-sign without throwing`() { + // No domain to isolate: lowercased whole for consumer, left as-is for generic. + assertEquals("root", normalizeEmailForAccountId("Root", lowercaseLocalPart = true)) + assertEquals("Root", normalizeEmailForAccountId("Root", lowercaseLocalPart = false)) + } + + @Test + fun `outlook derives a lowercased, deterministic id but keeps the displayed email casing`() { + val account = Account.outlook("User@Outlook.com") + + assertEquals("outlook:user@outlook.com", account.id) + assertEquals("User@Outlook.com", account.email) + assertEquals(AuthType.OAUTH_OUTLOOK, account.authType) + // Any casing of the same address resolves to one id, so a re-auth never duplicates the account. + assertEquals(Account.outlook("USER@OUTLOOK.COM").id, account.id) + } +} diff --git a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt index 6adc0e6..3dc890b 100644 --- a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt +++ b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt @@ -145,11 +145,23 @@ class MailProviderTest { fun `createAccount trims the email and derives a stable id and display name`() { val account = MailProvider.GMAIL.createAccount(" User@Gmail.com ") + // The displayed email/name keep the user's casing, but the id lowercases the whole address so + // it is stable across casing (issue #305). assertEquals("User@Gmail.com", account.email) - assertEquals("imap:User@Gmail.com", account.id) + assertEquals("imap:user@gmail.com", account.id) assertEquals("User@Gmail.com", account.displayName) } + @Test + fun `createAccount lowercases the whole address in the id so casing never duplicates an account`() { + // A consumer provider treats the address case-insensitively, so any casing maps to one id. + assertEquals( + MailProvider.GMAIL.createAccount("User@Gmail.com").id, + MailProvider.GMAIL.createAccount("user@gmail.com").id, + ) + assertEquals("imap:user@gmail.com", MailProvider.GMAIL.createAccount("USER@GMAIL.COM").id) + } + @Test fun `createAccount keeps an explicit non-blank display name`() { val account = MailProvider.GMAIL.createAccount("user@gmail.com", displayName = "Work") diff --git a/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt index 8da34b6..882e1de 100644 --- a/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/accountsetup/ManualSetupViewModelTest.kt @@ -135,6 +135,25 @@ class ManualSetupViewModelTest { assertEquals("imap:user@example.org", vm.form.value.addedAccountId) } + @Test + fun `testAndSave lowercases the domain in the id but keeps the local-part casing`() = runTest(dispatcher) { + // Generic IMAP servers may treat the local part case-sensitively, so only the domain is + // lowercased for the id (issue #305); the displayed email keeps the typed casing. + val repo = mockk() + val account = slot() + coEvery { repo.addImapAccount(capture(account), any()) } returns Result.success(emptyList()) + val vm = ManualSetupViewModel(repo) + vm.onEmail(" User@Example.ORG ") + vm.onPassword("secret") + vm.onImapHost("imap.example.org") + vm.onSmtpHost("smtp.example.org") + + vm.testAndSave() + + assertEquals("imap:User@example.org", account.captured.id) + assertEquals("User@Example.ORG", account.captured.email) + } + @Test fun `testAndSave falls back to the default ports when the port fields are blank`() = runTest(dispatcher) { val repo = mockk() -- 2.47.3