-
Notifications
You must be signed in to change notification settings - Fork 991
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
MBL-1077: Coroutines in ChangePasswordViewModel + Tests #1923
Conversation
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.
Looking great! Lets do it everywhere!
class ChangePasswordViewModelFactory(private val environment: Environment) : | ||
ViewModelProvider.Factory { | ||
override fun <T : ViewModel> create(modelClass: Class<T>): T { | ||
return ChangePasswordViewModel(environment) as T |
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.
Just seeing a class go down ~100 lines while maintaining functionality really excites me for coroutines and the UI state model
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.
especially since this is a smaller use case
|
||
fun updatePassword(oldPassword: String, newPassword: String) { | ||
viewModelScope.launch { | ||
// TODO: Avoid using GraphQL generated types such as UpdateUserPasswordMutation.Data, return data model defined within the app. |
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.
Do we want to propose a alternative to this as part of this migration effort? It would probably mean adding transformers/UiModels to the effort.
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.
Agreed, seems a small effort, what do you think @leighdouglas and @ycheng-kickstarter ?
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'll take care of this one in a following PR
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1923 +/- ##
============================================
+ Coverage 74.25% 74.26% +0.01%
- Complexity 1963 1964 +1
============================================
Files 336 336
Lines 19653 19614 -39
Branches 2718 2711 -7
============================================
- Hits 14593 14567 -26
+ Misses 3490 3478 -12
+ Partials 1570 1569 -1 ☔ View full report in Codecov by Sentry. |
📲 What
Adding coroutines flow in detail document with technical clarifications available here
Test file migrated to adapt to the coroutines usage
uiState: StateFlow<UpdatePasswordUIState>
🤔 Why
👀 See
| | |
📋 QA
Story 📖
MBL-1077