OAuth - request the token via REST API #19
Conversation
This module should be the place to keep the basic types shared between the modules.
| buildConfigField( | ||
| "String", | ||
| "OAUTH_CLIENT_SECRET", | ||
| "\"${properties["oauth.clientSecret"]?.toString() ?: ""}\"", | ||
| ) |
There was a problem hiding this comment.
Remember to add this to your local.properties
| HttpClient(OkHttp) { | ||
| install(ContentNegotiation) { | ||
| json( | ||
| Json { | ||
| ignoreUnknownKeys = true | ||
| } | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We can configure more stuff here, so we don't have to repeat it in each request, but let's do that as we need it. This is also the place to configure stuff like network loggers if we need to in the future.
| factoryOf(::RealAuthRepository) { bind<AuthRepository>() } | ||
| factoryOf(::WordPressClient) |
There was a problem hiding this comment.
I don't see a reason that those should be singletons. WDYT?
There was a problem hiding this comment.
Agree. We are preserving data in those instances, so no need for a singleton. 👌
| clientSecret: String, | ||
| redirectUri: String, | ||
| clientId: String | ||
| ): Result<String> = withContext(dispatcherProvider.io) { |
There was a problem hiding this comment.
Standard Result for now - but I want to introduce our own type for better error types, rather than exceptions.
There was a problem hiding this comment.
Works as described. I can see the access_token when reviewing the network requests.
I left a comment about DI and modules that could be worth discussing before approving it.
There are no tests, the error handling is not done, but I don't want this PR to grow, so I would rather do this stuff separately, once we have the full OAuth flow completed.
Sounds good. I would also include in those tests the Koin.Modules ones. :)
| } | ||
|
|
||
| @Composable | ||
| internal fun LoginScreen( |
There was a problem hiding this comment.
Preview in this class no longer works, as Koin won't be initialized. We could use KoinAppApplicationPreview:
@Preview
@Composable
private fun LoginScreenPreview() {
KoinApplicationPreview(application = { modules(loginUiModule) }) {
LoginScreen(
onLoggedIn = { },
)
}
}However, if you try that code, it will fail because it doesn't know how to resolve dispatcherModule.
dispatcherModule is being defined in the app module, so DispatcherProvider will be available everywhere in the app when needed. However, thinking in isolated modules, appmodule doesn't require :foundations. It's only required by loginUI.
I wonder if dispatcherModule should be defined in foundations and refactor a bit the dependencies.
Something like this:
adam/GRA-199...hamorillo/GRA-199-refactor-dispatcher
There was a problem hiding this comment.
This should be fixed now. I didn't update the test to use the proper composable that doesn't rely on ViewModel, but rather on a ui state class.
| factoryOf(::RealAuthRepository) { bind<AuthRepository>() } | ||
| factoryOf(::WordPressClient) |
There was a problem hiding this comment.
Agree. We are preserving data in those instances, so no need for a singleton. 👌
Description
This PR adds the logic for requesting the user's Token via WP REST API after the app receives the
codefrom the OAuth flow.Some new stuff that's introduced:
:foundationsmodule. This is the place for the lowest-level types that we will share between the modules. One example could be a custom Result type that should be common for all the modules.:userComponentmodule. This is the place for user-related domain/data stuff. At this point, we only do the auth, but I believe this is also the place where we can put stuff like getting the user profile.Let me know what you think about it 🙏
There are no tests, the error handling is not done, but I don't want this PR to grow, so I would rather do this stuff separately, once we have the full OAuth flow completed.
Testing Steps