Merge pull request #6996 from element-hq/feature/fga/location_permissions_rework

change(location): ensure permissions are always requested at least once

features/location/impl/src/main/kotlin/io/element/android/features/location/impl/common/LocationConstraintsCheck.kt

@@ -14,6 +14,7 @@ import io.element.android.features.location.impl.common.ui.LocationConstraintsDi
 sealed interface LocationConstraintsCheck {
     data object Success : LocationConstraintsCheck
     data object PermissionRationale : LocationConstraintsCheck
+    data object PermissionShouldBeRequested : LocationConstraintsCheck
     data object PermissionDenied : LocationConstraintsCheck
     data object LocationServiceDisabled : LocationConstraintsCheck
     data object NotEnoughPowerLevel : LocationConstraintsCheck
@@ -34,6 +35,7 @@ fun checkLocationConstraints(
             }
         }
         permissionsState.shouldShowRationale -> LocationConstraintsCheck.PermissionRationale
+        !permissionsState.permissionsAlreadyRequested -> LocationConstraintsCheck.PermissionShouldBeRequested
         else -> LocationConstraintsCheck.PermissionDenied
     }
 }
@@ -41,6 +43,7 @@ fun checkLocationConstraints(
 fun LocationConstraintsCheck.toDialogState(): LocationConstraintsDialogState {
     return when (this) {
         LocationConstraintsCheck.Success -> LocationConstraintsDialogState.None
+        LocationConstraintsCheck.PermissionShouldBeRequested -> LocationConstraintsDialogState.None
         LocationConstraintsCheck.PermissionRationale -> LocationConstraintsDialogState.PermissionRationale
         LocationConstraintsCheck.PermissionDenied -> LocationConstraintsDialogState.PermissionDenied
         LocationConstraintsCheck.LocationServiceDisabled -> LocationConstraintsDialogState.LocationServiceDisabled

features/location/impl/src/main/kotlin/io/element/android/features/location/impl/common/permissions/DefaultPermissionsPresenter.kt

@@ -9,6 +9,10 @@
 package io.element.android.features.location.impl.common.permissions
 
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
 import com.google.accompanist.permissions.ExperimentalPermissionsApi
 import com.google.accompanist.permissions.isGranted
 import com.google.accompanist.permissions.rememberMultiplePermissionsState
@@ -32,11 +36,16 @@ class DefaultPermissionsPresenter(
     @OptIn(ExperimentalPermissionsApi::class)
     @Composable
     override fun present(): PermissionsState {
-        val multiplePermissionsState = rememberMultiplePermissionsState(permissions = permissions)
+        var permissionsRequested by remember { mutableStateOf(false) }
+        val multiplePermissionsState = rememberMultiplePermissionsState(permissions = permissions) {
+            permissionsRequested = true
+        }
 
         fun handleEvent(event: PermissionsEvents) {
             when (event) {
-                PermissionsEvents.RequestPermissions -> multiplePermissionsState.launchMultiplePermissionRequest()
+                PermissionsEvents.RequestPermissions -> {
+                    multiplePermissionsState.launchMultiplePermissionRequest()
+                }
             }
         }
 
@@ -47,6 +56,7 @@ class DefaultPermissionsPresenter(
                 else -> PermissionsState.Permissions.NoneGranted
             },
             shouldShowRationale = multiplePermissionsState.shouldShowRationale,
+            permissionsAlreadyRequested = permissionsRequested,
             eventSink = ::handleEvent,
         )
     }

features/location/impl/src/main/kotlin/io/element/android/features/location/impl/common/permissions/PermissionsState.kt

@@ -11,6 +11,7 @@ package io.element.android.features.location.impl.common.permissions
 data class PermissionsState(
     val permissions: Permissions,
     val shouldShowRationale: Boolean,
+    val permissionsAlreadyRequested: Boolean,
     val eventSink: (PermissionsEvents) -> Unit,
 ) {
     sealed interface Permissions {

features/location/impl/src/main/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenter.kt

@@ -88,6 +88,8 @@ class ShareLocationPresenter(
         var dialogState: ShareLocationState.Dialog by remember {
             mutableStateOf(ShareLocationState.Dialog.None)
         }
+        // true when trying to initiate the live location share
+        var pendingLiveLocationShare by remember { mutableStateOf(false) }
         val startLiveLocationAction = remember { mutableStateOf<AsyncAction<Unit>>(AsyncAction.Uninitialized) }
         val currentUser by client.userProfile.collectAsState()
         val customMapStyleUrl by produceState(AsyncData.Loading()) {
@@ -100,34 +102,55 @@ class ShareLocationPresenter(
         val scope = rememberCoroutineScope()
 
         fun checkLocationConstraints() {
-            // No need to check SendLiveLocationPermissions here
+            val locationConstraints = checkLocationConstraints(
-            val locationConstraints = checkLocationConstraints(permissionsState, locationActions, SendLiveLocationPermissions.GRANTED)
+                permissionsState = permissionsState,
+                locationActions = locationActions,
+                // No need to check SendLiveLocationPermissions here
+                sendLiveLocationPermissions = SendLiveLocationPermissions.GRANTED
+            )
+            if (locationConstraints is LocationConstraintsCheck.PermissionShouldBeRequested) {
+                permissionsState.eventSink(PermissionsEvents.RequestPermissions)
+            }
             trackUserPosition = locationConstraints is LocationConstraintsCheck.Success
             dialogState = ShareLocationState.Dialog.Constraints(locationConstraints.toDialogState())
         }
 
-        suspend fun computeLiveLocationDialogState(): ShareLocationState.Dialog {
+        suspend fun checkLiveLocationConstraints() {
-            val hasAcceptedDisclaimer = liveLocationStore.hasAcceptedLiveLocationDisclaimer()
+            val locationConstraints = checkLocationConstraints(
-            val constraintsResult = checkLocationConstraints(permissionsState, locationActions, sendLiveLocationPermissions)
+                permissionsState = permissionsState,
-            return when {
+                locationActions = locationActions,
-                !hasAcceptedDisclaimer -> {
+                sendLiveLocationPermissions = sendLiveLocationPermissions,
-                    ShareLocationState.Dialog.LiveLocationDisclaimer
+            )
-                }
+            when (locationConstraints) {
-                constraintsResult is LocationConstraintsCheck.Success -> {
+                LocationConstraintsCheck.Success -> {
-                    val durations = LIVE_LOCATION_DURATIONS.map {
+                    val hasAcceptedDisclaimer = liveLocationStore.hasAcceptedLiveLocationDisclaimer()
-                        LiveLocationDuration(duration = it, formatted = durationFormatter.format(it))
+                    dialogState = if (!hasAcceptedDisclaimer) {
+                        ShareLocationState.Dialog.LiveLocationDisclaimer
+                    } else {
+                        val durations = LIVE_LOCATION_DURATIONS.map {
+                            LiveLocationDuration(duration = it, formatted = durationFormatter.format(it))
+                        }
+                        ShareLocationState.Dialog.LiveLocationDurations(durations.toImmutableList())
                     }
-                    ShareLocationState.Dialog.LiveLocationDurations(durations.toImmutableList())
                 }
                 else -> {
-                    ShareLocationState.Dialog.Constraints(constraintsResult.toDialogState())
+                    if (locationConstraints is LocationConstraintsCheck.PermissionShouldBeRequested) {
+                        permissionsState.eventSink(PermissionsEvents.RequestPermissions)
+                    }
+                    dialogState = ShareLocationState.Dialog.Constraints(locationConstraints.toDialogState())
                 }
             }
         }
 
         val userLocationState = userLocationStateFactory.create(permissionsState.isAnyGranted)
 
-        LaunchedEffect(permissionsState.permissions) { checkLocationConstraints() }
+        LaunchedEffect(permissionsState) {
+            if (pendingLiveLocationShare) {
+                checkLiveLocationConstraints()
+            } else {
+                checkLocationConstraints()
+            }
+        }
 
         fun handleEvent(event: ShareLocationEvent) {
             when (event) {
@@ -136,7 +159,10 @@ class ShareLocationPresenter(
                 }
                 ShareLocationEvent.StartTrackingUserLocation -> checkLocationConstraints()
                 ShareLocationEvent.StopTrackingUserLocation -> trackUserPosition = false
-                ShareLocationEvent.DismissDialog -> dialogState = ShareLocationState.Dialog.None
+                ShareLocationEvent.DismissDialog -> {
+                    pendingLiveLocationShare = false
+                    dialogState = ShareLocationState.Dialog.None
+                }
                 ShareLocationEvent.OpenAppSettings -> {
                     locationActions.openAppSettings()
                     dialogState = ShareLocationState.Dialog.None
@@ -146,15 +172,17 @@ class ShareLocationPresenter(
                     dialogState = ShareLocationState.Dialog.None
                 }
                 ShareLocationEvent.InitiateLiveLocationShare -> scope.launch {
-                    dialogState = computeLiveLocationDialogState()
+                    pendingLiveLocationShare = true
+                    checkLiveLocationConstraints()
                 }
                 ShareLocationEvent.AcceptLiveLocationDisclaimer -> scope.launch {
                     liveLocationStore.setAcceptedLiveLocationDisclaimer()
                         .onSuccess {
-                            dialogState = computeLiveLocationDialogState()
+                            checkLiveLocationConstraints()
                         }
                 }
                 is ShareLocationEvent.StartLiveLocationShare -> scope.launch {
+                    pendingLiveLocationShare = false
                     dialogState = ShareLocationState.Dialog.None
                     startLiveLocationAction.runUpdatingState {
                         liveLocationShareManager.startShare(

features/location/impl/src/main/kotlin/io/element/android/features/location/impl/show/ShowLocationPresenter.kt

@@ -90,10 +90,22 @@ import kotlinx.coroutines.launch
             value = AsyncData.Success(client.getMapStyleUrl().getOrNull())
         }
 
-        LaunchedEffect(permissionsState.permissions) {
+        fun checkLocationConstraints() {
-            if (permissionsState.isAnyGranted) {
+            val locationConstraints = checkLocationConstraints(
-                dialogState = LocationConstraintsDialogState.None
+                permissionsState = permissionsState,
+                locationActions = locationActions,
+                // No need to check SendLiveLocationPermissions here
+                sendLiveLocationPermissions = SendLiveLocationPermissions.GRANTED
+            )
+            if (locationConstraints is LocationConstraintsCheck.PermissionShouldBeRequested) {
+                permissionsState.eventSink(PermissionsEvents.RequestPermissions)
             }
+            isTrackMyLocation = locationConstraints is LocationConstraintsCheck.Success
+            dialogState = locationConstraints.toDialogState()
+        }
+
+        LaunchedEffect(permissionsState) {
+            checkLocationConstraints()
         }
 
         fun handleEvent(event: ShowLocationEvent) {
@@ -103,9 +115,7 @@ import kotlinx.coroutines.launch
                 }
                 is ShowLocationEvent.TrackMyLocation -> {
                     if (event.enabled) {
-                        val locationConstraints = checkLocationConstraints(permissionsState, locationActions, SendLiveLocationPermissions.GRANTED)
+                        checkLocationConstraints()
-                        isTrackMyLocation = locationConstraints is LocationConstraintsCheck.Success
-                        dialogState = locationConstraints.toDialogState()
                     } else {
                         isTrackMyLocation = false
                     }

features/location/impl/src/test/kotlin/io/element/android/features/location/impl/PermissionsStateFactory.kt

@@ -13,10 +13,12 @@ import io.element.android.features.location.impl.common.permissions.PermissionsS
 fun aPermissionsState(
     permissions: PermissionsState.Permissions = PermissionsState.Permissions.NoneGranted,
     shouldShowRationale: Boolean = false,
+    permissionsRequested: Boolean = false,
 ): PermissionsState {
     return PermissionsState(
         permissions = permissions,
         shouldShowRationale = shouldShowRationale,
+        permissionsAlreadyRequested = permissionsRequested,
         eventSink = {},
     )
 }

features/location/impl/src/test/kotlin/io/element/android/features/location/impl/common/LocationConstraintsCheckTest.kt

@@ -64,10 +64,25 @@ class LocationConstraintsCheckTest {
     }
 
     @Test
-    fun `checkLocationConstraints returns PermissionDenied when permissions denied without rationale`() {
+    fun `checkLocationConstraints returns PermissionShouldBeRequested when permissions not yet requested`() {
         val permissionsState = aPermissionsState(
             permissions = PermissionsState.Permissions.NoneGranted,
             shouldShowRationale = false,
+            permissionsRequested = false,
+        )
+        val locationActions = FakeLocationActions(isLocationEnabled = true)
+
+        val result = checkLocationConstraints(permissionsState, locationActions, SendLiveLocationPermissions.GRANTED)
+
+        assertThat(result).isEqualTo(LocationConstraintsCheck.PermissionShouldBeRequested)
+    }
+
+    @Test
+    fun `checkLocationConstraints returns PermissionDenied when permissions already requested and denied without rationale`() {
+        val permissionsState = aPermissionsState(
+            permissions = PermissionsState.Permissions.NoneGranted,
+            shouldShowRationale = false,
+            permissionsRequested = true,
         )
         val locationActions = FakeLocationActions(isLocationEnabled = true)
 

features/location/impl/src/test/kotlin/io/element/android/features/location/impl/common/permissions/FakePermissionsPresenter.kt

@@ -20,6 +20,7 @@ class FakePermissionsPresenter : PermissionsPresenter {
     private var state = PermissionsState(
         permissions = PermissionsState.Permissions.NoneGranted,
         shouldShowRationale = false,
+        permissionsAlreadyRequested = false,
         eventSink = ::handleEvent,
     )
         set(value) {

features/location/impl/src/test/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenterTest.kt

@@ -160,6 +160,24 @@ class ShareLocationPresenterTest {
         }
     }
 
+    @Test
+    fun `initial state with permissions not yet requested triggers permission request`() = runTest {
+        val shareLocationPresenter = createShareLocationPresenter()
+        fakePermissionsPresenter.givenState(
+            aPermissionsState(
+                permissions = PermissionsState.Permissions.NoneGranted,
+                shouldShowRationale = false,
+                permissionsRequested = false,
+            )
+        )
+
+        shareLocationPresenter.test {
+            skipItems(2)
+            cancelAndIgnoreRemainingEvents()
+            assertThat(fakePermissionsPresenter.events).contains(PermissionsEvents.RequestPermissions)
+        }
+    }
+
     @Test
     fun `initial state with permissions denied`() = runTest {
         val shareLocationPresenter = createShareLocationPresenter()
@@ -167,6 +185,7 @@ class ShareLocationPresenterTest {
             aPermissionsState(
                 permissions = PermissionsState.Permissions.NoneGranted,
                 shouldShowRationale = false,
+                permissionsRequested = true,
             )
         )
 
@@ -291,6 +310,7 @@ class ShareLocationPresenterTest {
             aPermissionsState(
                 permissions = PermissionsState.Permissions.NoneGranted,
                 shouldShowRationale = false,
+                permissionsRequested = true,
             )
         )
 
@@ -359,7 +379,12 @@ class ShareLocationPresenterTest {
 
     @Test
     fun `ShowLiveLocationDurationPicker shows disclaimer when acceptance is missing`() = runTest {
-        val presenter = createShareLocationPresenter()
+        val room = FakeJoinedRoom(
+            baseRoom = FakeBaseRoom(
+                roomPermissions = grantedSendLiveLocationPermissions()
+            )
+        )
+        val presenter = createShareLocationPresenter(joinedRoom = room)
         fakePermissionsPresenter.givenState(
             aPermissionsState(
                 permissions = PermissionsState.Permissions.AllGranted,
@@ -462,7 +487,12 @@ class ShareLocationPresenterTest {
 
     @Test
     fun `ShowLiveLocationDurationPicker uses the active session disclaimer state`() = runTest {
-        val joinedRoom = FakeJoinedRoom(baseRoom = FakeBaseRoom(sessionId = SessionId("@alice:server")))
+        val joinedRoom = FakeJoinedRoom(
+            baseRoom = FakeBaseRoom(
+                sessionId = SessionId("@alice:server"),
+                roomPermissions = grantedSendLiveLocationPermissions()
+            ),
+        )
         createLiveLocationStore(sessionId = SessionId("@bob:server"))
             .setAcceptedLiveLocationDisclaimer()
             .getOrThrow()
@@ -505,12 +535,13 @@ class ShareLocationPresenterTest {
             aPermissionsState(
                 permissions = PermissionsState.Permissions.NoneGranted,
                 shouldShowRationale = false,
+                permissionsRequested = true,
             )
         )
 
         shareLocationPresenter.test {
             val initialState = awaitFirstItem()
-            // Dismiss initial dialog
+            // Dismiss initial dialog to allow re-triggering it
             initialState.eventSink(ShareLocationEvent.DismissDialog)
             val dismissedState = awaitItem()
 

features/location/impl/src/test/kotlin/io/element/android/features/location/impl/show/ShowLocationPresenterTest.kt

@@ -37,7 +37,6 @@ import io.element.android.services.toolbox.test.strings.FakeStringProvider
 import io.element.android.tests.testutils.WarmUpRule
 import io.element.android.tests.testutils.test
 import kotlinx.coroutines.ExperimentalCoroutinesApi
-import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.runTest
 import org.junit.Rule
@@ -103,8 +102,8 @@ class ShowLocationPresenterTest {
 
         val presenter = createShowLocationPresenter()
         presenter.test {
-            val initialState = awaitItem()
+            assertThat(awaitItem().isTrackMyLocation).isFalse()
-            assertThat(initialState.isTrackMyLocation).isFalse()
+            assertThat(awaitItem().isTrackMyLocation).isTrue()
         }
     }
 
@@ -129,15 +128,12 @@ class ShowLocationPresenterTest {
             )
         )
         presenter.test {
-            skipItems(1)
             val initialState = awaitItem()
             assertThat(initialState.isTrackMyLocation).isFalse()
 
             initialState.eventSink(ShowLocationEvent.TrackMyLocation(true))
             val trackMyLocationState = awaitItem()
 
-            delay(1)
-
             assertThat(trackMyLocationState.isTrackMyLocation).isTrue()
 
             // Swipe the map to switch mode
@@ -201,12 +197,34 @@ class ShowLocationPresenterTest {
         }
     }
 
+    @Test
+    fun `TrackMyLocation with permissions not yet requested triggers permission request`() = runTest {
+        fakePermissionsPresenter.givenState(
+            aPermissionsState(
+                permissions = PermissionsState.Permissions.NoneGranted,
+                shouldShowRationale = false,
+                permissionsRequested = false,
+            )
+        )
+
+        val presenter = createShowLocationPresenter()
+        presenter.test {
+            val initialState = awaitItem()
+
+            initialState.eventSink(ShowLocationEvent.TrackMyLocation(true))
+
+            assertThat(fakePermissionsPresenter.events).contains(PermissionsEvents.RequestPermissions)
+            cancelAndIgnoreRemainingEvents()
+        }
+    }
+
     @Test
     fun `permission denied dialog dismiss`() = runTest {
         fakePermissionsPresenter.givenState(
             aPermissionsState(
                 permissions = PermissionsState.Permissions.NoneGranted,
                 shouldShowRationale = false,
+                permissionsRequested = true,
             )
         )
 
@@ -235,6 +253,7 @@ class ShowLocationPresenterTest {
             aPermissionsState(
                 permissions = PermissionsState.Permissions.NoneGranted,
                 shouldShowRationale = false,
+                permissionsRequested = true,
             )
         )
 

features/roommembermoderation/impl/src/test/kotlin/io/element/android/features/roommembermoderation/impl/RoomMemberModerationViewTest.kt

@@ -21,6 +21,7 @@ import io.element.android.features.roommembermoderation.api.RoomMemberModeration
 import io.element.android.libraries.architecture.AsyncAction
 import io.element.android.libraries.matrix.api.user.MatrixUser
 import io.element.android.libraries.testtags.TestTags
+import io.element.android.tests.testutils.EnsureNeverCalledWithParam
 import io.element.android.tests.testutils.EnsureNeverCalledWithTwoParams
 import io.element.android.tests.testutils.EventsRecorder
 import io.element.android.tests.testutils.clickOn
@@ -48,6 +49,8 @@ class RoomMemberModerationViewTest {
                 onSelectAction = callback
             )
             clickOn(R.string.screen_bottom_sheet_manage_room_member_member_user_info)
+            // Gives time for bottomsheet to hide
+            mainClock.advanceTimeBy(1_000)
         }
     }
 
@@ -217,11 +220,13 @@ class RoomMemberModerationViewTest {
 private fun AndroidComposeUiTest<ComponentActivity>.setRoomMemberModerationView(
     state: InternalRoomMemberModerationState,
     onSelectAction: (ModerationAction, MatrixUser) -> Unit = EnsureNeverCalledWithTwoParams(),
+    onAvatarClick: ((MatrixUser) -> Unit)? = EnsureNeverCalledWithParam(),
 ) {
     setSafeContent {
         RoomMemberModerationView(
             state = state,
             onSelectAction = onSelectAction,
+            onAvatarClick = onAvatarClick,
         )
     }
 }