Improve session recovery screens #2657

jmartinesp · 2024-04-04T11:56:40Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Update the UI and texts for the 'enter recovery key' screen.
Add new 'create new recovery key' info screen that can be displayed when there are no other active sessions for the current account.

Motivation and context

Closes #2579, also takes care of https://github.com/element-hq/element-internal/issues/583.

Screenshots / GIFs

In the PR.

Tests

Log into an account with no other active sessions, or modify VerifySelfSessionPresenter so isLastDevice is true.
Check the 'create new recovery key' button appears after logging in.
Verify the new 'create new recovery key' screen is displayed properly.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-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/element-hq/element-x-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

ElementBot · 2024-04-04T11:58:00Z

	Warnings
⚠️	features/roomdetails/impl/src/main/res/values-cs/translations.xml#L67 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	features/roomdetails/impl/src/main/res/values-sk/translations.xml#L67 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L4 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L22 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L152 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L174 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L4 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L22 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L151 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L173 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")

Generated by 🚫 dangerJS against 3c90bb4

github-actions · 2024-04-04T12:07:20Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/PRMVRE

codecov · 2024-04-04T12:17:12Z

Codecov Report

Attention: Patch coverage is 76.76768% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 73.58%. Comparing base (ecd2974) to head (3c90bb4).
Report is 1 commits behind head on develop.

Files	Patch %	Lines
...aries/designsystem/modifiers/SquareSizeModifier.kt	62.50%	8 Missing and 4 partials ⚠️
...ebackup/impl/createkey/CreateNewRecoveryKeyView.kt	76.92%	1 Missing and 8 partials ⚠️
...eatures/securebackup/api/SecureBackupEntryPoint.kt	0.00%	1 Missing ⚠️
...kup/impl/enter/SecureBackupEnterRecoveryKeyView.kt	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2657      +/-   ##
===========================================
- Coverage    73.58%   73.58%   -0.01%     
===========================================
  Files         1458     1460       +2     
  Lines        35256    35338      +82     
  Branches      6775     6793      +18     
===========================================
+ Hits         25942    26002      +60     
- Misses        5786     5796      +10     
- Partials      3528     3540      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…-session-recovery-to-ftue

bmarty · 2024-04-09T12:41:00Z

...n/kotlin/io/element/android/features/securebackup/impl/createkey/CreateNewRecoveryKeyView.kt

+
+@PreviewsDayNight
+@Composable
+internal fun ItemNumberPreview() {


Can you rename this function to CreateNewRecoveryKeyViewPreview please?

bmarty · 2024-04-09T12:42:04Z

...n/kotlin/io/element/android/features/securebackup/impl/createkey/CreateNewRecoveryKeyView.kt

+@Composable
+private fun Content() {
+    Column(modifier = Modifier.padding(horizontal = 16.dp), verticalArrangement = Arrangement.spacedBy(24.dp)) {
+        Item(index = 1, text = AnnotatedString(stringResource(R.string.screen_create_new_recovery_key_list_item_1)))


Element should not be hard-coded in the string.

Do you mean the index? What should we use instead?

Oh, I just realised you meant the name of the app. Sorry.

bmarty · 2024-04-09T12:43:50Z

...impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionNode.kt

-            it.onDone()
-        }
-    }
+    private val callback = plugins<VerifySessionEntryPoint.Callback>().first()


I know that we will have only one plugin for the callback, but the current practice is to invoke the method on all the Callback in the plugin list.

bmarty · 2024-04-09T12:46:53Z

...impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionView.kt

+                    onPositiveButtonClicked = onEnterRecoveryKey,
+                    negativeButtonTitle = stringResource(R.string.screen_identity_confirmation_create_new_recovery_key),
+                    onNegativeButtonClicked = onCreateNewRecoveryKey,
+                )


I do not think that it is a good idea to display the Create a new recovery key action only if this is the last device.
Users may have ghost sessions (i.e. not logged out sessions but not accessible anymore, for instance after uninstalling the application without signing out).
Giving such feedback in the PR is not great, and maybe this has been discussed internally. Sorry if it's the case.

If we always included it we'd have 3 buttons for 'use another device', 'use recovery key' or 'reset recovery key', which seems like a bit too much. The flows were designed this way, but maybe we can revisit them in the future and add it either on the root view or in the enter recovery key one.

bmarty · 2024-04-09T12:49:18Z

.../src/test/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionViewTest.kt

@@ -119,6 +123,7 @@ class VerifySelfSessionViewTest {
                    eventSink = eventsRecorder
                ),
                onEnterRecoveryKey = EnsureNeverCalled(),
+                onCreateNewRecoveryKey = EnsureNeverCalled(),


We should introduce an extension <R : TestRule> AndroidComposeTestRule<R, ComponentActivity>.setVerifySelfSessionView to avoid touching all the tests when a new callback is added.

…-session-recovery-to-ftue

sonarcloud · 2024-04-09T16:47:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

jmartinesp and others added 4 commits April 4, 2024 12:06

Move session recovery to the login flow

661dff6

Fix "Reset all" rendering

2709916

Add tests and fix lint issues

a47a389

Update screenshots

333f7e5

jmartinesp requested a review from a team as a code owner April 4, 2024 11:56

jmartinesp requested review from bmarty and removed request for a team April 4, 2024 11:56

jmartinesp added 2 commits April 4, 2024 15:45

Try to minimise coverage drop

ee0dcbc

Merge remote-tracking branch 'origin/develop' into misc/jme/2579-move…

c7ec591

…-session-recovery-to-ftue

jmartinesp force-pushed the misc/jme/2579-move-session-recovery-to-ftue branch from 3ecdda1 to c7ec591 Compare April 9, 2024 09:42

Update screenshots

7c0b243

jmartinesp changed the title ~~Move session recovery to the login flow~~ Improve session recovery screens Apr 9, 2024

bmarty approved these changes Apr 9, 2024

View reviewed changes

Fix review comments

8ddda61

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 9, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 9, 2024

jmartinesp added 2 commits April 9, 2024 17:50

Merge remote-tracking branch 'origin/develop' into misc/jme/2579-move…

25464f6

…-session-recovery-to-ftue

Fix tests after merge

3048a30

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 9, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 9, 2024

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Apr 9, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Apr 9, 2024

ElementBot and others added 2 commits April 9, 2024 16:31

Update screenshots

34336f6

Fix maestro flow

3c90bb4

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 9, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 9, 2024

jmartinesp enabled auto-merge (squash) April 9, 2024 17:01

jmartinesp merged commit cf072fa into develop Apr 9, 2024
19 checks passed

jmartinesp deleted the misc/jme/2579-move-session-recovery-to-ftue branch April 9, 2024 17:01

jmartinesp mentioned this pull request Apr 23, 2024

[Task] Reset Recovery Key Screen #2740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve session recovery screens #2657

Improve session recovery screens #2657

jmartinesp commented Apr 4, 2024 •

edited

Loading

ElementBot commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

bmarty Apr 9, 2024

bmarty Apr 9, 2024

jmartinesp Apr 9, 2024

jmartinesp Apr 9, 2024

bmarty Apr 9, 2024

bmarty Apr 9, 2024

jmartinesp Apr 9, 2024 •

edited

Loading

bmarty Apr 9, 2024

bmarty Apr 9, 2024

sonarcloud bot commented Apr 9, 2024

Improve session recovery screens #2657

Improve session recovery screens #2657

Conversation

jmartinesp commented Apr 4, 2024 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

ElementBot commented Apr 4, 2024 • edited Loading

github-actions bot commented Apr 4, 2024 • edited Loading

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

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

jmartinesp Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 9, 2024

Quality Gate passed

jmartinesp commented Apr 4, 2024 •

edited

Loading

ElementBot commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

jmartinesp Apr 9, 2024 •

edited

Loading