Skip to content

Commit 8eb4e48

Browse files
sdsantossdsantos
committed
Address code review comments
1 parent 1358e1c commit 8eb4e48

17 files changed

Lines changed: 207 additions & 138 deletions

File tree

‎app/src/test/kotlin/com/android/contacts/sim/SimImportViewModelTest.kt‎

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package com.android.contacts.sim
22

33
import androidx.lifecycle.SavedStateHandle
4+
import app.cash.turbine.test
45
import com.android.contacts.domain.accounts.model.AccountDisplayModel
56
import com.android.contacts.domain.accounts.model.AccountModel
67
import com.android.contacts.domain.accounts.usecase.GetDefaultAccount
78
import com.android.contacts.domain.accounts.usecase.LoadAccounts
89
import com.android.contacts.domain.sim.model.SimContactsResult
10+
import com.android.contacts.domain.sim.usecase.LoadSimCards
911
import com.android.contacts.domain.sim.usecase.LoadSimContacts
1012
import com.android.contacts.domain.sim.usecase.StartSimImport
1113
import com.android.contacts.model.SimContact
@@ -17,6 +19,7 @@ import com.android.contacts.ui.simimport.screen.SimImportViewModel
1719
import com.android.contacts.ui.simimport.screen.mapper.SimContactUiModelMapperImpl
1820
import com.android.contacts.ui.simimport.screen.model.AccountUiModel
1921
import com.android.contacts.ui.simimport.screen.model.SimImportAction as Action
22+
import com.android.contacts.ui.simimport.screen.model.SimImportEffect as Effect
2023
import kotlinx.collections.immutable.ImmutableList
2124
import kotlinx.collections.immutable.persistentListOf
2225
import kotlinx.collections.immutable.persistentMapOf
@@ -43,6 +46,21 @@ class SimImportViewModelTest {
4346
// Not mocking this mapper since it holds no logic
4447
private val simContactUiModelMapper = SimContactUiModelMapperImpl()
4548

49+
@Test
50+
fun subscriptionId_whenSimCardDoesNotExist_closes() = runTest {
51+
val subscriptionId = 2
52+
val subject = createViewModel(
53+
savedStateHandle = SavedStateHandle(
54+
mapOf(UIIntents.EXTRA_SUBSCRIPTION_ID to subscriptionId),
55+
),
56+
loadSimCards = { flowOf(listOf()) },
57+
)
58+
subject.effects.test {
59+
advanceUntilIdle()
60+
assertEquals(Effect.Close(isSuccessful = false), awaitItem())
61+
}
62+
}
63+
4664
@Test
4765
fun isLoading_whenBothLoadAccountsAndContactsFinish_isFalse() =
4866
runTest(context = mainDispatcherRule.testDispatcher) {
@@ -89,7 +107,7 @@ class SimImportViewModelTest {
89107
val account1 = AccountDisplayModelFactory.build()
90108
val account2 = AccountDisplayModelFactory.build()
91109
val subject = createViewModel(
92-
getDefaultAccount = { account2.toUiModel() },
110+
getDefaultAccount = { account2.account },
93111
loadAccounts = { flowOf(persistentListOf(account1, account2)) },
94112
)
95113
advanceUntilIdle()
@@ -108,7 +126,7 @@ class SimImportViewModelTest {
108126
val account2 = AccountDisplayModelFactory.build()
109127
val loadAccountsFlow = MutableStateFlow(persistentListOf(account1, account2))
110128
val subject = createViewModel(
111-
getDefaultAccount = { account1.toUiModel() },
129+
getDefaultAccount = { account1.account },
112130
loadAccounts = { loadAccountsFlow },
113131
)
114132

@@ -149,7 +167,7 @@ class SimImportViewModelTest {
149167
val savedStateHandle = SavedStateHandle()
150168
val subject1 = createViewModel(
151169
savedStateHandle = savedStateHandle,
152-
getDefaultAccount = { account1.toUiModel() },
170+
getDefaultAccount = { account1.account },
153171
loadAccounts = { flowOf(persistentListOf(account1, account2)) },
154172
)
155173
advanceUntilIdle()
@@ -159,7 +177,7 @@ class SimImportViewModelTest {
159177

160178
val subject2 = createViewModel(
161179
savedStateHandle = savedStateHandle,
162-
getDefaultAccount = { account1.toUiModel() },
180+
getDefaultAccount = { account1.account },
163181
loadAccounts = { flowOf(persistentListOf(account1, account2)) },
164182
)
165183
advanceUntilIdle()
@@ -349,7 +367,7 @@ class SimImportViewModelTest {
349367
SimContactsResult(
350368
contacts = persistentListOf(contact),
351369
existingContactsInAccounts = persistentMapOf(
352-
account.toUiModel() to setOf(contact),
370+
account.account to setOf(contact),
353371
),
354372
),
355373
)
@@ -380,37 +398,35 @@ class SimImportViewModelTest {
380398
)
381399
advanceUntilIdle()
382400

383-
subject.onAction(Action.ImportClicked)
384-
advanceUntilIdle()
385-
401+
subject.effects.test {
402+
subject.onAction(Action.ImportClicked)
403+
advanceUntilIdle()
404+
assertEquals(Effect.Close(isSuccessful = true), awaitItem())
405+
}
386406
assertNotNull(startSimImportCall)
387407
startSimImportCall!!.let { (callSubscriptionId, callContacts, callAccount) ->
388408
assertEquals(subscriptionId, callSubscriptionId)
389409
assertEquals(persistentListOf(contact), callContacts)
390-
assertEquals(account.toUiModel(), callAccount)
410+
assertEquals(account.account, callAccount)
391411
}
392412
}
393413

394414
private fun createViewModel(
395415
savedStateHandle: SavedStateHandle = SavedStateHandle(),
416+
loadSimCards: LoadSimCards = { emptyFlow() },
396417
getDefaultAccount: GetDefaultAccount = { null },
397418
loadSimContacts: LoadSimContacts = { emptyFlow() },
398419
loadAccounts: LoadAccounts = { emptyFlow() },
399420
startSimImport: StartSimImport = { _, _, _ -> },
400421
) = SimImportViewModel(
401422
savedStateHandle,
402423
getDefaultAccount = getDefaultAccount,
424+
loadSimCards = loadSimCards,
403425
loadSimContacts = loadSimContacts,
404426
loadAccounts = loadAccounts,
405427
startSimImport = startSimImport,
406428
simContactUiModelMapper = simContactUiModelMapper,
407429
)
408430

409-
private fun AccountDisplayModel.toUiModel() = AccountModel(
410-
name = name,
411-
type = type,
412-
dataSet = dataSet,
413-
)
414-
415431
private fun SimContact.toUiModel() = simContactUiModelMapper.map(this)
416432
}

‎app/src/test/kotlin/com/android/contacts/tests/AccountDisplayModelFactory.kt‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,20 @@ package com.android.contacts.tests
22

33
import android.graphics.drawable.Drawable
44
import com.android.contacts.domain.accounts.model.AccountDisplayModel
5+
import com.android.contacts.domain.accounts.model.AccountModel
56
import kotlin.random.Random
67

78
internal object AccountDisplayModelFactory {
89
fun build(
9-
name: String = "Account ${Random.nextInt().toString().take(4)}",
10-
type: String? = null,
11-
dataSet: String? = null,
10+
account: AccountModel = AccountModelFactory.build(),
11+
name: String = account.name ?: "Account ${Random.nextInt().toString().take(4)}",
12+
type: String? = account.type,
1213
icon: Drawable? = null,
1314
isDeviceAccount: Boolean = true,
1415
) = AccountDisplayModel(
16+
account = account,
1517
name = name,
1618
type = type,
17-
dataSet = dataSet,
1819
icon = icon,
1920
isDeviceAccount = isDeviceAccount,
2021
)

‎app/src/test/kotlin/com/android/contacts/tests/AccountModelFactory.kt‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package com.android.contacts.tests
2+
3+
import com.android.contacts.domain.accounts.model.AccountModel
4+
import kotlin.random.Random
5+
6+
internal object AccountModelFactory {
7+
fun build(
8+
name: String = "Account ${Random.nextInt().toString().take(4)}",
9+
type: String? = null,
10+
dataSet: String? = null,
11+
) = AccountModel(
12+
name = name,
13+
type = type,
14+
dataSet = dataSet,
15+
)
16+
}

‎app/src/test/kotlin/com/android/contacts/tests/AccountUiModelFactory.kt‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,22 @@
11
package com.android.contacts.tests
22

33
import android.graphics.drawable.Drawable
4+
import com.android.contacts.domain.accounts.model.AccountModel
45
import com.android.contacts.ui.simimport.screen.model.AccountUiModel
56
import kotlin.random.Random
67

78
internal object AccountUiModelFactory {
89
fun build(
9-
name: String = "Account ${Random.nextInt().toString().take(4)}",
10-
type: String? = null,
11-
dataSet: String? = null,
10+
account: AccountModel = AccountModelFactory.build(),
11+
name: String = account.name ?: "Account ${Random.nextInt().toString().take(4)}",
12+
type: String? = account.type,
1213
icon: Drawable? = null,
1314
) = AccountUiModel(
1415
name = name,
1516
type = type,
16-
dataSet = dataSet,
1717
icon = icon,
18+
accountName = account.name,
19+
accountType = account.type,
20+
accountDataSet = account.dataSet,
1821
)
1922
}

‎res/anim/slide_and_fade_out.xml‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

‎res/values/styles.xml‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -178,21 +178,6 @@
178178
<item name="titleTextAppearance">@style/ContactsActionBarTitleTextAppCompat</item>
179179
</style>
180180

181-
<style name="LightToolbarNavigationButtonStyle" parent="@style/Widget.AppCompat.Toolbar.Button.Navigation">
182-
<item name="android:tint">?android:textColorSecondary</item>
183-
</style>
184-
185-
<style name="LightToolbarThemeOverlay" parent="@style/ThemeOverlay.AppCompat.ActionBar">
186-
<item name="toolbarNavigationButtonStyle">@style/LightToolbarNavigationButtonStyle</item>
187-
</style>
188-
189-
<style name="LightToolbarStyle" parent="@style/Widget.AppCompat.Toolbar">
190-
<item name="android:background">@color/contextual_selection_bar_color</item>
191-
<item name="background">@color/contextual_selection_bar_color</item>
192-
<item name="android:titleTextAppearance">@style/ContactsActionBarTitleTextBlack</item>
193-
<item name="titleTextAppearance">@style/ContactsActionBarTitleTextBlack</item>
194-
</style>
195-
196181
<!-- Text in the action bar at the top of the screen -->
197182
<style name="ContactsActionBarTitleText"
198183
parent="@android:style/TextAppearance.Material.Widget.ActionBar.Title">
@@ -449,27 +434,6 @@ background and text color. See also android:style/Widget.Holo.TextView.ListSepar
449434
<item name="android:backgroundDimEnabled">false</item>
450435
</style>
451436

452-
<style name="FullScreenDialogAnimationStyle">
453-
<item name="android:windowEnterAnimation">@anim/slide_and_fade_in</item>
454-
<item name="android:windowExitAnimation">@anim/slide_and_fade_out</item>
455-
</style>
456-
457-
<style name="PeopleThemeAppCompat.FullScreenDialog">
458-
<item name="android:windowNoTitle">true</item>
459-
<item name="android:windowActionBar">false</item>
460-
<item name="windowNoTitle">true</item>
461-
<item name="windowActionBar">false</item>
462-
<item name="android:listSelector">?android:attr/listChoiceBackgroundIndicator</item>
463-
<item name="android:windowAnimationStyle">@style/FullScreenDialogAnimationStyle</item>
464-
</style>
465-
466-
<style name="PeopleThemeAppCompat.FullScreenDialog.SimImportActivity">
467-
<!-- This is necessary because the window is partially transparent during the enter
468-
and exit animations -->
469-
<item name="android:windowIsTranslucent">true</item>
470-
<item name="android:statusBarColor">@color/contextual_selection_bar_status_bar_color</item>
471-
</style>
472-
473437
<!-- Style for item in navigation drawer -->
474438
<style name="DrawerItemStyle">
475439
<item name="android:layout_width">match_parent</item>

‎src/com/android/contacts/di/core/CoreProvidesModule.kt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
package com.android.contacts.di.core
22

3+
import android.content.Context
4+
import android.telephony.SubscriptionManager
35
import com.android.contacts.util.concurrent.ContactsExecutors
46
import dagger.Module
57
import dagger.Provides
68
import dagger.Reusable
79
import dagger.hilt.InstallIn
10+
import dagger.hilt.android.qualifiers.ApplicationContext
811
import dagger.hilt.components.SingletonComponent
912
import kotlinx.coroutines.CoroutineDispatcher
1013
import kotlinx.coroutines.Dispatchers
@@ -14,6 +17,8 @@ import kotlinx.coroutines.asCoroutineDispatcher
1417
@InstallIn(SingletonComponent::class)
1518
internal class CoreProvidesModule {
1619

20+
// Coroutine Dispatchers
21+
1722
@Provides
1823
@Reusable
1924
@DefaultDispatcher
@@ -41,4 +46,14 @@ internal class CoreProvidesModule {
4146
fun provideSimReadDispatcher(): CoroutineDispatcher {
4247
return ContactsExecutors.getSimReadExecutor().asCoroutineDispatcher()
4348
}
49+
50+
// Others
51+
52+
@Provides
53+
@Reusable
54+
fun provideSubscriptionManager(
55+
@ApplicationContext context: Context,
56+
): SubscriptionManager {
57+
return context.getSystemService(SubscriptionManager::class.java)
58+
}
4459
}

‎src/com/android/contacts/di/sim/SimBindsModule.kt‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.android.contacts.di.sim
22

3+
import com.android.contacts.domain.sim.usecase.LoadSimCards
4+
import com.android.contacts.domain.sim.usecase.LoadSimCardsImpl
35
import com.android.contacts.domain.sim.usecase.LoadSimContacts
46
import com.android.contacts.domain.sim.usecase.LoadSimContactsImpl
57
import com.android.contacts.domain.sim.usecase.StartSimImport
@@ -14,6 +16,12 @@ import dagger.hilt.components.SingletonComponent
1416
@InstallIn(SingletonComponent::class)
1517
internal abstract class SimBindsModule {
1618

19+
@Binds
20+
@Reusable
21+
abstract fun bindLoadSimCards(
22+
impl: LoadSimCardsImpl,
23+
): LoadSimCards
24+
1725
@Binds
1826
@Reusable
1927
abstract fun bindLoadSimContacts(

‎src/com/android/contacts/domain/accounts/mapper/AccountDisplayModelMapper.kt‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,22 @@
11
package com.android.contacts.domain.accounts.mapper
22

33
import com.android.contacts.domain.accounts.model.AccountDisplayModel
4-
import com.android.contacts.domain.accounts.model.AccountModel
54
import com.android.contacts.model.account.AccountInfo
65
import javax.inject.Inject
76

87
internal interface AccountDisplayModelMapper {
98
fun map(accountInfo: AccountInfo): AccountDisplayModel
109
}
1110

12-
internal class AccountDisplayModelMapperImpl @Inject constructor() : AccountDisplayModelMapper {
11+
internal class AccountDisplayModelMapperImpl @Inject constructor(
12+
private val accountModelMapper: AccountModelMapper,
13+
) : AccountDisplayModelMapper {
1314
override fun map(accountInfo: AccountInfo): AccountDisplayModel {
15+
val account = accountModelMapper.map(accountInfo.account)
1416
return AccountDisplayModel(
15-
name = accountInfo.account.name,
16-
type = accountInfo.account.type,
17-
dataSet = accountInfo.account.dataSet,
17+
account = account,
18+
name = accountInfo.nameLabel?.toString(),
19+
type = accountInfo.typeLabel?.toString(),
1820
icon = accountInfo.icon,
1921
isDeviceAccount = accountInfo.isDeviceAccount,
2022
)

‎src/com/android/contacts/domain/accounts/model/AccountDisplayModel.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ import android.graphics.drawable.Drawable
66
* Immutable domain model to match {@link com.android.contacts.model.account.AccountDisplayInfo}
77
*/
88
internal data class AccountDisplayModel(
9+
val account: AccountModel,
910
val name: String?,
1011
val type: String? = null,
11-
val dataSet: String? = null,
1212
val icon: Drawable? = null,
1313
val isDeviceAccount: Boolean = true,
1414
)

0 commit comments

Comments
 (0)