From ca97d95bd4424ec68085bc94db50d2e7f5881b44 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 00:23:25 +0200 Subject: [PATCH 01/14] refactor(chat): extract the optimistic action pattern into one helper Four call sites had each grown their own copy of "apply locally, retry once on a transient failure, revert when it finally fails, revert when cancelled", with the retry predicate spelled out twice and one of them shipping a guard bug. optimisticAction() now carries that shape, including the case where the server refuses in the payload rather than in the status code, and the case where an answer means the change was already applied - which must not revert anything. No behaviour change; the repository tests of reactions, deletions, edits, pinning and dismissals carry the migration. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../network/OfflineFirstChatRepository.kt | 130 ++++++------------ .../reactions/ReactionsRepositoryImpl.kt | 110 ++++----------- .../nextcloud/talk/utils/OptimisticAction.kt | 69 ++++++++++ 3 files changed, 133 insertions(+), 176 deletions(-) create mode 100644 app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index 5b871c2ce3e..0ed819ea84e 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -31,8 +31,7 @@ import com.nextcloud.talk.models.json.converters.EnumActorTypeConverter import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.participants.ParticipantDto import com.nextcloud.talk.utils.message.SendMessageUtils -import com.nextcloud.talk.utils.revertOnCancellation -import com.nextcloud.talk.utils.withRetry +import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay @@ -809,33 +808,24 @@ class OfflineFirstChatRepository @Inject constructor( messageId: Long, text: String ): Result { - val restore = applyLocalEdit(messageId, text) + val result = optimisticAction( + apply = { applyLocalEdit(messageId, text) }, + isConfirmed = ::editAccepted, + request = { network.editChatMessage(credentials, url, text) } + ).onFailure { logger.e(TAG, "Failed to edit chat message $messageId", it) } - return try { - val response = revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { - network.editChatMessage(credentials, url, text) - } - } - val statusCode = response.ocs?.meta?.statusCode - if (statusCode != null && statusCode != HTTP_OK) { - // the server refused the edit, e.g. because the message is too old - restore?.invoke() - } else { - persistEditedMessage(messageId, response) - } - Result.success(response) - } catch (e: HttpException) { - logger.e(TAG, "Failed to edit chat message $messageId", e) - restore?.invoke() - Result.failure(e) - } catch (e: IOException) { - logger.e(TAG, "Failed to edit chat message $messageId", e) - restore?.invoke() - Result.failure(e) - } + result.getOrNull()?.takeIf(::editAccepted)?.let { persistEditedMessage(messageId, it) } + + return result } + /** + * A status code other than 200 means the server refused the edit, for instance because the + * message is too old, and the optimistic edit has to be taken back. + */ + private fun editAccepted(response: ChatOverallSingleMessage): Boolean = + response.ocs?.meta?.statusCode.let { it == null || it == HTTP_OK } + /** * Writes [text] into the cached message together with the edit metadata the bubble shows, and * returns the action that restores the previous version. The revert only applies while the message @@ -893,32 +883,21 @@ class OfflineFirstChatRepository @Inject constructor( url: String, messageId: Long, deletedPlaceholder: String - ): Result { - val restore = applyLocalDeletion(messageId, deletedPlaceholder) - - return try { - val response = revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { + ): Result = + optimisticAction( + apply = { applyLocalDeletion(messageId, deletedPlaceholder) }, + request = { + try { network.deleteChatMessage(credentials, url) + } catch (e: HttpException) { + if (e.code() != HTTP_NOT_FOUND) throw e + // the server does not know the message any more, so it is gone either way and the + // local deletion stands + logger.w(TAG, "Message $messageId was already gone on the server when deleting it", e) + null } } - Result.success(response) - } catch (e: HttpException) { - if (e.code() == HTTP_NOT_FOUND) { - // the server does not know the message any more, so it is gone either way - logger.w(TAG, "Message $messageId was already gone on the server when deleting it", e) - Result.success(null) - } else { - logger.e(TAG, "Failed to delete chat message $messageId", e) - restore?.invoke() - Result.failure(e) - } - } catch (e: IOException) { - logger.e(TAG, "Failed to delete chat message $messageId", e) - restore?.invoke() - Result.failure(e) - } - } + ).onFailure { logger.e(TAG, "Failed to delete chat message $messageId", it) } /** * Renders the message as deleted in the local database and returns the action that puts it back, @@ -1019,12 +998,6 @@ class OfflineFirstChatRepository @Inject constructor( network.unPinMessage(credentials, url) } - private fun isRetryable(error: Exception): Boolean = - when (error) { - is HttpException -> error.code() == HTTP_TOO_MANY_REQUESTS || error.code() >= HTTP_INTERNAL_SERVER_ERROR - else -> error is IOException - } - private suspend fun withLocalPinnedMessage( messageId: Long, pinned: Boolean, @@ -1038,23 +1011,14 @@ class OfflineFirstChatRepository @Inject constructor( null } - return try { - val overall = revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { request() } + return optimisticAction(apply = { restore }, request = request) + .onSuccess { + // the room refresh that follows is computed after this change reached the server, so + // the guard has done its job and must not outlive the request + conversationListUpdater.clearPendingPinnedMessage(internalConversationId) } - // the room refresh that follows is computed after this change reached the server, so the - // guard has done its job and must not outlive the request - conversationListUpdater.clearPendingPinnedMessage(internalConversationId) - Result.success(overall.ocs?.data?.toDomainModel()) - } catch (e: HttpException) { - Log.e(TAG, "Error while pinning or unpinning a message: $e") - restore?.invoke() - Result.failure(e) - } catch (e: IOException) { - Log.e(TAG, "Error while pinning or unpinning a message: $e") - restore?.invoke() - Result.failure(e) - } + .onFailure { logger.e(TAG, "Error while pinning or unpinning a message", it) } + .map { it.ocs?.data?.toDomainModel() } } private fun isChatDataInitialized(): Boolean = @@ -1068,23 +1032,10 @@ class OfflineFirstChatRepository @Inject constructor( null } - return try { - revertOnCancellation({ restore?.invoke() }) { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { - network.hidePinnedMessage(credentials, url) - } - } - conversationListUpdater.clearPendingHiddenPinnedMessage(internalConversationId) - Result.success(true) - } catch (e: HttpException) { - Log.e(TAG, "Error while hiding the pinned message: $e") - restore?.invoke() - Result.failure(e) - } catch (e: IOException) { - Log.e(TAG, "Error while hiding the pinned message: $e") - restore?.invoke() - Result.failure(e) - } + return optimisticAction(apply = { restore }, request = { network.hidePinnedMessage(credentials, url) }) + .onSuccess { conversationListUpdater.clearPendingHiddenPinnedMessage(internalConversationId) } + .onFailure { logger.e(TAG, "Error while hiding the pinned message", it) } + .map { true } } override suspend fun onSignalingChatMessageReceived(chatMessages: List) { @@ -1301,8 +1252,5 @@ class OfflineFirstChatRepository @Inject constructor( private const val MESSAGE_TYPE_DELETED = "comment_deleted" private const val HTTP_OK = 200 private const val HTTP_NOT_FOUND = 404 - private const val HTTP_TOO_MANY_REQUESTS = 429 - private const val HTTP_INTERNAL_SERVER_ERROR = 500 - private const val RETRY_DELAY_MS = 500L } } diff --git a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt index 46c683df547..9a01933cc3e 100644 --- a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt +++ b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt @@ -12,12 +12,10 @@ import com.nextcloud.talk.chat.data.model.ChatMessage import com.nextcloud.talk.data.database.dao.ChatMessagesDao import com.nextcloud.talk.models.domain.ReactionAddedModel import com.nextcloud.talk.models.domain.ReactionDeletedModel -import com.nextcloud.talk.utils.revertOnCancellation -import com.nextcloud.talk.utils.withRetry +import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import retrofit2.HttpException -import java.io.IOException import javax.inject.Inject /** @@ -45,23 +43,14 @@ class ReactionsRepositoryImpl @Inject constructor( val internalConversationId = "$userId@$roomToken" val messageId = message.jsonMessageId.toLong() - val applied = applyLocalAdd(internalConversationId, messageId, emoji) - val revert: suspend () -> Unit = { - if (applied) applyLocalRemove(internalConversationId, messageId, emoji) - } - - val confirmed = revertOnCancellation(revert) { - requestSucceeds( - successStatusCodes = ADD_SUCCESS_CODES, - alreadyAppliedHttpCodes = emptySet() - ) { - ncApiCoroutines.sendReaction(credentials, url, emoji).ocs?.meta?.statusCode - } - } - - if (!confirmed) { - revert() - } + val confirmed = optimisticAction( + apply = { + val applied = applyLocalAdd(internalConversationId, messageId, emoji) + revertWith(applied) { applyLocalRemove(internalConversationId, messageId, emoji) } + }, + isConfirmed = { statusCode -> statusCode in ADD_SUCCESS_CODES }, + request = { ncApiCoroutines.sendReaction(credentials, url, emoji).ocs?.meta?.statusCode } + ).logFailure().getOrNull() in ADD_SUCCESS_CODES return ReactionAddedModel(message, emoji, confirmed) } @@ -77,77 +66,32 @@ class ReactionsRepositoryImpl @Inject constructor( val internalConversationId = "$userId@$roomToken" val messageId = message.jsonMessageId.toLong() - val applied = applyLocalRemove(internalConversationId, messageId, emoji) - val revert: suspend () -> Unit = { - if (applied) applyLocalAdd(internalConversationId, messageId, emoji) - } - - val confirmed = revertOnCancellation(revert) { - requestSucceeds( - successStatusCodes = DELETE_SUCCESS_CODES, - alreadyAppliedHttpCodes = DELETE_ALREADY_APPLIED_CODES - ) { - ncApiCoroutines.deleteReaction(credentials, url, emoji).ocs?.meta?.statusCode - } - } - - if (!confirmed) { - revert() - } + val confirmed = optimisticAction( + apply = { + val applied = applyLocalRemove(internalConversationId, messageId, emoji) + revertWith(applied) { applyLocalAdd(internalConversationId, messageId, emoji) } + }, + isConfirmed = { statusCode -> statusCode in DELETE_SUCCESS_CODES }, + request = { deleteReactionStatusCode(credentials, url, emoji) } + ).logFailure().getOrNull() in DELETE_SUCCESS_CODES return ReactionDeletedModel(message, emoji, confirmed) } - private suspend fun requestSucceeds( - successStatusCodes: Set, - alreadyAppliedHttpCodes: Set, - call: suspend () -> Int? - ): Boolean = - try { - withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isRetryable) { - attemptRequest(successStatusCodes, alreadyAppliedHttpCodes, call) - } - } catch (e: IOException) { - Log.w(TAG, "Reaction request failed, the retry failed as well: $e") - false - } catch (e: HttpException) { - Log.w(TAG, "Reaction request failed with HTTP ${e.code()}, the retry failed as well: $e") - false - } - /** - * Returns whether the server applied the reaction, and throws for a failure that is worth another - * attempt: a connection problem, a rate limit or a server error. + * A reaction the server no longer knows is deleted as far as this call is concerned. */ - private suspend fun attemptRequest( - successStatusCodes: Set, - alreadyAppliedHttpCodes: Set, - call: suspend () -> Int? - ): Boolean = + private suspend fun deleteReactionStatusCode(credentials: String?, url: String, emoji: String): Int? = try { - val statusCode = call() - if (statusCode in successStatusCodes) { - true - } else { - Log.w(TAG, "Reaction request answered with unexpected status code $statusCode") - false - } + ncApiCoroutines.deleteReaction(credentials, url, emoji).ocs?.meta?.statusCode } catch (e: HttpException) { - when { - e.code() in alreadyAppliedHttpCodes -> true - isRetryable(e) -> throw e - else -> { - Log.w(TAG, "Reaction request rejected with HTTP ${e.code()}: $e") - false - } - } + if (e.code() == HTTP_NOT_FOUND) HTTP_OK else throw e } - private fun isRetryable(error: Exception): Boolean = - when (error) { - is HttpException -> error.code() == HTTP_TOO_MANY_REQUESTS || error.code() >= HTTP_INTERNAL_SERVER_ERROR - else -> error is IOException - } + private fun revertWith(applied: Boolean, revert: suspend () -> Unit): (suspend () -> Unit)? = + revert.takeIf { applied } + + private fun Result.logFailure(): Result = onFailure { Log.w(TAG, "Reaction request failed: $it") } /** * Adds the reaction to the cached message and reports whether that changed anything. A reaction the @@ -191,12 +135,8 @@ class ReactionsRepositoryImpl @Inject constructor( private const val HTTP_OK: Int = 200 private const val HTTP_CREATED: Int = 201 private const val HTTP_NOT_FOUND: Int = 404 - private const val HTTP_TOO_MANY_REQUESTS: Int = 429 - private const val HTTP_INTERNAL_SERVER_ERROR: Int = 500 - private const val RETRY_DELAY_MS: Long = 500 private val ADD_SUCCESS_CODES = setOf(HTTP_OK, HTTP_CREATED) private val DELETE_SUCCESS_CODES = setOf(HTTP_OK) - private val DELETE_ALREADY_APPLIED_CODES = setOf(HTTP_NOT_FOUND) } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt new file mode 100644 index 00000000000..856d93d77f5 --- /dev/null +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -0,0 +1,69 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.utils + +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.withContext +import retrofit2.HttpException +import java.io.IOException + +private const val HTTP_TOO_MANY_REQUESTS = 429 +private const val HTTP_INTERNAL_SERVER_ERROR = 500 +private const val RETRY_DELAY_MS = 500L + +/** + * Whether a failed request is worth another attempt: a connection problem, a rate limit or a server + * error. An answer the server actually gave - a refusal, a permission error - is not. + */ +fun isTransientFailure(error: Exception): Boolean = + when (error) { + is HttpException -> error.code() == HTTP_TOO_MANY_REQUESTS || error.code() >= HTTP_INTERNAL_SERVER_ERROR + else -> error is IOException + } + +/** + * Applies a change locally, has the server confirm it and undoes it when that fails, which is how + * every user action that the client can predict should behave: the screen reacts to the tap, the + * request follows, and only a request that finally fails takes the change back. + * + * [apply] writes the local state and returns the action that undoes it again, or null when there was + * nothing to change - in which case nothing is undone later either. [request] is retried once for a + * transient failure. [isConfirmed] decides whether an answer that did not throw still counts as a + * refusal, for endpoints that report one in the payload rather than in the status code. + * + * Cancellation is part of the contract: closing the screen while the request is in flight undoes the + * change as well, instead of leaving it applied although the server may never have heard of it. + */ +suspend fun optimisticAction( + apply: suspend () -> (suspend () -> Unit)?, + isConfirmed: (T) -> Boolean = { true }, + request: suspend () -> T +): Result { + val revert = apply() + // a revert that is itself cancelled would leave the change applied, which is the very thing it + // is there to prevent + val revertUninterruptibly: suspend () -> Unit = { withContext(NonCancellable) { revert?.invoke() } } + + return try { + val answer = revertOnCancellation(revertUninterruptibly) { + withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isTransientFailure) { + request() + } + } + if (!isConfirmed(answer)) { + revertUninterruptibly() + } + Result.success(answer) + } catch (e: HttpException) { + revertUninterruptibly() + Result.failure(e) + } catch (e: IOException) { + revertUninterruptibly() + Result.failure(e) + } +} From 424a03e73399a7fe6eb052b7a385ec443836b10c Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 00:55:21 +0200 Subject: [PATCH 02/14] perf(conversations): keep list and info actions correct when they fail Marking a conversation read or unread, favoriting, archiving and the important and sensitive toggles now go through the shared optimistic helper, so every one of them retries a connection problem once and takes its change back when the screen is left mid-request. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../viewmodel/ConversationInfoViewModel.kt | 164 ++++++++++++--- .../viewmodels/ConversationsListViewModel.kt | 187 ++++++++++-------- .../nextcloud/talk/utils/OptimisticAction.kt | 12 +- .../ConversationInfoViewModelTest.kt | 182 +++++++++++++++++ .../viewmodel/FakeConversationsRepository.kt | 36 +++- 5 files changed, 464 insertions(+), 117 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index d4c27267bac..9422d12ecaa 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -6,6 +6,7 @@ */ package com.nextcloud.talk.conversationinfo.viewmodel import android.util.Log +import androidx.annotation.VisibleForTesting import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LiveData @@ -51,6 +52,7 @@ import com.nextcloud.talk.utils.DateConstants import com.nextcloud.talk.utils.DisplayUtils import com.nextcloud.talk.utils.ParticipantRoleUtils import com.nextcloud.talk.utils.SpreedFeatures +import com.nextcloud.talk.utils.optimisticAction import com.nextcloud.talk.utils.preferences.preferencestorage.DatabaseStorageModule import io.reactivex.Observer import io.reactivex.android.schedulers.AndroidSchedulers @@ -409,6 +411,8 @@ class ConversationInfoViewModel @Inject constructor( val showSensitiveConversation = hasSpreedFeatureCapability(spreedCapabilities, SpreedFeatures.SENSITIVE_CONVERSATIONS) val sensitiveConversation = conversationModel.hasSensitive + confirmedSetting[IMPORTANT] = importantConversation + confirmedSetting[SENSITIVE] = sensitiveConversation val showArchiveConversation = hasSpreedFeatureCapability(spreedCapabilities, SpreedFeatures.ARCHIVE_CONVERSATIONS) @@ -868,43 +872,149 @@ class ConversationInfoViewModel @Inject constructor( _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbarText(text)) } + private var settingChanges = 0L + + /** The newest change started for each setting, so an older one cannot undo it. */ + private val newestSettingChange = mutableMapOf() + + /** The order of the newest change the server has accepted for each setting. */ + private val lastAcceptedChange = mutableMapOf() + + /** + * The value the server last confirmed for each setting. An undo restores this rather than + * whatever was on screen when its own change started: two changes that both fail would + * otherwise leave the newer undo restoring the optimistic value of the older one. + */ + private val confirmedSetting = mutableMapOf() + + /** + * Stamps a new change to [setting] and returns its order, so everything that happens afterwards + * can tell whether it is still the newest change the user made to that setting. + */ + @VisibleForTesting + internal fun startSettingChange(setting: String): Long { + val change = ++settingChanges + newestSettingChange[setting] = change + return change + } + + /** + * The undo for [change], which only runs while no later change to [setting] has been started. + * Comparing the displayed value is not enough: a user who picks A, B, A, B leaves the first B + * holding a value that matches the screen again, and its failure would put A back over the B the + * server has since accepted. + */ + @VisibleForTesting + internal fun undoIfNewest( + setting: String, + change: Long, + undo: (ConversationInfoUiState) -> ConversationInfoUiState + ): suspend () -> Unit = { if (isNewestChange(setting, change)) _uiState.update(undo) } + + private fun isNewestChange(setting: String, change: Long) = newestSettingChange[setting] == change + + /** + * An OCS payload can report a refusal in a response the HTTP layer calls a success, so the status + * code decides whether the optimistic change stands. A payload that carries no status code at all + * is taken at face value, since there is nothing in it to refuse with. + */ + private fun accepted(response: GenericOverall): Boolean = + response.ocs?.meta?.statusCode.let { it == null || it == NO_STATUS_CODE || it == HTTP_OK } + @Suppress("Detekt.TooGenericExceptionCaught") fun toggleImportantConversation(credentials: String, baseUrl: String, roomToken: String) { - val previousValue = _uiState.value.importantConversation - val newValue = !previousValue - _uiState.update { it.copy(importantConversation = newValue) } + val displayedValue = _uiState.value.importantConversation + val newValue = !displayedValue + // the first change of a session is made from the value the screen was built with, + // which is the one the server last gave us + val confirmedValue = confirmedSetting.getOrPut(IMPORTANT) { displayedValue } + val change = startSettingChange(IMPORTANT) + viewModelScope.launch { - try { - if (newValue) { - conversationsRepository.markConversationAsImportant(credentials, baseUrl, roomToken) - } else { - conversationsRepository.markConversationAsUnImportant(credentials, baseUrl, roomToken) + val outcome = optimisticAction( + apply = { + _uiState.update { it.copy(importantConversation = newValue) } + undoIfNewest(IMPORTANT, change) { + it.copy(importantConversation = confirmedSetting[IMPORTANT] ?: confirmedValue) + } + }, + isConfirmed = ::accepted, + request = { + if (newValue) { + conversationsRepository.markConversationAsImportant(credentials, baseUrl, roomToken) + } else { + conversationsRepository.markConversationAsUnImportant(credentials, baseUrl, roomToken) + } } - } catch (exception: Exception) { - _uiState.update { it.copy(importantConversation = previousValue) } - logger.e(TAG, "failed to toggle important conversation state", exception) - _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) - } + ) + reportSettingOutcome(outcome, IMPORTANT, change, newValue, "important") } } @Suppress("Detekt.TooGenericExceptionCaught") fun toggleSensitiveConversation(credentials: String, baseUrl: String, roomToken: String) { - val previousValue = _uiState.value.sensitiveConversation - val newValue = !previousValue - _uiState.update { it.copy(sensitiveConversation = newValue) } + val displayedValue = _uiState.value.sensitiveConversation + val newValue = !displayedValue + // the first change of a session is made from the value the screen was built with, + // which is the one the server last gave us + val confirmedValue = confirmedSetting.getOrPut(SENSITIVE) { displayedValue } + val change = startSettingChange(SENSITIVE) + viewModelScope.launch { - try { - if (newValue) { - conversationsRepository.markConversationAsSensitive(credentials, baseUrl, roomToken) - } else { - conversationsRepository.markConversationAsInsensitive(credentials, baseUrl, roomToken) + val outcome = optimisticAction( + apply = { + _uiState.update { it.copy(sensitiveConversation = newValue) } + undoIfNewest(SENSITIVE, change) { + it.copy(sensitiveConversation = confirmedSetting[SENSITIVE] ?: confirmedValue) + } + }, + isConfirmed = ::accepted, + request = { + if (newValue) { + conversationsRepository.markConversationAsSensitive(credentials, baseUrl, roomToken) + } else { + conversationsRepository.markConversationAsInsensitive(credentials, baseUrl, roomToken) + } } - } catch (exception: Exception) { - _uiState.update { it.copy(sensitiveConversation = previousValue) } - logger.e(TAG, "failed to toggle sensitive conversation state", exception) - _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) + ) + reportSettingOutcome(outcome, SENSITIVE, change, newValue, "sensitive") + } + } + + /** + * Records [newValue] as confirmed for [setting] when the server accepted it, and tells the user + * when it did not. A refusal the server reports in the payload leaves [optimisticAction] + * returning a success it has already taken back, so it needs reporting here as much as a + * request that threw. + * + * The baseline follows the newest change the server has *accepted*, not the newest the user has + * started. Two requests for one setting can be in flight at once and be accepted out of order, so + * an older acceptance arriving last must not overwrite a newer one - but while the newer one is + * still in flight, the older acceptance is the last thing the server agreed to, and dropping it + * would leave a later failure restoring a value the server no longer holds. + */ + @Suppress("Detekt.LongParameterList") + private suspend fun reportSettingOutcome( + outcome: Result, + setting: String, + change: Long, + newValue: Boolean, + description: String + ) { + val response = outcome.getOrNull() + if (response != null && accepted(response)) { + if (change > (lastAcceptedChange[setting] ?: 0)) { + lastAcceptedChange[setting] = change + confirmedSetting[setting] = newValue } + return + } + _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) + val cause = outcome.exceptionOrNull() + if (cause != null) { + logger.e(TAG, "failed to toggle $description conversation state", cause) + } else { + logger.e(TAG, "server refused to toggle $description conversation state") } } @@ -923,8 +1033,12 @@ class ConversationInfoViewModel @Inject constructor( companion object { private val TAG = ConversationInfoViewModel::class.java.simpleName + private const val HTTP_OK = 200 + private const val NO_STATUS_CODE = 0 private const val NEW_CONVERSATION_PARTICIPANTS_SEPARATOR = ", " private const val EXTENDED_CONVERSATION = "extended_conversation" + private const val IMPORTANT = "important" + private const val SENSITIVE = "sensitive" private const val GROUP_CONVERSATION_TYPE = "2" private const val MAX_ROOM_NAME_LENGTH = 255 private const val NOTIFICATION_LEVEL_ALWAYS: Int = 1 diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 29082a46a5a..331b32217c1 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -42,7 +42,7 @@ import com.nextcloud.talk.utils.ApiUtils import com.nextcloud.talk.utils.CapabilitiesUtil.hasSpreedFeatureCapability import com.nextcloud.talk.utils.SpreedFeatures import com.nextcloud.talk.utils.UserIdUtils -import com.nextcloud.talk.utils.withRetry +import com.nextcloud.talk.utils.optimisticAction import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject @@ -868,23 +868,24 @@ class ConversationsListViewModel @AssistedInject constructor( ) val url = ApiUtils.getUrlForChatReadMarker(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - messageId?.let { conversationListUpdater.markPendingReadMarker(conversation.internalId, it) } - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.markConversationAsRead(credentials, url, messageId) } - } - _readUnreadState.value = ConversationReadUnreadUiState.Success - } catch (e: Exception) { - logger.e(TAG, "Failed to mark conversation as read", e) - messageId?.let { conversationListUpdater.clearPendingReadMarker(conversation.internalId, it) } - withContext(Dispatchers.IO) { - repository.updateConversation(original) + val result = optimisticAction( + apply = { + messageId?.let { conversationListUpdater.markPendingReadMarker(conversation.internalId, it) } + applyLocally(optimistic) + revertTo(original) { + messageId?.let { conversationListUpdater.clearPendingReadMarker(conversation.internalId, it) } + } + }, + request = { conversationsRepository.markConversationAsRead(credentials, url, messageId) } + ) + + _readUnreadState.value = result.fold( + onSuccess = { ConversationReadUnreadUiState.Success }, + onFailure = { + logger.e(TAG, "Failed to mark conversation as read", it) + ConversationReadUnreadUiState.Error } - _readUnreadState.value = ConversationReadUnreadUiState.Error - } + ) } } @@ -898,26 +899,39 @@ class ConversationsListViewModel @AssistedInject constructor( ) val url = ApiUtils.getUrlForChatReadMarker(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingUnread(conversation.internalId) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.markConversationAsUnread(credentials, url) } - } - _readUnreadState.value = ConversationReadUnreadUiState.Success - } catch (e: Exception) { - logger.e(TAG, "Failed to mark conversation as unread", e) - conversationListUpdater.clearPendingUnread(conversation.internalId) - withContext(Dispatchers.IO) { - repository.updateConversation(original) + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingUnread(conversation.internalId) + applyLocally(optimistic) + revertTo(original) { conversationListUpdater.clearPendingUnread(conversation.internalId) } + }, + request = { conversationsRepository.markConversationAsUnread(credentials, url) } + ) + + _readUnreadState.value = result.fold( + onSuccess = { ConversationReadUnreadUiState.Success }, + onFailure = { + logger.e(TAG, "Failed to mark conversation as unread", it) + ConversationReadUnreadUiState.Error } - _readUnreadState.value = ConversationReadUnreadUiState.Error - } + ) } } + private suspend fun applyLocally(conversation: ConversationModel) { + withContext(Dispatchers.IO) { repository.updateConversation(conversation) } + } + + /** + * The action that puts [original] back and releases whatever guard the caller registered, for + * [optimisticAction] to run when the request finally fails or the screen is left mid-request. + */ + private fun revertTo(original: ConversationModel, releaseGuard: () -> Unit): suspend () -> Unit = + { + releaseGuard() + applyLocally(original) + } + fun resetFavoriteState() { _favoriteState.value = FavoriteUiState.None } @@ -934,29 +948,30 @@ class ConversationsListViewModel @AssistedInject constructor( val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1)) val url = ApiUtils.getUrlForArchive(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingArchived(conversation.internalId, desiredArchived) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { - if (desiredArchived) { - conversationsRepository.archiveConversation(credentials, url) - } else { - conversationsRepository.unarchiveConversation(credentials, url) - } + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingArchived(conversation.internalId, desiredArchived) + applyLocally(optimistic) + revertTo(original) { + conversationListUpdater.clearPendingArchived(conversation.internalId, desiredArchived) + } + }, + request = { + if (desiredArchived) { + conversationsRepository.archiveConversation(credentials, url) + } else { + conversationsRepository.unarchiveConversation(credentials, url) } } - _archiveState.value = ArchiveUiState.Success(desiredArchived, conversation.displayName) - } catch (e: Exception) { - logger.e(TAG, "Failed to toggle conversation archive state", e) - conversationListUpdater.clearPendingArchived(conversation.internalId, desiredArchived) - withContext(Dispatchers.IO) { - repository.updateConversation(original) + ) + + _archiveState.value = result.fold( + onSuccess = { ArchiveUiState.Success(desiredArchived, conversation.displayName) }, + onFailure = { + logger.e(TAG, "Failed to toggle conversation archive state", it) + ArchiveUiState.Error } - _archiveState.value = ArchiveUiState.Error - } + ) } } @@ -967,23 +982,24 @@ class ConversationsListViewModel @AssistedInject constructor( val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1)) val url = ApiUtils.getUrlForRoomFavorite(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = true) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.addConversationToFavorites(credentials, url) } - } - _favoriteState.value = FavoriteUiState.Success - } catch (e: Exception) { - logger.e(TAG, "Failed to add conversation to favorites", e) - conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = true) - withContext(Dispatchers.IO) { - repository.updateConversation(original) + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = true) + applyLocally(optimistic) + revertTo(original) { + conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = true) + } + }, + request = { conversationsRepository.addConversationToFavorites(credentials, url) } + ) + + _favoriteState.value = result.fold( + onSuccess = { FavoriteUiState.Success }, + onFailure = { + logger.e(TAG, "Failed to add conversation to favorites", it) + FavoriteUiState.Error } - _favoriteState.value = FavoriteUiState.Error - } + ) } } @@ -994,23 +1010,24 @@ class ConversationsListViewModel @AssistedInject constructor( val apiVersion = ApiUtils.getConversationApiVersion(currentUser, intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1)) val url = ApiUtils.getUrlForRoomFavorite(apiVersion, currentUser.baseUrl, conversation.token) viewModelScope.launch { - conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = false) - withContext(Dispatchers.IO) { - repository.updateConversation(optimistic) - } - try { - withContext(Dispatchers.IO) { - withRetry(1) { conversationsRepository.removeConversationFromFavorites(credentials, url) } - } - _favoriteState.value = FavoriteUiState.Success - } catch (e: Exception) { - logger.e(TAG, "Failed to remove conversation from favorites", e) - conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = false) - withContext(Dispatchers.IO) { - repository.updateConversation(original) + val result = optimisticAction( + apply = { + conversationListUpdater.markPendingFavorite(conversation.internalId, favorite = false) + applyLocally(optimistic) + revertTo(original) { + conversationListUpdater.clearPendingFavorite(conversation.internalId, favorite = false) + } + }, + request = { conversationsRepository.removeConversationFromFavorites(credentials, url) } + ) + + _favoriteState.value = result.fold( + onSuccess = { FavoriteUiState.Success }, + onFailure = { + logger.e(TAG, "Failed to remove conversation from favorites", it) + FavoriteUiState.Error } - _favoriteState.value = FavoriteUiState.Error - } + ) } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt index 856d93d77f5..6637c223132 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -7,6 +7,7 @@ package com.nextcloud.talk.utils +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.withContext import retrofit2.HttpException @@ -34,11 +35,13 @@ fun isTransientFailure(error: Exception): Boolean = * [apply] writes the local state and returns the action that undoes it again, or null when there was * nothing to change - in which case nothing is undone later either. [request] is retried once for a * transient failure. [isConfirmed] decides whether an answer that did not throw still counts as a - * refusal, for endpoints that report one in the payload rather than in the status code. + * refusal, for endpoints that report one in the payload rather than in the status code. A request + * that fails for any other reason is not retried, but the change is still taken back. * * Cancellation is part of the contract: closing the screen while the request is in flight undoes the * change as well, instead of leaving it applied although the server may never have heard of it. */ +@Suppress("TooGenericExceptionCaught") suspend fun optimisticAction( apply: suspend () -> (suspend () -> Unit)?, isConfirmed: (T) -> Boolean = { true }, @@ -59,10 +62,9 @@ suspend fun optimisticAction( revertUninterruptibly() } Result.success(answer) - } catch (e: HttpException) { - revertUninterruptibly() - Result.failure(e) - } catch (e: IOException) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { revertUninterruptibly() Result.failure(e) } diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt index c7250b4be1f..11133ca9b73 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt @@ -9,13 +9,16 @@ package com.nextcloud.talk.conversationinfo.viewmodel import com.nextcloud.talk.api.NcApi import com.nextcloud.talk.chat.data.network.ChatNetworkDataSource +import com.nextcloud.talk.conversationinfo.ConversationInfoUiEvent import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.logger.Logger import com.nextcloud.talk.models.json.capabilities.CapabilitiesDto import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto import com.nextcloud.talk.repositories.passwordpolicy.PasswordPolicyRepository import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest @@ -23,6 +26,7 @@ import kotlinx.coroutines.test.setMain import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.mockito.kotlin.mock @@ -110,6 +114,184 @@ class ConversationInfoViewModelTest { assertFalse(model.uiState.value.guestsAllowed) } + @Test + fun `an important toggle that the server never accepts goes back to what it was`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingImportantRequests = Int.MAX_VALUE } + val model = viewModel(repository) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.runCurrent() + assertEquals(true, model.uiState.value.importantConversation) + + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.importantConversation) + } + + @Test + fun `an important toggle survives a single connection problem`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingImportantRequests = 1 } + val model = viewModel(repository) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertEquals(2, repository.importantRequests) + assertEquals(true, model.uiState.value.importantConversation) + } + + // undoIfNewest(): a setting can have several changes in flight at once, and an older one failing + // must not put its value back over a newer one the server already accepted. The rule is asserted + // on the guard itself: a two-state toggle cannot show the difference, because the older change's + // undo value is the newer change's applied value, and the multi-value settings read their labels + // from Android resources that a plain unit test has no access to. + + @Test + fun `the newest change to a setting can be undone`() = + runTest(dispatcher) { + val model = viewModel(FakeConversationsRepository()) + + val change = model.startSettingChange("level") + + model.undoIfNewest("level", change) { it.copy(notificationLevel = "restored") }.invoke() + + assertEquals("restored", model.uiState.value.notificationLevel) + } + + @Test + fun `an older change cannot undo a newer one, even back to the same value`() = + runTest(dispatcher) { + val model = viewModel(FakeConversationsRepository()) + val levelBefore = model.uiState.value.notificationLevel + + // A, B, A, B on one setting: the first B fails last, and the screen shows B again + val first = model.startSettingChange("level") + val firstUndo = model.undoIfNewest("level", first) { it.copy(notificationLevel = "restored") } + model.startSettingChange("level") + firstUndo.invoke() + + assertEquals(levelBefore, model.uiState.value.notificationLevel) + } + + @Test + fun `a change to one setting does not hold back the undo of another`() = + runTest(dispatcher) { + val model = viewModel(FakeConversationsRepository()) + + val level = model.startSettingChange("level") + val levelUndo = model.undoIfNewest("level", level) { it.copy(notificationLevel = "restored") } + model.startSettingChange("expiration") + levelUndo.invoke() + + assertEquals("restored", model.uiState.value.notificationLevel) + } + + @Test + fun `a toggle the server refuses in the payload is taken back`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { importantResponseStatusCode = HTTP_FORBIDDEN } + val model = viewModel(repository) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.importantConversation) + } + + @Test + fun `a toggle the server refuses in the payload tells the user`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { importantResponseStatusCode = HTTP_FORBIDDEN } + val model = viewModel(repository) + val seen = mutableListOf() + val collector = launch { model.uiEvent.collect { seen.add(it) } } + dispatcher.scheduler.runCurrent() + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertTrue(seen.any { it is ConversationInfoUiEvent.ShowSnackbar }) + collector.cancel() + } + + @Test + fun `two changes that both fail go back to the value the server confirmed`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingImportantRequests = Int.MAX_VALUE } + val model = viewModel(repository) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.runCurrent() + assertEquals(true, model.uiState.value.importantConversation) + + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.runCurrent() + assertFalse(model.uiState.value.importantConversation) + + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.importantConversation) + } + + @Test + fun `an older accepted toggle does not become the value a later failure restores`() = + runTest(dispatcher) { + val gate = CompletableDeferred() + val repository = FakeConversationsRepository().apply { importantGate = gate } + val model = viewModel(repository) + + // the first change is still in flight when the second is made and accepted + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.runCurrent() + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + // the older one is accepted last, and must not become the confirmed value + gate.complete(Unit) + dispatcher.scheduler.advanceUntilIdle() + + repository.failingImportantRequests = Int.MAX_VALUE + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.importantConversation) + } + + @Test + fun `an acceptance that arrives while a newer change is in flight still becomes the baseline`() = + runTest(dispatcher) { + val gate = CompletableDeferred() + val repository = FakeConversationsRepository().apply { importantGate = gate } + val model = viewModel(repository) + + // the first change is accepted, so the server holds its value, but only after the second + // change was started and failed + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.runCurrent() + repository.failingImportantRequests = ATTEMPTS_PER_REQUEST + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + gate.complete(Unit) + dispatcher.scheduler.advanceUntilIdle() + + // a later failure has to go back to what the server accepted, not to what it held before + repository.failingImportantRequests = ATTEMPTS_PER_REQUEST + model.toggleImportantConversation("credentials", user.baseUrl!!, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertTrue(model.uiState.value.importantConversation) + } + + private companion object { + const val HTTP_FORBIDDEN = 403 + + /** One attempt plus the single retry `optimisticAction` makes. */ + const val ATTEMPTS_PER_REQUEST = 2 + } + @Test fun `createConversationNameByParticipants should combine names correctly`() { val original = listOf("Dave", null, "Charlie") diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt index de89a69398b..9ffcb08ad4c 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt @@ -9,11 +9,15 @@ package com.nextcloud.talk.conversationinfo.viewmodel import com.nextcloud.talk.conversationinfo.CreateRoomRequestDto import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.json.conversations.RoomOverall +import com.nextcloud.talk.models.json.generic.GenericOCS +import com.nextcloud.talk.models.json.generic.GenericMetaDto import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.participants.TalkBanDto import com.nextcloud.talk.models.json.profile.ProfileDto import com.nextcloud.talk.repositories.conversations.ConversationsRepository import io.reactivex.Observable +import kotlinx.coroutines.CompletableDeferred +import java.io.IOException /** * Answers with what a test needs of [ConversationsRepository.allowGuests], and fails everything @@ -24,6 +28,16 @@ class FakeConversationsRepository : ConversationsRepository { var failAllowGuests = false var lastAllowGuestsPassword: String? = null + /** How many of the next important-conversation requests fail with a connection problem. */ + var failingImportantRequests = 0 + var importantRequests = 0 + + /** The OCS status code the important-conversation requests answer with. */ + var importantResponseStatusCode = 200 + + /** When set, the next important-conversation request waits for this before it answers. */ + var importantGate: CompletableDeferred? = null + override suspend fun allowGuests( user: User, url: String, @@ -93,13 +107,31 @@ class FakeConversationsRepository : ConversationsRepository { credentials: String, baseUrl: String, roomToken: String - ): GenericOverall = throw UnsupportedOperationException() + ): GenericOverall { + importantRequests++ + importantGate?.let { gate -> + importantGate = null + gate.await() + } + if (failingImportantRequests > 0) { + failingImportantRequests-- + throw IOException("no connection") + } + return GenericOverall(GenericOCS(GenericMetaDto(statusCode = importantResponseStatusCode))) + } override suspend fun markConversationAsUnImportant( credentials: String, baseUrl: String, roomToken: String - ): GenericOverall = throw UnsupportedOperationException() + ): GenericOverall { + importantRequests++ + if (failingImportantRequests > 0) { + failingImportantRequests-- + throw IOException("no connection") + } + return GenericOverall(GenericOCS(GenericMetaDto(statusCode = importantResponseStatusCode))) + } override suspend fun markConversationAsRead(credentials: String, url: String, messageId: Int?): GenericOverall = throw UnsupportedOperationException() From a0551b4c3724d852ab5e4d7fddb27bf213f01446 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 00:58:17 +0200 Subject: [PATCH 03/14] fix(conversation info): tell the user when a setting did not save The notification level, the message expiration and the call notification switch logged their failure and left the screen showing a setting the server never accepted. They now go back to the previous value and say so. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../viewmodel/ConversationInfoViewModel.kt | 58 +++++++--- .../DatabaseStorageModule.kt | 108 +++++++----------- 2 files changed, 88 insertions(+), 78 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index 9422d12ecaa..cb19bf8af4f 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -825,10 +825,18 @@ class ConversationInfoViewModel @Inject constructor( } fun toggleCallNotifications() { - val newEnabled = !_uiState.value.callNotificationsEnabled - _uiState.update { it.copy(callNotificationsEnabled = newEnabled) } + val previousEnabled = _uiState.value.callNotificationsEnabled + val newEnabled = !previousEnabled + val change = startSettingChange("call notifications") + viewModelScope.launch { - databaseStorageModule?.saveBoolean("call_notifications_switch", newEnabled) + optimisticAction( + apply = { + _uiState.update { it.copy(callNotificationsEnabled = newEnabled) } + undoIfNewest("call notifications", change) { it.copy(callNotificationsEnabled = previousEnabled) } + }, + request = { databaseStorageModule?.saveBoolean("call_notifications_switch", newEnabled) } + ).onFailure { throwable -> reportSettingFailure("call notifications", throwable) } } } @@ -836,11 +844,23 @@ class ConversationInfoViewModel @Inject constructor( val res = NextcloudTalkApplication.sharedApplication!!.resources val values = res.getStringArray(R.array.message_notification_levels_entry_values) val descriptions = res.getStringArray(R.array.message_notification_levels) - if (position in values.indices && position in descriptions.indices) { - _uiState.update { it.copy(notificationLevel = descriptions[position]) } - viewModelScope.launch { - databaseStorageModule?.saveString("conversation_info_message_notifications_dropdown", values[position]) - } + if (position !in values.indices || position !in descriptions.indices) return + + val previousLevel = _uiState.value.notificationLevel + val change = startSettingChange("notification level") + viewModelScope.launch { + optimisticAction( + apply = { + _uiState.update { it.copy(notificationLevel = descriptions[position]) } + undoIfNewest("notification level", change) { it.copy(notificationLevel = previousLevel) } + }, + request = { + databaseStorageModule?.saveString( + "conversation_info_message_notifications_dropdown", + values[position] + ) + } + ).onFailure { throwable -> reportSettingFailure("the notification level", throwable) } } } @@ -848,14 +868,26 @@ class ConversationInfoViewModel @Inject constructor( val res = NextcloudTalkApplication.sharedApplication!!.resources val values = res.getStringArray(R.array.message_expiring_values) val descriptions = res.getStringArray(R.array.message_expiring_descriptions) - if (position in values.indices && position in descriptions.indices) { - _uiState.update { it.copy(messageExpirationLabel = descriptions[position]) } - viewModelScope.launch { - databaseStorageModule?.saveString("conversation_settings_dropdown", values[position]) - } + if (position !in values.indices || position !in descriptions.indices) return + + val previousLabel = _uiState.value.messageExpirationLabel + val change = startSettingChange("message expiration") + viewModelScope.launch { + optimisticAction( + apply = { + _uiState.update { it.copy(messageExpirationLabel = descriptions[position]) } + undoIfNewest("message expiration", change) { it.copy(messageExpirationLabel = previousLabel) } + }, + request = { databaseStorageModule?.saveString("conversation_settings_dropdown", values[position]) } + ).onFailure { throwable -> reportSettingFailure("the message expiration", throwable) } } } + private suspend fun reportSettingFailure(setting: String, throwable: Throwable) { + _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) + logger.e(TAG, "failed to save $setting", throwable) + } + fun setUpcomingEvent(summary: String?, time: String?) { _uiState.update { it.copy(upcomingEventSummary = summary, upcomingEventTime = time) } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt index 449858e123a..604658b405f 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt @@ -8,8 +8,6 @@ */ package com.nextcloud.talk.utils.preferences.preferencestorage -import android.text.TextUtils -import android.util.Log import autodagger.AutoInjector import com.nextcloud.talk.api.NcApi import com.nextcloud.talk.api.NcApiCoroutines @@ -63,7 +61,6 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { this.conversationToken = conversationToken } - @Suppress("Detekt.TooGenericExceptionCaught") suspend fun saveBoolean(key: String, value: Boolean) { if ("call_notifications_switch" == key) { val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4)) @@ -71,12 +68,7 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { val credentials = getCredentials(conversationUser.username, conversationUser.token) val notificationLevel = if (value) 1 else 0 withContext(Dispatchers.IO) { - try { - ncApiCoroutines!!.notificationCalls(credentials!!, url, notificationLevel) - Log.d(TAG, "Toggled notification calls") - } catch (e: Exception) { - Log.e(TAG, "Error when trying to toggle notification calls", e) - } + ncApiCoroutines!!.notificationCalls(credentials!!, url, notificationLevel) } } if ("lobby_switch" != key) { @@ -91,67 +83,54 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { } } - @Suppress("Detekt.TooGenericExceptionCaught") suspend fun saveString(key: String, value: String) { when (key) { - "conversation_settings_dropdown" -> { - try { - val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(API_VERSION_4)) - val trimmedValue = value.replace("expire_", "") - val valueInt = trimmedValue.toInt() - withContext(Dispatchers.IO) { - ncApiCoroutines!!.setMessageExpiration( - getCredentials(conversationUser.username, conversationUser.token)!!, - getUrlForMessageExpiration(apiVersion, conversationUser.baseUrl, conversationToken), - valueInt - ) - messageExpiration = valueInt - } - } catch (exception: Exception) { - Log.e(TAG, "Error when trying to set message expiration", exception) - } - } - "conversation_info_message_notifications_dropdown" -> { - try { - if (hasSpreedFeatureCapability( - conversationUser.capabilities!!.spreedCapability!!, - SpreedFeatures.NOTIFICATION_LEVELS - ) - ) { - if (TextUtils.isEmpty(messageNotificationLevel) || messageNotificationLevel != value) { - val intValue = when (value) { - "never" -> NOTIFICATION_NEVER - "mention" -> NOTIFICATION_MENTION - "always" -> NOTIFICATION_ALWAYS - else -> 0 - } - val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4, 1)) - withContext(Dispatchers.IO) { - ncApiCoroutines!!.setNotificationLevel( - getCredentials(conversationUser.username, conversationUser.token)!!, - getUrlForRoomNotificationLevel( - apiVersion, - conversationUser.baseUrl, - conversationToken - ), - intValue - ) - messageNotificationLevel = value - } - } else { - messageNotificationLevel = value - } - } - } catch (exception: Exception) { - Log.e(TAG, "Error trying to set notification level", exception) - } - } - else -> { - arbitraryStorageManager!!.storeStorageSetting(accountIdentifier, key, value, conversationToken) + "conversation_settings_dropdown" -> saveMessageExpiration(value) + "conversation_info_message_notifications_dropdown" -> saveNotificationLevel(value) + else -> arbitraryStorageManager!!.storeStorageSetting(accountIdentifier, key, value, conversationToken) + } + } + + private suspend fun saveMessageExpiration(value: String) { + val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(API_VERSION_4)) + val valueInt = value.replace("expire_", "").toInt() + withContext(Dispatchers.IO) { + ncApiCoroutines!!.setMessageExpiration( + getCredentials(conversationUser.username, conversationUser.token)!!, + getUrlForMessageExpiration(apiVersion, conversationUser.baseUrl, conversationToken), + valueInt + ) + messageExpiration = valueInt + } + } + + private suspend fun saveNotificationLevel(value: String) { + val spreedCapability = conversationUser.capabilities?.spreedCapability ?: return + + if (!hasSpreedFeatureCapability(spreedCapability, SpreedFeatures.NOTIFICATION_LEVELS)) { + // a server that cannot be told keeps the level as whatever this screen remembers + messageNotificationLevel = value + } else if (messageNotificationLevel != value) { + val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4, 1)) + withContext(Dispatchers.IO) { + ncApiCoroutines!!.setNotificationLevel( + getCredentials(conversationUser.username, conversationUser.token)!!, + getUrlForRoomNotificationLevel(apiVersion, conversationUser.baseUrl, conversationToken), + notificationLevelOf(value) + ) + messageNotificationLevel = value } } } + private fun notificationLevelOf(value: String): Int = + when (value) { + "never" -> NOTIFICATION_NEVER + "mention" -> NOTIFICATION_MENTION + "always" -> NOTIFICATION_ALWAYS + else -> 0 + } + fun getBoolean(key: String, defaultVal: Boolean): Boolean = if ("lobby_switch" == key) { lobbyValue @@ -186,7 +165,6 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { } companion object { - private const val TAG = "DatabaseStorageModule" private const val EXPIRE_1_HOUR = 3600 private const val EXPIRE_8_HOURS = 28800 private const val EXPIRE_1_DAY = 86400 From 48bae7122ad449fb039e854b1f33657fec3886da Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 01:00:36 +0200 Subject: [PATCH 04/14] perf(conversation list): drop a deleted conversation from the list at once Deleting left the row in place until the worker had finished and the list had been fetched again. It now disappears on confirmation, reusing the guard that hides a conversation being left, and comes back if the delete fails. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../ConversationsListActivity.kt | 61 +++++++++++-------- .../viewmodels/ConversationsListViewModel.kt | 27 ++++---- 2 files changed, 50 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt index 03f698d4fda..735693f3ab8 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt @@ -1328,7 +1328,7 @@ class ConversationsListActivity : BaseActivity() { @SuppressLint("StringFormatInvalid") private fun showLeaveConversationSnackbar(conversation: ConversationModel) { val token = conversation.token ?: return - conversationsListViewModel.markConversationPendingLeave(token) + conversationsListViewModel.markConversationPendingRemoval(token) lifecycleScope.launch { val result = snackbarHostState.showSnackbar( message = String.format(resources.getString(R.string.left_conversation), conversation.displayName), @@ -1336,7 +1336,7 @@ class ConversationsListActivity : BaseActivity() { duration = SnackbarDuration.Long ) when (result) { - SnackbarResult.ActionPerformed -> conversationsListViewModel.clearConversationPendingLeave(token) + SnackbarResult.ActionPerformed -> conversationsListViewModel.clearConversationPendingRemoval(token) SnackbarResult.Dismissed -> leaveConversation(conversation) } } @@ -1364,11 +1364,11 @@ class ConversationsListActivity : BaseActivity() { resources.getString(R.string.nc_shortcut_conversation_deleted) ) } - conversationsListViewModel.onConversationLeft(currentUser, token) + conversationsListViewModel.onConversationRemoved(currentUser, token) } WorkInfo.State.FAILED -> { logger.e(TAG, "LeaveConversationWorker failed for token $token") - conversationsListViewModel.clearConversationPendingLeave(token) + conversationsListViewModel.clearConversationPendingRemoval(token) showSnackbar(resources.getString(R.string.nc_common_error_sorry)) } else -> {} @@ -1385,6 +1385,7 @@ class ConversationsListActivity : BaseActivity() { .setTitle(R.string.nc_delete_call) .setMessage(R.string.nc_delete_conversation_more) .setPositiveButton(R.string.nc_delete) { _, _ -> + conversation.token?.let { conversationsListViewModel.markConversationPendingRemoval(it) } deleteConversation(conversation) } .setNegativeButton(R.string.nc_cancel) { _, _ -> @@ -1652,33 +1653,39 @@ class ConversationsListActivity : BaseActivity() { WorkManager.getInstance(context).getWorkInfoByIdLiveData(deleteConversationWorker.id) .observeForever { workInfo: WorkInfo? -> - if (workInfo != null) { - when (workInfo.state) { - WorkInfo.State.SUCCEEDED -> { - currentUser.id?.let { userId -> - ShortcutManagerHelper.disableConversationShortcut( - context, - conversation.token, - userId, - context.resources.getString(R.string.nc_shortcut_conversation_deleted) - ) - } - showSnackbar( - String.format( - context.resources.getString(R.string.deleted_conversation), - conversation.displayName - ) + when (workInfo?.state) { + WorkInfo.State.SUCCEEDED -> { + currentUser.id?.let { userId -> + ShortcutManagerHelper.disableConversationShortcut( + context, + conversation.token, + userId, + context.resources.getString(R.string.nc_shortcut_conversation_deleted) ) - fetchRooms(forceFullSync = true) } + // the worker only deletes the room on the server, so the entry has to stay + // hidden until the full sync that drops it locally has landed + conversation.token?.let { conversationsListViewModel.onConversationRemoved(currentUser, it) } + showSnackbar( + String.format( + context.resources.getString(R.string.deleted_conversation), + conversation.displayName + ) + ) + } - WorkInfo.State.FAILED -> { - logger.e(TAG, "DeleteConversationWorker failed for token ${conversation.token}") - showSnackbar(context.resources.getString(R.string.nc_common_error_sorry)) - } + WorkInfo.State.FAILED -> { + logger.e(TAG, "DeleteConversationWorker failed for token ${conversation.token}") + conversation.token?.let { conversationsListViewModel.clearConversationPendingRemoval(it) } + showSnackbar(context.resources.getString(R.string.nc_common_error_sorry)) + } - else -> { - } + WorkInfo.State.CANCELLED -> { + // nothing was deleted, so the entry must not stay hidden + conversation.token?.let { conversationsListViewModel.clearConversationPendingRemoval(it) } + } + + else -> { } } } diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 331b32217c1..4bf3f65170d 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -255,10 +255,10 @@ class ConversationsListViewModel @AssistedInject constructor( private val hideRoomToken = MutableStateFlow(null) - /** Tokens of rooms being left; hidden optimistically while the leave-undo snackbar is showing. */ - private val pendingLeaveTokens = MutableStateFlow>(emptySet()) + /** Tokens of rooms being left or deleted; hidden optimistically until the request finished. */ + private val pendingRemovalTokens = MutableStateFlow>(emptySet()) - private val excludedRoomTokens = combine(hideRoomToken, pendingLeaveTokens) { hideToken, pendingTokens -> + private val excludedRoomTokens = combine(hideRoomToken, pendingRemovalTokens) { hideToken, pendingTokens -> if (hideToken != null) pendingTokens + hideToken else pendingTokens } @@ -400,14 +400,14 @@ class ConversationsListViewModel @AssistedInject constructor( hideRoomToken.value = token } - /** Optimistically hide a room while its leave-undo snackbar is showing. */ - fun markConversationPendingLeave(token: String) { - pendingLeaveTokens.value = pendingLeaveTokens.value + token + /** Optimistically hide a room that is being left or deleted. */ + fun markConversationPendingRemoval(token: String) { + pendingRemovalTokens.value = pendingRemovalTokens.value + token } - /** Un-hide a room, either because the leave was undone or because it finished/failed. */ - fun clearConversationPendingLeave(token: String) { - pendingLeaveTokens.value = pendingLeaveTokens.value - token + /** Un-hide a room, either because the removal was undone or because it finished or failed. */ + fun clearConversationPendingRemoval(token: String) { + pendingRemovalTokens.value = pendingRemovalTokens.value - token } fun getFederationInvitations() { @@ -642,11 +642,16 @@ class ConversationsListViewModel @AssistedInject constructor( * until the full sync that deletes its row has finished; un-hiding it before would show the * still stored row again for as long as the sync takes. */ - fun onConversationLeft(user: User, token: String) { + /** + * Keeps a conversation the user left or deleted out of the list until the sync that drops it for + * good has landed, so it cannot flash back in between the request succeeding and the list + * catching up. + */ + fun onConversationRemoved(user: User, token: String) { val job = getRooms(user, forceFullSync = true) viewModelScope.launch { job.join() - clearConversationPendingLeave(token) + clearConversationPendingRemoval(token) } } From 53433d6c8250eb578becca10ac865738debd8733 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 09:20:16 +0200 Subject: [PATCH 05/14] test(utils): cover the optimistic action contract directly The helper was only exercised through the repositories that use it, so the rules every optimistic action depends on - applied before the answer, taken back on a refusal or a cancellation, kept after one hiccup - had no test of their own. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../talk/utils/OptimisticActionTest.kt | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt diff --git a/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt new file mode 100644 index 00000000000..6e48a2e0e38 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt @@ -0,0 +1,139 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.utils + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import retrofit2.HttpException +import retrofit2.Response +import java.io.IOException + +/** + * The contract every optimistic action in the app relies on: the change is visible before the answer + * arrives, it survives a hiccup, and it is taken back whenever the request does not finally succeed - + * including when the screen is left while the request is still in flight. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class OptimisticActionTest { + + private var state = "before" + + private val applyChange: suspend () -> (suspend () -> Unit)? = { + val previous = state + state = "after" + { state = previous } + } + + @Test + fun `a change the server accepts stays applied`() = + runTest { + val result = optimisticAction(apply = applyChange, request = { "ok" }) + + assertEquals("ok", result.getOrNull()) + assertEquals("after", state) + } + + @Test + fun `a change the server refuses is taken back`() = + runTest { + val result = optimisticAction(apply = applyChange, request = { throw forbidden() }) + + assertTrue(result.isFailure) + assertEquals("before", state) + } + + @Test + fun `a connection problem is retried once before the change is taken back`() = + runTest { + var attempts = 0 + + val result = optimisticAction( + apply = applyChange, + request = { + attempts++ + throw IOException("no connection") + } + ) + + assertEquals(2, attempts) + assertTrue(result.isFailure) + assertEquals("before", state) + } + + @Test + fun `a single connection problem does not cost the change`() = + runTest { + var attempts = 0 + + optimisticAction( + apply = applyChange, + request = { + attempts++ + if (attempts == 1) throw IOException("no connection") else "ok" + } + ) + + assertEquals("after", state) + } + + @Test + fun `a refusal the server reports in the payload is taken back as well`() = + runTest { + val result = optimisticAction( + apply = applyChange, + isConfirmed = { answer: String -> answer == "ok" }, + request = { "refused" } + ) + + // the request itself did not fail, so the caller still sees the answer it got + assertEquals("refused", result.getOrNull()) + assertEquals("before", state) + } + + @Test + fun `nothing is taken back when nothing was applied`() = + runTest { + val result = optimisticAction(apply = { null }, request = { throw forbidden() }) + + assertTrue(result.isFailure) + assertEquals("before", state) + } + + @Test + fun `leaving the screen while the request is in flight takes the change back`() = + runTest { + val job = launch { + optimisticAction(apply = applyChange, request = { awaitForever() }) + } + runCurrent() + assertEquals("after", state) + + job.cancel() + advanceUntilIdle() + + assertEquals("before", state) + } + + private suspend fun awaitForever(): String = suspendCancellableCoroutine { } + + private fun forbidden(): HttpException = + HttpException(Response.error(HTTP_FORBIDDEN, "".toResponseBody("text/plain".toMediaType()))) + + private companion object { + const val HTTP_FORBIDDEN = 403 + } +} From 3e8a08b10fa0e96761c5911bd6932ffa064031be Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 13:34:32 +0200 Subject: [PATCH 06/14] fix(utils): run an optimistic revert only once when it fails A refusal reported in the payload reverted inside the try block, so a revert that threw was caught and run a second time. The revert now runs after the request has been handled. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../nextcloud/talk/utils/OptimisticAction.kt | 16 ++++++++------ .../talk/utils/OptimisticActionTest.kt | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt index 6637c223132..c87ca20a919 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -52,20 +52,22 @@ suspend fun optimisticAction( // is there to prevent val revertUninterruptibly: suspend () -> Unit = { withContext(NonCancellable) { revert?.invoke() } } - return try { - val answer = revertOnCancellation(revertUninterruptibly) { + val answer = try { + revertOnCancellation(revertUninterruptibly) { withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isTransientFailure) { request() } } - if (!isConfirmed(answer)) { - revertUninterruptibly() - } - Result.success(answer) } catch (e: CancellationException) { throw e } catch (e: Exception) { revertUninterruptibly() - Result.failure(e) + return Result.failure(e) + } + + // kept outside the try, so a revert that throws here is not caught and run a second time + if (!isConfirmed(answer)) { + revertUninterruptibly() } + return Result.success(answer) } diff --git a/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt index 6e48a2e0e38..ec877ad7c87 100644 --- a/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt +++ b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt @@ -104,6 +104,28 @@ class OptimisticActionTest { assertEquals("before", state) } + @Test + fun `a revert that fails after a refusal in the payload is not run twice`() = + runTest { + var reverts = 0 + + val result = runCatching { + optimisticAction( + apply = { + { + reverts++ + throw IllegalStateException("database closed") + } + }, + isConfirmed = { answer: String -> answer == "ok" }, + request = { "refused" } + ) + } + + assertTrue(result.exceptionOrNull() is IllegalStateException) + assertEquals(1, reverts) + } + @Test fun `nothing is taken back when nothing was applied`() = runTest { From 75f2885b414b94d275652e466e36f5f0636ed9e4 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 13:54:10 +0200 Subject: [PATCH 07/14] fix(conversation info): undo a failed setting to what the server confirmed The call notification switch, the notification level and the message expiration undid a failed change to the value shown when it started. Two changes that both failed left the second undo restoring the first one's optimistic value, which the server never held. They now restore the last value the server confirmed, as the important and sensitive toggles do. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../viewmodel/ConversationInfoViewModel.kt | 92 ++++++++++++------- 1 file changed, 60 insertions(+), 32 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index cb19bf8af4f..1729b17f37f 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -413,6 +413,9 @@ class ConversationInfoViewModel @Inject constructor( val sensitiveConversation = conversationModel.hasSensitive confirmedSetting[IMPORTANT] = importantConversation confirmedSetting[SENSITIVE] = sensitiveConversation + confirmedSetting[CALL_NOTIFICATIONS] = callNotificationsEnabled + confirmedSetting[NOTIFICATION_LEVEL] = notificationLevelStr + confirmedSetting[MESSAGE_EXPIRATION] = messageExpirationLabel val showArchiveConversation = hasSpreedFeatureCapability(spreedCapabilities, SpreedFeatures.ARCHIVE_CONVERSATIONS) @@ -825,18 +828,22 @@ class ConversationInfoViewModel @Inject constructor( } fun toggleCallNotifications() { - val previousEnabled = _uiState.value.callNotificationsEnabled - val newEnabled = !previousEnabled - val change = startSettingChange("call notifications") + val newEnabled = !_uiState.value.callNotificationsEnabled + val confirmedEnabled = confirmedValueOf(CALL_NOTIFICATIONS, _uiState.value.callNotificationsEnabled) + val change = startSettingChange(CALL_NOTIFICATIONS) viewModelScope.launch { optimisticAction( apply = { _uiState.update { it.copy(callNotificationsEnabled = newEnabled) } - undoIfNewest("call notifications", change) { it.copy(callNotificationsEnabled = previousEnabled) } + undoIfNewest(CALL_NOTIFICATIONS, change) { + it.copy(callNotificationsEnabled = confirmedValueOf(CALL_NOTIFICATIONS, confirmedEnabled)) + } }, request = { databaseStorageModule?.saveBoolean("call_notifications_switch", newEnabled) } - ).onFailure { throwable -> reportSettingFailure("call notifications", throwable) } + ) + .onSuccess { recordAccepted(CALL_NOTIFICATIONS, change, newEnabled) } + .onFailure { throwable -> reportSettingFailure("call notifications", throwable) } } } @@ -846,13 +853,15 @@ class ConversationInfoViewModel @Inject constructor( val descriptions = res.getStringArray(R.array.message_notification_levels) if (position !in values.indices || position !in descriptions.indices) return - val previousLevel = _uiState.value.notificationLevel - val change = startSettingChange("notification level") + val confirmedLevel = confirmedValueOf(NOTIFICATION_LEVEL, _uiState.value.notificationLevel) + val change = startSettingChange(NOTIFICATION_LEVEL) viewModelScope.launch { optimisticAction( apply = { _uiState.update { it.copy(notificationLevel = descriptions[position]) } - undoIfNewest("notification level", change) { it.copy(notificationLevel = previousLevel) } + undoIfNewest(NOTIFICATION_LEVEL, change) { + it.copy(notificationLevel = confirmedValueOf(NOTIFICATION_LEVEL, confirmedLevel)) + } }, request = { databaseStorageModule?.saveString( @@ -860,7 +869,9 @@ class ConversationInfoViewModel @Inject constructor( values[position] ) } - ).onFailure { throwable -> reportSettingFailure("the notification level", throwable) } + ) + .onSuccess { recordAccepted(NOTIFICATION_LEVEL, change, descriptions[position]) } + .onFailure { throwable -> reportSettingFailure("the notification level", throwable) } } } @@ -870,16 +881,20 @@ class ConversationInfoViewModel @Inject constructor( val descriptions = res.getStringArray(R.array.message_expiring_descriptions) if (position !in values.indices || position !in descriptions.indices) return - val previousLabel = _uiState.value.messageExpirationLabel - val change = startSettingChange("message expiration") + val confirmedLabel = confirmedValueOf(MESSAGE_EXPIRATION, _uiState.value.messageExpirationLabel) + val change = startSettingChange(MESSAGE_EXPIRATION) viewModelScope.launch { optimisticAction( apply = { _uiState.update { it.copy(messageExpirationLabel = descriptions[position]) } - undoIfNewest("message expiration", change) { it.copy(messageExpirationLabel = previousLabel) } + undoIfNewest(MESSAGE_EXPIRATION, change) { + it.copy(messageExpirationLabel = confirmedValueOf(MESSAGE_EXPIRATION, confirmedLabel)) + } }, request = { databaseStorageModule?.saveString("conversation_settings_dropdown", values[position]) } - ).onFailure { throwable -> reportSettingFailure("the message expiration", throwable) } + ) + .onSuccess { recordAccepted(MESSAGE_EXPIRATION, change, descriptions[position]) } + .onFailure { throwable -> reportSettingFailure("the message expiration", throwable) } } } @@ -917,7 +932,30 @@ class ConversationInfoViewModel @Inject constructor( * whatever was on screen when its own change started: two changes that both fail would * otherwise leave the newer undo restoring the optimistic value of the older one. */ - private val confirmedSetting = mutableMapOf() + private val confirmedSetting = mutableMapOf() + + /** + * The value the server last confirmed for [setting]. The first change of a session is made from + * [displayed], the value the screen was built with and so the one the server last gave us. + */ + private inline fun confirmedValueOf(setting: String, displayed: T): T = + confirmedSetting.getOrPut(setting) { displayed } as? T ?: displayed + + /** + * Makes [value] the baseline for [setting] once the server accepted [change]. + * + * The baseline follows the newest change the server has *accepted*, not the newest the user has + * started. Two requests for one setting can be in flight at once and be accepted out of order, so + * an older acceptance arriving last must not overwrite a newer one - but while the newer one is + * still in flight, the older acceptance is the last thing the server agreed to, and dropping it + * would leave a later failure restoring a value the server no longer holds. + */ + private fun recordAccepted(setting: String, change: Long, value: Any) { + if (change > (lastAcceptedChange[setting] ?: 0)) { + lastAcceptedChange[setting] = change + confirmedSetting[setting] = value + } + } /** * Stamps a new change to [setting] and returns its order, so everything that happens afterwards @@ -957,9 +995,7 @@ class ConversationInfoViewModel @Inject constructor( fun toggleImportantConversation(credentials: String, baseUrl: String, roomToken: String) { val displayedValue = _uiState.value.importantConversation val newValue = !displayedValue - // the first change of a session is made from the value the screen was built with, - // which is the one the server last gave us - val confirmedValue = confirmedSetting.getOrPut(IMPORTANT) { displayedValue } + val confirmedValue = confirmedValueOf(IMPORTANT, displayedValue) val change = startSettingChange(IMPORTANT) viewModelScope.launch { @@ -967,7 +1003,7 @@ class ConversationInfoViewModel @Inject constructor( apply = { _uiState.update { it.copy(importantConversation = newValue) } undoIfNewest(IMPORTANT, change) { - it.copy(importantConversation = confirmedSetting[IMPORTANT] ?: confirmedValue) + it.copy(importantConversation = confirmedValueOf(IMPORTANT, confirmedValue)) } }, isConfirmed = ::accepted, @@ -987,9 +1023,7 @@ class ConversationInfoViewModel @Inject constructor( fun toggleSensitiveConversation(credentials: String, baseUrl: String, roomToken: String) { val displayedValue = _uiState.value.sensitiveConversation val newValue = !displayedValue - // the first change of a session is made from the value the screen was built with, - // which is the one the server last gave us - val confirmedValue = confirmedSetting.getOrPut(SENSITIVE) { displayedValue } + val confirmedValue = confirmedValueOf(SENSITIVE, displayedValue) val change = startSettingChange(SENSITIVE) viewModelScope.launch { @@ -997,7 +1031,7 @@ class ConversationInfoViewModel @Inject constructor( apply = { _uiState.update { it.copy(sensitiveConversation = newValue) } undoIfNewest(SENSITIVE, change) { - it.copy(sensitiveConversation = confirmedSetting[SENSITIVE] ?: confirmedValue) + it.copy(sensitiveConversation = confirmedValueOf(SENSITIVE, confirmedValue)) } }, isConfirmed = ::accepted, @@ -1018,12 +1052,6 @@ class ConversationInfoViewModel @Inject constructor( * when it did not. A refusal the server reports in the payload leaves [optimisticAction] * returning a success it has already taken back, so it needs reporting here as much as a * request that threw. - * - * The baseline follows the newest change the server has *accepted*, not the newest the user has - * started. Two requests for one setting can be in flight at once and be accepted out of order, so - * an older acceptance arriving last must not overwrite a newer one - but while the newer one is - * still in flight, the older acceptance is the last thing the server agreed to, and dropping it - * would leave a later failure restoring a value the server no longer holds. */ @Suppress("Detekt.LongParameterList") private suspend fun reportSettingOutcome( @@ -1035,10 +1063,7 @@ class ConversationInfoViewModel @Inject constructor( ) { val response = outcome.getOrNull() if (response != null && accepted(response)) { - if (change > (lastAcceptedChange[setting] ?: 0)) { - lastAcceptedChange[setting] = change - confirmedSetting[setting] = newValue - } + recordAccepted(setting, change, newValue) return } _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) @@ -1071,6 +1096,9 @@ class ConversationInfoViewModel @Inject constructor( private const val EXTENDED_CONVERSATION = "extended_conversation" private const val IMPORTANT = "important" private const val SENSITIVE = "sensitive" + private const val CALL_NOTIFICATIONS = "call notifications" + private const val NOTIFICATION_LEVEL = "notification level" + private const val MESSAGE_EXPIRATION = "message expiration" private const val GROUP_CONVERSATION_TYPE = "2" private const val MAX_ROOM_NAME_LENGTH = 255 private const val NOTIFICATION_LEVEL_ALWAYS: Int = 1 From 8b32b2e6550931a8a60f3d19f8b375e59813f0ea Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 14:04:03 +0200 Subject: [PATCH 08/14] refactor(utils): name the undo an optimistic action hands back The undo of an optimistic change was spelled out as a raw function type, which made the helper's signature a closure returning a closure. Undo now names it, wherever an undo for optimisticAction() is built. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../talk/chat/data/network/OfflineFirstChatRepository.kt | 6 ++++-- .../viewmodel/ConversationInfoViewModel.kt | 3 ++- .../data/network/ConversationListUpdater.kt | 8 +++----- .../viewmodels/ConversationsListViewModel.kt | 3 ++- .../repositories/reactions/ReactionsRepositoryImpl.kt | 4 ++-- .../java/com/nextcloud/talk/utils/OptimisticAction.kt | 7 +++++-- .../java/com/nextcloud/talk/utils/OptimisticActionTest.kt | 2 +- 7 files changed, 19 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index 0ed819ea84e..221bf6e18b8 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -30,6 +30,8 @@ import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.converters.EnumActorTypeConverter import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.participants.ParticipantDto +import com.nextcloud.talk.utils.Undo +import com.nextcloud.talk.utils.bundle.BundleKeys import com.nextcloud.talk.utils.message.SendMessageUtils import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.Dispatchers @@ -831,7 +833,7 @@ class OfflineFirstChatRepository @Inject constructor( * returns the action that restores the previous version. The revert only applies while the message * still carries the local edit, so a server payload that arrived meanwhile stays authoritative. */ - private suspend fun applyLocalEdit(messageId: Long, text: String): (suspend () -> Unit)? { + private suspend fun applyLocalEdit(messageId: Long, text: String): Undo? { val message = chatDao.getChatMessageEntity(internalConversationId, messageId) if (message == null || message.message == text) return null @@ -904,7 +906,7 @@ class OfflineFirstChatRepository @Inject constructor( * or null when there was nothing to change. The revert only applies while the message still carries * the local deletion, so a server payload that arrived meanwhile stays authoritative. */ - private suspend fun applyLocalDeletion(messageId: Long, deletedPlaceholder: String): (suspend () -> Unit)? { + private suspend fun applyLocalDeletion(messageId: Long, deletedPlaceholder: String): Undo? { val message = chatDao.getChatMessageEntity(internalConversationId, messageId) if (message == null || message.messageType == MESSAGE_TYPE_DELETED) return null diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index 1729b17f37f..3abbe067f7d 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -52,6 +52,7 @@ import com.nextcloud.talk.utils.DateConstants import com.nextcloud.talk.utils.DisplayUtils import com.nextcloud.talk.utils.ParticipantRoleUtils import com.nextcloud.talk.utils.SpreedFeatures +import com.nextcloud.talk.utils.Undo import com.nextcloud.talk.utils.optimisticAction import com.nextcloud.talk.utils.preferences.preferencestorage.DatabaseStorageModule import io.reactivex.Observer @@ -979,7 +980,7 @@ class ConversationInfoViewModel @Inject constructor( setting: String, change: Long, undo: (ConversationInfoUiState) -> ConversationInfoUiState - ): suspend () -> Unit = { if (isNewestChange(setting, change)) _uiState.update(undo) } + ): Undo = { if (isNewestChange(setting, change)) _uiState.update(undo) } private fun isNewestChange(setting: String, change: Long) = newestSettingChange[setting] == change diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt index 5c0a9d9be4f..8866b601d44 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt @@ -14,6 +14,7 @@ import com.nextcloud.talk.data.database.dao.ChatBlocksDao import com.nextcloud.talk.data.database.dao.ChatMessagesDao import com.nextcloud.talk.data.database.dao.ConversationsDao import com.nextcloud.talk.data.database.model.ConversationEntity +import com.nextcloud.talk.utils.Undo import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext @@ -195,7 +196,7 @@ class ConversationListUpdater @Inject constructor( target: ChatMessageSyncer.SyncTarget, messageId: Long, pinned: Boolean - ): (suspend () -> Unit)? { + ): Undo? { val conversation = withContext(Dispatchers.IO) { conversationsDao.getConversationForUser(target.accountId, target.roomToken).first() } ?: return null @@ -234,10 +235,7 @@ class ConversationListUpdater @Inject constructor( * instead of after the request and the room refresh that follows it. Returns the action that * brings it back, or null when the conversation isn't cached. */ - suspend fun updateLocalHiddenPinnedMessage( - target: ChatMessageSyncer.SyncTarget, - messageId: Long - ): (suspend () -> Unit)? { + suspend fun updateLocalHiddenPinnedMessage(target: ChatMessageSyncer.SyncTarget, messageId: Long): Undo? { val conversation = withContext(Dispatchers.IO) { conversationsDao.getConversationForUser(target.accountId, target.roomToken).first() } ?: return null diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 4bf3f65170d..10f815284f4 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -41,6 +41,7 @@ import com.nextcloud.talk.users.UserManager import com.nextcloud.talk.utils.ApiUtils import com.nextcloud.talk.utils.CapabilitiesUtil.hasSpreedFeatureCapability import com.nextcloud.talk.utils.SpreedFeatures +import com.nextcloud.talk.utils.Undo import com.nextcloud.talk.utils.UserIdUtils import com.nextcloud.talk.utils.optimisticAction import dagger.assisted.Assisted @@ -931,7 +932,7 @@ class ConversationsListViewModel @AssistedInject constructor( * The action that puts [original] back and releases whatever guard the caller registered, for * [optimisticAction] to run when the request finally fails or the screen is left mid-request. */ - private fun revertTo(original: ConversationModel, releaseGuard: () -> Unit): suspend () -> Unit = + private fun revertTo(original: ConversationModel, releaseGuard: () -> Unit): Undo = { releaseGuard() applyLocally(original) diff --git a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt index 9a01933cc3e..9b00f34f673 100644 --- a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt +++ b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt @@ -12,6 +12,7 @@ import com.nextcloud.talk.chat.data.model.ChatMessage import com.nextcloud.talk.data.database.dao.ChatMessagesDao import com.nextcloud.talk.models.domain.ReactionAddedModel import com.nextcloud.talk.models.domain.ReactionDeletedModel +import com.nextcloud.talk.utils.Undo import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext @@ -88,8 +89,7 @@ class ReactionsRepositoryImpl @Inject constructor( if (e.code() == HTTP_NOT_FOUND) HTTP_OK else throw e } - private fun revertWith(applied: Boolean, revert: suspend () -> Unit): (suspend () -> Unit)? = - revert.takeIf { applied } + private fun revertWith(applied: Boolean, revert: Undo): Undo? = revert.takeIf { applied } private fun Result.logFailure(): Result = onFailure { Log.w(TAG, "Reaction request failed: $it") } diff --git a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt index c87ca20a919..a0e7c97498b 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -17,6 +17,9 @@ private const val HTTP_TOO_MANY_REQUESTS = 429 private const val HTTP_INTERNAL_SERVER_ERROR = 500 private const val RETRY_DELAY_MS = 500L +/** Takes an optimistic local change back again. */ +typealias Undo = suspend () -> Unit + /** * Whether a failed request is worth another attempt: a connection problem, a rate limit or a server * error. An answer the server actually gave - a refusal, a permission error - is not. @@ -43,14 +46,14 @@ fun isTransientFailure(error: Exception): Boolean = */ @Suppress("TooGenericExceptionCaught") suspend fun optimisticAction( - apply: suspend () -> (suspend () -> Unit)?, + apply: suspend () -> Undo?, isConfirmed: (T) -> Boolean = { true }, request: suspend () -> T ): Result { val revert = apply() // a revert that is itself cancelled would leave the change applied, which is the very thing it // is there to prevent - val revertUninterruptibly: suspend () -> Unit = { withContext(NonCancellable) { revert?.invoke() } } + val revertUninterruptibly: Undo = { withContext(NonCancellable) { revert?.invoke() } } val answer = try { revertOnCancellation(revertUninterruptibly) { diff --git a/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt index ec877ad7c87..a8c3cb1de3a 100644 --- a/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt +++ b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt @@ -32,7 +32,7 @@ class OptimisticActionTest { private var state = "before" - private val applyChange: suspend () -> (suspend () -> Unit)? = { + private val applyChange: suspend () -> Undo? = { val previous = state state = "after" { state = previous } From a5bcec0894aa935e8a339b8cdad16d0341180fc0 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 14:09:00 +0200 Subject: [PATCH 09/14] chore: drop a stale doc comment and unused suppressions onConversationRemoved still carried the doc comment of the function it replaced, and the important and sensitive toggles suppressed a generic exception catch they no longer have. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../conversationinfo/viewmodel/ConversationInfoViewModel.kt | 2 -- .../viewmodels/ConversationsListViewModel.kt | 5 ----- 2 files changed, 7 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index 3abbe067f7d..821d0b551d0 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -992,7 +992,6 @@ class ConversationInfoViewModel @Inject constructor( private fun accepted(response: GenericOverall): Boolean = response.ocs?.meta?.statusCode.let { it == null || it == NO_STATUS_CODE || it == HTTP_OK } - @Suppress("Detekt.TooGenericExceptionCaught") fun toggleImportantConversation(credentials: String, baseUrl: String, roomToken: String) { val displayedValue = _uiState.value.importantConversation val newValue = !displayedValue @@ -1020,7 +1019,6 @@ class ConversationInfoViewModel @Inject constructor( } } - @Suppress("Detekt.TooGenericExceptionCaught") fun toggleSensitiveConversation(credentials: String, baseUrl: String, roomToken: String) { val displayedValue = _uiState.value.sensitiveConversation val newValue = !displayedValue diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index 10f815284f4..103c7fa7ba8 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -638,11 +638,6 @@ class ConversationsListViewModel @AssistedInject constructor( return job } - /** - * Removes the room [token] that [user] has just left from the list. The room stays hidden - * until the full sync that deletes its row has finished; un-hiding it before would show the - * still stored row again for as long as the sync takes. - */ /** * Keeps a conversation the user left or deleted out of the list until the sync that drops it for * good has landed, so it cannot flash back in between the request succeeding and the list From 718cad7515a42a5f012e24172ffb33c1063b8476 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 14:19:58 +0200 Subject: [PATCH 10/14] fix(reactions): log a reaction the server refuses in the payload Moving reactions onto the optimistic helper dropped the warning for an answer with an unexpected status code, so only a request that threw left a trace. Both are logged again. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../reactions/ReactionsRepositoryImpl.kt | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt index 9b00f34f673..12677eb3f33 100644 --- a/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt +++ b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt @@ -51,7 +51,7 @@ class ReactionsRepositoryImpl @Inject constructor( }, isConfirmed = { statusCode -> statusCode in ADD_SUCCESS_CODES }, request = { ncApiCoroutines.sendReaction(credentials, url, emoji).ocs?.meta?.statusCode } - ).logFailure().getOrNull() in ADD_SUCCESS_CODES + ).isAccepted(ADD_SUCCESS_CODES) return ReactionAddedModel(message, emoji, confirmed) } @@ -74,7 +74,7 @@ class ReactionsRepositoryImpl @Inject constructor( }, isConfirmed = { statusCode -> statusCode in DELETE_SUCCESS_CODES }, request = { deleteReactionStatusCode(credentials, url, emoji) } - ).logFailure().getOrNull() in DELETE_SUCCESS_CODES + ).isAccepted(DELETE_SUCCESS_CODES) return ReactionDeletedModel(message, emoji, confirmed) } @@ -91,7 +91,21 @@ class ReactionsRepositoryImpl @Inject constructor( private fun revertWith(applied: Boolean, revert: Undo): Undo? = revert.takeIf { applied } - private fun Result.logFailure(): Result = onFailure { Log.w(TAG, "Reaction request failed: $it") } + /** Whether the server applied the reaction. Every way it did not is logged. */ + private fun Result.isAccepted(successCodes: Set): Boolean = + fold( + onSuccess = { statusCode -> + val accepted = statusCode in successCodes + if (!accepted) { + Log.w(TAG, "Reaction request answered with unexpected status code $statusCode") + } + accepted + }, + onFailure = { error -> + Log.w(TAG, "Reaction request failed: $error") + false + } + ) /** * Adds the reaction to the cached message and reports whether that changed anything. A reaction the From f50fff4d7492572b5b8f37c2ac88e1e38e575bd3 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 14:33:40 +0200 Subject: [PATCH 11/14] perf(conversation info): keep the read-only lock correct when it fails The lock toggle was the last setting with its own try/catch. It now goes through the shared optimistic helper, so it retries a connection problem once, takes its change back when the server refuses in the payload or the screen is left mid-request, and undoes a failure to the value the server last confirmed. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../viewmodel/ConversationInfoViewModel.kt | 70 +++++++++++++------ 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index 821d0b551d0..ad6be94542b 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -6,6 +6,7 @@ */ package com.nextcloud.talk.conversationinfo.viewmodel import android.util.Log +import androidx.annotation.StringRes import androidx.annotation.VisibleForTesting import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner @@ -417,6 +418,7 @@ class ConversationInfoViewModel @Inject constructor( confirmedSetting[CALL_NOTIFICATIONS] = callNotificationsEnabled confirmedSetting[NOTIFICATION_LEVEL] = notificationLevelStr confirmedSetting[MESSAGE_EXPIRATION] = messageExpirationLabel + confirmedSetting[LOCKED] = isConversationLocked val showArchiveConversation = hasSpreedFeatureCapability(spreedCapabilities, SpreedFeatures.ARCHIVE_CONVERSATIONS) @@ -805,29 +807,55 @@ class ConversationInfoViewModel @Inject constructor( }) } - @Suppress("Detekt.TooGenericExceptionCaught") fun toggleLock(user: User, token: String) { - val previousLocked = _uiState.value.isConversationLocked - val newLocked = !previousLocked - _uiState.update { it.copy(isConversationLocked = newLocked) } + val newLocked = !_uiState.value.isConversationLocked + val confirmedLocked = confirmedValueOf(LOCKED, _uiState.value.isConversationLocked) + val change = startSettingChange(LOCKED) + viewModelScope.launch { - databaseStorageModule?.saveBoolean("lock_switch", newLocked) - try { - val apiVersion = ApiUtils.getConversationApiVersion( - user, - intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1) - ) - val url = ApiUtils.getUrlForConversationReadOnly(apiVersion, user.baseUrl!!, token) - conversationsRepository.setConversationReadOnly(user = user, url = url, state = if (newLocked) 1 else 0) - } catch (exception: Exception) { - logger.e(TAG, "Failed to toggle read-only lock state", exception) - _uiState.update { it.copy(isConversationLocked = previousLocked) } - databaseStorageModule?.saveBoolean("lock_switch", previousLocked) - _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.conversation_read_only_failed)) - } + val outcome = optimisticAction( + apply = { + showLocked(newLocked) + undoLockIfNewest(change, confirmedLocked) + }, + isConfirmed = ::accepted, + request = { + val apiVersion = ApiUtils.getConversationApiVersion( + user, + intArrayOf(ApiUtils.API_V4, ApiUtils.API_V1) + ) + val url = ApiUtils.getUrlForConversationReadOnly(apiVersion, user.baseUrl!!, token) + conversationsRepository.setConversationReadOnly( + user = user, + url = url, + state = if (newLocked) 1 else 0 + ) + } + ) + reportSettingOutcome( + outcome = outcome, + setting = LOCKED, + change = change, + newValue = newLocked, + description = "read-only lock", + failureMessage = R.string.conversation_read_only_failed + ) } } + /** The lock state is shown from local storage as well, so both have to change together. */ + private suspend fun showLocked(locked: Boolean) { + _uiState.update { it.copy(isConversationLocked = locked) } + databaseStorageModule?.saveBoolean("lock_switch", locked) + } + + private fun undoLockIfNewest(change: Long, confirmedLocked: Boolean): Undo = + { + if (isNewestChange(LOCKED, change)) { + showLocked(confirmedValueOf(LOCKED, confirmedLocked)) + } + } + fun toggleCallNotifications() { val newEnabled = !_uiState.value.callNotificationsEnabled val confirmedEnabled = confirmedValueOf(CALL_NOTIFICATIONS, _uiState.value.callNotificationsEnabled) @@ -1058,14 +1086,15 @@ class ConversationInfoViewModel @Inject constructor( setting: String, change: Long, newValue: Boolean, - description: String + description: String, + @StringRes failureMessage: Int = R.string.nc_common_error_sorry ) { val response = outcome.getOrNull() if (response != null && accepted(response)) { recordAccepted(setting, change, newValue) return } - _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry)) + _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(failureMessage)) val cause = outcome.exceptionOrNull() if (cause != null) { logger.e(TAG, "failed to toggle $description conversation state", cause) @@ -1098,6 +1127,7 @@ class ConversationInfoViewModel @Inject constructor( private const val CALL_NOTIFICATIONS = "call notifications" private const val NOTIFICATION_LEVEL = "notification level" private const val MESSAGE_EXPIRATION = "message expiration" + private const val LOCKED = "locked" private const val GROUP_CONVERSATION_TYPE = "2" private const val MAX_ROOM_NAME_LENGTH = 255 private const val NOTIFICATION_LEVEL_ALWAYS: Int = 1 From d917b3ac2aecd18f665ceb168642c50592fb217c Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 14:56:11 +0200 Subject: [PATCH 12/14] fix(conversation info): fail a notification level change that cannot be saved Without capabilities the change returned quietly and the screen kept a level that was never saved; it now fails, so the screen takes it back and says so. A server without notification levels is no longer sent a level that is stored locally instead, which matches how it behaved before. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../preferencestorage/DatabaseStorageModule.kt | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt index 604658b405f..b8660efcdb2 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt @@ -105,12 +105,13 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { } private suspend fun saveNotificationLevel(value: String) { - val spreedCapability = conversationUser.capabilities?.spreedCapability ?: return + val spreedCapability = checkNotNull(conversationUser.capabilities?.spreedCapability) { + "No spreed capabilities to set the notification level with" + } - if (!hasSpreedFeatureCapability(spreedCapability, SpreedFeatures.NOTIFICATION_LEVELS)) { - // a server that cannot be told keeps the level as whatever this screen remembers - messageNotificationLevel = value - } else if (messageNotificationLevel != value) { + if (hasSpreedFeatureCapability(spreedCapability, SpreedFeatures.NOTIFICATION_LEVELS) && + messageNotificationLevel != value + ) { val apiVersion = getConversationApiVersion(conversationUser, intArrayOf(ApiUtils.API_V4, 1)) withContext(Dispatchers.IO) { ncApiCoroutines!!.setNotificationLevel( From a418aee33cc9a32b13b85199535f8ffa612cb0c2 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 15:18:11 +0200 Subject: [PATCH 13/14] test(conversations): cover the optimistic settings and list actions The read-only lock, the settings saved through DatabaseStorageModule and the conversation list's read, unread, favorite and archive actions had no test of their own, nor had hiding a conversation that is being left or deleted. Each now has one for the change standing, being taken back, and two failed changes going back to the value the server confirmed. The view model's storage module is visible to tests so a failing one can be put in. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../network/OfflineFirstChatRepository.kt | 3 +- .../viewmodel/ConversationInfoViewModel.kt | 5 +- .../ConversationInfoStoredSettingsTest.kt | 209 +++++++++++++ .../ConversationInfoViewModelTest.kt | 84 ++++++ .../viewmodel/FakeConversationsRepository.kt | 23 +- ...tionsListViewModelOptimisticActionsTest.kt | 283 ++++++++++++++++++ 6 files changed, 601 insertions(+), 6 deletions(-) create mode 100644 app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoStoredSettingsTest.kt create mode 100644 app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelOptimisticActionsTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index 221bf6e18b8..25e0cbeaa1c 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -31,11 +31,10 @@ import com.nextcloud.talk.models.json.converters.EnumActorTypeConverter import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.participants.ParticipantDto import com.nextcloud.talk.utils.Undo -import com.nextcloud.talk.utils.bundle.BundleKeys import com.nextcloud.talk.utils.message.SendMessageUtils import com.nextcloud.talk.utils.optimisticAction -import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index ad6be94542b..a4278f49948 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -116,7 +116,10 @@ class ConversationInfoViewModel @Inject constructor( get() = _getUnBanActorState private var currentUser: User? = null private var currentToken: String = "" - private var databaseStorageModule: DatabaseStorageModule? = null + + @VisibleForTesting + internal var databaseStorageModule: DatabaseStorageModule? = null + val passwordValidation = PasswordPolicyValidator(passwordPolicyRepository, viewModelScope) { currentUser } private val _uiState = MutableStateFlow(ConversationInfoUiState()) val uiState: StateFlow = _uiState.asStateFlow() diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoStoredSettingsTest.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoStoredSettingsTest.kt new file mode 100644 index 00000000000..11a356bedd7 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoStoredSettingsTest.kt @@ -0,0 +1,209 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.conversationinfo.viewmodel + +import android.content.res.Resources +import com.nextcloud.talk.R +import com.nextcloud.talk.api.NcApi +import com.nextcloud.talk.application.NextcloudTalkApplication +import com.nextcloud.talk.chat.data.network.ChatNetworkDataSource +import com.nextcloud.talk.conversationinfo.ConversationInfoUiEvent +import com.nextcloud.talk.logger.Logger +import com.nextcloud.talk.repositories.passwordpolicy.PasswordPolicyRepository +import com.nextcloud.talk.utils.preferences.preferencestorage.DatabaseStorageModule +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.doSuspendableAnswer +import org.mockito.kotlin.mock +import org.mockito.kotlin.stub +import java.io.IOException + +/** + * The settings [ConversationInfoViewModel] saves through [DatabaseStorageModule]: call notifications, + * the notification level and the message expiration. A change that does not reach the server has to + * go back to the value the server last confirmed, and tell the user. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class ConversationInfoStoredSettingsTest { + + private val dispatcher = StandardTestDispatcher() + + private val storage = mock() + + @Before + fun setUp() { + Dispatchers.setMain(dispatcher) + // the notification level and the message expiration read their choices from the app resources + val resources = mock { + on { getStringArray(R.array.message_notification_levels_entry_values) } doReturn LEVEL_VALUES + on { getStringArray(R.array.message_notification_levels) } doReturn LEVEL_DESCRIPTIONS + on { getStringArray(R.array.message_expiring_values) } doReturn EXPIRATION_VALUES + on { getStringArray(R.array.message_expiring_descriptions) } doReturn EXPIRATION_DESCRIPTIONS + } + setSharedApplication(mock { on { this.resources } doReturn resources }) + } + + @After + fun tearDown() { + setSharedApplication(null) + Dispatchers.resetMain() + } + + private fun viewModel() = + ConversationInfoViewModel( + chatNetworkDataSource = mock(), + conversationsRepository = FakeConversationsRepository(), + ncApi = mock(), + passwordPolicyRepository = mock(), + logger = mock() + ).apply { databaseStorageModule = storage } + + private fun failEverySave() { + storage.stub { + onBlocking { saveBoolean(any(), any()) } doSuspendableAnswer { throw IOException("no connection") } + onBlocking { saveString(any(), any()) } doSuspendableAnswer { throw IOException("no connection") } + } + } + + @Test + fun `call notifications that cannot be saved go back and say so`() = + runTest(dispatcher) { + failEverySave() + val model = viewModel() + val seen = mutableListOf() + val collector = launch { model.uiEvent.collect { seen.add(it) } } + dispatcher.scheduler.runCurrent() + + model.toggleCallNotifications() + dispatcher.scheduler.runCurrent() + assertFalse(model.uiState.value.callNotificationsEnabled) + + dispatcher.scheduler.advanceUntilIdle() + + assertTrue(model.uiState.value.callNotificationsEnabled) + assertTrue(seen.contains(ConversationInfoUiEvent.ShowSnackbar(R.string.nc_common_error_sorry))) + collector.cancel() + } + + @Test + fun `call notifications that are saved stay changed`() = + runTest(dispatcher) { + val model = viewModel() + + model.toggleCallNotifications() + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.callNotificationsEnabled) + } + + @Test + fun `two call notification changes that both fail go back to the value the server confirmed`() = + runTest(dispatcher) { + failEverySave() + val model = viewModel() + + model.toggleCallNotifications() + dispatcher.scheduler.runCurrent() + model.toggleCallNotifications() + dispatcher.scheduler.runCurrent() + + dispatcher.scheduler.advanceUntilIdle() + + assertTrue(model.uiState.value.callNotificationsEnabled) + } + + @Test + fun `a notification level that cannot be saved goes back to the previous one`() = + runTest(dispatcher) { + failEverySave() + val model = viewModel() + + model.saveNotificationLevel(ALWAYS) + dispatcher.scheduler.runCurrent() + assertEquals(LEVEL_DESCRIPTIONS[ALWAYS], model.uiState.value.notificationLevel) + + dispatcher.scheduler.advanceUntilIdle() + + assertEquals("", model.uiState.value.notificationLevel) + } + + @Test + fun `two notification levels that both fail go back to the level the server confirmed`() = + runTest(dispatcher) { + failEverySave() + val model = viewModel() + + model.saveNotificationLevel(NEVER) + dispatcher.scheduler.runCurrent() + model.saveNotificationLevel(ALWAYS) + dispatcher.scheduler.runCurrent() + + dispatcher.scheduler.advanceUntilIdle() + + assertEquals("", model.uiState.value.notificationLevel) + } + + @Test + fun `a message expiration that cannot be saved goes back to the previous one`() = + runTest(dispatcher) { + failEverySave() + val model = viewModel() + + model.saveMessageExpiration(ONE_DAY) + dispatcher.scheduler.runCurrent() + assertEquals(EXPIRATION_DESCRIPTIONS[ONE_DAY], model.uiState.value.messageExpirationLabel) + + dispatcher.scheduler.advanceUntilIdle() + + assertEquals("", model.uiState.value.messageExpirationLabel) + } + + @Test + fun `a message expiration that is saved stays changed`() = + runTest(dispatcher) { + val model = viewModel() + + model.saveMessageExpiration(ONE_DAY) + dispatcher.scheduler.advanceUntilIdle() + + assertEquals(EXPIRATION_DESCRIPTIONS[ONE_DAY], model.uiState.value.messageExpirationLabel) + } + + // NextcloudTalkApplication.sharedApplication's setter is `protected` (only meant to be set + // from onCreate()/onTerminate()); reflection is the only way in from a test. + private fun setSharedApplication(application: NextcloudTalkApplication?) { + NextcloudTalkApplication.Companion::class.java + .getDeclaredMethod("setSharedApplication", NextcloudTalkApplication::class.java) + .apply { isAccessible = true } + .invoke(NextcloudTalkApplication.Companion, application) + } + + private companion object { + const val NEVER = 0 + const val ALWAYS = 2 + const val ONE_DAY = 3 + + val LEVEL_VALUES = arrayOf("never", "mention", "always") + val LEVEL_DESCRIPTIONS = arrayOf("Never", "Mentions", "Always") + val EXPIRATION_VALUES = arrayOf("expire_0", "expire_2419200", "expire_604800", "expire_86400") + val EXPIRATION_DESCRIPTIONS = arrayOf("Off", "4 weeks", "1 week", "1 day") + } +} diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt index 11133ca9b73..5b78047ae7b 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModelTest.kt @@ -7,6 +7,7 @@ package com.nextcloud.talk.conversationinfo.viewmodel +import com.nextcloud.talk.R import com.nextcloud.talk.api.NcApi import com.nextcloud.talk.chat.data.network.ChatNetworkDataSource import com.nextcloud.talk.conversationinfo.ConversationInfoUiEvent @@ -15,6 +16,7 @@ import com.nextcloud.talk.logger.Logger import com.nextcloud.talk.models.json.capabilities.CapabilitiesDto import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto import com.nextcloud.talk.repositories.passwordpolicy.PasswordPolicyRepository +import com.nextcloud.talk.utils.preferences.preferencestorage.DatabaseStorageModule import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.Dispatchers @@ -29,8 +31,10 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test +import org.mockito.kotlin.inOrder import org.mockito.kotlin.mock +@Suppress("TooManyFunctions") @OptIn(ExperimentalCoroutinesApi::class) class ConversationInfoViewModelTest { @@ -285,6 +289,86 @@ class ConversationInfoViewModelTest { assertTrue(model.uiState.value.importantConversation) } + // toggleLock(): the read-only lock goes through the same optimistic path as the other settings, + // and also mirrors its state into local storage, which the screen is built from + + @Test + fun `a lock the server never accepts goes back and says so`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingReadOnlyRequests = Int.MAX_VALUE } + val model = viewModel(repository) + val seen = mutableListOf() + val collector = launch { model.uiEvent.collect { seen.add(it) } } + dispatcher.scheduler.runCurrent() + + model.toggleLock(user, "token") + dispatcher.scheduler.runCurrent() + assertTrue(model.uiState.value.isConversationLocked) + + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.isConversationLocked) + assertTrue(seen.contains(ConversationInfoUiEvent.ShowSnackbar(R.string.conversation_read_only_failed))) + collector.cancel() + } + + @Test + fun `a lock survives a single connection problem`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingReadOnlyRequests = 1 } + val model = viewModel(repository) + + model.toggleLock(user, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertEquals(ATTEMPTS_PER_REQUEST, repository.readOnlyRequests) + assertTrue(model.uiState.value.isConversationLocked) + } + + @Test + fun `a lock the server refuses in the payload is taken back`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { readOnlyResponseStatusCode = HTTP_FORBIDDEN } + val model = viewModel(repository) + + model.toggleLock(user, "token") + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.isConversationLocked) + } + + @Test + fun `a failed lock takes the stored lock state back as well`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingReadOnlyRequests = Int.MAX_VALUE } + val storage = mock() + val model = viewModel(repository).apply { databaseStorageModule = storage } + + model.toggleLock(user, "token") + dispatcher.scheduler.advanceUntilIdle() + + val order = inOrder(storage) + order.verify(storage).saveBoolean("lock_switch", true) + order.verify(storage).saveBoolean("lock_switch", false) + } + + @Test + fun `two lock changes that both fail go back to the value the server confirmed`() = + runTest(dispatcher) { + val repository = FakeConversationsRepository().apply { failingReadOnlyRequests = Int.MAX_VALUE } + val model = viewModel(repository) + + model.toggleLock(user, "token") + dispatcher.scheduler.runCurrent() + model.toggleLock(user, "token") + dispatcher.scheduler.runCurrent() + assertFalse(model.uiState.value.isConversationLocked) + + dispatcher.scheduler.advanceUntilIdle() + + assertFalse(model.uiState.value.isConversationLocked) + } + private companion object { const val HTTP_FORBIDDEN = 403 diff --git a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt index 9ffcb08ad4c..bd10c586d56 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationinfo/viewmodel/FakeConversationsRepository.kt @@ -33,11 +33,18 @@ class FakeConversationsRepository : ConversationsRepository { var importantRequests = 0 /** The OCS status code the important-conversation requests answer with. */ - var importantResponseStatusCode = 200 + var importantResponseStatusCode = HTTP_OK /** When set, the next important-conversation request waits for this before it answers. */ var importantGate: CompletableDeferred? = null + /** How many of the next read-only requests fail with a connection problem. */ + var failingReadOnlyRequests = 0 + var readOnlyRequests = 0 + + /** The OCS status code the read-only requests answer with. */ + var readOnlyResponseStatusCode = HTTP_OK + override suspend fun allowGuests( user: User, url: String, @@ -79,8 +86,14 @@ class FakeConversationsRepository : ConversationsRepository { override suspend fun setPassword(user: User, url: String, password: String): GenericOverall = GenericOverall() - override suspend fun setConversationReadOnly(user: User, url: String, state: Int): GenericOverall = - throw UnsupportedOperationException() + override suspend fun setConversationReadOnly(user: User, url: String, state: Int): GenericOverall { + readOnlyRequests++ + if (failingReadOnlyRequests > 0) { + failingReadOnlyRequests-- + throw IOException("no connection") + } + return GenericOverall(GenericOCS(GenericMetaDto(statusCode = readOnlyResponseStatusCode))) + } override suspend fun clearChatHistory(user: User, url: String): GenericOverall = throw UnsupportedOperationException() @@ -144,4 +157,8 @@ class FakeConversationsRepository : ConversationsRepository { override suspend fun removeConversationFromFavorites(credentials: String, url: String): GenericOverall = throw UnsupportedOperationException() + + private companion object { + const val HTTP_OK = 200 + } } diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelOptimisticActionsTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelOptimisticActionsTest.kt new file mode 100644 index 00000000000..db5dfc7e8a8 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelOptimisticActionsTest.kt @@ -0,0 +1,283 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.conversationlist.viewmodels + +import android.app.Application +import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager +import com.nextcloud.talk.contacts.ContactsRepository +import com.nextcloud.talk.conversationlist.data.OfflineConversationsRepository +import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater +import com.nextcloud.talk.conversationlist.viewmodels.ConversationsListViewModel.ArchiveUiState +import com.nextcloud.talk.conversationlist.viewmodels.ConversationsListViewModel.ConversationReadUnreadUiState +import com.nextcloud.talk.conversationlist.viewmodels.ConversationsListViewModel.FavoriteUiState +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.invitation.data.InvitationsRepository +import com.nextcloud.talk.logger.Logger +import com.nextcloud.talk.models.domain.ConversationModel +import com.nextcloud.talk.models.json.capabilities.CapabilitiesDto +import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto +import com.nextcloud.talk.models.json.conversations.ConversationEnums +import com.nextcloud.talk.models.json.generic.GenericOverall +import com.nextcloud.talk.models.json.participants.ParticipantDto +import com.nextcloud.talk.openconversations.data.OpenConversationsRepository +import com.nextcloud.talk.repositories.conversations.ConversationsRepository +import com.nextcloud.talk.repositories.unifiedsearch.UnifiedSearchRepository +import com.nextcloud.talk.threadsoverview.data.ThreadsRepository +import com.nextcloud.talk.users.UserManager +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.emptyFlow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain +import kotlinx.coroutines.withTimeout +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.doSuspendableAnswer +import org.mockito.kotlin.eq +import org.mockito.kotlin.inOrder +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.stub +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyBlocking +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import retrofit2.HttpException +import retrofit2.Response + +/** + * The conversation list's optimistic actions: marking read or unread, favoriting and archiving are + * shown at once and taken back when the server does not accept them, and a conversation being left + * or deleted stays out of the list until its removal has gone through or failed. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class ConversationsListViewModelOptimisticActionsTest { + + private val repository: OfflineConversationsRepository = mock() + private val conversationsRepository: ConversationsRepository = mock() + private val conversationListUpdater: ConversationListUpdater = mock() + private val rooms = MutableStateFlow(listOf(CONVERSATION)) + + private lateinit var viewModel: ConversationsListViewModel + + @Before + fun setUp() { + Dispatchers.setMain(UnconfinedTestDispatcher()) + whenever(repository.roomListFlow(any())).thenReturn(rooms) + whenever(repository.syncErrorFlow).thenReturn(emptyFlow()) + + viewModel = ConversationsListViewModel( + repository, + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + mock(), + conversationsRepository, + conversationListUpdater, + mock(), + USER + ) + } + + @After + fun tearDown() { + Dispatchers.resetMain() + } + + @Test + fun `a favorite the server accepts stays applied`() { + conversationsRepository.stub { + onBlocking { addConversationToFavorites(any(), any()) } doReturn GenericOverall() + } + + viewModel.addConversationToFavorites(CONVERSATION) + + assertEquals(FavoriteUiState.Success, awaitOutcome(viewModel.favoriteState, FavoriteUiState.None)) + verifyBlocking(repository) { updateConversation(CONVERSATION.copy(favorite = true)) } + verifyBlocking(repository, never()) { updateConversation(CONVERSATION) } + verify(conversationListUpdater, never()).clearPendingFavorite(any(), any()) + } + + @Test + fun `a favorite the server refuses is taken back`() { + conversationsRepository.stub { + onBlocking { addConversationToFavorites(any(), any()) } doSuspendableAnswer { throw forbidden() } + } + + viewModel.addConversationToFavorites(CONVERSATION) + + assertEquals(FavoriteUiState.Error, awaitOutcome(viewModel.favoriteState, FavoriteUiState.None)) + assertTakenBack(CONVERSATION.copy(favorite = true)) + verify(conversationListUpdater).clearPendingFavorite(CONVERSATION.internalId, favorite = true) + } + + @Test + fun `an unfavorite the server refuses is taken back`() { + val favorite = CONVERSATION.copy(favorite = true) + conversationsRepository.stub { + onBlocking { removeConversationFromFavorites(any(), any()) } doSuspendableAnswer { throw forbidden() } + } + + viewModel.removeConversationFromFavorites(favorite) + + assertEquals(FavoriteUiState.Error, awaitOutcome(viewModel.favoriteState, FavoriteUiState.None)) + assertTakenBack(CONVERSATION, original = favorite) + verify(conversationListUpdater).clearPendingFavorite(CONVERSATION.internalId, favorite = false) + } + + @Test + fun `marking read that the server refuses is taken back`() { + val unread = CONVERSATION.copy(unreadMessages = UNREAD_MESSAGES) + conversationsRepository.stub { + onBlocking { markConversationAsRead(any(), any(), anyOrNull()) } doSuspendableAnswer { throw forbidden() } + } + + viewModel.markConversationAsRead(unread) + + assertEquals( + ConversationReadUnreadUiState.Error, + awaitOutcome(viewModel.readUnreadState, ConversationReadUnreadUiState.None) + ) + assertTakenBack(unread.copy(unreadMessages = 0), original = unread) + } + + @Test + fun `marking unread that the server refuses is taken back`() { + conversationsRepository.stub { + onBlocking { markConversationAsUnread(any(), any()) } doSuspendableAnswer { throw forbidden() } + } + + viewModel.markConversationAsUnread(CONVERSATION) + + assertEquals( + ConversationReadUnreadUiState.Error, + awaitOutcome(viewModel.readUnreadState, ConversationReadUnreadUiState.None) + ) + assertTakenBack(CONVERSATION.copy(unreadMessages = 1)) + verify(conversationListUpdater).clearPendingUnread(CONVERSATION.internalId) + } + + @Test + fun `an archive the server refuses is taken back`() { + conversationsRepository.stub { + onBlocking { archiveConversation(any(), any()) } doSuspendableAnswer { throw forbidden() } + } + + viewModel.toggleConversationArchive(CONVERSATION) + + assertEquals(ArchiveUiState.Error, awaitOutcome(viewModel.archiveState, ArchiveUiState.None)) + assertTakenBack(CONVERSATION.copy(hasArchived = true)) + verify(conversationListUpdater).clearPendingArchived(CONVERSATION.internalId, true) + } + + @Test + fun `a conversation being removed is hidden until the removal is undone`() { + viewModel.markConversationPendingRemoval(CONVERSATION.token) + assertFalse(isListed()) + + viewModel.clearConversationPendingRemoval(CONVERSATION.token) + assertTrue(isListed()) + } + + @Test + fun `a removed conversation stays hidden until the sync that drops it has finished`() { + val sync = Job() + whenever(repository.getRooms(eq(USER), eq(true))).thenReturn(sync) + viewModel.markConversationPendingRemoval(CONVERSATION.token) + + viewModel.onConversationRemoved(USER, CONVERSATION.token) + assertFalse(isListed()) + + sync.complete() + assertTrue(isListed()) + } + + private fun isListed(): Boolean = viewModel.visibleRoomsFlow.value.any { it.token == CONVERSATION.token } + + /** Waits for the action launched on the view model scope, whose local writes run on the IO dispatcher. */ + private fun awaitOutcome(state: StateFlow, pending: S): S = + runBlocking { withTimeout(TIMEOUT_MS) { state.first { it != pending } } } + + private fun assertTakenBack(optimistic: ConversationModel, original: ConversationModel = CONVERSATION) { + val order = inOrder(repository) + runBlocking { + order.verify(repository).updateConversation(optimistic) + order.verify(repository).updateConversation(original) + } + } + + private fun forbidden(): HttpException = + HttpException(Response.error(HTTP_FORBIDDEN, "".toResponseBody("text/plain".toMediaType()))) + + companion object { + private const val HTTP_FORBIDDEN = 403 + private const val UNREAD_MESSAGES = 3 + private const val TIMEOUT_MS = 5_000L + + private val USER = User( + id = 1, + userId = "me", + username = "me", + token = "app-password", + baseUrl = "https://server.example.com", + capabilities = CapabilitiesDto().apply { + spreedCapability = SpreedCapabilityDto().apply { + features = listOf("conversation-v4", "chat-v2") + } + } + ) + + private val CONVERSATION = ConversationModel( + internalId = "1@token", + accountId = 1L, + token = "token", + name = "conversation", + displayName = "Conversation", + description = "", + type = ConversationEnums.ConversationType.ROOM_GROUP_CALL, + participantType = ParticipantDto.ParticipantType.USER, + sessionId = "", + actorId = "me", + actorType = "users", + objectType = ConversationEnums.ObjectType.DEFAULT, + notificationLevel = ConversationEnums.NotificationLevel.DEFAULT, + conversationReadOnlyState = ConversationEnums.ConversationReadOnlyState.CONVERSATION_READ_WRITE, + lobbyState = ConversationEnums.LobbyState.LOBBY_STATE_ALL_PARTICIPANTS, + lobbyTimer = 0L, + canLeaveConversation = true, + canDeleteConversation = true, + unreadMentionDirect = false, + notificationCalls = 0, + avatarVersion = "", + hasCustomAvatar = false, + callStartTime = 0L + ) + } +} From d440ed1d2ea2b3cc15378446002db355591bd544 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 12 Oct 2026 15:39:20 +0200 Subject: [PATCH 14/14] fix(conversation info): undo a setting the server refuses in the payload Call notifications, the notification level and the message expiration only failed on an HTTP error, so a refusal reported in the OCS payload left the screen and the local copy showing a value the server never took. The save now fails on such a refusal before anything is remembered, and the screen takes the change back like any other failure. The acceptance rule the read-only lock and the important and sensitive toggles already used moves to GenericOverall, so all of them share it. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe --- .../viewmodel/ConversationInfoViewModel.kt | 19 +- .../models/json/generic/GenericOverall.kt | 11 ++ .../DatabaseStorageModule.kt | 12 +- .../DatabaseStorageModuleTest.kt | 163 ++++++++++++++++++ 4 files changed, 189 insertions(+), 16 deletions(-) create mode 100644 app/src/test/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModuleTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index a4278f49948..36761633d8b 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt @@ -31,6 +31,7 @@ import com.nextcloud.talk.models.json.autocomplete.AutocompleteUserDto import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto import com.nextcloud.talk.models.json.conversations.ConversationEnums import com.nextcloud.talk.models.json.generic.GenericOverall +import com.nextcloud.talk.models.json.generic.isAccepted import com.nextcloud.talk.models.json.participants.ParticipantDto import com.nextcloud.talk.models.json.participants.ParticipantDto.ActorType.CIRCLES import com.nextcloud.talk.models.json.participants.ParticipantDto.ActorType.EMAILS @@ -821,7 +822,7 @@ class ConversationInfoViewModel @Inject constructor( showLocked(newLocked) undoLockIfNewest(change, confirmedLocked) }, - isConfirmed = ::accepted, + isConfirmed = GenericOverall::isAccepted, request = { val apiVersion = ApiUtils.getConversationApiVersion( user, @@ -1015,14 +1016,6 @@ class ConversationInfoViewModel @Inject constructor( private fun isNewestChange(setting: String, change: Long) = newestSettingChange[setting] == change - /** - * An OCS payload can report a refusal in a response the HTTP layer calls a success, so the status - * code decides whether the optimistic change stands. A payload that carries no status code at all - * is taken at face value, since there is nothing in it to refuse with. - */ - private fun accepted(response: GenericOverall): Boolean = - response.ocs?.meta?.statusCode.let { it == null || it == NO_STATUS_CODE || it == HTTP_OK } - fun toggleImportantConversation(credentials: String, baseUrl: String, roomToken: String) { val displayedValue = _uiState.value.importantConversation val newValue = !displayedValue @@ -1037,7 +1030,7 @@ class ConversationInfoViewModel @Inject constructor( it.copy(importantConversation = confirmedValueOf(IMPORTANT, confirmedValue)) } }, - isConfirmed = ::accepted, + isConfirmed = GenericOverall::isAccepted, request = { if (newValue) { conversationsRepository.markConversationAsImportant(credentials, baseUrl, roomToken) @@ -1064,7 +1057,7 @@ class ConversationInfoViewModel @Inject constructor( it.copy(sensitiveConversation = confirmedValueOf(SENSITIVE, confirmedValue)) } }, - isConfirmed = ::accepted, + isConfirmed = GenericOverall::isAccepted, request = { if (newValue) { conversationsRepository.markConversationAsSensitive(credentials, baseUrl, roomToken) @@ -1093,7 +1086,7 @@ class ConversationInfoViewModel @Inject constructor( @StringRes failureMessage: Int = R.string.nc_common_error_sorry ) { val response = outcome.getOrNull() - if (response != null && accepted(response)) { + if (response != null && response.isAccepted()) { recordAccepted(setting, change, newValue) return } @@ -1121,8 +1114,6 @@ class ConversationInfoViewModel @Inject constructor( companion object { private val TAG = ConversationInfoViewModel::class.java.simpleName - private const val HTTP_OK = 200 - private const val NO_STATUS_CODE = 0 private const val NEW_CONVERSATION_PARTICIPANTS_SEPARATOR = ", " private const val EXTENDED_CONVERSATION = "extended_conversation" private const val IMPORTANT = "important" diff --git a/app/src/main/java/com/nextcloud/talk/models/json/generic/GenericOverall.kt b/app/src/main/java/com/nextcloud/talk/models/json/generic/GenericOverall.kt index f95bae6840d..8ebcf5f1c32 100644 --- a/app/src/main/java/com/nextcloud/talk/models/json/generic/GenericOverall.kt +++ b/app/src/main/java/com/nextcloud/talk/models/json/generic/GenericOverall.kt @@ -21,3 +21,14 @@ data class GenericOverall( // This constructor is added to work with the 'com.bluelinelabs.logansquare.annotation.JsonObject' constructor() : this(null) } + +private const val HTTP_OK = 200 +private const val NO_STATUS_CODE = 0 + +/** + * Whether the server accepted the request. An OCS payload can report a refusal in a response the HTTP + * layer calls a success, so its status code decides. A payload that carries no status code at all is + * taken at face value, since there is nothing in it to refuse with. + */ +fun GenericOverall.isAccepted(): Boolean = + ocs?.meta?.statusCode.let { it == null || it == NO_STATUS_CODE || it == HTTP_OK } diff --git a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt index b8660efcdb2..27800c57ae3 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModule.kt @@ -16,6 +16,8 @@ import com.nextcloud.talk.application.NextcloudTalkApplication.Companion.sharedA import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager import com.nextcloud.talk.data.storage.model.ArbitraryStorage import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.models.json.generic.GenericOverall +import com.nextcloud.talk.models.json.generic.isAccepted import com.nextcloud.talk.utils.ApiUtils import com.nextcloud.talk.utils.ApiUtils.getConversationApiVersion import com.nextcloud.talk.utils.ApiUtils.getCredentials @@ -69,6 +71,7 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { val notificationLevel = if (value) 1 else 0 withContext(Dispatchers.IO) { ncApiCoroutines!!.notificationCalls(credentials!!, url, notificationLevel) + .requireAccepted("call notifications") } } if ("lobby_switch" != key) { @@ -99,7 +102,7 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { getCredentials(conversationUser.username, conversationUser.token)!!, getUrlForMessageExpiration(apiVersion, conversationUser.baseUrl, conversationToken), valueInt - ) + ).requireAccepted("the message expiration") messageExpiration = valueInt } } @@ -118,12 +121,17 @@ class DatabaseStorageModule(conversationUser: User, conversationToken: String) { getCredentials(conversationUser.username, conversationUser.token)!!, getUrlForRoomNotificationLevel(apiVersion, conversationUser.baseUrl, conversationToken), notificationLevelOf(value) - ) + ).requireAccepted("the notification level") messageNotificationLevel = value } } } + /** Fails the save when the server refused it in the payload, before anything is remembered locally. */ + private fun GenericOverall.requireAccepted(setting: String) { + check(isAccepted()) { "The server refused to save $setting, OCS status ${ocs?.meta?.statusCode}" } + } + private fun notificationLevelOf(value: String): Int = when (value) { "never" -> NOTIFICATION_NEVER diff --git a/app/src/test/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModuleTest.kt b/app/src/test/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModuleTest.kt new file mode 100644 index 00000000000..37dd3072de8 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/utils/preferences/preferencestorage/DatabaseStorageModuleTest.kt @@ -0,0 +1,163 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ + +package com.nextcloud.talk.utils.preferences.preferencestorage + +import com.nextcloud.talk.api.NcApiCoroutines +import com.nextcloud.talk.application.NextcloudTalkApplication +import com.nextcloud.talk.application.NextcloudTalkApplicationComponent +import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.models.json.capabilities.CapabilitiesDto +import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto +import com.nextcloud.talk.models.json.generic.GenericMetaDto +import com.nextcloud.talk.models.json.generic.GenericOCS +import com.nextcloud.talk.models.json.generic.GenericOverall +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.stub +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyBlocking + +/** + * The conversation settings [DatabaseStorageModule] sends to the server. A refusal the server reports in + * the payload rather than in the status code has to fail the save, and must not be remembered locally. + */ +class DatabaseStorageModuleTest { + + private val api = mock() + private val storage = mock() + + private lateinit var module: DatabaseStorageModule + + @Before + fun setUp() { + // the module injects its dependencies from the application component when it is created + val component = mock() + setSharedApplication(mock { on { componentApplication } doReturn component }) + + module = DatabaseStorageModule(USER, "token").apply { + ncApiCoroutines = api + arbitraryStorageManager = storage + } + } + + @After + fun tearDown() { + setSharedApplication(null) + } + + @Test + fun `call notifications the server accepts are stored`() = + runTest { + api.stub { onBlocking { notificationCalls(any(), any(), any()) } doReturn answer(HTTP_OK) } + + module.saveBoolean(CALL_NOTIFICATIONS, true) + + verify(storage).storeStorageSetting(any(), any(), anyOrNull(), anyOrNull()) + } + + @Test + fun `call notifications the server refuses in the payload fail and are not stored`() = + runTest { + api.stub { onBlocking { notificationCalls(any(), any(), any()) } doReturn answer(HTTP_FORBIDDEN) } + + val result = runCatching { module.saveBoolean(CALL_NOTIFICATIONS, true) } + + assertTrue(result.exceptionOrNull() is IllegalStateException) + verify(storage, never()).storeStorageSetting(any(), any(), anyOrNull(), anyOrNull()) + } + + @Test + fun `a message expiration the server accepts is remembered`() = + runTest { + api.stub { onBlocking { setMessageExpiration(any(), any(), any()) } doReturn answer(HTTP_OK) } + + module.saveString(MESSAGE_EXPIRATION, ONE_HOUR) + + assertEquals(ONE_HOUR, module.getString(MESSAGE_EXPIRATION, "")) + } + + @Test + fun `a message expiration the server refuses in the payload fails and is not remembered`() = + runTest { + api.stub { onBlocking { setMessageExpiration(any(), any(), any()) } doReturn answer(HTTP_FORBIDDEN) } + + val result = runCatching { module.saveString(MESSAGE_EXPIRATION, ONE_HOUR) } + + assertTrue(result.exceptionOrNull() is IllegalStateException) + assertEquals(EXPIRATION_OFF, module.getString(MESSAGE_EXPIRATION, "")) + } + + @Test + fun `a notification level the server refuses in the payload fails and is sent again next time`() = + runTest { + api.stub { onBlocking { setNotificationLevel(any(), any(), any()) } doReturn answer(HTTP_FORBIDDEN) } + + val result = runCatching { module.saveString(NOTIFICATION_LEVEL, "always") } + runCatching { module.saveString(NOTIFICATION_LEVEL, "always") } + + assertTrue(result.exceptionOrNull() is IllegalStateException) + verifyBlocking(api, times(2)) { setNotificationLevel(any(), any(), any()) } + } + + @Test + fun `a notification level the server accepts is not sent again`() = + runTest { + api.stub { onBlocking { setNotificationLevel(any(), any(), any()) } doReturn answer(HTTP_OK) } + + module.saveString(NOTIFICATION_LEVEL, "always") + module.saveString(NOTIFICATION_LEVEL, "always") + + verifyBlocking(api, times(1)) { setNotificationLevel(any(), any(), any()) } + } + + private fun answer(statusCode: Int) = GenericOverall(GenericOCS(GenericMetaDto(statusCode = statusCode))) + + // NextcloudTalkApplication.sharedApplication's setter is `protected` (only meant to be set + // from onCreate()/onTerminate()); reflection is the only way in from a test. + private fun setSharedApplication(application: NextcloudTalkApplication?) { + NextcloudTalkApplication.Companion::class.java + .getDeclaredMethod("setSharedApplication", NextcloudTalkApplication::class.java) + .apply { isAccessible = true } + .invoke(NextcloudTalkApplication.Companion, application) + } + + private companion object { + const val HTTP_OK = 200 + const val HTTP_FORBIDDEN = 403 + + const val CALL_NOTIFICATIONS = "call_notifications_switch" + const val MESSAGE_EXPIRATION = "conversation_settings_dropdown" + const val NOTIFICATION_LEVEL = "conversation_info_message_notifications_dropdown" + const val ONE_HOUR = "expire_3600" + const val EXPIRATION_OFF = "expire_0" + + val USER = User( + id = 1, + userId = "me", + username = "me", + token = "app-password", + baseUrl = "https://server.example.com", + capabilities = CapabilitiesDto().apply { + spreedCapability = SpreedCapabilityDto().apply { + features = listOf("conversation-v4", "notification-levels") + } + } + ) + } +}