diff --git a/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt b/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt index 0dd3f2bfa9..6a51f00de5 100644 --- a/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt +++ b/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt @@ -30,6 +30,7 @@ import com.nextcloud.talk.models.json.participants.TalkBanOverall import com.nextcloud.talk.models.json.passwordResult.PasswordResultOverall import com.nextcloud.talk.models.json.profile.ProfileOverall import com.nextcloud.talk.models.json.reactions.ReactionsOverall +import com.nextcloud.talk.models.json.reminder.ReminderOverall import com.nextcloud.talk.models.json.status.StatusOverall import com.nextcloud.talk.models.json.status.predefined.PredefinedStatusOverall import com.nextcloud.talk.models.json.tags.AssignConversationTagsRequestDto @@ -625,4 +626,30 @@ interface NcApiCoroutines { @GET suspend fun getUserProfile(@Header("Authorization") authorization: String, @Url url: String): UserProfileOverall + + @GET + suspend fun getReminder(@Header("Authorization") authorization: String, @Url url: String): ReminderOverall + + @DELETE + suspend fun deleteReminder(@Header("Authorization") authorization: String, @Url url: String): GenericOverall + + @FormUrlEncoded + @POST + suspend fun setReminder( + @Header("Authorization") authorization: String, + @Url url: String, + @Field("timestamp") timestamp: Int + ): ReminderOverall + + @Suppress("LongParameterList") + @FormUrlEncoded + @POST + suspend fun sendLocation( + @Header("Authorization") authorization: String, + @Url url: String, + @Field("objectType") objectType: String, + @Field("objectId") objectId: String, + @Field("metaData") metaData: String, + @Field("referenceId") referenceId: String + ): GenericOverall } diff --git a/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt b/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt index 164916cc03..f908072178 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt @@ -1540,6 +1540,12 @@ class ChatActivity : } } + lifecycleScope.launch { + chatViewModel.actionFailures.collect { message -> + Snackbar.make(binding.root, message, Snackbar.LENGTH_LONG).show() + } + } + lifecycleScope.launch { chatViewModel.noMoreSearchResults.collect { val inSearchMode = chatViewModel.chatMode.value == ChatViewModel.ChatMode.SEARCH_MODE diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt index 2b96e46943..553f1e1aea 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt @@ -26,32 +26,37 @@ interface ChatNetworkDataSource { suspend fun getRoom(user: User, roomToken: String): ConversationModel fun getCapabilities(user: User, roomToken: String): Observable fun joinRoom(user: User, roomToken: String, roomPassword: String): Observable - fun setReminder( + suspend fun setReminder( user: User, roomToken: String, messageId: String, timeStamp: Int, chatApiVersion: Int - ): Observable + ): ReminderDto - fun getReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): Observable - fun deleteReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): Observable - fun shareToNotes( + suspend fun getReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): ReminderDto + suspend fun deleteReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): GenericOverall + + @Suppress("LongParameterList") + suspend fun shareToNotes( credentials: String, url: String, message: String, - displayName: String - ): Observable + displayName: String, + referenceId: String + ): ChatOverallSingleMessage suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall - fun shareLocationToNotes( + @Suppress("LongParameterList") + suspend fun shareLocationToNotes( credentials: String, url: String, objectType: String, objectId: String, - metadata: String - ): Observable + metadata: String, + referenceId: String + ): GenericOverall suspend fun leaveRoom(credentials: String, url: String): GenericOverall suspend fun sendChatMessage( diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt index 9392602aaa..606642563c 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt @@ -21,7 +21,6 @@ import com.nextcloud.talk.models.json.reminder.ReminderDto import com.nextcloud.talk.models.json.upcomingEvents.UpcomingEventsOverall import com.nextcloud.talk.models.json.userAbsence.UserAbsenceOverall import com.nextcloud.talk.utils.ApiUtils -import com.nextcloud.talk.utils.message.SendMessageUtils import io.reactivex.Observable import retrofit2.Response @@ -59,81 +58,61 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines: ).map { ConversationModel.mapToConversationModel(it.ocs?.data!!, user) } } - override fun setReminder( + override suspend fun setReminder( user: User, roomToken: String, messageId: String, timeStamp: Int, chatApiVersion: Int - ): Observable { - val credentials: String = ApiUtils.getCredentials(user.username, user.token)!! - return ncApi.setReminder( - credentials, + ): ReminderDto = + ncApiCoroutines.setReminder( + ApiUtils.getCredentials(user.username, user.token)!!, ApiUtils.getUrlForReminder(user, roomToken, messageId, chatApiVersion), timeStamp - ).map { - it.ocs!!.data - } - } + ).ocs!!.data!! - override fun getReminder( + override suspend fun getReminder( user: User, roomToken: String, messageId: String, chatApiVersion: Int - ): Observable { - val credentials: String = ApiUtils.getCredentials(user.username, user.token)!! - return ncApi.getReminder( - credentials, + ): ReminderDto = + ncApiCoroutines.getReminder( + ApiUtils.getCredentials(user.username, user.token)!!, ApiUtils.getUrlForReminder(user, roomToken, messageId, chatApiVersion) - ).map { - it.ocs!!.data - } - } + ).ocs!!.data!! - override fun deleteReminder( + override suspend fun deleteReminder( user: User, roomToken: String, messageId: String, chatApiVersion: Int - ): Observable { - val credentials: String = ApiUtils.getCredentials(user.username, user.token)!! - return ncApi.deleteReminder( - credentials, + ): GenericOverall = + ncApiCoroutines.deleteReminder( + ApiUtils.getCredentials(user.username, user.token)!!, ApiUtils.getUrlForReminder(user, roomToken, messageId, chatApiVersion) - ).map { - it - } - } + ) - override fun shareToNotes( + override suspend fun shareToNotes( credentials: String, url: String, message: String, - displayName: String - ): Observable = - ncApi.sendChatMessage( - credentials, - url, - message, - displayName, - null, - false, - SendMessageUtils().generateReferenceId() - ).map { - it - } + displayName: String, + referenceId: String + ): ChatOverallSingleMessage = + ncApiCoroutines.sendChatMessage(credentials, url, message, displayName, 0, false, referenceId, null) override suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall = ncApiCoroutines.getNoteToSelfRoom(credentials, url) - override fun shareLocationToNotes( + override suspend fun shareLocationToNotes( credentials: String, url: String, objectType: String, objectId: String, - metadata: String - ): Observable = ncApi.sendLocation(credentials, url, objectType, objectId, metadata).map { it } + metadata: String, + referenceId: String + ): GenericOverall = ncApiCoroutines.sendLocation(credentials, url, objectType, objectId, metadata, referenceId) override suspend fun leaveRoom(credentials: String, url: String): GenericOverall = ncApiCoroutines.leaveRoom(credentials, url) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 5a3e643797..276a406d3c 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -12,6 +12,8 @@ import android.net.Uri import android.os.Bundle import android.provider.OpenableColumns import android.util.Log +import androidx.annotation.StringRes +import androidx.annotation.VisibleForTesting import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LiveData @@ -60,7 +62,6 @@ import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.ConversationEnums import com.nextcloud.talk.models.json.conversations.RoomOverall -import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.opengraph.OpenGraphObjectDto import com.nextcloud.talk.models.json.reminder.ReminderDto import com.nextcloud.talk.models.json.threads.ThreadInfoDto @@ -82,6 +83,7 @@ import com.nextcloud.talk.utils.UserIdUtils import com.nextcloud.talk.utils.throttleLatest import com.nextcloud.talk.utils.bundle.BundleKeys import com.nextcloud.talk.utils.database.user.CurrentUserProvider +import com.nextcloud.talk.utils.isTransientFailure import com.nextcloud.talk.utils.message.SendMessageUtils import com.nextcloud.talk.utils.message.groupHashOf import com.nextcloud.talk.utils.preferences.AppPreferences @@ -637,6 +639,10 @@ class ChatViewModel @AssistedInject constructor( private val _reactionFailures = MutableSharedFlow(extraBufferCapacity = 1) val reactionFailures: SharedFlow = _reactionFailures + /** A reminder or a share to notes that the server refused, for the chat to report. */ + private val _actionFailures = MutableSharedFlow(extraBufferCapacity = 1) + val actionFailures: SharedFlow = _actionFailures + private val reactionLocks = mutableMapOf() @Volatile private var firstUnreadMessageId: Int? = null @@ -1843,17 +1849,21 @@ class ChatViewModel @AssistedInject constructor( } fun setReminder(user: User, roomToken: String, messageId: String, timestamp: Int, chatApiVersion: Int) { - chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(SetReminderObserver()) + requestWithFeedback("set the reminder", R.string.nc_common_error_sorry) { + chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion) + } } + @Suppress("Detekt.TooGenericExceptionCaught") fun getReminder(user: User, roomToken: String, messageId: String, chatApiVersion: Int) { - chatNetworkDataSource.getReminder(user, roomToken, messageId, chatApiVersion) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(GetReminderObserver()) + viewModelScope.launch { + _getReminderExistState.value = try { + GetReminderExistState(chatNetworkDataSource.getReminder(user, roomToken, messageId, chatApiVersion)) + } catch (e: Exception) { + logger.d(TAG, "Error when getting reminder", e) + GetReminderStartState + } + } } fun overrideReminderState() { @@ -1861,26 +1871,10 @@ class ChatViewModel @AssistedInject constructor( } fun deleteReminder(user: User, roomToken: String, messageId: String, chatApiVersion: Int) { - chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(object : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(genericOverall: GenericOverall) { - _getReminderExistState.value = GetReminderStartState - } - - override fun onError(e: Throwable) { - Log.d(TAG, "Error when deleting reminder", e) - } - - override fun onComplete() { - // unused atm - } - }) + requestWithFeedback("delete the reminder", R.string.nc_common_error_sorry) { + chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion) + _getReminderExistState.value = GetReminderStartState + } } fun leaveRoom(credentials: String, url: String, functionToCallAfterLeave: (() -> Unit)?) { @@ -2140,26 +2134,9 @@ class ChatViewModel @AssistedInject constructor( } fun shareToNotes(credentials: String, url: String, message: String, displayName: String) { - chatNetworkDataSource.shareToNotes(credentials, url, message, displayName) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(object : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(genericOverall: ChatOverallSingleMessage) { - // unused atm - } - - override fun onError(e: Throwable) { - Log.d(TAG, "Error when sharing to notes $e") - } - - override fun onComplete() { - // unused atm - } - }) + shareToNotes("share the message to notes", credentials, url) { referenceId -> + chatNetworkDataSource.shareToNotes(credentials, url, message, displayName, referenceId) + } } suspend fun checkForNoteToSelf(credentials: String, baseUrl: String): ConversationModel? = @@ -2182,26 +2159,98 @@ class ChatViewModel @AssistedInject constructor( } fun shareLocationToNotes(credentials: String, url: String, objectType: String, objectId: String, metadata: String) { - chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(object : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } + shareToNotes("share the location to notes", credentials, url) { referenceId -> + chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata, referenceId) + } + } - override fun onNext(genericOverall: GenericOverall) { - // unused atm - } + /** + * Shares to the note to self and, when the share failed for a transient reason, asks the note to + * self whether the message arrived anyway before sending it a second time. The server stores the + * reference id of a message without ever looking at it again, so it deduplicates nothing: a blind + * second attempt after a timeout the server did accept posts the note twice. + */ + private fun shareToNotes( + description: String, + credentials: String, + notesUrl: String, + share: suspend (String) -> Unit + ) { + val referenceId = SendMessageUtils().generateReferenceId() - override fun onError(e: Throwable) { - Log.e(TAG, "Error when sharing location to notes $e") - } + viewModelScope.launch { + val failure = attemptShare(share, referenceId) ?: return@launch - override fun onComplete() { - // unused atm - } - }) + if (!isTransientFailure(failure)) { + reportShareFailure(description, failure) + return@launch + } + + if (!alreadyShared(credentials, notesUrl, referenceId)) { + attemptShare(share, referenceId)?.let { reportShareFailure(description, it) } + } + } + } + + /** Runs the share once and reports what went wrong, or null when it worked. */ + @Suppress("Detekt.TooGenericExceptionCaught") + private suspend fun attemptShare(share: suspend (String) -> Unit, referenceId: String): Exception? = + try { + share(referenceId) + null + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + e + } + + /** + * Whether the note to self already holds the message of this share. A lookup that cannot be + * answered counts as "it might", because posting a second copy is worse than not retrying. + */ + @Suppress("Detekt.TooGenericExceptionCaught") + private suspend fun alreadyShared(credentials: String, notesUrl: String, referenceId: String): Boolean = + try { + val fieldMap = hashMapOf( + "lookIntoFuture" to 0, + "includeLastKnown" to 1, + "setReadMarker" to 0, + "markNotificationsAsRead" to 0, + "timeout" to 0, + "limit" to SHARE_LOOKUP_LIMIT + ) + val recent = chatNetworkDataSource.pullChatMessages(credentials, notesUrl, fieldMap) + holdsShare(recent.body()?.ocs?.data, referenceId) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + logger.w(TAG, "Could not check whether the share arrived, so it is not sent again", e) + true + } + + private fun reportShareFailure(description: String, throwable: Throwable) { + logger.e(TAG, "Failed to $description", throwable) + _actionFailures.tryEmit(R.string.nc_message_not_added_to_notes) + } + + /** + * Runs an action whose only visible outcome is whether it worked. The reminder dialog is already + * dismissed and the chat has already said the message was sent by the time the answer arrives, so + * a failure that is not reported here is never reported at all. + * + * Deliberately without a retry: sharing to notes posts a message with a reference id generated per + * call, so a second attempt after a timeout the server did accept would post it twice. + */ + @Suppress("Detekt.TooGenericExceptionCaught") + private fun requestWithFeedback(description: String, @StringRes failureMessage: Int, request: suspend () -> Unit) { + viewModelScope.launch { + try { + request() + } catch (e: Exception) { + logger.e(TAG, "Failed to $description", e) + _actionFailures.tryEmit(failureMessage) + } + } } fun deleteReaction(roomToken: String, chatMessage: ChatMessage, emoji: String) { @@ -2489,43 +2538,6 @@ class ChatViewModel @AssistedInject constructor( } } - inner class SetReminderObserver : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(reminder: ReminderDto) { - Log.d(TAG, "reminder set successfully") - } - - override fun onError(e: Throwable) { - Log.e(TAG, "Error when sending reminder, $e") - } - - override fun onComplete() { - // unused atm - } - } - - inner class GetReminderObserver : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(reminder: ReminderDto) { - _getReminderExistState.value = GetReminderExistState(reminder) - } - - override fun onError(e: Throwable) { - Log.d(TAG, "Error when getting reminder $e") - _getReminderExistState.value = GetReminderStartState - } - - override fun onComplete() { - // unused atm - } - } - @Suppress("Detekt.TooGenericExceptionCaught") fun outOfOfficeStatusOfUser(credentials: String, baseUrl: String, userId: String) { viewModelScope.launch { @@ -2693,6 +2705,18 @@ class ChatViewModel @AssistedInject constructor( companion object { private val TAG = ChatViewModel::class.java.simpleName + /** Enough of the note to self to cover what a share of ours could have landed behind. */ + private const val SHARE_LOOKUP_LIMIT = 20 + + /** + * Whether [messages] already hold the share that was sent with [referenceId]. Messages the + * server did not return - because the lookup failed or answered "not modified" - are read as + * "it may well be there", since sending a second copy is worse than not sending one at all. + */ + @VisibleForTesting + fun holdsShare(messages: List?, referenceId: String): Boolean = + messages?.any { it.referenceId == referenceId } ?: true + /** * Returns the read marker that makes [messageId] the first unread message: the id of the * message right before it, or 0 when it is the oldest message there is, which marks the whole diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 45bcaed003..b4f776e943 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -558,6 +558,7 @@ How to translate with transifex: Scheduled thread Reply to thread: %1$s Message added to notes + Could not add the message to notes Failed to send message Failed to send message: Add attachment diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt index 39838d7eb2..e67dbec3da 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt @@ -10,8 +10,10 @@ package com.nextcloud.talk.chat.viewmodels import com.nextcloud.talk.chat.ui.model.ChatMessageUi import com.nextcloud.talk.chat.ui.model.MessageStatusIcon import com.nextcloud.talk.chat.ui.model.MessageTypeContent +import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.utils.message.SendMessageUtils import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -62,6 +64,36 @@ class ChatViewModelTest { assertNull(ChatViewModel.findFirstUnreadMessageId(messages, lastReadMessage = 40)) } + // holdsShare(): whether a share to the note to self that failed transiently already arrived. + // The server stores a reference id without ever looking at it again, so it deduplicates nothing + // and a second attempt that is not needed posts the message twice. + + @Test + fun `a share the note to self already holds is not sent again`() { + val messages = listOf(messageWithReference("other-1"), messageWithReference("ours")) + + assertTrue(ChatViewModel.holdsShare(messages, "ours")) + } + + @Test + fun `a share that did not arrive is sent again`() { + val messages = listOf(messageWithReference("other-1"), messageWithReference("other-2")) + + assertFalse(ChatViewModel.holdsShare(messages, "ours")) + } + + @Test + fun `a lookup that could not be answered does not send the share again`() { + assertTrue(ChatViewModel.holdsShare(null, "ours")) + } + + @Test + fun `messages without a reference id of their own are no proof either way`() { + assertFalse(ChatViewModel.holdsShare(listOf(messageWithReference(null)), "ours")) + } + + private fun messageWithReference(referenceId: String?) = ChatMessageDto(referenceId = referenceId) + // combineFileShareGroups(): combining batch-uploaded file shares into grouped "album" bubbles. // Mirrors web's combineFileMessages.ts tests - see https://github.com/nextcloud/spreed/pull/19040