-
Notifications
You must be signed in to change notification settings - Fork 768
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
Iterate on user consent dialog for identity server #4587
Conversation
import org.matrix.android.sdk.api.session.Session | ||
import org.matrix.android.sdk.api.session.terms.TermsService | ||
|
||
suspend fun Session.fetchIdentityServerWithTerms(userLanguage: String): IdentityServerWithTerms? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 for lifting out of the view model
import im.vector.app.core.platform.VectorViewEvents | ||
import im.vector.app.features.discovery.IdentityServerWithTerms | ||
|
||
sealed class ContactsBookViewEvents : VectorViewEvents { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worlds smallest nit: could be a sealed interface, saves having to write ()
in the extends 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at the beginning of the project sealed interface was not a thing, but now, yes. I prefer to perform a global replace on a dedicated PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(created #4588 about it)
@@ -40,16 +45,36 @@ fun Context.displayInWebView(url: String) { | |||
.show() | |||
} | |||
|
|||
fun Context.showIdentityServerConsentDialog(configuredIdentityServer: String?, policyLinkCallback: () -> Unit, consentCallBack: (() -> Unit)) { | |||
fun Context.showIdentityServerConsentDialog(identityServerWithTerms: IdentityServerWithTerms?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general question about identity servers, are privacy policies enforced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean can we have an IS without any privacy policy? I do not think so, but the code should be robust to that.
By the way I will create some issues (defect, but no P1) I have seen in the area
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Just pushed a small commit |
Implements #4577
New dialogs: #4577 (comment)
We need to retrieved the T&C, that's why many changes are necessary. Sadly there is some code duplication between the ViewModels, we may find a way to reduce that later.