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

Feature/bma/fix fdroid notification #3035

Merged
merged 30 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
148177f
Fix typo in log.
bmarty Jun 14, 2024
907fae1
Pusher add more log and change comment to log.
bmarty Jun 14, 2024
acde970
Extract function and add more logs.
bmarty Jun 14, 2024
101a6e6
Ensure that the code is not run twice.
bmarty Jun 14, 2024
247b60b
Add Timber tag.
bmarty Jun 14, 2024
68d2a1a
More log.
bmarty Jun 14, 2024
3d5951c
Add test on pusher registration
bmarty Jun 14, 2024
21ce1c4
Add pusher status in the state.
bmarty Jun 14, 2024
725c383
Render an error dialog in case registering a pusher fails.
bmarty Jun 17, 2024
675f93a
Create component ErrorDialogWithDoNotShowAgain
bmarty Jun 17, 2024
64930e4
Add ability to not show the pusher registration again.
bmarty Jun 17, 2024
5fa3802
Update screenshots
Jun 17, 2024
abc0e7e
Move setIgnoreRegistrationError call.
bmarty Jun 17, 2024
7d1e841
Rename member.
bmarty Jun 17, 2024
eb07ae2
Add test on `ignoreRegistrationError` and `setIgnoreRegistrationError`
bmarty Jun 17, 2024
e31fc17
Add Unit test on UserPushStoreDataStore
bmarty Jun 17, 2024
5180ce3
Add a shortcut to navigate to the notification settings in case of er…
bmarty Jun 17, 2024
1f8b525
Fix back navigation issue, when opening directly the notification tro…
bmarty Jun 17, 2024
f72fc36
Update PushProvider API, remove `isAvailable()`, but instead rely on …
bmarty Jun 17, 2024
b4b407a
Store the first provider even if no distributor is available, else er…
bmarty Jun 17, 2024
e12f723
Fix test compilation issue.
bmarty Jun 17, 2024
0908e9b
Fix test issue.
bmarty Jun 17, 2024
892a6d5
Add test about selecting the first provider with a distributor.
bmarty Jun 17, 2024
d97db21
Rather use NoDistributorsAvailable, it has more chance to happen IRL.
bmarty Jun 17, 2024
46f71de
Update screenshots
Jun 17, 2024
85eae46
Cleanup
bmarty Jun 17, 2024
69dbb08
Merge branch 'develop' into feature/bma/fixFdroidNotification
bmarty Jun 18, 2024
f09b77f
Update test after merging develop.
bmarty Jun 18, 2024
bc30aee
Iterate on sessionVerificationService.sessionVerifiedStatus and fix t…
bmarty Jun 18, 2024
d69a5ee
Also fix same issue for analytics.
bmarty Jun 18, 2024
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 appnav/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ dependencies {
testImplementation(libs.test.turbine)
testImplementation(projects.libraries.matrix.test)
testImplementation(projects.libraries.push.test)
testImplementation(projects.libraries.pushproviders.test)
testImplementation(projects.features.networkmonitor.test)
testImplementation(projects.features.login.impl)
testImplementation(projects.tests.testutils)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,12 @@ class LoggedInFlowNode @AssistedInject constructor(
return when (navTarget) {
NavTarget.Placeholder -> createNode<PlaceholderNode>(buildContext)
NavTarget.LoggedInPermanent -> {
createNode<LoggedInNode>(buildContext)
val callback = object : LoggedInNode.Callback {
override fun navigateToNotificationTroubleshoot() {
backstack.push(NavTarget.Settings(PreferencesEntryPoint.InitialTarget.NotificationTroubleshoot))
}
}
createNode<LoggedInNode>(buildContext, listOf(callback))
}
NavTarget.RoomList -> {
val callback = object : RoomListEntryPoint.Callback {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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.appnav.loggedin

sealed interface LoggedInEvents {
data class CloseErrorDialog(val doNotShowAgain: Boolean) : LoggedInEvents
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.compose.ui.Modifier
import com.bumble.appyx.core.modality.BuildContext
import com.bumble.appyx.core.node.Node
import com.bumble.appyx.core.plugin.Plugin
import com.bumble.appyx.core.plugin.plugins
import dagger.assisted.Assisted
import dagger.assisted.AssistedInject
import io.element.android.anvilannotations.ContributesNode
Expand All @@ -35,11 +36,22 @@ class LoggedInNode @AssistedInject constructor(
buildContext = buildContext,
plugins = plugins
) {
interface Callback : Plugin {
fun navigateToNotificationTroubleshoot()
}

private fun navigateToNotificationTroubleshoot() {
plugins<Callback>().forEach {
it.navigateToNotificationTroubleshoot()
}
}

@Composable
override fun View(modifier: Modifier) {
val loggedInState = loggedInPresenter.present()
LoggedInView(
state = loggedInState,
navigateToNotificationTroubleshoot = ::navigateToNotificationTroubleshoot,
modifier = modifier
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,36 @@

import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.derivedStateOf
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import im.vector.app.features.analytics.plan.CryptoSessionStateChange
import im.vector.app.features.analytics.plan.UserProperties
import io.element.android.features.networkmonitor.api.NetworkMonitor
import io.element.android.features.networkmonitor.api.NetworkStatus
import io.element.android.libraries.architecture.AsyncData
import io.element.android.libraries.architecture.Presenter
import io.element.android.libraries.core.log.logger.LoggerTag
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.matrix.api.encryption.EncryptionService
import io.element.android.libraries.matrix.api.encryption.RecoveryState
import io.element.android.libraries.matrix.api.roomlist.RoomListService
import io.element.android.libraries.matrix.api.verification.SessionVerificationService
import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
import io.element.android.libraries.push.api.PushService
import io.element.android.libraries.pushproviders.api.RegistrationFailure
import io.element.android.services.analytics.api.AnalyticsService
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.launch
import timber.log.Timber
import javax.inject.Inject

private val pusherTag = LoggerTag("Pusher", LoggerTag.PushLoggerTag)

class LoggedInPresenter @Inject constructor(
private val matrixClient: MatrixClient,
private val networkMonitor: NetworkMonitor,
Expand All @@ -49,36 +58,23 @@
) : Presenter<LoggedInState> {
@Composable
override fun present(): LoggedInState {
val coroutineScope = rememberCoroutineScope()
val isVerified by remember {
sessionVerificationService.sessionVerifiedStatus.map { it == SessionVerifiedStatus.Verified }
}.collectAsState(initial = false)

LaunchedEffect(isVerified) {
if (isVerified) {
// Ensure pusher is registered
val currentPushProvider = pushService.getCurrentPushProvider()
val result = if (currentPushProvider == null) {
// Register with the first available push provider
val pushProvider = pushService.getAvailablePushProviders().firstOrNull() ?: return@LaunchedEffect
val distributor = pushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect
pushService.registerWith(matrixClient, pushProvider, distributor)
} else {
val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient)
if (currentPushDistributor == null) {
// Register with the first available distributor
val distributor = currentPushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect
pushService.registerWith(matrixClient, currentPushProvider, distributor)
} else {
// Re-register with the current distributor
pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor)
}
}
result.onFailure {
Timber.e(it, "Failed to register pusher")
}
val ignoreRegistrationError by remember {
pushService.ignoreRegistrationError(matrixClient.sessionId)
}.collectAsState(initial = false)
val pusherRegistrationState = remember<MutableState<AsyncData<Unit>>> { mutableStateOf(AsyncData.Uninitialized) }
if (isVerified) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Checking isVerified first, else ensurePusherIsRegistered() is called twice:

image

By checking first the value:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by bc30aee

LaunchedEffect(Unit) {
ensurePusherIsRegistered(pusherRegistrationState)
}
} else {
LaunchedEffect(Unit) {
pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.AccountNotVerified())
}
}

val syncIndicator by matrixClient.roomListService.syncIndicator.collectAsState()
val networkStatus by networkMonitor.connectivity.collectAsState()
val showSyncSpinner by remember {
Expand All @@ -92,8 +88,77 @@
reportCryptoStatusToAnalytics(verificationState, recoveryState)
}

fun handleEvent(event: LoggedInEvents) {
when (event) {
is LoggedInEvents.CloseErrorDialog -> {
pusherRegistrationState.value = AsyncData.Uninitialized
if (event.doNotShowAgain) {
coroutineScope.launch {
pushService.setIgnoreRegistrationError(matrixClient.sessionId, true)
}
}
}
}
}

return LoggedInState(
showSyncSpinner = showSyncSpinner,
pusherRegistrationState = pusherRegistrationState.value,
ignoreRegistrationError = ignoreRegistrationError,
eventSink = ::handleEvent
)
}

private suspend fun ensurePusherIsRegistered(pusherRegistrationState: MutableState<AsyncData<Unit>>) {
Timber.tag(pusherTag.value).d("Ensure pusher is registered")
val currentPushProvider = pushService.getCurrentPushProvider()
val result = if (currentPushProvider == null) {
Timber.tag(pusherTag.value).d("Register with the first available push provider with at least one distributor")
val pushProvider = pushService.getAvailablePushProviders()
.firstOrNull { it.getDistributors().isNotEmpty() }
// Else fallback to the first available push provider (the list should never be empty)
?: pushService.getAvailablePushProviders().firstOrNull()
?: return Unit
.also { Timber.tag(pusherTag.value).w("No push providers available") }
.also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoProvidersAvailable()) }
val distributor = pushProvider.getDistributors().firstOrNull()
?: return Unit
.also { Timber.tag(pusherTag.value).w("No distributors available") }
.also {
// In this case, consider the push provider is chosen.
pushService.selectPushProvider(matrixClient, pushProvider)
}
.also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable()) }
pushService.registerWith(matrixClient, pushProvider, distributor)
} else {
val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient)
if (currentPushDistributor == null) {
Timber.tag(pusherTag.value).d("Register with the first available distributor")
val distributor = currentPushProvider.getDistributors().firstOrNull()
?: return Unit
.also { Timber.tag(pusherTag.value).w("No distributors available") }
.also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable()) }
pushService.registerWith(matrixClient, currentPushProvider, distributor)
} else {
Timber.tag(pusherTag.value).d("Re-register with the current distributor")
pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor)
}
}
result.fold(
onSuccess = {
Timber.tag(pusherTag.value).d("Pusher registered")
pusherRegistrationState.value = AsyncData.Success(Unit)
},
onFailure = {
Timber.tag(pusherTag.value).e(it, "Failed to register pusher")
if (it is RegistrationFailure) {
pusherRegistrationState.value = AsyncData.Failure(
PusherRegistrationFailure.RegistrationFailure(it.clientException, it.isRegisteringAgain)

Check warning on line 156 in appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt

View check run for this annotation

Codecov / codecov/patch

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt#L155-L156

Added lines #L155 - L156 were not covered by tests
)
} else {
pusherRegistrationState.value = AsyncData.Failure(it)
}
}
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@

package io.element.android.appnav.loggedin

import io.element.android.libraries.architecture.AsyncData

data class LoggedInState(
val showSyncSpinner: Boolean,
val pusherRegistrationState: AsyncData<Unit>,
val ignoreRegistrationError: Boolean,
val eventSink: (LoggedInEvents) -> Unit,
)
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,23 @@
package io.element.android.appnav.loggedin

import androidx.compose.ui.tooling.preview.PreviewParameterProvider
import io.element.android.libraries.architecture.AsyncData

open class LoggedInStateProvider : PreviewParameterProvider<LoggedInState> {
override val values: Sequence<LoggedInState>
get() = sequenceOf(
aLoggedInState(false),
aLoggedInState(true),
// Add other state here
aLoggedInState(),
aLoggedInState(showSyncSpinner = true),
aLoggedInState(pusherRegistrationState = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable())),
)
}

fun aLoggedInState(
showSyncSpinner: Boolean = true,
showSyncSpinner: Boolean = false,
pusherRegistrationState: AsyncData<Unit> = AsyncData.Uninitialized,
) = LoggedInState(
showSyncSpinner = showSyncSpinner,
pusherRegistrationState = pusherRegistrationState,
ignoreRegistrationError = false,
eventSink = {},
)
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,19 @@
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.tooling.preview.PreviewParameter
import io.element.android.libraries.architecture.AsyncData
import io.element.android.libraries.designsystem.components.dialogs.ErrorDialogWithDoNotShowAgain
import io.element.android.libraries.designsystem.preview.ElementPreview
import io.element.android.libraries.designsystem.preview.PreviewsDayNight
import io.element.android.libraries.matrix.api.exception.isNetworkError
import io.element.android.libraries.ui.strings.CommonStrings

@Composable
fun LoggedInView(
state: LoggedInState,
navigateToNotificationTroubleshoot: () -> Unit,
modifier: Modifier = Modifier
) {
Box(
Expand All @@ -41,12 +47,53 @@
isVisible = state.showSyncSpinner,
)
}
when (state.pusherRegistrationState) {
is AsyncData.Uninitialized,
is AsyncData.Loading,
is AsyncData.Success -> Unit
is AsyncData.Failure -> {
state.pusherRegistrationState.errorOrNull()
?.takeIf { !state.ignoreRegistrationError }
?.getReason()
?.let { reason ->
ErrorDialogWithDoNotShowAgain(
content = stringResource(id = CommonStrings.common_error_registering_pusher_android, reason),
cancelText = stringResource(id = CommonStrings.common_settings),
onDismiss = {
state.eventSink(LoggedInEvents.CloseErrorDialog(it))

Check warning on line 63 in appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt

View check run for this annotation

Codecov / codecov/patch

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt#L63

Added line #L63 was not covered by tests
},
onCancel = {
state.eventSink(LoggedInEvents.CloseErrorDialog(false))
navigateToNotificationTroubleshoot()

Check warning on line 67 in appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt

View check run for this annotation

Codecov / codecov/patch

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt#L66-L67

Added lines #L66 - L67 were not covered by tests
}
)
}
}
}
}

private fun Throwable.getReason(): String? {
return when (this) {
is PusherRegistrationFailure.RegistrationFailure -> {
if (isRegisteringAgain && clientException.isNetworkError()) {
// When registering again, ignore network error
null

Check warning on line 80 in appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt

View check run for this annotation

Codecov / codecov/patch

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt#L80

Added line #L80 was not covered by tests
} else {
clientException.message ?: "Unknown error"
}
}
is PusherRegistrationFailure.AccountNotVerified -> null
is PusherRegistrationFailure.NoDistributorsAvailable -> "No distributors available"
is PusherRegistrationFailure.NoProvidersAvailable -> "No providers available"
else -> "Other error"

Check warning on line 88 in appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt

View check run for this annotation

Codecov / codecov/patch

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt#L88

Added line #L88 was not covered by tests
}
}

@PreviewsDayNight
@Composable
internal fun LoggedInViewPreview(@PreviewParameter(LoggedInStateProvider::class) state: LoggedInState) = ElementPreview {
LoggedInView(
state = state
state = state,
navigateToNotificationTroubleshoot = {},
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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.appnav.loggedin

import io.element.android.libraries.matrix.api.exception.ClientException

sealed class PusherRegistrationFailure : Exception() {
class AccountNotVerified : PusherRegistrationFailure()
class NoProvidersAvailable : PusherRegistrationFailure()
class NoDistributorsAvailable : PusherRegistrationFailure()

/**
* @param clientException the failure that occurred.
* @param isRegisteringAgain true if the server should already have a the same pusher registered.
*/
class RegistrationFailure(
val clientException: ClientException,
val isRegisteringAgain: Boolean,
) : PusherRegistrationFailure()

Check warning on line 33 in appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt

View check run for this annotation

Codecov / codecov/patch

appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt#L31-L33

Added lines #L31 - L33 were not covered by tests
}
Loading
Loading