Skip to content

Render blocked user data (behind a disabled feature flag) #2930

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/1.misc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.misc? Should we use 2930.misc instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oups, good catch 21802be

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a feature flag ShowBlockedUsersDetails, disabled by default to render display name and avatar of blocked users in the blocked users list.
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,26 @@ import androidx.compose.runtime.MutableState
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.produceState
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.architecture.Presenter
import io.element.android.libraries.architecture.runUpdatingState
import io.element.android.libraries.featureflag.api.FeatureFlagService
import io.element.android.libraries.featureflag.api.FeatureFlags
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.user.MatrixUser
import kotlinx.collections.immutable.toPersistentList
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import javax.inject.Inject

class BlockedUsersPresenter @Inject constructor(
private val matrixClient: MatrixClient,
private val featureFlagService: FeatureFlagService,
) : Presenter<BlockedUsersState> {
@Composable
override fun present(): BlockedUsersState {
Expand All @@ -47,7 +53,24 @@ class BlockedUsersPresenter @Inject constructor(
mutableStateOf(AsyncAction.Uninitialized)
}

val renderBlockedUsersDetail = featureFlagService
.isFeatureEnabledFlow(FeatureFlags.ShowBlockedUsersDetails)
.collectAsState(initial = false)
val ignoredUserIds by matrixClient.ignoredUsersFlow.collectAsState()
val ignoredMatrixUser by produceState(
initialValue = ignoredUserIds.map { MatrixUser(userId = it) },
key1 = renderBlockedUsersDetail.value,
key2 = ignoredUserIds
) {
value = ignoredUserIds.map {
if (renderBlockedUsersDetail.value) {
matrixClient.getProfile(it).getOrNull()
} else {
null
}
?: MatrixUser(userId = it)
}
}

fun handleEvents(event: BlockedUsersEvents) {
when (event) {
Expand All @@ -68,7 +91,7 @@ class BlockedUsersPresenter @Inject constructor(
}
}
return BlockedUsersState(
blockedUsers = ignoredUserIds,
blockedUsers = ignoredMatrixUser.toPersistentList(),
unblockUserAction = unblockUserAction.value,
eventSink = ::handleEvents
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
package io.element.android.features.preferences.impl.blockedusers

import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.user.MatrixUser
import kotlinx.collections.immutable.ImmutableList

data class BlockedUsersState(
val blockedUsers: ImmutableList<UserId>,
val blockedUsers: ImmutableList<MatrixUser>,
val unblockUserAction: AsyncAction<Unit>,
val eventSink: (BlockedUsersEvents) -> Unit,
)
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,31 @@ package io.element.android.features.preferences.impl.blockedusers

import androidx.compose.ui.tooling.preview.PreviewParameterProvider
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.user.MatrixUser
import io.element.android.libraries.matrix.ui.components.aMatrixUserList
import kotlinx.collections.immutable.toPersistentList

class BlockedUsersStatePreviewProvider : PreviewParameterProvider<BlockedUsersState> {
override val values: Sequence<BlockedUsersState>
get() = sequenceOf(
aBlockedUsersState(),
aBlockedUsersState(blockedUsers = aMatrixUserList().map { it.copy(displayName = null, avatarUrl = null) }),
aBlockedUsersState(blockedUsers = emptyList()),
aBlockedUsersState(unblockUserAction = AsyncAction.Confirming),
// Sadly there's no good way to preview Loading or Failure states since they're presented with an animation
// All these 3 screen states will be displayed as the Uninitialized one
aBlockedUsersState(unblockUserAction = AsyncAction.Loading),
aBlockedUsersState(unblockUserAction = AsyncAction.Failure(Throwable("Failed to unblock user"))),
aBlockedUsersState(unblockUserAction = AsyncAction.Success(Unit)),
)
}

internal fun aBlockedUsersState(
blockedUsers: List<UserId> = aMatrixUserList().map { it.userId },
blockedUsers: List<MatrixUser> = aMatrixUserList(),
unblockUserAction: AsyncAction<Unit> = AsyncAction.Uninitialized,
eventSink: (BlockedUsersEvents) -> Unit = {},
): BlockedUsersState {
return BlockedUsersState(
blockedUsers = blockedUsers.toPersistentList(),
unblockUserAction = unblockUserAction,
eventSink = {},
eventSink = eventSink,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ fun BlockedUsersView(
LazyColumn(
modifier = Modifier.padding(padding)
) {
items(state.blockedUsers) { userId ->
items(state.blockedUsers) { matrixUser ->
BlockedUserItem(
userId = userId,
matrixUser = matrixUser,
onClick = { state.eventSink(BlockedUsersEvents.Unblock(it)) }
)
}
Expand Down Expand Up @@ -121,12 +121,12 @@ fun BlockedUsersView(

@Composable
private fun BlockedUserItem(
userId: UserId,
matrixUser: MatrixUser,
onClick: (UserId) -> Unit,
) {
MatrixUserRow(
modifier = Modifier.clickable { onClick(userId) },
matrixUser = MatrixUser(userId),
modifier = Modifier.clickable { onClick(matrixUser.userId) },
matrixUser = matrixUser,
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
* Copyright (c) 2024 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.element.android.features.preferences.impl.blockedusers

import androidx.activity.ComponentActivity
import androidx.compose.ui.test.junit4.AndroidComposeTestRule
import androidx.compose.ui.test.junit4.createAndroidComposeRule
import androidx.compose.ui.test.onNodeWithText
import androidx.compose.ui.test.performClick
import androidx.test.ext.junit.runners.AndroidJUnit4
import io.element.android.features.preferences.impl.R
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.matrix.ui.components.aMatrixUserList
import io.element.android.libraries.ui.strings.CommonStrings
import io.element.android.tests.testutils.EnsureNeverCalled
import io.element.android.tests.testutils.EventsRecorder
import io.element.android.tests.testutils.clickOn
import io.element.android.tests.testutils.ensureCalledOnce
import io.element.android.tests.testutils.pressBack
import org.junit.Rule
import org.junit.Test
import org.junit.rules.TestRule
import org.junit.runner.RunWith

@RunWith(AndroidJUnit4::class)
class BlockedUserViewTest {
@get:Rule
val rule = createAndroidComposeRule<ComponentActivity>()

@Test
fun `clicking on back invokes back callback`() {
val eventsRecorder = EventsRecorder<BlockedUsersEvents>(expectEvents = false)
ensureCalledOnce { callback ->
rule.setLogoutView(
aBlockedUsersState(
eventSink = eventsRecorder
),
onBackClicked = callback,
)
rule.pressBack()
}
}

@Test
fun `clicking on a user emits the expected Event`() {
val eventsRecorder = EventsRecorder<BlockedUsersEvents>()
val userList = aMatrixUserList()
rule.setLogoutView(
aBlockedUsersState(
blockedUsers = userList,
eventSink = eventsRecorder
),
)
rule.onNodeWithText(userList.first().displayName.orEmpty()).performClick()
eventsRecorder.assertSingle(BlockedUsersEvents.Unblock(userList.first().userId))
}

@Test
fun `clicking on cancel sends a BlockedUsersEvents`() {
val eventsRecorder = EventsRecorder<BlockedUsersEvents>()
rule.setLogoutView(
aBlockedUsersState(
unblockUserAction = AsyncAction.Confirming,
eventSink = eventsRecorder
),
)
rule.clickOn(CommonStrings.action_cancel)
eventsRecorder.assertSingle(BlockedUsersEvents.Cancel)
}

@Test
fun `clicking on confirm sends a BlockedUsersEvents`() {
val eventsRecorder = EventsRecorder<BlockedUsersEvents>()
rule.setLogoutView(
aBlockedUsersState(
unblockUserAction = AsyncAction.Confirming,
eventSink = eventsRecorder
),
)
rule.clickOn(R.string.screen_blocked_users_unblock_alert_action)
eventsRecorder.assertSingle(BlockedUsersEvents.ConfirmUnblock)
}
}

private fun <R : TestRule> AndroidComposeTestRule<R, ComponentActivity>.setLogoutView(
state: BlockedUsersState,
onBackClicked: () -> Unit = EnsureNeverCalled(),
) {
setContent {
BlockedUsersView(
state = state,
onBackPressed = onBackClicked,
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ import app.cash.molecule.moleculeFlow
import app.cash.turbine.test
import com.google.common.truth.Truth.assertThat
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.featureflag.api.FeatureFlagService
import io.element.android.libraries.featureflag.api.FeatureFlags
import io.element.android.libraries.featureflag.test.FakeFeatureFlagService
import io.element.android.libraries.matrix.api.user.MatrixUser
import io.element.android.libraries.matrix.test.AN_EXCEPTION
import io.element.android.libraries.matrix.test.A_USER_ID
import io.element.android.libraries.matrix.test.A_USER_ID_2
import io.element.android.libraries.matrix.test.FakeMatrixClient
Expand Down Expand Up @@ -52,7 +57,7 @@ class BlockedUsersPresenterTests {
presenter.present()
}.test {
with(awaitItem()) {
assertThat(blockedUsers).isEqualTo(persistentListOf(A_USER_ID))
assertThat(blockedUsers).isEqualTo(persistentListOf(MatrixUser(A_USER_ID)))
assertThat(unblockUserAction).isEqualTo(AsyncAction.Uninitialized)
}
}
Expand All @@ -68,14 +73,39 @@ class BlockedUsersPresenterTests {
presenter.present()
}.test {
with(awaitItem()) {
assertThat(blockedUsers).containsAtLeastElementsIn(persistentListOf(A_USER_ID))
assertThat(unblockUserAction).isEqualTo(AsyncAction.Uninitialized)
assertThat(blockedUsers).isEqualTo(listOf(MatrixUser(A_USER_ID)))
}

matrixClient.ignoredUsersFlow.value = persistentListOf(A_USER_ID, A_USER_ID_2)
skipItems(1)
with(awaitItem()) {
assertThat(blockedUsers).isEqualTo(persistentListOf(A_USER_ID, A_USER_ID_2))
assertThat(unblockUserAction).isEqualTo(AsyncAction.Uninitialized)
assertThat(blockedUsers).isEqualTo(listOf(MatrixUser(A_USER_ID), MatrixUser(A_USER_ID_2)))
}
}
}

@Test
fun `present - blocked users list with data`() = runTest {
val alice = MatrixUser(A_USER_ID, displayName = "Alice", avatarUrl = "aliceAvatar")
val matrixClient = FakeMatrixClient().apply {
ignoredUsersFlow.value = persistentListOf(A_USER_ID, A_USER_ID_2)
givenGetProfileResult(A_USER_ID, Result.success(alice))
givenGetProfileResult(A_USER_ID_2, Result.failure(AN_EXCEPTION))
}
val presenter = aBlockedUsersPresenter(
matrixClient = matrixClient,
featureFlagService = FakeFeatureFlagService().apply {
setFeatureEnabled(FeatureFlags.ShowBlockedUsersDetails, true)
}
)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
with(awaitItem()) {
assertThat(blockedUsers).isEqualTo(listOf(MatrixUser(A_USER_ID), MatrixUser(A_USER_ID_2)))
}
// Alice is resolved
with(awaitItem()) {
assertThat(blockedUsers).isEqualTo(listOf(alice, MatrixUser(A_USER_ID_2)))
}
}
}
Expand Down Expand Up @@ -157,5 +187,9 @@ class BlockedUsersPresenterTests {

private fun aBlockedUsersPresenter(
matrixClient: FakeMatrixClient = FakeMatrixClient(),
) = BlockedUsersPresenter(matrixClient)
featureFlagService: FeatureFlagService = FakeFeatureFlagService(),
) = BlockedUsersPresenter(
matrixClient = matrixClient,
featureFlagService = featureFlagService,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,12 @@ enum class FeatureFlags(
description = "Allow user to search for public rooms in their homeserver",
defaultValue = false,
isFinished = false,
)
),
ShowBlockedUsersDetails(
key = "feature.showBlockedUsersDetails",
title = "Show blocked users details",
description = "Show the name and avatar of blocked users in the blocked users list",
defaultValue = false,
isFinished = false,
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
FeatureFlags.Mentions -> true
FeatureFlags.MarkAsUnread -> true
FeatureFlags.RoomDirectorySearch -> false
FeatureFlags.ShowBlockedUsersDetails -> false

Check warning on line 44 in libraries/featureflag/impl/src/main/kotlin/io/element/android/libraries/featureflag/impl/StaticFeatureFlagProvider.kt

View check run for this annotation

Codecov / codecov/patch

libraries/featureflag/impl/src/main/kotlin/io/element/android/libraries/featureflag/impl/StaticFeatureFlagProvider.kt#L44

Added line #L44 was not covered by tests
}
} else {
false
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading