[Session manager] Multi-session signout (PSG-857) #7456

mnaturel · 2022-10-26T12:58:05Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adding capability to signout several sessions at once. On SDK side, adding new delete_devices endpoint.

Motivation and context

Closes #7418

Screenshots / GIFs

Tests

Enable the new session manager labs setting
Go to Settings -> Security & Privacy -> Show all sessions
In the other sessions section check you have the action to signout all other sessions in the overflow menu
Press view All button (you need more than 5 other sessions)
Check you have the signout all sessions action in the overflow menu
Go into select mode
Check there is the signout button outside the overflow menu when there is at least one selected session
Select several sessions
Press the signout button
Confirm the action
Check the selected session disappear from the list
Check the select mode is left

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

Florian14

Tested ok, code seems good.

Just a small thing, there is a lot of duplicated code between fragments and ViewModels, would it be possible to have a new ViewModel used by both fragments?

...x-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/DeleteDeviceTask.kt

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

…ions section

…ions

…dation screens

mnaturel · 2022-11-07T10:43:02Z

Tested ok, code seems good.

Just a small thing, there is a lot of duplicated code between fragments and ViewModels, would it be possible to have a new ViewModel used by both fragments?

Thanks for the review, yes I agree the duplicated code is annoying. I could not find a best way to solve this but I didn't think of having a dedicated ViewModel for the signout part. In my mind, I was limiting one ViewModel per Fragment. Let me try your suggestion.

mnaturel · 2022-11-07T15:58:16Z

Tested ok, code seems good.
Just a small thing, there is a lot of duplicated code between fragments and ViewModels, would it be possible to have a new ViewModel used by both fragments?

Thanks for the review, yes I agree the duplicated code is annoying. I could not find a best way to solve this but I didn't think of having a dedicated ViewModel for the signout part. In my mind, I was limiting one ViewModel per Fragment. Let me try your suggestion.

Eventually, I have reworked a little the use case into a single one for signout of sessions. See 6d26208. I haven't created a new ViewModel since we update the ViewState during the process and we have a single viewState for the whole Fragment. Duplication is only about ViewEvents and ViewState which I think is reasonable.

sonarqubecloud · 2022-11-07T16:39:33Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

60.5% Coverage
1.5% Duplication

Florian14 · 2022-11-08T14:41:07Z

...r/src/main/java/im/vector/app/features/settings/devices/v2/signout/SignoutSessionsUseCase.kt

        }
+
+        deleteDevices(deviceIds, authInterceptor)
+        Timber.d("end execute")


Did you want to keep these logs?

Yes indeed I wanted to keep these logs. Maybe I could have put the verbose tag instead.

Florian14

Thanks for the update!

mnaturel marked this pull request as ready for review October 26, 2022 13:18

mnaturel requested review from a team and ericdecanini and removed request for a team October 26, 2022 13:18

mnaturel force-pushed the feature/mna/session_manager_multi_signout branch 2 times, most recently from 4c3ba34 to 0e66d13 Compare November 2, 2022 08:38

mnaturel requested review from a team and Florian14 and removed request for ericdecanini and a team November 2, 2022 08:38

mnaturel force-pushed the feature/mna/session_manager_multi_signout branch from 0e66d13 to ef8c0a5 Compare November 4, 2022 08:29

Florian14 approved these changes Nov 4, 2022

View reviewed changes

Maxime NATUREL added 18 commits November 7, 2022 11:09

Adding changelog entry

97cfc7d

Adding new menu entry for multi signout

f45cc71

Adding overflow menu capability in sessions list header view

1ed92e5

Handling press on multi signout action in other sessions list screen

ae4a728

Handling press on multi signout action from header menu in other sess…

810c93c

…ions section

Updating the action title to include sessions number

7e836c0

Adding new "delete_devices" request API

bb262f0

Calling signout multi sessions use case in other sessions screen

1bda543

Calling signout multi sessions use case in main screen for other sess…

0f8e591

…ions

Adding confirmation dialog before signout process

727c746

Adding unit tests for signout sessions use case

a968ac0

Adding unit tests for other sessions list view model

5bcf2ac

Adding unit tests about reAuth actions for devices view model

880ee40

Adding unit tests about multi signout action for devices view model

a3df90a

Fixing a name of a mocked component

e0d511a

Fixing code quality issues

4b0b335

Removing some TODOs

76e2b6b

Fix post rebase unit tests

db42d1c

Maxime NATUREL added 5 commits November 7, 2022 11:09

Fix forbidden usage of AlertDialog

ef5aaf7

Exclude the current session from other sessions and security recommen…

d2d9da3

…dation screens

Removing unused string

3c7ba85

Use SHOW_AS_ACTION_IF_ROOM tag

5515cd3

Adding min size annotation to task params

1d2b8e7

mnaturel force-pushed the feature/mna/session_manager_multi_signout branch from ef8c0a5 to 1b940e8 Compare November 7, 2022 10:43

Removing error formatting from ViewModel

45050e8

mnaturel force-pushed the feature/mna/session_manager_multi_signout branch from 1b940e8 to 45050e8 Compare November 7, 2022 10:45

Moving UI auth interceptor into use case

6d26208

mnaturel merged commit fc24bea into develop Nov 8, 2022

mnaturel deleted the feature/mna/session_manager_multi_signout branch November 8, 2022 13:21

Florian14 reviewed Nov 8, 2022

View reviewed changes

Florian14 approved these changes Nov 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Session manager] Multi-session signout (PSG-857) #7456

[Session manager] Multi-session signout (PSG-857) #7456

mnaturel commented Oct 26, 2022 •

edited

Loading

Florian14 left a comment

mnaturel commented Nov 7, 2022

mnaturel commented Nov 7, 2022 •

edited

Loading

sonarqubecloud bot commented Nov 7, 2022

Florian14 Nov 8, 2022

mnaturel Nov 8, 2022

Florian14 left a comment

[Session manager] Multi-session signout (PSG-857) #7456

[Session manager] Multi-session signout (PSG-857) #7456

Conversation

mnaturel commented Oct 26, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Florian14 left a comment

Choose a reason for hiding this comment

mnaturel commented Nov 7, 2022

mnaturel commented Nov 7, 2022 • edited Loading

sonarqubecloud bot commented Nov 7, 2022

Florian14 Nov 8, 2022

Choose a reason for hiding this comment

mnaturel Nov 8, 2022

Choose a reason for hiding this comment

Florian14 left a comment

Choose a reason for hiding this comment

mnaturel commented Oct 26, 2022 •

edited

Loading

mnaturel commented Nov 7, 2022 •

edited

Loading