Skip to content
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

AsyncAction confirming with param #3667

Merged
merged 4 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class AccountDeactivationPresenter @Inject constructor(
action
)
} else {
action.value = AsyncAction.Confirming
action.value = AsyncAction.ConfirmingNoParams
}
AccountDeactivationEvents.CloseDialogs -> {
action.value = AsyncAction.Uninitialized
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ open class AccountDeactivationStateProvider : PreviewParameterProvider<AccountDe
),
anAccountDeactivationState(
deactivateFormState = filledForm,
accountDeactivationAction = AsyncAction.Confirming,
accountDeactivationAction = AsyncAction.ConfirmingNoParams,
),
anAccountDeactivationState(
deactivateFormState = filledForm,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fun AccountDeactivationActionDialog(
when (state) {
AsyncAction.Uninitialized ->
Unit
AsyncAction.Confirming ->
is AsyncAction.Confirming ->
AccountDeactivationConfirmationDialog(
onSubmitClick = onConfirmClick,
onDismiss = onDismissDialog
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class AccountDeactivationPresenterTest {
skipItems(1)
initialState.eventSink(AccountDeactivationEvents.DeactivateAccount(isRetry = false))
val updatedState = awaitItem()
assertThat(updatedState.accountDeactivationAction).isEqualTo(AsyncAction.Confirming)
assertThat(updatedState.accountDeactivationAction).isEqualTo(AsyncAction.ConfirmingNoParams)
updatedState.eventSink(AccountDeactivationEvents.DeactivateAccount(isRetry = false))
val updatedState2 = awaitItem()
assertThat(updatedState2.accountDeactivationAction).isEqualTo(AsyncAction.Loading)
Expand Down Expand Up @@ -102,7 +102,7 @@ class AccountDeactivationPresenterTest {
skipItems(2)
initialState.eventSink(AccountDeactivationEvents.DeactivateAccount(isRetry = false))
val updatedState = awaitItem()
assertThat(updatedState.accountDeactivationAction).isEqualTo(AsyncAction.Confirming)
assertThat(updatedState.accountDeactivationAction).isEqualTo(AsyncAction.ConfirmingNoParams)
updatedState.eventSink(AccountDeactivationEvents.DeactivateAccount(isRetry = false))
val updatedState2 = awaitItem()
assertThat(updatedState2.accountDeactivationAction).isEqualTo(AsyncAction.Loading)
Expand Down Expand Up @@ -135,7 +135,7 @@ class AccountDeactivationPresenterTest {
skipItems(2)
initialState.eventSink(AccountDeactivationEvents.DeactivateAccount(isRetry = false))
val updatedState = awaitItem()
assertThat(updatedState.accountDeactivationAction).isEqualTo(AsyncAction.Confirming)
assertThat(updatedState.accountDeactivationAction).isEqualTo(AsyncAction.ConfirmingNoParams)
updatedState.eventSink(AccountDeactivationEvents.DeactivateAccount(isRetry = false))
val updatedState2 = awaitItem()
assertThat(updatedState2.accountDeactivationAction).isEqualTo(AsyncAction.Loading)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class AccountDeactivationViewTest {
deactivateFormState = aDeactivateFormState(
password = A_PASSWORD,
),
accountDeactivationAction = AsyncAction.Confirming,
accountDeactivationAction = AsyncAction.ConfirmingNoParams,
eventSink = eventsRecorder,
),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@ package io.element.android.features.invite.api.response

import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.matrix.api.core.RoomId
import java.util.Optional

data class AcceptDeclineInviteState(
val invite: Optional<InviteData>,
val acceptAction: AsyncAction<RoomId>,
val declineAction: AsyncAction<RoomId>,
val eventSink: (AcceptDeclineInviteEvents) -> Unit,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,20 @@ package io.element.android.features.invite.api.response
import androidx.compose.ui.tooling.preview.PreviewParameterProvider
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.matrix.api.core.RoomId
import java.util.Optional

open class AcceptDeclineInviteStateProvider : PreviewParameterProvider<AcceptDeclineInviteState> {
override val values: Sequence<AcceptDeclineInviteState>
get() = sequenceOf(
anAcceptDeclineInviteState(),
anAcceptDeclineInviteState(
invite = Optional.of(
InviteData(RoomId("!room:matrix.org"), isDm = true, roomName = "Alice"),
declineAction = ConfirmingDeclineInvite(
InviteData(RoomId("!room:matrix.org"), isDm = true, roomName = "Alice")
),
declineAction = AsyncAction.Confirming,
),
anAcceptDeclineInviteState(
invite = Optional.of(
InviteData(RoomId("!room:matrix.org"), isDm = false, roomName = "Some room"),
declineAction = ConfirmingDeclineInvite(
InviteData(RoomId("!room:matrix.org"), isDm = false, roomName = "Some room")
),
declineAction = AsyncAction.Confirming,
),
anAcceptDeclineInviteState(
acceptAction = AsyncAction.Failure(Throwable("Whoops")),
Expand All @@ -38,12 +35,10 @@ open class AcceptDeclineInviteStateProvider : PreviewParameterProvider<AcceptDec
}

fun anAcceptDeclineInviteState(
invite: Optional<InviteData> = Optional.empty(),
acceptAction: AsyncAction<RoomId> = AsyncAction.Uninitialized,
declineAction: AsyncAction<RoomId> = AsyncAction.Uninitialized,
eventSink: (AcceptDeclineInviteEvents) -> Unit = {}
) = AcceptDeclineInviteState(
invite = invite,
acceptAction = acceptAction,
declineAction = declineAction,
eventSink = eventSink,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright 2024 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only
* Please see LICENSE in the repository root for full details.
*/

package io.element.android.features.invite.api.response

import io.element.android.libraries.architecture.AsyncAction

data class ConfirmingDeclineInvite(
val inviteData: InviteData,
) : AsyncAction.Confirming
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@ package io.element.android.features.invite.impl.response

import androidx.compose.runtime.Composable
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import im.vector.app.features.analytics.plan.JoinedRoom
import io.element.android.features.invite.api.response.AcceptDeclineInviteEvents
import io.element.android.features.invite.api.response.AcceptDeclineInviteState
import io.element.android.features.invite.api.response.InviteData
import io.element.android.features.invite.api.response.ConfirmingDeclineInvite
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.architecture.Presenter
import io.element.android.libraries.architecture.runCatchingUpdatingState
Expand All @@ -29,9 +27,7 @@ import io.element.android.libraries.matrix.api.room.join.JoinRoom
import io.element.android.libraries.push.api.notifications.NotificationCleaner
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import java.util.Optional
import javax.inject.Inject
import kotlin.jvm.optionals.getOrNull

class AcceptDeclineInvitePresenter @Inject constructor(
private val client: MatrixClient,
Expand All @@ -43,35 +39,22 @@ class AcceptDeclineInvitePresenter @Inject constructor(
val localCoroutineScope = rememberCoroutineScope()
val acceptedAction: MutableState<AsyncAction<RoomId>> = remember { mutableStateOf(AsyncAction.Uninitialized) }
val declinedAction: MutableState<AsyncAction<RoomId>> = remember { mutableStateOf(AsyncAction.Uninitialized) }
var currentInvite by remember {
mutableStateOf<Optional<InviteData>>(Optional.empty())
}

fun handleEvents(event: AcceptDeclineInviteEvents) {
when (event) {
is AcceptDeclineInviteEvents.AcceptInvite -> {
// currentInvite is used to render the decline confirmation dialog
// and to reuse the roomId when the user confirm the rejection of the invitation.
// Just set it to empty here.
currentInvite = Optional.empty()
localCoroutineScope.acceptInvite(event.invite.roomId, acceptedAction)
}

is AcceptDeclineInviteEvents.DeclineInvite -> {
currentInvite = Optional.of(event.invite)
declinedAction.value = AsyncAction.Confirming
declinedAction.value = ConfirmingDeclineInvite(event.invite)
}

is InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite -> {
declinedAction.value = AsyncAction.Uninitialized
currentInvite.getOrNull()?.let {
localCoroutineScope.declineInvite(it.roomId, declinedAction)
}
currentInvite = Optional.empty()
localCoroutineScope.declineInvite(event.roomId, declinedAction)
}

is InternalAcceptDeclineInviteEvents.CancelDeclineInvite -> {
currentInvite = Optional.empty()
declinedAction.value = AsyncAction.Uninitialized
}

Expand All @@ -86,7 +69,6 @@ class AcceptDeclineInvitePresenter @Inject constructor(
}

return AcceptDeclineInviteState(
invite = currentInvite,
acceptAction = acceptedAction.value,
declineAction = declinedAction.value,
eventSink = ::handleEvents
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import androidx.compose.ui.tooling.preview.PreviewParameter
import io.element.android.features.invite.api.response.AcceptDeclineInviteState
import io.element.android.features.invite.api.response.AcceptDeclineInviteStateProvider
import io.element.android.features.invite.api.response.ConfirmingDeclineInvite
import io.element.android.features.invite.api.response.InviteData
import io.element.android.features.invite.impl.R
import io.element.android.libraries.designsystem.components.async.AsyncActionView
Expand All @@ -22,7 +23,6 @@
import io.element.android.libraries.designsystem.preview.PreviewsDayNight
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.ui.strings.CommonStrings
import kotlin.jvm.optionals.getOrNull

@Composable
fun AcceptDeclineInviteView(
Expand All @@ -45,13 +45,13 @@
onErrorDismiss = {
state.eventSink(InternalAcceptDeclineInviteEvents.DismissDeclineError)
},
confirmationDialog = {
val invite = state.invite.getOrNull()
if (invite != null) {
confirmationDialog = { confirming ->
// Note: confirming will always be of type ConfirmingDeclineInvite.
if (confirming is ConfirmingDeclineInvite) {
Comment on lines +49 to +50
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just force cast it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but why take the risk of a crash?
Previous code was not val invite = state.invite!!, so I prefer to keep the code safe.

Copy link
Member

Choose a reason for hiding this comment

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

We could but why take the risk of a crash?

Can we then log an error or upload some error report to sentry? I'm worried this might just silently fail, although the tests should cover it.

Copy link
Member Author

Choose a reason for hiding this comment

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

My previous attempt to fix this would have avoid this drawback. But it was too invasive, so we go with this approach. This is not a new issue, so I guess this is fine.

DeclineConfirmationDialog(
invite = invite,
invite = confirming.inviteData,
onConfirmClick = {
state.eventSink(InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite)
state.eventSink(InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite(confirming.inviteData.roomId))

Check warning on line 54 in features/invite/impl/src/main/kotlin/io/element/android/features/invite/impl/response/AcceptDeclineInviteView.kt

View check run for this annotation

Codecov / codecov/patch

features/invite/impl/src/main/kotlin/io/element/android/features/invite/impl/response/AcceptDeclineInviteView.kt#L54

Added line #L54 was not covered by tests
},
onDismissClick = {
state.eventSink(InternalAcceptDeclineInviteEvents.CancelDeclineInvite)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
package io.element.android.features.invite.impl.response

import io.element.android.features.invite.api.response.AcceptDeclineInviteEvents
import io.element.android.libraries.matrix.api.core.RoomId

sealed interface InternalAcceptDeclineInviteEvents : AcceptDeclineInviteEvents {
data object ConfirmDeclineInvite : InternalAcceptDeclineInviteEvents
data class ConfirmDeclineInvite(val roomId: RoomId) : InternalAcceptDeclineInviteEvents
data object CancelDeclineInvite : InternalAcceptDeclineInviteEvents
data object DismissAcceptError : InternalAcceptDeclineInviteEvents
data object DismissDeclineError : InternalAcceptDeclineInviteEvents
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ package io.element.android.features.invite.impl.response
import com.google.common.truth.Truth.assertThat
import im.vector.app.features.analytics.plan.JoinedRoom
import io.element.android.features.invite.api.response.AcceptDeclineInviteEvents
import io.element.android.features.invite.api.response.ConfirmingDeclineInvite
import io.element.android.features.invite.api.response.InviteData
import io.element.android.libraries.architecture.AsyncAction
import io.element.android.libraries.matrix.api.MatrixClient
Expand All @@ -33,7 +34,6 @@ import io.element.android.tests.testutils.test
import kotlinx.coroutines.test.runTest
import org.junit.Rule
import org.junit.Test
import java.util.Optional

class AcceptDeclineInvitePresenterTest {
@get:Rule
Expand All @@ -46,7 +46,6 @@ class AcceptDeclineInvitePresenterTest {
awaitItem().also { state ->
assertThat(state.acceptAction).isInstanceOf(AsyncAction.Uninitialized::class.java)
assertThat(state.declineAction).isInstanceOf(AsyncAction.Uninitialized::class.java)
assertThat(state.invite).isEqualTo(Optional.empty<InviteData>())
}
}
}
Expand All @@ -61,17 +60,13 @@ class AcceptDeclineInvitePresenterTest {
AcceptDeclineInviteEvents.DeclineInvite(inviteData)
)
}
skipItems(1)
awaitItem().also { state ->
assertThat(state.invite).isEqualTo(Optional.of(inviteData))
assertThat(state.declineAction).isInstanceOf(AsyncAction.Confirming::class.java)
assertThat(state.declineAction).isEqualTo(ConfirmingDeclineInvite(inviteData))
state.eventSink(
InternalAcceptDeclineInviteEvents.CancelDeclineInvite
)
}
skipItems(1)
awaitItem().also { state ->
assertThat(state.invite).isEqualTo(Optional.empty<InviteData>())
assertThat(state.declineAction).isInstanceOf(AsyncAction.Uninitialized::class.java)
}
}
Expand All @@ -93,22 +88,20 @@ class AcceptDeclineInvitePresenterTest {
AcceptDeclineInviteEvents.DeclineInvite(inviteData)
)
}
skipItems(1)
awaitItem().also { state ->
assertThat(state.declineAction).isEqualTo(ConfirmingDeclineInvite(inviteData))
state.eventSink(
InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite
InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite(inviteData.roomId)
)
}
skipItems(2)
assertThat(awaitItem().declineAction.isLoading()).isTrue()
awaitItem().also { state ->
assertThat(state.declineAction).isInstanceOf(AsyncAction.Failure::class.java)
state.eventSink(
InternalAcceptDeclineInviteEvents.DismissDeclineError
)
}
skipItems(1)
awaitItem().also { state ->
assertThat(state.invite).isEqualTo(Optional.empty<InviteData>())
assertThat(state.declineAction).isInstanceOf(AsyncAction.Uninitialized::class.java)
}
cancelAndConsumeRemainingEvents()
Expand Down Expand Up @@ -141,13 +134,13 @@ class AcceptDeclineInvitePresenterTest {
AcceptDeclineInviteEvents.DeclineInvite(inviteData)
)
}
skipItems(1)
awaitItem().also { state ->
assertThat(state.declineAction).isEqualTo(ConfirmingDeclineInvite(inviteData))
state.eventSink(
InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite
InternalAcceptDeclineInviteEvents.ConfirmDeclineInvite(inviteData.roomId)
)
}
skipItems(2)
assertThat(awaitItem().declineAction.isLoading()).isTrue()
awaitItem().also { state ->
assertThat(state.declineAction).isInstanceOf(AsyncAction.Success::class.java)
}
Expand All @@ -173,7 +166,6 @@ class AcceptDeclineInvitePresenterTest {
)
}
awaitItem().also { state ->
assertThat(state.invite).isEqualTo(Optional.empty<InviteData>())
assertThat(state.acceptAction).isEqualTo(AsyncAction.Loading)
}
awaitItem().also { state ->
Expand All @@ -183,7 +175,6 @@ class AcceptDeclineInvitePresenterTest {
)
}
awaitItem().also { state ->
assertThat(state.invite).isEqualTo(Optional.empty<InviteData>())
assertThat(state.acceptAction).isInstanceOf(AsyncAction.Uninitialized::class.java)
}
cancelAndConsumeRemainingEvents()
Expand Down Expand Up @@ -220,7 +211,6 @@ class AcceptDeclineInvitePresenterTest {
)
}
awaitItem().also { state ->
assertThat(state.invite).isEqualTo(Optional.empty<InviteData>())
assertThat(state.acceptAction).isEqualTo(AsyncAction.Loading)
}
awaitItem().also { state ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ fun PinUnlockView(
latestOnSuccessLogout(state.signOutAction.data)
}
}
AsyncAction.Confirming,
is AsyncAction.Confirming,
is AsyncAction.Failure,
AsyncAction.Uninitialized -> Unit
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ private fun ColumnScope.Buttons(
}
}
AsyncAction.Uninitialized,
AsyncAction.Confirming -> Unit
is AsyncAction.Confirming -> Unit
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ open class DirectLogoutStateProvider : PreviewParameterProvider<DirectLogoutStat
override val values: Sequence<DirectLogoutState>
get() = sequenceOf(
aDirectLogoutState(),
aDirectLogoutState(logoutAction = AsyncAction.Confirming),
aDirectLogoutState(logoutAction = AsyncAction.ConfirmingNoParams),
aDirectLogoutState(logoutAction = AsyncAction.Loading),
aDirectLogoutState(logoutAction = AsyncAction.Failure(Exception("Error"))),
aDirectLogoutState(logoutAction = AsyncAction.Success("success")),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class LogoutPresenter @Inject constructor(
if (logoutAction.value.isConfirming() || event.ignoreSdkError) {
localCoroutineScope.logout(logoutAction, event.ignoreSdkError)
} else {
logoutAction.value = AsyncAction.Confirming
logoutAction.value = AsyncAction.ConfirmingNoParams
}
}
LogoutEvents.CloseDialogs -> {
Expand Down
Loading
Loading