Add support for login link #4752
Conversation
bmarty
added
PR-Feature
github-actions
bot
removed
the
Record-Screenshots
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4752 +/- ##
===========================================
+ Coverage 80.31% 80.32% +0.01%
===========================================
Files 2127 2135 +8
Lines 56330 56441 +111
Branches 7057 7069 +12
===========================================
+ Hits 45241 45336 +95
- Misses 8672 8688 +16
Partials 2417 2417 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
While testing it, I got this error (and a dialog for it), then the log in progressed to the 'confirm your identity' screen soon after: |
.idea/codeStyles/Project.xml
Outdated
There was a problem hiding this comment.
Was this accidentally added? I think you mentioned this was generated by the IDE before.
There was a problem hiding this comment.
Yes :/
Does your IDE removes the added section? I do not know how to get ride of this change that I always have to not commit.
I'll revert the change for this PR.
| switchToNotLoggedInFlow(params) | ||
| } else { | ||
| // Just ignore the login link if we already have a session | ||
| Timber.w("Login link ignored, we already have a session") |
There was a problem hiding this comment.
I think we should try to give some feedback to the user in this case, although I guess there's no easy way to display a dialog/toast from here 🫤 .
There was a problem hiding this comment.
Maybe. Showing a dialog is not in the flow. If one day we have multi account, we could be able to handle the link in all cases.
| import kotlinx.parcelize.Parcelize | ||
|
|
||
| @Parcelize | ||
| data class LoginParams( |
There was a problem hiding this comment.
Maybe add some docs for what these are used for?
| get() = loginFlowAction.value | ||
|
|
||
| @Composable | ||
| fun Start() { |
There was a problem hiding this comment.
Maybe this could behave like collectAsState, directly returning the loginFlowAction current value so you don't have to expose it separately?
| private val defaultLoginUserStory: DefaultLoginUserStory, | ||
| private val webClientUrlForAuthenticationRetriever: WebClientUrlForAuthenticationRetriever, | ||
| ) { | ||
| private lateinit var loginFlowAction: MutableState<AsyncData<LoginFlow>> |
There was a problem hiding this comment.
I think you can initialise the state here instead of in the Start composable and it'll work the same, without the lateinit added complexity.
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
|
|
||
| class LoginHelper @Inject constructor( |
There was a problem hiding this comment.
Some docs for this class would be quite helpful.
| import io.element.android.libraries.ui.strings.CommonStrings | ||
|
|
||
| @Composable | ||
| fun LoginFlowView( |
There was a problem hiding this comment.
I know it's named like this because it's based on the LoginFlow value, but it kind of conflicts with the FlowNode components we have, so it took me a while to understand this is not managing a flow of screen/components. Maybe we should try thinking about a better name...
There was a problem hiding this comment.
Yes, I do not like this name either. Do you have a better idea? I can't think of any...
There was a problem hiding this comment.
Maybe rename LoginFlow to AccountSetup so you have:
sealed interface AccountSetup {
data object PasswordLogin : AccountSetup
data class OidcLogin(val oidcDetails: OidcDetails) : AccountSetup
data class AccountCreation(val url: String) : AccountSetup
}And this can now be AccountSetupView. I can't come up with a better name, it's tricky one 🫤 .
There was a problem hiding this comment.
I think I will go for LoginMode and extract this interface.
There was a problem hiding this comment.
Ok, although AccountCreation is technically not a login mode 🤓 .
Move LoginFlow to package io.element.android.features.login.impl.login Rename some implementation of LoginMode Rename LoginFlowView to LoginModeView
Using launchMode singleTask to avoid multiple instances of the Activity when the app is already open. This is important for incoming share and for opening the application from a mobile.element.io link. Closes #4074
Thanks for reporting. It was due to multiple instance of the application to run in parallel. It was possible when opening a mobile.element.io link from an application that do not create a separate task like Google Keep Note. Google Chrome create a separate task. I fixed that in 59a508f Note that this also has the effect to close #4074 and incoming share still work as expected. |
|
|
Content
Add support for login link
Motivation and context
Let the application be opened when the user opens link like
https://mobile.element.io/element?account_provider=example.org&login_hint=mxid:@alice:example.org
Closes #4705
Screenshots / GIFs
SignInDeepLink.mp4
Tests
Tested devices
Checklist