[Device Manager] Render security recommendations (PSG-681) #6965

onurays · 2022-08-30T11:29:14Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Implement current session section as in the new layout.

Motivation and context

Closes #6964

Screenshots / GIFs

Tests

Enable the new device management feature flag
Go to "Security & Privacy" settings
Press the "Show all sessions (V2, WIP)" entry
See the security recommendations are rendered as in the new layout

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

bmarty

LGTM, just small remarks. Did not test the code.

bmarty · 2022-08-31T15:41:52Z

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

+    fun execute(lastSeenTs: Long): Boolean {
+        val lastSeenDate = DateProvider.toLocalDateTime(lastSeenTs)
+        val currentDate = DateProvider.currentLocalDateTime()
+        val diffMilliseconds = currentDate.toTimestamp() - lastSeenDate.toTimestamp()


I would inject a Clock in the constructor and just make a diff between lastSeenTs and clock.epochMillis().

Right, easier, done.

bmarty · 2022-08-31T15:46:47Z

vector/src/main/res/values/strings.xml

+    <string name="device_manager_unverified_sessions_title">Unverified sessions</string>
+    <string name="device_manager_unverified_sessions_description">Verify or sign out from unverified sessions.</string>
+    <string name="device_manager_inactive_sessions_title">Inactive sessions</string>
+    <string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (90 days or older) you don’t use anymore.</string>


Is there a word missing here?

Suggested change

<string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (90 days or older) you don’t use anymore.</string>

<string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (90 days or older) you don’t use them anymore.</string>

Also maybe 90 days or more is better than 90 days or older.

Disclaimer: I am not a native speaker!

Also 90 is hard-coded here, we could use %d and use SESSION_IS_MARKED_AS_INACTIVE_AFTER_DAYS for more flexibility (Element or their forks).

Done. I believe the missing word was "that".

bmarty · 2022-09-01T09:25:14Z

vector/src/main/res/values/strings.xml

@@ -3222,6 +3222,7 @@
    <string name="device_manager_other_sessions_view_all">View All (%1$d)</string>
    <string name="device_manager_other_sessions_description_verified">Verified · Last activity %1$s</string>
    <string name="device_manager_other_sessions_description_unverified">Unverified · Last activity %1$s</string>
+    <string name="device_manager_other_sessions_description_inactive">Inactive for %1$d+ days (%2$s)</string>


ElementBot is right (I did not think about it) please use a plurals. In English it will not change anything (since I do not think we will use the value 1, but on some other languages we can have difference.

Fixed in 5791a4d

bmarty · 2022-09-01T09:26:44Z

vector/src/main/res/values/strings.xml

@@ -3228,6 +3228,6 @@
    <string name="device_manager_unverified_sessions_title">Unverified sessions</string>
    <string name="device_manager_unverified_sessions_description">Verify or sign out from unverified sessions.</string>
    <string name="device_manager_inactive_sessions_title">Inactive sessions</string>
-    <string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (90 days or older) you don’t use anymore.</string>
+    <string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (%1$d days or more) that you don’t use anymore.</string>


Please also use a plurals here. Thanks!

Fixed in 5791a4d

mnaturel · 2022-09-02T09:33:15Z

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

@@ -42,18 +44,26 @@ class OtherSessionsController @Inject constructor(
        } else {
            data.take(NUMBER_OF_OTHER_DEVICES_TO_RENDER).forEach { device ->
                val formattedLastActivityDate = host.dateFormatter.format(device.deviceInfo.lastSeenTs, DateFormatKind.DEFAULT_DATE_AND_TIME)


To format the date when inactive, I think we should use DateFormatKind.TIMELINE_DAY_DIVIDER to only get the date.

Fixed in fbff8d6

mnaturel · 2022-09-02T09:42:08Z

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

+        views.recommendationShieldImageView.backgroundTintList = ColorStateList.valueOf(backgroundTint)
+    }
+
+    fun setTitle(title: String) {


I would create a dedicated SecurityRecommendationViewState in a separate file to encapsulate all data used to render the view. Then we would have a unique method render(viewState: SecurityRecommendationViewState) to be called when needed.

ViewState added in c53a8f3

mnaturel · 2022-09-02T09:52:11Z

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

@@ -68,6 +72,7 @@ abstract class OtherSessionItem : VectorEpoxyModel<OtherSessionItem.Holder>(R.la
        holder.otherSessionVerificationStatusImageView.render(roomEncryptionTrustLevel)
        holder.otherSessionNameTextView.text = sessionName
        holder.otherSessionDescriptionTextView.text = sessionDescription
+        holder.otherSessionDescriptionTextView.setCompoundDrawablesWithIntrinsicBounds(sessionDescriptionDrawable, null, null, null)


I am not sure the drawable will have the same tint color as the text. Can you check both in light and dark mode?

Fixed in dc0a36

sonarqubecloud · 2022-09-02T16:03:02Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

onurays linked an issue Aug 30, 2022 that may be closed by this pull request

[Device Manager] Render Security Recommendations #6964

Closed

onurays changed the base branch from develop to feature/ons/device_manager_other_session_list August 30, 2022 13:40

onurays requested review from a team and bmarty and removed request for a team August 30, 2022 14:22

Base automatically changed from feature/ons/device_manager_other_session_list to develop August 31, 2022 14:01

onurays marked this pull request as ready for review August 31, 2022 14:06

bmarty reviewed Aug 31, 2022

View reviewed changes

onurays requested a review from bmarty August 31, 2022 17:21

bmarty reviewed Sep 1, 2022

View reviewed changes

mnaturel reviewed Sep 2, 2022

View reviewed changes

onurays and others added 16 commits September 2, 2022 14:27

Create security recommendations section header.

55d0a06

Create security recommendations view layout.

7e8bbda

Create generic security recommendations view.

e6c9409

Implement security recommendation view class.

c23a4e4

Render security recommendations.

5884603

Add changelog.

2f94b80

Format layout.

32f1a66

Fix image background.

abd8265

Move inactive session logic into use case.

aeb6489

Test inactive sessions use case.

396da93

Fix description of inactive sessions.

ed1244a

Code review fixes.

8d37840

Post rebase fix

2b50208

Using plurals when needed

5791a4d

Use a different date format when device is inactive

fbff8d6

Creation of a viewState for the SecurityRecommendationView

c53a8f3

Apply correct tint to inactive status drawable

9dc0a36

mnaturel force-pushed the feature/ons/device_manager_security_recommendations branch from 287c153 to 9dc0a36 Compare September 2, 2022 14:16

mnaturel approved these changes Sep 2, 2022

View reviewed changes

mnaturel merged commit e81f02f into develop Sep 5, 2022

mnaturel deleted the feature/ons/device_manager_security_recommendations branch September 5, 2022 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Device Manager] Render security recommendations (PSG-681) #6965

[Device Manager] Render security recommendations (PSG-681) #6965

onurays commented Aug 30, 2022 •

edited by mnaturel

Loading

bmarty left a comment

bmarty Aug 31, 2022

onurays Aug 31, 2022

bmarty Aug 31, 2022

onurays Aug 31, 2022

bmarty Sep 1, 2022

mnaturel Sep 2, 2022

bmarty Sep 1, 2022

mnaturel Sep 2, 2022

mnaturel Sep 2, 2022

mnaturel Sep 2, 2022

mnaturel Sep 2, 2022

mnaturel Sep 2, 2022 •

edited

Loading

mnaturel Sep 2, 2022

mnaturel Sep 2, 2022

sonarqubecloud bot commented Sep 2, 2022

	<string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (90 days or older) you don’t use anymore.</string>
	<string name="device_manager_inactive_sessions_description">Consider signing out from old sessions (90 days or older) you don’t use them anymore.</string>

[Device Manager] Render security recommendations (PSG-681) #6965

[Device Manager] Render security recommendations (PSG-681) #6965

Conversation

onurays commented Aug 30, 2022 • edited by mnaturel Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

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

mnaturel Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 2, 2022

onurays commented Aug 30, 2022 •

edited by mnaturel

Loading

mnaturel Sep 2, 2022 •

edited

Loading