[Device Manager] Parse user agents (PSG-762) #7247

onurays · 2022-09-28T15:21:27Z

Type of change

Feature
Bugfix
Technical
Other :

Content

We need to parse user agents of all platforms to be able to render them on new device manager screens.

Motivation and context

Screenshots / GIFs

Tests

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

kerryarchibald · 2022-09-29T08:58:12Z

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

+import org.amshove.kluent.shouldBeEqualTo
+import org.junit.Test
+
+private val A_USER_AGENT_LIST_FOR_ANDROID = listOf(


I think it would be good to test some weird user agents since they can be fully customised by other applications or browser extensions like empty string, curl/7.55.1, some random string etc

Right, added. Thanks.

mnaturel · 2022-09-29T12:30:08Z

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

+
+import im.vector.app.features.settings.devices.v2.list.DeviceType
+
+data class DeviceUserAgent(


Maybe we should name it DeviceExtendedInfo to avoid referencing the source of the data? We could decide in the future to use something different than the user agent but the info will still be the same.

Make sense, done.

mnaturel · 2022-09-29T12:43:15Z

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

+    }
+
+    private fun isMobile(browserSegments: List<String>): Boolean {
+        return browserSegments.lastOrNull()?.startsWith("Safari").orFalse() &&


Out of curiosity, why do we check for Safari keyword for mobile browser case?

Nice catch! Fixed.

mnaturel · 2022-09-29T12:46:46Z

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

+        } else {
+            deviceOperatingSystemSegments.getOrNull(0)
+        }
+        return DeviceUserAgent(DeviceType.DESKTOP, browserName, deviceOperatingSystem, null, null)


In the case of Web/Desktop, we cannot extract an app name and an app version?

Yes, web browsers are generating it automatically without app name. Instead, web will put them inside account_data. So we will need both extended device info and account data to be able to render session details screen.

mnaturel · 2022-09-29T12:56:28Z

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

@@ -110,21 +117,24 @@ class GetDeviceFullInfoListUseCaseTest {
                cryptoDeviceInfo = cryptoDeviceInfo1,
                roomEncryptionTrustLevel = RoomEncryptionTrustLevel.Trusted,
                isInactive = true,
-                isCurrentDevice = true
+                isCurrentDevice = true,
+                deviceUserAgent = DeviceUserAgent(DeviceType.MOBILE)


This test should not pass normally. We expect to have the mocked result as deviceUseAgent value:

DeviceUserAgent( DeviceType.MOBILE, "Xiaomi Mi 9T", "Android 11", "Element dbg", "1.5.0-dev" )

This means the assertion seems to be not correct in a certain way since it should not be equal to DeviceUserAgent(DeviceType.MOBILE).

It is because filter use case is a mockk and returning expected result. Anyway, we don't need to test it, removed.

mnaturel

Nice work! The code is clean and fairly easy to read regarding the complexity of the non-standardization of the data structure. I added some minor comments to look into.

bmarty

Some thoughts.

bmarty · 2022-09-29T13:28:06Z

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

+        val unstableLastSeenUserAgent: String? = null,
+
+        @Json(name = "last_seen_user_agent")
+        val lastSeenUserAgent: String? = null,


This is strange to introduce both unstable and stable keys in the same PR.
You should use only unstable, then when the MSC is merged, use both, then after a while, remove unstable, that the servers should not used anymore.
If the MSC is already merged, you can directly use the stable key.

Let's say we used only unstable one. MSC is merged and server is not using unstable one anymore and sending us the stable one. But I didn't updated EA in my device. So we will miss this field, no?

From my understanding, the server has to use both stable and unstable field for a limited period of time.

See in the doc:

Implementations MUST NOT use stable endpoints before the MSC has completed FCP. Once that has occurred, implementations are allowed to use stable endpoints, but are not required to.

This is about endpoint, but I think the same rule applies for Json key.

I will ask this to Backend team. I would be happy if we can get rid of this. For the record; we also have them for live location and polls.

bmarty · 2022-09-29T13:32:21Z

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

+    companion object {
+        // Element dbg/1.5.0-dev (Xiaomi; Mi 9T; Android 11; RKQ1.200826.002 test-keys; Flavour GooglePlay; MatrixAndroidSdk2 1.5.0)
+        // Legacy : Element/1.0.0 (Linux; U; Android 6.0.1; SM-A510F Build/MMB29; Flavour GPlay; MatrixAndroidSdk2 1.0)
+        private const val ANDROID_KEYWORD = "; MatrixAndroidSdk2"


Note: this will probably change when we will use the Rust SDK.

Right, I couldn't find safer way for now :/ I can make this keyword in a const val to be safer. But then again we can't parse non-updated clients.

bmarty · 2022-09-29T13:39:40Z

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

+        val deviceInfoSegments = userAgent.substringAfter("(").substringBeforeLast(")").split("; ")
+        val deviceModel = deviceInfoSegments.getOrNull(0)
+        val deviceOperatingSystem = deviceInfoSegments.getOrNull(1)
+        return DeviceUserAgent(DeviceType.MOBILE, deviceModel, deviceOperatingSystem, appName, appVersion)


We do not distinguish iOS and Android devices? I thought the idea would be to render them differently in the UI.

Yes, the design suggests only one icon for mobile devices. Instead, we will render device model info in session details.

bmarty · 2022-09-29T13:42:20Z

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

+        AN_UNKNOWN_USER_AGENT_LIST.forEachIndexed { index, userAgent ->
+            parseDeviceUserAgentUseCase.execute(userAgent) shouldBeEqualTo AN_UNKNOWN_USER_AGENT_EXPECTED_RESULT_LIST[index]
+        }
+    }


Maybe to avoid repeating the logic in all the test, you could extract it:

private fun doTest(userAgentList: List<String>, expectedResultList: List<DeviceUserAgent>) { userAgentList.forEachIndexed { index, userAgent -> parseDeviceUserAgentUseCase.execute(userAgent) shouldBeEqualTo expectedResultList[index] } }

and then call:

doTest(AN_UNKNOWN_USER_AGENT_LIST, AN_UNKNOWN_USER_AGENT_EXPECTED_RESULT_LIST)

It is easier to navigate to failed test case in Android Studio or see the which one is failing on CI by checking function name on logs. If we make this generic it would take some more time to detect which platform is failing.

mnaturel · 2022-09-30T07:36:11Z

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

+        /**
+         * i.e. Chrome.
+         */
+        val browser: String? = null,


Is this field used? I am not seeing it is used in the parsing use case. I think we should use clientName and clientVersion field to set the browser info in case of a Web device.

mnaturel · 2022-09-30T07:39:27Z

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

+            deviceModel = deviceInfoSegments.getOrNull(0)
+            deviceOperatingSystem = deviceInfoSegments.getOrNull(1)
+        }
+        return DeviceExtendedInfo(DeviceType.MOBILE, deviceModel, deviceOperatingSystem, appName, appVersion)


Could we set the fields of the DeviceExtendedInfo by naming them to avoid confusion? For example:

DeviceExtendedInfo( deviceType = DeviceType.MOBILE, deviceModel = deviceModel, deviceOperatingSystem = deviceOperatingSystem, clientName = appName, clientVersion = appVersion, )

Yes, better. Done.

mnaturel · 2022-09-30T07:52:06Z

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

+        } else {
+            deviceOperatingSystemSegments.getOrNull(0)
+        }
+        return DeviceExtendedInfo(DeviceType.DESKTOP, browserName, deviceOperatingSystem, null, null)


I think in case of the Web device type, we should set browser info into the clientName and clientVersion. It seems we should only use these fields for the case of Web devices to render a "Browser" section. For the section "Application", we will use info coming from accountData event.

sonarcloud · 2022-09-30T14:52:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

87.8% Coverage
0.0% Duplication

Onuray Sahin added 7 commits September 26, 2022 14:39

Add user agent into device info.

c70b620

Implement use case to parse user agent.

3e66a65

Refactor user agent parsing.

04a305b

Add unit test for Android user agents.

2bcf0c3

Add unit test for iOS user agents.

41643ff

Add unit test for desktop and web user agents.

5666383

Add changelog.

7a36b10

onurays requested review from a team and mnaturel and removed request for a team September 28, 2022 15:22

Onuray Sahin added 2 commits September 29, 2022 11:36

Add more user agent support.

c16b5d6

Lint fixes.

4c173a7

kerryarchibald reviewed Sep 29, 2022

View reviewed changes

Onuray Sahin added 2 commits September 29, 2022 12:35

Add tests for unknown user agents.

8663fe8

Fix broken tests.

38cd2be

mnaturel reviewed Sep 29, 2022

View reviewed changes

bmarty reviewed Sep 29, 2022

View reviewed changes

Onuray Sahin added 3 commits September 29, 2022 17:35

Code review fixes.

6d459a0

Code review fixes.

81e8ddf

Lint fix.

0f8637b

onurays requested a review from mnaturel September 29, 2022 15:08

mnaturel reviewed Sep 30, 2022

View reviewed changes

Onuray Sahin added 2 commits September 30, 2022 13:21

Code review fixes.

bf4576d

Parse browser version.

ea8dc45

onurays requested a review from mnaturel September 30, 2022 14:36

mnaturel approved these changes Sep 30, 2022

View reviewed changes

onurays merged commit d0dd446 into develop Sep 30, 2022

onurays deleted the feature/ons/parse_user_agent branch September 30, 2022 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Device Manager] Parse user agents (PSG-762) #7247

[Device Manager] Parse user agents (PSG-762) #7247

onurays commented Sep 28, 2022 •

edited

kerryarchibald Sep 29, 2022

onurays Sep 29, 2022

mnaturel Sep 29, 2022

onurays Sep 29, 2022

mnaturel Sep 29, 2022

onurays Sep 29, 2022

mnaturel Sep 29, 2022

onurays Sep 29, 2022

mnaturel Sep 29, 2022

onurays Sep 29, 2022

mnaturel left a comment

bmarty left a comment

bmarty Sep 29, 2022

onurays Sep 29, 2022

bmarty Sep 29, 2022

bmarty Sep 29, 2022

onurays Sep 30, 2022

bmarty Sep 29, 2022

onurays Sep 29, 2022

bmarty Sep 29, 2022

onurays Sep 29, 2022

bmarty Sep 29, 2022

onurays Sep 29, 2022

mnaturel Sep 30, 2022

mnaturel Sep 30, 2022

onurays Sep 30, 2022

mnaturel Sep 30, 2022

sonarcloud bot commented Sep 30, 2022


		import im.vector.app.features.settings.devices.v2.list.DeviceType

		data class DeviceUserAgent(

[Device Manager] Parse user agents (PSG-762) #7247

[Device Manager] Parse user agents (PSG-762) #7247

Conversation

onurays commented Sep 28, 2022 • edited

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

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

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

sonarcloud bot commented Sep 30, 2022

onurays commented Sep 28, 2022 •

edited