Toggle IP address visibility (PSG-860) #7546

onurays · 2022-11-08T16:27:18Z

Type of change

Feature
Bugfix
Technical
Other :

Content

In new Session Management screens we should hide the IP addresses of devices by default for the sake of privacy (in case users may demo these screens to someone else etc.). In this PR, we will provide a menu item to be able to toggle the visibility of IP addresses. Once a user selects to show or hide the IP addresses then this option is saved to the shared preferences and will be reflected to the screens on next app launches.

Motivation and context

Screenshots / GIFs

toggle_ip_address.mp4

Tests

Enable new session management screen from Labs settings
Navigate to Settings > Security & Privacy > Show All sessions
Scroll down to Other Sessions section, click to the menu
Toggle Show/Hide IP address menu item and see the IP addresses visibility changes
Click View All button at the bottom of the Other Sessions section
Toggle Show/Hide IP address menu item and see the changes on UI
Select a session to navigate to the session overview screen
Toggle Show/Hide IP address menu item in session overview screen

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

# Conflicts: # vector/src/test/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

mnaturel · 2022-11-16T10:30:30Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/DevicesViewModel.kt

+    }
+
+    private fun refreshIpAddressVisibility() {
+        val shouldShowIpAddress = sharedPreferences.getBoolean(VectorPreferences.SETTINGS_SESSION_MANAGER_SHOW_IP_ADDRESS, false)


I think we should either define the default value for this preference somewhere in VectorPreferences or create a method to retrieve this setting so that it can be reused accross the screens.

Good idea, done.

mnaturel · 2022-11-16T10:35:44Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/DevicesViewModel.kt

+        setState {
+            copy(isShowingIpAddress = !isShowingIpAddress)
+        }
+        sharedPreferences.edit {


I think it would be better to encapsulate the domain layer code inside a UseCase. I would see 2 options in prefered order:

if we have a way to observe settings in the app, we should have a usecase to observe the ip address visiblity setting and another one to toggle the setting.

if we don't have a way to observe settings, we should have a usecase to set the setting to a certain value.

What do you think?

I created a use case to set the value.

mnaturel · 2022-11-16T10:37:49Z

vector/src/test/java/im/vector/app/features/settings/devices/v2/DevicesViewModelTest.kt

@@ -72,6 +73,7 @@ class DevicesViewModelTest {
    private val fakeInterceptSignoutFlowResponseUseCase = mockk<InterceptSignoutFlowResponseUseCase>()
    private val fakePendingAuthHandler = FakePendingAuthHandler()
    private val fakeRefreshDevicesUseCase = mockk<RefreshDevicesUseCase>(relaxUnitFun = true)
+    private val fakeSharedPreferences = FakeSharedPreferences()


New tests should be added:

to cover the new toggle visibility action

to cover the update of viewState at the initialization

mnaturel · 2022-11-16T12:10:24Z

vector/src/main/java/im/vector/app/features/settings/devices/v2/list/SessionInfoView.kt

@@ -187,6 +189,7 @@ class SessionInfoView @JvmOverloads constructor(
            views.sessionInfoLastActivityTextView.isGone = true
        }
        views.sessionInfoLastIPAddressTextView.setTextOrHide(deviceInfo.lastSeenIp?.takeIf { isLastSeenDetailsVisible })


I think we could write instead:
views.sessionInfoLastIPAddressTextView.setTextOrHide(deviceInfo.lastSeenIp?.takeIf { isLastSeenDetailsVisible && isShowingIpAddress })

Good idea, done.

mnaturel · 2022-11-16T12:18:46Z

...main/java/im/vector/app/features/settings/devices/v2/othersessions/OtherSessionsViewModel.kt

+    }
+
+    private fun handleToggleIpAddressVisibility() = withState { state ->
+        val isShowingIpAddress = state.isShowingIpAddress


Same remark for adding a reusable usecase.

Refactored.

mnaturel · 2022-11-16T12:19:42Z

.../java/im/vector/app/features/settings/devices/v2/othersessions/OtherSessionsViewModelTest.kt

@@ -66,6 +67,7 @@ class OtherSessionsViewModelTest {
    private val fakeRefreshDevicesUseCase = mockk<RefreshDevicesUseCase>(relaxed = true)
    private val fakeSignoutSessionsUseCase = FakeSignoutSessionsUseCase()
    private val fakePendingAuthHandler = FakePendingAuthHandler()
+    private val fakeSharedPreferences = FakeSharedPreferences()

    private fun createViewModel(viewState: OtherSessionsViewState = OtherSessionsViewState(defaultArgs)) =


We should add units tests to cover the new action and the change of the viewState in the initialization.

I think tests have not been added for this ViewModel.

mnaturel · 2022-11-16T12:22:09Z

...rc/main/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModel.kt

+    }
+
+    private fun handleToggleIpAddressVisibility() = withState { state ->
+        val isShowingIpAddress = state.isShowingIpAddress


Same remark for the need of a usecase.

Refactored.

mnaturel · 2022-11-16T12:28:50Z

...est/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

@@ -103,6 +106,7 @@ class SessionOverviewViewModelTest {
                A_SESSION_ID_1,
                notificationsStatus
        )
+        fakeSharedPreferences.givenSessionManagerShowIpAddress(false)


Same remark for the missing new unit tests.

mnaturel · 2022-11-16T12:56:58Z

vector/src/main/res/layout/item_other_session.xml

        app:layout_constraintEnd_toEndOf="@id/otherSessionNameTextView"
        app:layout_constraintStart_toStartOf="@id/otherSessionNameTextView"
        app:layout_constraintTop_toBottomOf="@id/otherSessionNameTextView"
        tools:text="@string/device_manager_verification_status_verified" />

+    <TextView


I think we should update some constraints on this layout:

For the view with id otherSessionItemBackground we should replace app:layout_constraintBottom_toBottomOf="@id/otherSessionVerificationStatusImageView" by app:layout_constraintBottom_toBottomOf="@id/otherSessionIpAddressTextView"

For the view with id otherSessionNameTextView we should replace app:layout_constraintTop_toTopOf="@id/otherSessionDeviceTypeImageView" by app:layout_constraintTop_toTopOf="@id/otherSessionItemBackground"

Without these changes, you can see in the below screen there is an issue with the background for selected items when in select mode:

Good catch, done.

mnaturel

When testing it, I have found a small bug which could be fixed by observing the setting as I suggested instead of getting the setting value when ViewModel is initialized. To reproduce the bug:

Make sure the IP address is not visible
Go to the other sessions list
Go to a session overview screen
From this screen, show the IP address
Press back
We can see IP address are not displayed since the ViewModel is using the setting value when the ViewModel was created.

… enabled." This reverts commit f63c6c3.

mnaturel · 2022-11-22T14:15:10Z

vector-config/src/main/res/values/config-settings.xml

@@ -51,6 +51,7 @@
    <bool name="settings_labs_rich_text_editor_default">false</bool>
    <bool name="settings_labs_enable_voice_broadcast_visible">true</bool>
    <bool name="settings_labs_enable_voice_broadcast_default">false</bool>
+    <bool name="settings_device_manager_show_ip_address">false</bool>


Thanks for adding the default value here. But I think it should not be in the labs section, maybe in a new section Level 3: Security & Privacy, Sessions?

mnaturel · 2022-11-22T14:17:42Z

vector/src/main/java/im/vector/app/features/settings/VectorPreferences.kt

@@ -1228,4 +1231,14 @@ class VectorPreferences @Inject constructor(
        return vectorFeatures.isVoiceBroadcastEnabled() &&
                defaultPrefs.getBoolean(SETTINGS_LABS_VOICE_BROADCAST_KEY, getDefault(R.bool.settings_labs_enable_voice_broadcast_default))
    }
+
+    fun showIpAddressInDeviceManagerScreens(): Boolean {


Small question about naming, should we align the methods and preference setting names on SessionManager instead of DeviceManager?

mnaturel · 2022-11-22T14:26:29Z

...src/main/java/im/vector/app/features/settings/devices/v2/ToggleIpAddressVisibilityUseCase.kt

+import im.vector.app.features.settings.VectorPreferences
+import javax.inject.Inject
+
+class ToggleIpAddressVisibilityUseCase @Inject constructor(


I think we should add unit tests for this new use case.

mnaturel · 2022-11-22T14:32:01Z

...est/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

@@ -77,6 +79,8 @@ class SessionOverviewViewModelTest {
    private val togglePushNotificationUseCase = FakeTogglePushNotificationUseCase()
    private val fakeGetNotificationsStatusUseCase = FakeGetNotificationsStatusUseCase()
    private val notificationsStatus = NotificationsStatus.ENABLED
+    private val fakeVectorPreferences = FakeVectorPreferences()
+    private val toggleIpAddressVisibilityUseCase = mockk<ToggleIpAddressVisibilityUseCase>()

    private fun createViewModel() = SessionOverviewViewModel(


New tests should also be added for this ViewModel. Maybe a FakeToggleIpAddressVisibilityUseCase can be created to reuse it in the other ViewModel?

mnaturel

Thanks for the updates! I have added some small comments we should handle before merge.

ElementBot · 2022-11-22T15:31:26Z

	Warnings
⚠️	vector/src/main/res/layout/item_other_session.xml#L7 - Attribute `android:foreground` has no effect on API levels lower than 23 (current min is 21)
⚠️	vector/src/main/res/layout/item_other_session.xml#L7 - Attribute `android:foreground` has no effect on API levels lower than 23 (current min is 21)

Generated by 🚫 dangerJS against abea9b6

sonarcloud · 2022-11-22T16:29:10Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

47.2% Coverage
0.0% Duplication

Onuray Sahin added 5 commits November 7, 2022 18:26

Add toggle ip address menu option.

456762a

Toggle ip address on others section of the main screen.

eed2a74

Toggle ip address on other sessions screen.

b5e8375

Toggle ip address on sessions overview screen.

0868869

Persist user preference of ip address visibility.

f6bc28f

onurays requested review from a team and jonnyandrew and removed request for a team November 8, 2022 16:27

Onuray Sahin added 2 commits November 8, 2022 19:31

Add changelog.

8bc7000

Lint fix.

e888c11

onurays requested review from a team, fedrunov, jonnyandrew and mnaturel and removed request for jonnyandrew, a team and fedrunov November 9, 2022 10:55

Onuray Sahin added 4 commits November 9, 2022 16:57

Fix unit tests.

46c60f5

Merge branch 'develop' into feature/ons/toggle_ip_address_visibility

b81fc4f

# Conflicts: # vector/src/test/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

Merge branch 'develop' into feature/ons/toggle_ip_address_visibility

6f997e8

Merge branch 'develop' into feature/ons/toggle_ip_address_visibility

202c0c5

# Conflicts: # vector/src/test/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

onurays requested review from a team and bmarty and removed request for jonnyandrew, mnaturel and a team November 14, 2022 07:26

mnaturel self-requested a review November 16, 2022 10:12

mnaturel reviewed Nov 16, 2022

View reviewed changes

mnaturel suggested changes Nov 16, 2022

View reviewed changes

Onuray Sahin added 5 commits November 17, 2022 13:19

Fix italic text is truncated when bubble mode and markdown is enabled.

f63c6c3

Revert "Fix italic text is truncated when bubble mode and markdown is…

c788dea

… enabled." This reverts commit f63c6c3.

Code review fixes.

6c45490

Code review fixes.

ab749ee

Code review fixes.

5eb786b

onurays requested a review from mnaturel November 22, 2022 13:21

mnaturel reviewed Nov 22, 2022

View reviewed changes

mnaturel approved these changes Nov 22, 2022

View reviewed changes

Code review fixes.

abea9b6

onurays merged commit 0957b38 into develop Nov 22, 2022

onurays deleted the feature/ons/toggle_ip_address_visibility branch November 22, 2022 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggle IP address visibility (PSG-860) #7546

Toggle IP address visibility (PSG-860) #7546

onurays commented Nov 8, 2022 •

edited

Loading

mnaturel Nov 16, 2022

onurays Nov 22, 2022

mnaturel Nov 16, 2022

onurays Nov 22, 2022

mnaturel Nov 16, 2022

mnaturel Nov 16, 2022 •

edited

Loading

onurays Nov 22, 2022

mnaturel Nov 16, 2022

onurays Nov 22, 2022

mnaturel Nov 16, 2022

onurays Nov 22, 2022

mnaturel Nov 22, 2022

mnaturel Nov 16, 2022

onurays Nov 22, 2022

mnaturel Nov 16, 2022

mnaturel Nov 16, 2022

mnaturel Nov 16, 2022

onurays Nov 22, 2022

mnaturel left a comment

mnaturel Nov 22, 2022

mnaturel Nov 22, 2022

mnaturel Nov 22, 2022

mnaturel Nov 22, 2022 •

edited

Loading

mnaturel left a comment

ElementBot commented Nov 22, 2022

sonarcloud bot commented Nov 22, 2022

Toggle IP address visibility (PSG-860) #7546

Toggle IP address visibility (PSG-860) #7546

Conversation

onurays commented Nov 8, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

ElementBot commented Nov 22, 2022

sonarcloud bot commented Nov 22, 2022

onurays commented Nov 8, 2022 •

edited

Loading

mnaturel Nov 16, 2022 •

edited

Loading

mnaturel Nov 22, 2022 •

edited

Loading