From 9581994050f0802fdf45be854c1581d2226e8ebc Mon Sep 17 00:00:00 2001 From: jeffrey-signal Date: Thu, 26 Feb 2026 10:34:16 -0500 Subject: [PATCH] Handle network and permissions errors when saving group member label. --- .../ConversationSettingsFragment.kt | 2 +- .../GroupInsufficientRightsException.java | 2 +- .../groups/memberlabel/MemberLabelFragment.kt | 48 +++++++++++++++---- .../memberlabel/MemberLabelRepository.kt | 13 ++--- .../memberlabel/MemberLabelViewModel.kt | 24 +++++++++- app/src/main/res/values/strings.xml | 16 ++++--- .../memberlabel/MemberLabelViewModelTest.kt | 43 +++++++++++++++++ 7 files changed, 119 insertions(+), 29 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsFragment.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsFragment.kt index a3bd2d1dd8..c273cb3f24 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsFragment.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsFragment.kt @@ -863,7 +863,7 @@ class ConversationSettingsFragment : navController.safeNavigate(action) }, onDisabledClicked = { - Snackbar.make(requireView(), R.string.ConversationSettingsFragment__only_admins_can_add_member_labels, Snackbar.LENGTH_SHORT).show() + Snackbar.make(requireView(), R.string.GroupMemberLabel__error_no_edit_permission, Snackbar.LENGTH_SHORT).show() } ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupInsufficientRightsException.java b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupInsufficientRightsException.java index 24554b4a65..1209f6735a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/GroupInsufficientRightsException.java +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/GroupInsufficientRightsException.java @@ -2,7 +2,7 @@ package org.thoughtcrime.securesms.groups; public final class GroupInsufficientRightsException extends GroupChangeException { - GroupInsufficientRightsException(Throwable throwable) { + public GroupInsufficientRightsException(Throwable throwable) { super(throwable); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelFragment.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelFragment.kt index 9c73728484..f9f427bb12 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelFragment.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelFragment.kt @@ -21,6 +21,8 @@ import androidx.compose.foundation.verticalScroll import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.SnackbarHost +import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text import androidx.compose.material3.TextFieldDefaults import androidx.compose.runtime.Composable @@ -42,6 +44,7 @@ import androidx.core.os.bundleOf import androidx.lifecycle.compose.collectAsStateWithLifecycle import org.signal.core.ui.compose.AllDevicePreviews import org.signal.core.ui.compose.Buttons +import org.signal.core.ui.compose.CircularProgressWrapper import org.signal.core.ui.compose.ClearableTextField import org.signal.core.ui.compose.ComposeFragment import org.signal.core.ui.compose.Previews @@ -87,6 +90,7 @@ class MemberLabelFragment : ComposeFragment(), ReactWithAnyEmojiBottomSheetDialo override fun FragmentContent() { val uiState by viewModel.uiState.collectAsStateWithLifecycle() val backPressedDispatcher = LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher + val snackbarHostState = remember { SnackbarHostState() } val callbacks = remember { object : MemberLabelUiCallbacks { @@ -102,16 +106,34 @@ class MemberLabelFragment : ComposeFragment(), ReactWithAnyEmojiBottomSheetDialo } } + val networkErrorMessage = stringResource(R.string.GroupMemberLabel__error_cant_save_no_network) + val noPermissionErrorMessage = stringResource(R.string.GroupMemberLabel__error_no_edit_permission) + LaunchedEffect(uiState.saveState) { - if (uiState.saveState is SaveState.Success) { - backPressedDispatcher?.onBackPressed() - viewModel.onSaveStateConsumed() + when (uiState.saveState) { + is SaveState.Success -> { + backPressedDispatcher?.onBackPressed() + viewModel.onSaveStateConsumed() + } + + is SaveState.NetworkError -> { + snackbarHostState.showSnackbar(networkErrorMessage) + viewModel.onSaveStateConsumed() + } + + is SaveState.InsufficientRights -> { + snackbarHostState.showSnackbar(noPermissionErrorMessage) + viewModel.onSaveStateConsumed() + } + + is SaveState.InProgress, null -> Unit } } MemberLabelScreenUi( state = uiState, - callbacks = callbacks + callbacks = callbacks, + snackbarHostState = snackbarHostState ) } @@ -127,13 +149,15 @@ class MemberLabelFragment : ComposeFragment(), ReactWithAnyEmojiBottomSheetDialo @Composable private fun MemberLabelScreenUi( state: MemberLabelUiState, - callbacks: MemberLabelUiCallbacks + callbacks: MemberLabelUiCallbacks, + snackbarHostState: SnackbarHostState = remember { SnackbarHostState() } ) { Scaffolds.Settings( title = stringResource(R.string.GroupMemberLabel__title), onNavigationClick = callbacks::onClosePressed, navigationIcon = SignalIcons.X.imageVector, - navigationContentDescription = stringResource(R.string.GroupMemberLabel__accessibility_close_screen) + navigationContentDescription = stringResource(R.string.GroupMemberLabel__accessibility_close_screen), + snackbarHost = { SnackbarHost(snackbarHostState) } ) { paddingValues -> val focusRequester = remember { FocusRequester() } @@ -191,13 +215,17 @@ private fun MemberLabelScreenUi( } } - SaveButton( - enabled = state.isSaveEnabled, - onClick = callbacks::onSaveClicked, + CircularProgressWrapper( + isLoading = state.saveState is SaveState.InProgress, modifier = Modifier .align(Alignment.BottomEnd) .padding(end = 24.dp, bottom = 16.dp) - ) + ) { + SaveButton( + enabled = state.isSaveEnabled, + onClick = callbacks::onSaveClicked + ) + } } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelRepository.kt index 358baabb5d..875a4fd860 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelRepository.kt @@ -22,6 +22,7 @@ import org.thoughtcrime.securesms.groups.GroupManager import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.util.RemoteConfig +import org.whispersystems.signalservice.api.NetworkResult /** * Handles the retrieval and modification of group member labels. @@ -52,12 +53,6 @@ class MemberLabelRepository private constructor( @WorkerThread fun getLabelJava(groupId: GroupId.V2, recipient: Recipient): MemberLabel? = runBlocking { getLabel(groupId, recipient) } - /** - * Checks whether the [Recipient] has permission to set their member label in the given group (blocking version for Java compatibility). - */ - @WorkerThread - fun canSetLabelJava(groupId: GroupId.V2, recipient: Recipient): Boolean = runBlocking { canSetLabel(groupId, recipient) } - /** * Gets the member label for a specific recipient in the group. */ @@ -117,13 +112,15 @@ class MemberLabelRepository private constructor( /** * Sets the group member label for the current user. */ - suspend fun setLabel(groupId: GroupId.V2, label: MemberLabel): Unit = withContext(Dispatchers.IO) { + suspend fun setLabel(groupId: GroupId.V2, label: MemberLabel): NetworkResult = withContext(Dispatchers.IO) { if (!RemoteConfig.sendMemberLabels) { throw IllegalStateException("Set member label not allowed due to remote config.") } val sanitizedLabel = label.sanitized() - GroupManager.updateMemberLabel(context, groupId, sanitizedLabel.text, sanitizedLabel.emoji.orEmpty()) + NetworkResult.fromFetch { + GroupManager.updateMemberLabel(context, groupId, sanitizedLabel.text, sanitizedLabel.emoji.orEmpty()) + } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModel.kt index cbfb62cbba..af792e05d4 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModel.kt @@ -16,9 +16,11 @@ import org.signal.core.util.StringUtil import org.signal.core.util.concurrent.SignalDispatchers import org.thoughtcrime.securesms.conversation.colors.NameColor import org.thoughtcrime.securesms.groups.GroupId +import org.thoughtcrime.securesms.groups.GroupInsufficientRightsException import org.thoughtcrime.securesms.groups.memberlabel.MemberLabelUiState.SaveState import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.recipients.RecipientId +import org.whispersystems.signalservice.api.NetworkResult class MemberLabelViewModel( private val memberLabelRepo: MemberLabelRepository = MemberLabelRepository.instance, @@ -97,7 +99,7 @@ class MemberLabelViewModel( } val currentState = internalUiState.value - memberLabelRepo.setLabel( + val result = memberLabelRepo.setLabel( groupId = groupId, label = MemberLabel( emoji = currentState.labelEmoji.ifEmpty { null }, @@ -105,8 +107,24 @@ class MemberLabelViewModel( ) ) + val newSaveState: SaveState = when (result) { + is NetworkResult.Success -> SaveState.Success + + is NetworkResult.NetworkError<*> -> SaveState.NetworkError + + is NetworkResult.ApplicationError<*> -> { + if (result.throwable is GroupInsufficientRightsException) { + SaveState.InsufficientRights + } else { + throw result.throwable + } + } + + is NetworkResult.StatusCodeError<*> -> throw result.exception + } + internalUiState.update { - it.copy(saveState = SaveState.Success) + it.copy(saveState = newSaveState) } } } @@ -142,5 +160,7 @@ data class MemberLabelUiState( sealed interface SaveState { data object InProgress : SaveState data object Success : SaveState + data object NetworkError : SaveState + data object InsufficientRights : SaveState } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index d628e9c685..a6bca092bd 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -330,7 +330,7 @@ Send - + Recent contacts Signal contacts Signal groups @@ -4383,7 +4383,6 @@ Update now - Security setup @@ -4957,7 +4956,7 @@ Error connecting to service Backups - + Unlock Signal Use your Android device lock settings to unlock Signal. @@ -5986,8 +5985,6 @@ Group link Member Label - - Only admins can add member labels in this group. Add as a contact Unmute @@ -7894,7 +7891,7 @@ Turn on Signal Secure Backups - + Never lose a message when you get a new phone or reinstall Signal. Turn on @@ -9379,13 +9376,18 @@ Hello! Save + + Add a member label to describe yourself or your role in this group. Labels are only visible within this group. + + Couldn\'t save label. Check your network and try again. + + Only admins can add member labels in this group. Select emoji Close screen Clear label - Add a member label to describe yourself or your role in this group. Labels are only visible within this group. Member labels diff --git a/app/src/test/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModelTest.kt b/app/src/test/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModelTest.kt index e4ae3c8977..269137cbf6 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModelTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/groups/memberlabel/MemberLabelViewModelTest.kt @@ -19,8 +19,12 @@ import org.junit.Rule import org.junit.Test import org.thoughtcrime.securesms.conversation.colors.NameColor import org.thoughtcrime.securesms.groups.GroupId +import org.thoughtcrime.securesms.groups.GroupInsufficientRightsException +import org.thoughtcrime.securesms.groups.memberlabel.MemberLabelUiState.SaveState import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.testing.CoroutineDispatcherRule +import org.whispersystems.signalservice.api.NetworkResult +import java.io.IOException @OptIn(ExperimentalCoroutinesApi::class) class MemberLabelViewModelTest { @@ -180,6 +184,7 @@ class MemberLabelViewModelTest { @Test fun `save calls setLabel with truncated label when label exceeds max length`() = runTest(testDispatcher) { coEvery { memberLabelRepo.getLabel(groupId, any()) } returns null + coEvery { memberLabelRepo.setLabel(any(), any()) } returns NetworkResult.Success(Unit) val viewModel = MemberLabelViewModel(memberLabelRepo, groupId, recipientId) viewModel.onLabelTextChanged("A".repeat(30)) @@ -207,6 +212,7 @@ class MemberLabelViewModelTest { @Test fun `save calls setLabel when label change is valid`() = runTest(testDispatcher) { coEvery { memberLabelRepo.getLabel(groupId, any()) } returns MemberLabel(emoji = null, text = "Original") + coEvery { memberLabelRepo.setLabel(any(), any()) } returns NetworkResult.Success(Unit) val viewModel = MemberLabelViewModel(memberLabelRepo, groupId, recipientId) viewModel.onLabelTextChanged("New Label") @@ -221,6 +227,7 @@ class MemberLabelViewModelTest { @Test fun `save calls setLabel with cleared values when clearLabel is called`() = runTest(testDispatcher) { coEvery { memberLabelRepo.getLabel(groupId, any()) } returns MemberLabel(emoji = "🎉", text = "Original") + coEvery { memberLabelRepo.setLabel(any(), any()) } returns NetworkResult.Success(Unit) val viewModel = MemberLabelViewModel(memberLabelRepo, groupId, recipientId) viewModel.clearLabel() @@ -314,4 +321,40 @@ class MemberLabelViewModelTest { assertTrue(viewModel.uiState.value.isSaveEnabled) } + + @Test + fun `save sets saveState to Success when setLabel returns NetworkResult Success`() = runTest(testDispatcher) { + coEvery { memberLabelRepo.getLabel(groupId, any()) } returns MemberLabel(emoji = null, text = "Original") + coEvery { memberLabelRepo.setLabel(any(), any()) } returns NetworkResult.Success(Unit) + + val viewModel = MemberLabelViewModel(memberLabelRepo, groupId, recipientId) + viewModel.onLabelTextChanged("New Label") + viewModel.save() + + assertEquals(SaveState.Success, viewModel.uiState.value.saveState) + } + + @Test + fun `save sets saveState to NetworkError when setLabel returns NetworkResult NetworkError`() = runTest(testDispatcher) { + coEvery { memberLabelRepo.getLabel(groupId, any()) } returns MemberLabel(emoji = null, text = "Original") + coEvery { memberLabelRepo.setLabel(any(), any()) } returns NetworkResult.NetworkError(IOException("Network failure")) + + val viewModel = MemberLabelViewModel(memberLabelRepo, groupId, recipientId) + viewModel.onLabelTextChanged("New Label") + viewModel.save() + + assertEquals(SaveState.NetworkError, viewModel.uiState.value.saveState) + } + + @Test + fun `save sets saveState to InsufficientRights when setLabel returns ApplicationError with GroupInsufficientRightsException`() = runTest(testDispatcher) { + coEvery { memberLabelRepo.getLabel(groupId, any()) } returns MemberLabel(emoji = null, text = "Original") + coEvery { memberLabelRepo.setLabel(any(), any()) } returns NetworkResult.ApplicationError(GroupInsufficientRightsException(RuntimeException("Insufficient rights (test)"))) + + val viewModel = MemberLabelViewModel(memberLabelRepo, groupId, recipientId) + viewModel.onLabelTextChanged("New Label") + viewModel.save() + + assertEquals(SaveState.InsufficientRights, viewModel.uiState.value.saveState) + } }