Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Setting up account" screen #1149

Merged
merged 15 commits into from Aug 28, 2023
Merged

"Setting up account" screen #1149

merged 15 commits into from Aug 28, 2023

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Aug 25, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Extract SunsetPage from WaitListView to resuse the same composable for MigrationScreenView.
  • Introduce a LoggedInAppScopeFlowNode to be able to inject object with SessionScope in LoggedInFlowNode
  • Add a migrating screen which is displayed the first time the user log in on their account.
    The hashed UserId is stored in the SharedPref to avoid showing this screen twice. Note that this is only local and the screen can be shown even if the sliding sync proxy has done an init sync of the account already. Clearing the cache from the developer setting also delete this information (for testing purpose). Note that when getting the app upgrade, this screen will be displayed for a short time (i.e. this screen is not only displayed just after a user log in). This is fine I think, since the app is not in prod yet.

Motivation and context

Closes #1145

Screenshots / GIFs

See the screenshot of the new screen in the file diff.

The screen is shown the first time the user log in, but not the second time:

MigrationScreen

Tests

  • Sign in to an account and see the new screen displayed until the room list is loaded. Once loaded, the FTUE flow continues to the other existing screens.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@bmarty bmarty requested a review from a team as a code owner August 25, 2023 13:20
@bmarty bmarty requested review from julioromano and removed request for a team August 25, 2023 13:20
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/UUFiUv

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Patch coverage: 71.76% and project coverage change: -0.04% ⚠️

Comparison is base (5525573) 56.90% compared to head (e47d137) 56.87%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1149      +/-   ##
===========================================
- Coverage    56.90%   56.87%   -0.04%     
===========================================
  Files         1033     1039       +6     
  Lines        26736    26812      +76     
  Branches      5523     5544      +21     
===========================================
+ Hits         15215    15248      +33     
- Misses        9136     9168      +32     
- Partials      2385     2396      +11     
Files Changed Coverage Δ
...element/android/features/ftue/impl/FtueFlowNode.kt 0.00% <ø> (ø)
.../impl/migration/SharedPrefsMigrationScreenStore.kt 0.00% <0.00%> (ø)
...lement/android/libraries/androidutils/hash/Hash.kt 0.00% <0.00%> (ø)
...eatures/ftue/impl/migration/MigrationScreenView.kt 30.76% <30.76%> (ø)
...es/ftue/impl/migration/MigrationScreenPresenter.kt 80.00% <80.00%> (ø)
.../libraries/designsystem/atomic/pages/SunsetPage.kt 85.50% <85.50%> (ø)
.../login/impl/screens/waitlistscreen/WaitListView.kt 64.28% <88.88%> (-14.46%) ⬇️
...id/libraries/designsystem/text/AnnotatedStrings.kt 57.50% <88.88%> (+9.11%) ⬆️
...droid/features/ftue/impl/state/DefaultFtueState.kt 87.17% <90.00%> (-2.83%) ⬇️
...atures/ftue/impl/migration/MigrationScreenState.kt 100.00% <100.00%> (ø)

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

@ElementBot
Copy link
Collaborator

ElementBot commented Aug 25, 2023

Warnings
⚠️

libraries/ui-strings/src/main/res/values-cs/translations.xml#L160 - 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#L165 - 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#L160 - 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#L165 - For locale "sk" (Slovak) the following quantity should also be defined: many (e.g. "10.0 dňa")

Generated by 🚫 dangerJS against e47d137

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why those 2 files have been renamed...

@julioromano julioromano requested review from jmartinesp and removed request for julioromano August 28, 2023 08:34
Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few minor comments.

import kotlinx.parcelize.Parcelize

@ContributesNode(AppScope::class)
class LoggedInAppScopeFlowNode @AssistedInject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some documentation added as a commit message, but I'd also add it here as docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

private fun SessionId.toKey(): String {
// Hash the sessionId to get ride of exotic char and take only the first 16 chars,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (typo): it should be rid here, not ride.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks, done.

/**
* Convert a string to an [AnnotatedString] with colored end period if present.
*/
fun withColoredPeriod(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is used only in SunsetPage I'd move it there instead, since it seems quite specific. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, maybe, but I prefer to keep it here, and extend it if necessary for future use. For instance, pass a list of char that should be colored (?, !, etc.) and pass the color as parameter.

@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@bmarty bmarty merged commit 16b4db7 into develop Aug 28, 2023
16 of 17 checks passed
@bmarty bmarty deleted the feature/bma/settingUpAccount branch August 28, 2023 12:26
@Aquathing
Copy link

I think it would make sense to tell the user whether they can close the app and come back later or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We're missing the "setting up account" login screen
4 participants