Skip to content
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

Migrate account view to compose #4906

Merged
merged 6 commits into from
Jul 28, 2023

Conversation

sabercodic
Copy link
Contributor

@sabercodic sabercodic commented Jul 17, 2023

migrate account screen into compose

Fixes: DROID-63


This change is Reviewable

@linear
Copy link

linear bot commented Jul 17, 2023

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from 401f645 to d06fe70 Compare July 18, 2023 07:28
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @sabercodic)


-- commits line 8 at r2:
typo: account

Code quote:

acount

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 49 at r2 (raw file):

            AccountUiState(
                deviceName = "Test Name",
                accountNumber = "1234567890",

I suggest using 16 digits to look similar to the most regular account number length, eg 1234123412341234

Code quote:

1234567890

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 58 at r2 (raw file):

@ExperimentalMaterial3Api
@Composable
fun AccountScreen(

Can't we use some common scaffolding? E.g. it looks very similar to settings.

Code quote:

AccountScreen

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 16 at r2 (raw file):

    private val expiryFormatter = DateFormat.getDateTimeInstance(dateStyle, timeStyle)

    var expiryString = accountExpiry?.toDate()?.let { expiryFormatter.format(it) } ?: ""

I suggest handling moving this formatting logic to the existing DateExtensions

Code quote:

    private val expiryFormatter = DateFormat.getDateTimeInstance(dateStyle, timeStyle)

    var expiryString = accountExpiry?.toDate()?.let { expiryFormatter.format(it) } ?: ""

android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt line 93 at r2 (raw file):

    viewModel { SelectLocationViewModel(get()) }
    viewModel { SettingsViewModel(get(), get()) }
    viewModel { AccountViewModel(get(), get()) }

I suggest placing this in alphabetical order within the VM section.

Code quote:

AccountViewModel

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 20 at r2 (raw file):

    private var deviceRepository: DeviceRepository
) : ViewModel() {
    private val showAccountNumber = MutableStateFlow<Boolean>(false)

Should be part of the combined flow in order to properly update the state.

Code quote:

showAccountNumber

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 58 at r2 (raw file):

    fun onCopyAccountNumberClick() {}
    fun onManageAccountClick(context: Context) {
        deviceRepository.deviceState.value.token()?.let { context.openAccountPageInBrowser(it) }

We usually want to avoid interactions with the context in the VM. This call should instead be made form the view.

Code quote:

context.openAccountPageInBrowser(it)

android/app/src/main/res/values/strings.xml line 222 at r2 (raw file):

    <string name="device_management_name_description">This is the name assigned to the device. Each device logged in on a Mullvad account gets a unique name that helps you identify it when you manage your devices in the app or on the website.</string>
    <string name="device_management_device_limits">You can have up to 5 devices logged in on one Mullvad account.</string>
    <string name="device_management_refresh_name">If you log out, the device and the device name is removed. When you log back in again, the device will get a new name.</string>

Will these be used in this PR?

Code quote:

    <string name="device_management_name_description">This is the name assigned to the device. Each device logged in on a Mullvad account gets a unique name that helps you identify it when you manage your devices in the app or on the website.</string>
    <string name="device_management_device_limits">You can have up to 5 devices logged in on one Mullvad account.</string>
    <string name="device_management_refresh_name">If you log out, the device and the device name is removed. When you log back in again, the device will get a new name.</string>

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch 2 times, most recently from abf0351 to f51c4ee Compare July 21, 2023 14:35
@sabercodic sabercodic added the Android Issues related to Android label Jul 24, 2023
@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch 2 times, most recently from 303b880 to a1e4456 Compare July 24, 2023 13:30
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 9 files at r3.
Reviewable status: 5 of 12 files reviewed, 7 unresolved discussions (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/theme/dimensions/Dimensions.kt line 25 at r3 (raw file):

    val indentedCellStartPadding: Dp = 38.dp,
    val infoButtonVerticalPadding: Dp = 13.dp,
    val informationIconSize: Dp = 28.dp,

Is this size different than other info buttons?

Code quote:

informationIconSize

Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 12 files reviewed, 7 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 49 at r2 (raw file):

Previously, albin-mullvad wrote…

I suggest using 16 digits to look similar to the most regular account number length, eg 1234123412341234

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 16 at r2 (raw file):

Previously, albin-mullvad wrote…

I suggest handling moving this formatting logic to the existing DateExtensions

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/theme/dimensions/Dimensions.kt line 25 at r3 (raw file):

Previously, albin-mullvad wrote…

Is this size different than other info buttons?

Naming was wrong, fixed.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt line 93 at r2 (raw file):

Previously, albin-mullvad wrote…

I suggest placing this in alphabetical order within the VM section.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 20 at r2 (raw file):

Previously, albin-mullvad wrote…

Should be part of the combined flow in order to properly update the state.

Removed.


android/app/src/main/res/values/strings.xml line 222 at r2 (raw file):

Previously, albin-mullvad wrote…

Will these be used in this PR?

Done.

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from a1e4456 to fc18f4d Compare July 25, 2023 15:53
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 8 files at r4.
Reviewable status: 6 of 13 files reviewed, 8 unresolved discussions (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 16 at r2 (raw file):

Previously, sabercodic wrote…

Done.

As part of using an extension my idea was to also skip the var and instead just either (1) use the extension function in the view or (2) set in the ui state as String rather than DateTime.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 13 at r4 (raw file):

    var accountExpiry: DateTime?
) {
    val deviceName = deviceName.capitalizeFirstCharOfEachWord()

It's unnessary to store this value. Just call the extension function from the view instead.

Code quote:

val deviceName = deviceName.capitalizeFirstCharOfEachWord()

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/theme/dimensions/Dimensions.kt line 45 at r4 (raw file):

    val sideMargin: Dp = 22.dp,
    val smallPadding: Dp = 8.dp,
    val spinnerSize: Dp = 28.dp,

We already have loadingSpinnerSize and progressIndicatorSize and this another naming. These should be named in a common way and be prefix with something like small*, medium* and large*

Code quote:

val spinnerSize: Dp = 28.dp,

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/AccountFragment.kt line 50 at r4 (raw file):

    }

    /*

Please keep reference stuff like this locally, at least when the code is being reviewed.

Code quote:

 /*

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from fc18f4d to cf938f9 Compare July 26, 2023 09:52
Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 13 files reviewed, 8 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/AccountFragment.kt line 50 at r4 (raw file):

Previously, albin-mullvad wrote…

Please keep reference stuff like this locally, at least when the code is being reviewed.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 58 at r2 (raw file):

Previously, albin-mullvad wrote…

We usually want to avoid interactions with the context in the VM. This call should instead be made form the view.

Done.

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 9 files at r3, 4 of 5 files at r5, all commit messages.
Reviewable status: 10 of 13 files reviewed, 14 unresolved discussions (waiting on @sabercodic)


-- commits line 7 at r5:
typo

Code quote:

e

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/AccountNumberView.kt line 8 at r5 (raw file):

@Composable
fun AccountNumberView(accountNumber: String, isShown: Boolean) {

I suggest changing this arg name since it's not about hiding/showing. I'm not sure about the best suitable arg name, but something like "obfuscate" seems less confusing imo.

Code quote:

isShown

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CopyableInformationView.kt line 28 at r5 (raw file):

@Composable
fun CopyableInformationView(content: String) {

This is both for copying and "obfuscating" (with dots), right? In that case the name should reflect that.

Code quote:

CopyableInformationView(

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/InformationView.kt line 35 at r5 (raw file):

fun InformationView(
    content: String,
    error: String? = null,

Are we showing any errors in the account view? 🤔

Code quote:

error: String? = null,

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 96 at r5 (raw file):

    ) {
        LaunchedEffect(Unit) {
            viewActionSharedFlow.distinctUntilChanged().collect { viewAction ->

This one should not be needed since.

Code quote:

distinctUntilChanged()

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 16 at r2 (raw file):

Previously, albin-mullvad wrote…

As part of using an extension my idea was to also skip the var and instead just either (1) use the extension function in the view or (2) set in the ui state as String rather than DateTime.

This feedback still applies, including for the device name.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/AccountFragment.kt line 33 at r5 (raw file):

                    AccountScreen(
                        uiState = state,
                        viewActionSharedFlow = vm.viewActions,

I suggest chaning to just viewActions

Code quote:

viewActionSharedFlow

android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 38 at r5 (raw file):

    }

fun String.copyToClipboard(

This should be a Context extension rather than a String extension.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 49 at r5 (raw file):

    clipboard.setPrimaryClip(clipData)

    Toast.makeText(context, toastMessage, Toast.LENGTH_SHORT).show()

Can we toast only on Sdk versions where the built-in copy prompt isn't show? Either in this PR or create a new Linear issue.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 64 at r5 (raw file):

fun String.groupPasswordModeWithSpaces(groupCharSize: Int = 4): String {
    return BIG_DOT_CHAR.repeat(this.length).groupWithSpaces(groupCharSize)
}

We should try to avoid creating too many String extensions if possible since those are global unless they are explicitly scoped. Perhaps they can be moved to the AccountNumberView unless/until used somewhere else?

Code quote:

fun String.groupWithSpaces(groupCharSize: Int = 4): String {
    return fold(StringBuilder()) { formattedText, nextDigit ->
            if ((formattedText.length % (groupCharSize + 1)) == groupCharSize) {
                formattedText.append(SPACE_CHAR)
            }
            formattedText.append(nextDigit)
        }
        .toString()
}

fun String.groupPasswordModeWithSpaces(groupCharSize: Int = 4): String {
    return BIG_DOT_CHAR.repeat(this.length).groupWithSpaces(groupCharSize)
}

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 21 at r5 (raw file):

    private var accountRepository: AccountRepository,
    deviceRepository: DeviceRepository,
    private var serviceConnectionManager: ServiceConnectionManager

I suggest putting the private var arguments first.

Code quote:

    private var accountRepository: AccountRepository,
    deviceRepository: DeviceRepository,
    private var serviceConnectionManager: ServiceConnectionManager

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 10 of 13 files reviewed, 17 unresolved discussions (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CopyableInformationView.kt line 32 at r5 (raw file):

    val isShown = remember { mutableStateOf(false) }
    val clipboardLabel = stringResource(id = R.string.mullvad_account_number)
    val copiedToastMessage = stringResource(id = R.string.copied_mullvad_account_number)

Skip defining val:s for these

Code quote:

    val clipboardLabel = stringResource(id = R.string.mullvad_account_number)
    val copiedToastMessage = stringResource(id = R.string.copied_mullvad_account_number)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CopyableInformationView.kt line 34 at r5 (raw file):

    val copiedToastMessage = stringResource(id = R.string.copied_mullvad_account_number)

    return Row(verticalAlignment = Alignment.CenterVertically) {

Why do we need a return here?

Code quote:

return

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/InformationView.kt line 98 at r5 (raw file):

enum class MissingPolicy {
    NOTHING,

The intention of NOTHING is a bit unclear. Please clarify and perhaps come up with a more descriptive name.

Code quote:

NOTHING

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from cf938f9 to 32e8b0a Compare July 27, 2023 13:23
Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 10 of 13 files reviewed, 16 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/AccountNumberView.kt line 8 at r5 (raw file):

Previously, albin-mullvad wrote…

I suggest changing this arg name since it's not about hiding/showing. I'm not sure about the best suitable arg name, but something like "obfuscate" seems less confusing imo.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CopyableInformationView.kt line 28 at r5 (raw file):

Previously, albin-mullvad wrote…

This is both for copying and "obfuscating" (with dots), right? In that case the name should reflect that.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CopyableInformationView.kt line 32 at r5 (raw file):

Previously, albin-mullvad wrote…

Skip defining val:s for these

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CopyableInformationView.kt line 34 at r5 (raw file):

Previously, albin-mullvad wrote…

Why do we need a return here?

Removed.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/InformationView.kt line 35 at r5 (raw file):

Previously, albin-mullvad wrote…

Are we showing any errors in the account view? 🤔

removed


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/InformationView.kt line 98 at r5 (raw file):

Previously, albin-mullvad wrote…

The intention of NOTHING is a bit unclear. Please clarify and perhaps come up with a more descriptive name.

It just transformed from attr xml file, changed into SHOW_VIEW


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 58 at r2 (raw file):

Previously, albin-mullvad wrote…

Can't we use some common scaffolding? E.g. it looks very similar to settings.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 96 at r5 (raw file):

Previously, albin-mullvad wrote…

This one should not be needed since.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 16 at r2 (raw file):

Previously, albin-mullvad wrote…

This feedback still applies, including for the device name.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/AccountUiState.kt line 13 at r4 (raw file):

Previously, albin-mullvad wrote…

It's unnessary to store this value. Just call the extension function from the view instead.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/theme/dimensions/Dimensions.kt line 45 at r4 (raw file):

Previously, albin-mullvad wrote…

We already have loadingSpinnerSize and progressIndicatorSize and this another naming. These should be named in a common way and be prefix with something like small*, medium* and large*

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/AccountFragment.kt line 33 at r5 (raw file):

Previously, albin-mullvad wrote…

I suggest chaning to just viewActions

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 38 at r5 (raw file):

Previously, albin-mullvad wrote…

This should be a Context extension rather than a String extension.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 49 at r5 (raw file):

Previously, albin-mullvad wrote…

Can we toast only on Sdk versions where the built-in copy prompt isn't show? Either in this PR or create a new Linear issue.

Starting Android 13 the system toast will show based on documentation: https://developer.android.com/develop/ui/views/touch-and-input/copy-paste#:~:text=Starting%20in%20Android%2013%2C%20the,preview%20of%20the%20copied%20content.
Should I apply SDK check or investigate more on detecting system events?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 64 at r5 (raw file):

Previously, albin-mullvad wrote…

We should try to avoid creating too many String extensions if possible since those are global unless they are explicitly scoped. Perhaps they can be moved to the AccountNumberView unless/until used somewhere else?

Moved into Context extension


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 21 at r5 (raw file):

Previously, albin-mullvad wrote…

I suggest putting the private var arguments first.

Done.

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from 32e8b0a to 564ff22 Compare July 27, 2023 13:34
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 11 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/AccountNumberView.kt line 8 at r5 (raw file):

Previously, sabercodic wrote…

Done.

The new name doesn't sound gramatically correct. I suggest doObfuscateWithPasswordDots


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/extension/ContextExtensions.kt line 71 at r6 (raw file):

    clipboard.setPrimaryClip(clipData)

    Toast.makeText(this, toastMessage, Toast.LENGTH_SHORT).show()

I believe it's better to skip the Toast here and instead handle that on the callsite.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/AccountFragment.kt line 33 at r5 (raw file):

Previously, sabercodic wrote…

Done.

nit: missing s in the end


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 49 at r5 (raw file):

Previously, sabercodic wrote…

Starting Android 13 the system toast will show based on documentation: https://developer.android.com/develop/ui/views/touch-and-input/copy-paste#:~:text=Starting%20in%20Android%2013%2C%20the,preview%20of%20the%20copied%20content.
Should I apply SDK check or investigate more on detecting system events?

If it's just a sdk version check I think it's best to just do it now.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 42 at r6 (raw file):

                AccountUiState(deviceName = "", accountNumber = "", accountExpiry = null)
            )
    val uiState =

nit: add newline before this line


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModel.kt line 58 at r6 (raw file):

        }
    }
    fun onLogoutClick() {

nit: add newline before this line

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from 564ff22 to a94c809 Compare July 28, 2023 09:38
Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 27 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/AccountNumberView.kt line 8 at r5 (raw file):

Previously, albin-mullvad wrote…

The new name doesn't sound gramatically correct. I suggest doObfuscateWithPasswordDots

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/extension/ContextExtensions.kt line 71 at r6 (raw file):

Previously, albin-mullvad wrote…

I believe it's better to skip the Toast here and instead handle that on the callsite.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/StringExtensions.kt line 49 at r5 (raw file):

Previously, albin-mullvad wrote…

If it's just a sdk version check I think it's best to just do it now.

Done.

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 19 of 19 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad marked this pull request as ready for review July 28, 2023 09:46
@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from a94c809 to 831875c Compare July 28, 2023 13:59
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @sabercodic)


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 40 at r8 (raw file):

    private val accountExpiryState = MutableStateFlow(AccountExpiry.Missing)

    private val mockAuthTokenCache: AuthTokenCache = mockk()

Move to the other mocks

Code quote:

private val mockAuthTokenCache: AuthTokenCache = mockk()

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 41 at r8 (raw file):

    private val mockAuthTokenCache: AuthTokenCache = mockk()
    private val mockAccountAndDevice: AccountAndDevice =

Rename to dummyAccountAndDevice

Code quote:

mockAccountAndDevice

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 47 at r8 (raw file):

                id = "fake_id",
                name = "fake_name",
                pubkey = ByteArray(1234),

Use an empty array here instead.

Code quote:

 ByteArray(1234)

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 72 at r8 (raw file):

    @After
    fun tearDown() {
        viewModel.viewModelScope.coroutineContext.cancel()

Is this one necssary?

Code quote:

viewModel.viewModelScope.coroutineContext.cancel()

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 77 at r8 (raw file):

    @Test
    fun test_account_log_in_state() =

Skip underscores to align the name with the logout test.

Code quote:

test_account_log_in_state

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 78 at r8 (raw file):

    @Test
    fun test_account_log_in_state() =
        runTest(testCoroutineRule.testDispatcher) {

Is this one necessary?

Code quote:

testCoroutineRule.testDispatcher

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 82 at r8 (raw file):

            viewModel.uiState.test {
                var result = awaitItem()
                assertNotEquals("1111222233334444", result.accountNumber)

Are we not expecting the initial state here? In that case I suggest asserting on that class instead.

Code quote:

assertNotEquals("1111222233334444", result.accountNumber)

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 84 at r8 (raw file):

                assertNotEquals("1111222233334444", result.accountNumber)
                deviceState.value = DeviceState.LoggedIn(accountAndDevice = mockAccountAndDevice)
                accountExpiryState.value = AccountExpiry.Missing

Is this necessary?

Code quote:

accountExpiryState.value = AccountExpiry.Missing

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 86 at r8 (raw file):

                accountExpiryState.value = AccountExpiry.Missing
                result = awaitItem()
                assertEquals("1111222233334444", result.accountNumber)

I suggest using the "dummy" property here instead

Code quote:

"1111222233334444"

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 92 at r8 (raw file):

    @Test
    fun testOnLogoutClick() {

nit: remove this empty line

Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @albin-mullvad)


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 40 at r8 (raw file):

Previously, albin-mullvad wrote…

Move to the other mocks

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 41 at r8 (raw file):

Previously, albin-mullvad wrote…

Rename to dummyAccountAndDevice

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 47 at r8 (raw file):

Previously, albin-mullvad wrote…

Use an empty array here instead.

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 72 at r8 (raw file):

Previously, albin-mullvad wrote…

Is this one necssary?

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 77 at r8 (raw file):

Previously, albin-mullvad wrote…

Skip underscores to align the name with the logout test.

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 78 at r8 (raw file):

Previously, albin-mullvad wrote…

Is this one necessary?

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 82 at r8 (raw file):

Previously, albin-mullvad wrote…

Are we not expecting the initial state here? In that case I suggest asserting on that class instead.

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 84 at r8 (raw file):

Previously, albin-mullvad wrote…

Is this necessary?

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/AccountViewModelTest.kt line 86 at r8 (raw file):

Previously, albin-mullvad wrote…

I suggest using the "dummy" property here instead

Done.

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from 831875c to 70b9595 Compare July 28, 2023 14:44
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)


-- commits line 7 at r9:
typo

Code quote:

screeen

@sabercodic sabercodic force-pushed the migrate-account-view-to-compose-droid-63 branch from 70b9595 to 1fa025a Compare July 28, 2023 14:57
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad merged commit f300cda into main Jul 28, 2023
13 checks passed
@albin-mullvad albin-mullvad deleted the migrate-account-view-to-compose-droid-63 branch July 28, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants