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..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 @@ -30,11 +30,11 @@ 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.message.SendMessageUtils -import com.nextcloud.talk.utils.revertOnCancellation -import com.nextcloud.talk.utils.withRetry -import kotlinx.coroutines.Dispatchers +import com.nextcloud.talk.utils.optimisticAction import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow @@ -809,39 +809,30 @@ 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 * 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 @@ -893,39 +884,28 @@ 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, * 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 @@ -1019,12 +999,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 +1012,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 +1033,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 +1253,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/conversationinfo/viewmodel/ConversationInfoViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationinfo/viewmodel/ConversationInfoViewModel.kt index d4c27267bac..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 @@ -6,6 +6,8 @@ */ 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 import androidx.lifecycle.LiveData @@ -29,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 @@ -51,6 +54,8 @@ 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 import io.reactivex.android.schedulers.AndroidSchedulers @@ -112,7 +117,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() @@ -409,6 +417,12 @@ class ConversationInfoViewModel @Inject constructor( val showSensitiveConversation = hasSpreedFeatureCapability(spreedCapabilities, SpreedFeatures.SENSITIVE_CONVERSATIONS) val sensitiveConversation = conversationModel.hasSensitive + confirmedSetting[IMPORTANT] = importantConversation + confirmedSetting[SENSITIVE] = sensitiveConversation + confirmedSetting[CALL_NOTIFICATIONS] = callNotificationsEnabled + confirmedSetting[NOTIFICATION_LEVEL] = notificationLevelStr + confirmedSetting[MESSAGE_EXPIRATION] = messageExpirationLabel + confirmedSetting[LOCKED] = isConversationLocked val showArchiveConversation = hasSpreedFeatureCapability(spreedCapabilities, SpreedFeatures.ARCHIVE_CONVERSATIONS) @@ -797,34 +811,72 @@ 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 = GenericOverall::isAccepted, + 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 - _uiState.update { it.copy(callNotificationsEnabled = newEnabled) } + val confirmedEnabled = confirmedValueOf(CALL_NOTIFICATIONS, _uiState.value.callNotificationsEnabled) + 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 = confirmedValueOf(CALL_NOTIFICATIONS, confirmedEnabled)) + } + }, + request = { databaseStorageModule?.saveBoolean("call_notifications_switch", newEnabled) } + ) + .onSuccess { recordAccepted(CALL_NOTIFICATIONS, change, newEnabled) } + .onFailure { throwable -> reportSettingFailure("call notifications", throwable) } } } @@ -832,11 +884,27 @@ 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 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 = confirmedValueOf(NOTIFICATION_LEVEL, confirmedLevel)) + } + }, + request = { + databaseStorageModule?.saveString( + "conversation_info_message_notifications_dropdown", + values[position] + ) + } + ) + .onSuccess { recordAccepted(NOTIFICATION_LEVEL, change, descriptions[position]) } + .onFailure { throwable -> reportSettingFailure("the notification level", throwable) } } } @@ -844,14 +912,30 @@ 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 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 = confirmedValueOf(MESSAGE_EXPIRATION, confirmedLabel)) + } + }, + request = { databaseStorageModule?.saveString("conversation_settings_dropdown", values[position]) } + ) + .onSuccess { recordAccepted(MESSAGE_EXPIRATION, change, descriptions[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) } } @@ -868,43 +952,150 @@ class ConversationInfoViewModel @Inject constructor( _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbarText(text)) } - @Suppress("Detekt.TooGenericExceptionCaught") + 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() + + /** + * 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 + * 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 + ): Undo = { if (isNewestChange(setting, change)) _uiState.update(undo) } + + private fun isNewestChange(setting: String, change: Long) = newestSettingChange[setting] == change + 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 + val confirmedValue = confirmedValueOf(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 = confirmedValueOf(IMPORTANT, confirmedValue)) + } + }, + isConfirmed = GenericOverall::isAccepted, + 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 + val confirmedValue = confirmedValueOf(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 = confirmedValueOf(SENSITIVE, confirmedValue)) + } + }, + isConfirmed = GenericOverall::isAccepted, + 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. + */ + @Suppress("Detekt.LongParameterList") + private suspend fun reportSettingOutcome( + outcome: Result, + setting: String, + change: Long, + newValue: Boolean, + description: String, + @StringRes failureMessage: Int = R.string.nc_common_error_sorry + ) { + val response = outcome.getOrNull() + if (response != null && response.isAccepted()) { + recordAccepted(setting, change, newValue) + return + } + _uiEvent.emit(ConversationInfoUiEvent.ShowSnackbar(failureMessage)) + 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") } } @@ -925,6 +1116,12 @@ class ConversationInfoViewModel @Inject constructor( private val TAG = ConversationInfoViewModel::class.java.simpleName 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 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 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/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 29082a46a5a..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 @@ -41,8 +41,9 @@ 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.withRetry +import com.nextcloud.talk.utils.optimisticAction import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject @@ -255,10 +256,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 +401,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() { @@ -638,15 +639,15 @@ class ConversationsListViewModel @AssistedInject constructor( } /** - * 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 + * catching up. */ - fun onConversationLeft(user: User, token: String) { + fun onConversationRemoved(user: User, token: String) { val job = getRooms(user, forceFullSync = true) viewModelScope.launch { job.join() - clearConversationPendingLeave(token) + clearConversationPendingRemoval(token) } } @@ -868,23 +869,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 +900,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): Undo = + { + releaseGuard() + applyLocally(original) + } + fun resetFavoriteState() { _favoriteState.value = FavoriteUiState.None } @@ -934,29 +949,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 +983,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 +1011,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/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/repositories/reactions/ReactionsRepositoryImpl.kt b/app/src/main/java/com/nextcloud/talk/repositories/reactions/ReactionsRepositoryImpl.kt index 46c683df547..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 @@ -12,12 +12,11 @@ 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.Undo +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 +44,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 } + ).isAccepted(ADD_SUCCESS_CODES) return ReactionAddedModel(message, emoji, confirmed) } @@ -77,77 +67,45 @@ 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) } + ).isAccepted(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: Undo): Undo? = revert.takeIf { applied } + + /** 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 @@ -191,12 +149,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..a0e7c97498b --- /dev/null +++ b/app/src/main/java/com/nextcloud/talk/utils/OptimisticAction.kt @@ -0,0 +1,76 @@ +/* + * 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.CancellationException +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 + +/** 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. + */ +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. 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 () -> 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: Undo = { withContext(NonCancellable) { revert?.invoke() } } + + val answer = try { + revertOnCancellation(revertUninterruptibly) { + withRetry(retries = 1, initialDelayMillis = RETRY_DELAY_MS, retryOn = ::isTransientFailure) { + request() + } + } + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + revertUninterruptibly() + 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/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..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 @@ -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 @@ -18,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 @@ -63,7 +63,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 +70,8 @@ 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) + .requireAccepted("call notifications") } } if ("lobby_switch" != key) { @@ -91,67 +86,60 @@ 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 + ).requireAccepted("the message expiration") + messageExpiration = valueInt + } + } + + private suspend fun saveNotificationLevel(value: String) { + val spreedCapability = checkNotNull(conversationUser.capabilities?.spreedCapability) { + "No spreed capabilities to set the notification level with" + } + + if (hasSpreedFeatureCapability(spreedCapability, SpreedFeatures.NOTIFICATION_LEVELS) && + 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) + ).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 + "mention" -> NOTIFICATION_MENTION + "always" -> NOTIFICATION_ALWAYS + else -> 0 + } + fun getBoolean(key: String, defaultVal: Boolean): Boolean = if ("lobby_switch" == key) { lobbyValue @@ -186,7 +174,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 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 c7250b4be1f..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,15 +7,20 @@ 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 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 com.nextcloud.talk.utils.preferences.preferencestorage.DatabaseStorageModule 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,10 +28,13 @@ 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.inOrder import org.mockito.kotlin.mock +@Suppress("TooManyFunctions") @OptIn(ExperimentalCoroutinesApi::class) class ConversationInfoViewModelTest { @@ -110,6 +118,264 @@ 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) + } + + // 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 + + /** 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..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 @@ -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,23 @@ 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 = 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, @@ -65,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() @@ -93,13 +120,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() @@ -112,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 + ) + } +} 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..a8c3cb1de3a --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/utils/OptimisticActionTest.kt @@ -0,0 +1,161 @@ +/* + * 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 () -> Undo? = { + 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 `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 { + 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 + } +} 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") + } + } + ) + } +}