Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update authorization state when editing server settings #7365

Merged
merged 2 commits into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ class InMemoryAccountStore(
private val accountMap: MutableMap<String, Account> = mutableMapOf(),
) : AccountCreator, AccountServerSettingsUpdater, AccountStateLoader {

suspend fun load(accountUuid: String): Account? {
return accountMap[accountUuid]
}

override suspend fun loadAccountState(accountUuid: String): AccountState? {
return accountMap[accountUuid]?.let { mapToAccountState(it) }
}
Expand All @@ -33,16 +29,23 @@ class InMemoryAccountStore(
accountUuid: String,
isIncoming: Boolean,
serverSettings: ServerSettings,
authorizationState: AuthorizationState?,
): AccountUpdaterResult {
return if (!accountMap.containsKey(accountUuid)) {
AccountUpdaterResult.Failure(AccountUpdaterFailure.AccountNotFound(accountUuid))
} else {
val account = accountMap[accountUuid]!!

accountMap[account.uuid] = if (isIncoming) {
account.copy(incomingServerSettings = serverSettings)
account.copy(
incomingServerSettings = serverSettings,
authorizationState = authorizationState?.state,
)
} else {
account.copy(outgoingServerSettings = serverSettings)
account.copy(
outgoingServerSettings = serverSettings,
authorizationState = authorizationState?.state,
)
}

AccountUpdaterResult.Success(account.uuid)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.fsck.k9.account

import app.k9mail.core.common.mail.Protocols
import app.k9mail.feature.account.common.domain.entity.AuthorizationState
import app.k9mail.feature.account.edit.AccountEditExternalContract
import app.k9mail.feature.account.edit.AccountEditExternalContract.AccountUpdaterFailure
import app.k9mail.feature.account.edit.AccountEditExternalContract.AccountUpdaterResult
Expand All @@ -26,10 +27,11 @@ class AccountServerSettingsUpdater(
accountUuid: String,
isIncoming: Boolean,
serverSettings: ServerSettings,
authorizationState: AuthorizationState?,
): AccountUpdaterResult {
return try {
withContext(coroutineDispatcher) {
updateSettings(accountUuid, isIncoming, serverSettings)
updateSettings(accountUuid, isIncoming, serverSettings, authorizationState)
}
} catch (error: Exception) {
Timber.e(error, "Error while updating account server settings with UUID %s", accountUuid)
Expand All @@ -42,6 +44,7 @@ class AccountServerSettingsUpdater(
accountUuid: String,
isIncoming: Boolean,
serverSettings: ServerSettings,
authorizationState: AuthorizationState?,
): AccountUpdaterResult {
val account = accountManager.getAccount(accountUuid = accountUuid) ?: return AccountUpdaterResult.Failure(
AccountUpdaterFailure.AccountNotFound(accountUuid),
Expand All @@ -64,6 +67,8 @@ class AccountServerSettingsUpdater(
account.outgoingServerSettings = serverSettings
}

account.oAuthState = authorizationState?.state

accountManager.saveAccount(account)

return AccountUpdaterResult.Success(accountUuid)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.fsck.k9.account

import app.k9mail.feature.account.common.domain.entity.AuthorizationState
import app.k9mail.feature.account.edit.AccountEditExternalContract.AccountUpdaterFailure
import app.k9mail.feature.account.edit.AccountEditExternalContract.AccountUpdaterResult
import assertk.all
Expand All @@ -15,7 +16,7 @@ import kotlinx.coroutines.test.runTest
import org.junit.Test
import com.fsck.k9.Account as K9Account

class AccountUpdaterTest {
class AccountServerSettingsUpdaterTest {

@Test
fun `updateServerSettings() SHOULD return account not found exception WHEN none present with uuid`() = runTest {
Expand All @@ -26,6 +27,7 @@ class AccountUpdaterTest {
accountUuid = ACCOUNT_UUID,
isIncoming = true,
serverSettings = INCOMING_SERVER_SETTINGS,
authorizationState = AUTHORIZATION_STATE,
)

assertThat(result).isEqualTo(AccountUpdaterResult.Failure(AccountUpdaterFailure.AccountNotFound(ACCOUNT_UUID)))
Expand All @@ -35,12 +37,14 @@ class AccountUpdaterTest {
fun `updateServerSettings() SHOULD return success with updated incoming settings WHEN is incoming`() = runTest {
val accountManager = FakeAccountManager(accounts = mutableMapOf(ACCOUNT_UUID to createAccount(ACCOUNT_UUID)))
val updatedIncomingServerSettings = INCOMING_SERVER_SETTINGS.copy(port = 123)
val updatedAuthorizationState = AuthorizationState("new")
val testSubject = AccountServerSettingsUpdater(accountManager)

val result = testSubject.updateServerSettings(
accountUuid = ACCOUNT_UUID,
isIncoming = true,
serverSettings = updatedIncomingServerSettings,
authorizationState = updatedAuthorizationState,
)

assertThat(result).isEqualTo(AccountUpdaterResult.Success(ACCOUNT_UUID))
Expand All @@ -49,19 +53,22 @@ class AccountUpdaterTest {
assertThat(k9Account).isNotNull().all {
prop(K9Account::incomingServerSettings).isEqualTo(updatedIncomingServerSettings)
prop(K9Account::outgoingServerSettings).isEqualTo(OUTGOING_SERVER_SETTINGS)
prop(K9Account::oAuthState).isEqualTo(updatedAuthorizationState.state)
}
}

@Test
fun `updateServerSettings() SHOULD return success with updated outgoing settings WHEN is not incoming`() = runTest {
val accountManager = FakeAccountManager(accounts = mutableMapOf(ACCOUNT_UUID to createAccount(ACCOUNT_UUID)))
val updatedOutgoingServerSettings = OUTGOING_SERVER_SETTINGS.copy(port = 123)
val updatedAuthorizationState = AuthorizationState("new")
val testSubject = AccountServerSettingsUpdater(accountManager)

val result = testSubject.updateServerSettings(
accountUuid = ACCOUNT_UUID,
isIncoming = false,
serverSettings = updatedOutgoingServerSettings,
authorizationState = updatedAuthorizationState,
)

assertThat(result).isEqualTo(AccountUpdaterResult.Success(ACCOUNT_UUID))
Expand All @@ -70,6 +77,7 @@ class AccountUpdaterTest {
assertThat(k9Account).isNotNull().all {
prop(K9Account::incomingServerSettings).isEqualTo(INCOMING_SERVER_SETTINGS)
prop(K9Account::outgoingServerSettings).isEqualTo(updatedOutgoingServerSettings)
prop(K9Account::oAuthState).isEqualTo(updatedAuthorizationState.state)
}
}

Expand All @@ -85,6 +93,7 @@ class AccountUpdaterTest {
accountUuid = ACCOUNT_UUID,
isIncoming = true,
serverSettings = INCOMING_SERVER_SETTINGS,
authorizationState = AUTHORIZATION_STATE,
)

assertThat(result).isInstanceOf<AccountUpdaterResult.Failure>()
Expand Down Expand Up @@ -119,12 +128,15 @@ class AccountUpdaterTest {
extra = emptyMap(),
)

val AUTHORIZATION_STATE = AuthorizationState("auth state")

fun createAccount(accountUuid: String): K9Account {
return K9Account(
uuid = accountUuid,
).apply {
incomingServerSettings = INCOMING_SERVER_SETTINGS
outgoingServerSettings = OUTGOING_SERVER_SETTINGS
oAuthState = AUTHORIZATION_STATE.state
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package app.k9mail.feature.account.edit

import app.k9mail.feature.account.common.domain.entity.AuthorizationState
import com.fsck.k9.mail.ServerSettings

interface AccountEditExternalContract {
Expand All @@ -19,6 +20,7 @@ interface AccountEditExternalContract {
accountUuid: String,
isIncoming: Boolean,
serverSettings: ServerSettings,
authorizationState: AuthorizationState?,
): AccountUpdaterResult
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package app.k9mail.feature.account.edit.domain.usecase

import app.k9mail.feature.account.common.domain.entity.AccountState
import app.k9mail.feature.account.common.domain.entity.AuthorizationState
import app.k9mail.feature.account.edit.AccountEditExternalContract.AccountServerSettingsUpdater
import app.k9mail.feature.account.edit.AccountEditExternalContract.AccountUpdaterResult
import app.k9mail.feature.account.edit.domain.AccountEditDomainContract.UseCase
Expand All @@ -10,33 +12,37 @@ class SaveServerSettings(
private val serverSettingsUpdater: AccountServerSettingsUpdater,
) : UseCase.SaveServerSettings {
override suspend fun execute(accountUuid: String, isIncoming: Boolean) {
val serverSettings = loadServerSettings(accountUuid, isIncoming)
val accountState = getAccountState.execute(accountUuid)

val serverSettings = accountState.getServerSettings(isIncoming)
val authorizationState = accountState.authorizationState

if (serverSettings != null) {
updateServerSettings(accountUuid, isIncoming, serverSettings)
updateServerSettings(accountUuid, isIncoming, serverSettings, authorizationState)
} else {
error("Server settings not found")
}
}

private suspend fun loadServerSettings(accountUuid: String, isIncoming: Boolean): ServerSettings? {
val accountState = getAccountState.execute(accountUuid)
return if (isIncoming) {
accountState.incomingServerSettings
} else {
accountState.outgoingServerSettings
}
}

private suspend fun updateServerSettings(accountUuid: String, isIncoming: Boolean, serverSettings: ServerSettings) {
private suspend fun updateServerSettings(
accountUuid: String,
isIncoming: Boolean,
serverSettings: ServerSettings,
authorizationState: AuthorizationState?,
) {
val result = serverSettingsUpdater.updateServerSettings(
accountUuid = accountUuid,
isIncoming = isIncoming,
serverSettings = serverSettings,
authorizationState = authorizationState,
)

if (result is AccountUpdaterResult.Failure) {
error("Server settings update failed")
}
}

private fun AccountState.getServerSettings(isIncoming: Boolean): ServerSettings? {
return if (isIncoming) incomingServerSettings else outgoingServerSettings
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ class SaveServerSettingsTest {
var recordedAccountUuid: String? = null
var recordedIsIncoming: Boolean? = null
var recordedServerSettings: ServerSettings? = null
var recordedAuthorizationState: AuthorizationState? = null
val testSubject = SaveServerSettings(
getAccountState = { _ -> ACCOUNT_STATE },
serverSettingsUpdater = { accountUuid, isIncoming, serverSettings ->
serverSettingsUpdater = { accountUuid, isIncoming, serverSettings, authorizationState ->
recordedAccountUuid = accountUuid
recordedIsIncoming = isIncoming
recordedServerSettings = serverSettings
recordedAuthorizationState = authorizationState

AccountUpdaterResult.Success(accountUuid)
},
Expand All @@ -39,13 +41,14 @@ class SaveServerSettingsTest {
assertThat(recordedAccountUuid).isEqualTo(ACCOUNT_UUID)
assertThat(recordedIsIncoming).isEqualTo(true)
assertThat(recordedServerSettings).isEqualTo(INCOMING_SERVER_SETTINGS)
assertThat(recordedAuthorizationState).isEqualTo(AUTHORIZATION_STATE)
}

@Test
fun `should throw exception WHEN no incoming server settings present`() = runTest {
val testSubject = SaveServerSettings(
getAccountState = { _ -> ACCOUNT_STATE.copy(incomingServerSettings = null) },
serverSettingsUpdater = { accountUuid, _, _ ->
serverSettingsUpdater = { accountUuid, _, _, _ ->
AccountUpdaterResult.Success(accountUuid)
},
)
Expand All @@ -61,12 +64,14 @@ class SaveServerSettingsTest {
var recordedAccountUuid: String? = null
var recordedIsIncoming: Boolean? = null
var recordedServerSettings: ServerSettings? = null
var recordedAuthorizationState: AuthorizationState? = null
val testSubject = SaveServerSettings(
getAccountState = { _ -> ACCOUNT_STATE },
serverSettingsUpdater = { accountUuid, isIncoming, serverSettings ->
serverSettingsUpdater = { accountUuid, isIncoming, serverSettings, authorizationState ->
recordedAccountUuid = accountUuid
recordedIsIncoming = isIncoming
recordedServerSettings = serverSettings
recordedAuthorizationState = authorizationState

AccountUpdaterResult.Success(accountUuid)
},
Expand All @@ -77,13 +82,14 @@ class SaveServerSettingsTest {
assertThat(recordedAccountUuid).isEqualTo(ACCOUNT_UUID)
assertThat(recordedIsIncoming).isEqualTo(false)
assertThat(recordedServerSettings).isEqualTo(OUTGOING_SERVER_SETTINGS)
assertThat(recordedAuthorizationState).isEqualTo(AUTHORIZATION_STATE)
}

@Test
fun `should throw exception WHEN no outgoing server settings present`() = runTest {
val testSubject = SaveServerSettings(
getAccountState = { _ -> ACCOUNT_STATE.copy(outgoingServerSettings = null) },
serverSettingsUpdater = { accountUuid, _, _ ->
serverSettingsUpdater = { accountUuid, _, _, _ ->
AccountUpdaterResult.Success(accountUuid)
},
)
Expand All @@ -98,7 +104,7 @@ class SaveServerSettingsTest {
fun `should throw exception WHEN update failed`() = runTest {
val testSubject = SaveServerSettings(
getAccountState = { _ -> ACCOUNT_STATE },
serverSettingsUpdater = { _, _, _ ->
serverSettingsUpdater = { _, _, _, _ ->
AccountUpdaterResult.Failure(
AccountUpdaterFailure.AccountNotFound(ACCOUNT_UUID),
)
Expand Down