[Devices Management] Session overview screen (PSG-691, PSG-693) #7005

mnaturel · 2022-09-05T08:22:29Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adding the new Session overview screen. In this screen, we only see a single view with session info for now. This new screen can be reached from a tap on current session info view (whole card or details button) and from any session item from other sessions list.
Mutualizing the SessionInfoView which can be reused for current session or any session.
Adding UseCases with tests to compute the trust level of a given Session.

Motivation and context

Closes #6961

Screenshots / GIFs

Tests

Enable the feature flag for new device management
Go to Settings -> Security&Privacy -> Show all Sessions (V2, WIP)
Press the current session view or the details button
Check the Session overview screen is opened with the correct info corresponding to the current session
Press back
Press any session item from the "Other sessions" list
Check the Session overview screen is opened with the correct info corresponding to the selected session

Tested devices

Physical
Emulator
OS version(s): Android 11

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()

ElementBot · 2022-09-05T08:24:37Z

	Warnings
⚠️	vector/src/main/AndroidManifest.xml#L220 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L220 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L223 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L223 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L277 - Expecting `android:screenOrientation="unspecified"` or `"fullSensor"` for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices
⚠️	vector/src/main/AndroidManifest.xml#L277 - Expecting `android:screenOrientation="unspecified"` or `"fullSensor"` for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices
⚠️	vector/src/main/AndroidManifest.xml#L286 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L286 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L293 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L293 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L299 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L299 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L415 - Exported receiver does not require permission
⚠️	vector/src/main/AndroidManifest.xml#L415 - Exported receiver does not require permission
⚠️	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 669beb1

mnaturel · 2022-09-05T08:45:27Z

Messages
📖 This pull request seems relatively large. Please consider splitting it into multiple smaller ones.

Generated by 🚫 dangerJS against 838064d

This is due to the fact I had to do many small renamings and extract reusable use cases to reduce the existing technical debt on this area. So the PR is large but modifications are not complex. I am not sure how to split it without rewriting the whole history into different branches...

…e id

…n device id

…odel

fedrunov

LGTM

sonarcloud · 2022-09-07T17:24:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

4.2% Coverage
0.0% Duplication

bmarty

Nice work! Some post merge remarks, feel free to handle them in a quick PR or ignore, the goal is also to give you some feedback. Thanks!

bmarty · 2022-09-08T09:49:03Z

library/ui-styles/src/main/res/values/stylable_devices_list_header_view.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <resources>

-    <declare-styleable name="DevicesListHeaderView">
+    <declare-styleable name="SessionsListHeaderView">


Maybe rename the file?

Yes I renamed it in a following PR 😄

bmarty · 2022-09-08T09:50:17Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt


-    fun getDeviceInfo(deviceId: String, callback: MatrixCallback<DeviceInfo>)
+    fun getMyDevicesInfoLive(deviceId: String): LiveData<Optional<DeviceInfo>>


a changelog with .sdk extension is required when there are changes in the API (especially SDK Services)

Also in this case, I think the fun should be named getMyDeviceInfoLive (no s on Devices)

bmarty · 2022-09-08T09:52:30Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt

-    fun getDeviceInfo(userId: String, deviceId: String?): CryptoDeviceInfo?
+    fun getCryptoDeviceInfo(userId: String, deviceId: String?): CryptoDeviceInfo?
+
+    fun getCryptoDeviceInfo(deviceId: String, callback: MatrixCallback<DeviceInfo>)


We've got 2 models: CryptoDeviceInfo and DeviceInfo. I think the fun name should match the model. This is not the case here.

bmarty · 2022-09-08T09:53:25Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt

+
+    fun getLiveCryptoDeviceInfoWithId(deviceId: String): LiveData<Optional<CryptoDeviceInfo>>
+
+    fun getLiveCryptoDeviceInfo(userId: String): LiveData<List<CryptoDeviceInfo>>


Maybe rename to getLiveCryptoDeviceInfos (plurals)

bmarty · 2022-09-08T09:53:39Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt

+
+    fun getCryptoDeviceInfo(userId: String): List<CryptoDeviceInfo>
+
+    fun getLiveCryptoDeviceInfo(): LiveData<List<CryptoDeviceInfo>>


Maybe rename to getLiveCryptoDeviceInfos (plurals), or maybe more English correct getLiveCryptoDevicesInfo

bmarty · 2022-09-08T10:00:33Z

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

+    private fun appendLearnMoreToVerificationStatus() {
+        val status = views.sessionInfoVerificationStatusDetailTextView.text
+        val learnMore = context.getString(R.string.action_learn_more)
+        val stringBuilder = StringBuilder()


Nit: Kotlin stdlib provides a handy buildString {} which can be nice to use in such case.

I didn't know this method. Thanks, I will update.

bmarty · 2022-09-08T10:02:55Z

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

+                }
+                ?: run {
+                    views.sessionInfoLastIPAddressTextView.isGone = true
+                }


Could be rewrite to shorter version using setTextOrHide:

views.sessionInfoLastIPAddressTextView.setTextOrHide(deviceInfo.lastSeenIp?.takeIf { isLastSeenDetailsVisible })

bmarty · 2022-09-08T10:06:32Z

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

+ * Display the overview info about a Session.
+ */
+@AndroidEntryPoint
+class SessionOverviewActivity : SimpleFragmentActivity() {


About the naming, the convention in the project would dictate to name this class SessionDetailActivity.

We will have in a next PR another screen for "Session details" which will be named SessionDetailsActivity. It will be accessed from this "Session overview" screen.

bmarty · 2022-09-08T10:08:17Z

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

+
+    private fun observeSessionInfo(deviceId: String) {
+        getDeviceFullInfoUseCase.execute(deviceId)
+                .mapNotNull { it.getOrNull() }


You could replace by .unwrap()

I didn't see there was this extension method. I will use it inside the getDeviceFullInfoUseCase.

bmarty · 2022-09-08T10:09:30Z

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

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="utf-8"?>
+<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"


I would have used a ScrollView here.

Yes it might be a good idea for small devices. It will be done in a next PR (session details screen).

mnaturel · 2022-09-08T11:23:20Z

Nice work! Some post merge remarks, feel free to handle them in a quick PR or ignore, the goal is also to give you some feedback. Thanks!

Thanks for the feedbacks, I will handle some of them in a dedicated future PR.

mnaturel requested review from a team and fedrunov and removed request for a team September 5, 2022 08:32

mnaturel marked this pull request as ready for review September 5, 2022 08:33

Maxime NATUREL added 24 commits September 7, 2022 09:32

Adding changelog entry

e2aa66e

Renaming header list view to be consistent

9696637

Navigation from current session

ebf707d

Unit tests for navigator

26dbd31

Renaming view state

3eb29b4

Small renamings/reorganization in CryptoService

2a599d9

Adding a method to retrieve livedata of device info for a given devic…

f74a0b0

…e id

Adding a method to retrieve livedata of crypto device info for a give…

e542dc4

…n device id

Adding use case to get full device info for a given device id

68f106e

Adding unit tests for the new use case

0c3310d

Adding unit tests for mapper

611749c

Adding unit tests for viewModel

b995f79

Introducing some reusable usecases

a5ee4fa

Renaming CurrentSessionView into SessionInfoView to be more generic

52006c1

Show info in overview screen

f4865df

Navigation from other session item

78c2b10

Updating existing unit tests

3f30cf0

Unit tests for GetCurrentSessionCrossSigningInfoUseCase

38a04c3

Unit tests for computing trust level of device

fde7072

Adding learn more link in verification status details

d12bb09

Adding last seen details + fix observation of wrong deviceId in ViewM…

1ad585e

…odel

Fixing wrong copyright title

fb8d466

Fix post rebase

52d05b3

Adding comment with examples of some parametrized strings

0b105e2

Maxime NATUREL added 3 commits September 7, 2022 09:32

Rendering inactive status in SessionInfoView

6e9b2ba

Update unit tests

2148270

Fix unused string warning

682905d

mnaturel force-pushed the feature/mna/session-overview-screen branch from eb59a53 to 682905d Compare September 7, 2022 07:40

Fix missing mapper in CryptoStoreHelper for tests

669beb1

fedrunov approved these changes Sep 7, 2022

View reviewed changes

mnaturel merged commit 6c2bf35 into develop Sep 8, 2022

mnaturel deleted the feature/mna/session-overview-screen branch September 8, 2022 07:18

bmarty reviewed Sep 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Devices Management] Session overview screen (PSG-691, PSG-693) #7005

[Devices Management] Session overview screen (PSG-691, PSG-693) #7005

mnaturel commented Sep 5, 2022 •

edited

Loading

ElementBot commented Sep 5, 2022 •

edited

Loading

mnaturel commented Sep 5, 2022

fedrunov left a comment

sonarcloud bot commented Sep 7, 2022

bmarty left a comment

bmarty Sep 8, 2022

mnaturel Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

mnaturel Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

mnaturel Sep 8, 2022

bmarty Sep 8, 2022

mnaturel Sep 8, 2022

bmarty Sep 8, 2022

mnaturel Sep 8, 2022

mnaturel commented Sep 8, 2022


		fun getDeviceInfo(deviceId: String, callback: MatrixCallback<DeviceInfo>)
		fun getMyDevicesInfoLive(deviceId: String): LiveData<Optional<DeviceInfo>>


		fun getLiveCryptoDeviceInfoWithId(deviceId: String): LiveData<Optional<CryptoDeviceInfo>>

		fun getLiveCryptoDeviceInfo(userId: String): LiveData<List<CryptoDeviceInfo>>


		fun getCryptoDeviceInfo(userId: String): List<CryptoDeviceInfo>

		fun getLiveCryptoDeviceInfo(): LiveData<List<CryptoDeviceInfo>>

		@@ -0,0 +1,20 @@
		<?xml version="1.0" encoding="utf-8"?>
		<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"

[Devices Management] Session overview screen (PSG-691, PSG-693) #7005

[Devices Management] Session overview screen (PSG-691, PSG-693) #7005

Conversation

mnaturel commented Sep 5, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

ElementBot commented Sep 5, 2022 • edited Loading

mnaturel commented Sep 5, 2022

fedrunov left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 7, 2022

bmarty 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

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 commented Sep 8, 2022

mnaturel commented Sep 5, 2022 •

edited

Loading

ElementBot commented Sep 5, 2022 •

edited

Loading