Fix clipboard paste bug

[sabercodic]

This change is 

[linear]

DROID-402 Fix clipboard copy/paste bug on VoucherRedeemDialog when it contains "-"

[Pururun]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Rawa]

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

[Rawa]

Reviewable status: complete! all files reviewed, all discussions resolved

[Pururun]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

---

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

    private val voucherInput = MutableStateFlow(LoginUiState.INITIAL.accountNumberInput)

    lateinit var voucherRedeemer: VoucherRedeemer

I'm not sure I like this change, to make something public just to enable tests feels a bit weird. Maybe we can keep it this way and when we have updated the messaging architecture we can revert this and make the tests better.

[albin-mullvad]

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

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I'm not sure I like this change, to make something public just to enable tests feels a bit weird. Maybe we can keep it this way and when we have updated the messaging architecture we can revert this and make the tests better.

Yes, this should not be exposed. I wonder why this isn't catched by our compose test case @Rawa 🤔

Test name: ensure public properties use permitted names

[albin-mullvad]

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

---

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

Previously, albin-mullvad wrote…

Yes, this should not be exposed. I wonder why this isn't catched by our compose test case @Rawa 🤔

Test name: ensure public properties use permitted names

compose -> konsist *

[albin-mullvad]

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

---

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

Previously, albin-mullvad wrote…

compose -> konsist *

Here's a PR to fix the konsist issue: #5288

[Rawa]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @Pururun and @sabercodic)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 59 at r4 (raw file):

    @Test
    fun test_submit_invalid_voucher() = runTest {
        val voucher = DUMMY_VALID_VOUCHER

We call this test test_submit_invalid_voucher but we send in a DUMMY_VALID_VOUCHER.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 69 at r4 (raw file):

        // Act, Assert
        viewModel.onRedeem(voucher)
        coVerify(exactly = 1) { mockVoucherRedeemer.submit(voucher) }

Do we actually test_submit_invalid_voucher? To be me it looks like we verify that if we call viewModel.onRedeem we will have a call to voucherRedeemer.submit with the same voucher? No matter if the voucher is valid or invalid?

[sabercodic]

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @Pururun and @Rawa)

---

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

Previously, albin-mullvad wrote…

Here's a PR to fix the konsist issue: #5288

great

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 59 at r4 (raw file):

Previously, Rawa (David Göransson) wrote…

We call this test test_submit_invalid_voucher but we send in a DUMMY_VALID_VOUCHER.

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 69 at r4 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we actually test_submit_invalid_voucher? To be me it looks like we verify that if we call viewModel.onRedeem we will have a call to voucherRedeemer.submit with the same voucher? No matter if the voucher is valid or invalid?

Done.

[albin-mullvad]

Reviewed 1 of 2 files at r5.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @Pururun and @Rawa)

---

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

Previously, sabercodic wrote…

great

There's a reason the konsist test was added and we should not introduce code that isn't compliant with it. This property shouldn't be public and that hasn't been addressed.

[sabercodic]

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad, @Pururun, and @Rawa)

---

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

Previously, albin-mullvad wrote…

There's a reason the konsist test was added and we should not introduce code that isn't compliant with it. This property shouldn't be public and that hasn't been addressed.

Done.

[Pururun]

Reviewed 2 of 2 files at r5, 1 of 1 files at r6, 3 of 4 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 29 at r10 (raw file):

class VoucherDialogViewModel(
    val serviceConnectionManager: ServiceConnectionManager,

This should be private.

[sabercodic]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 29 at r10 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This should be private.

Done.

[albin-mullvad]

Reviewed 6 of 9 files at r15, all commit messages.
Reviewable status: 10 of 13 files reviewed, 6 unresolved discussions (waiting on @sabercodic)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 40 at r15 (raw file):

    @Before
    fun setup() {
        MockKAnnotations.init(this)

There's no @MockK annotations in this test, so this one can be skipped.

Code quote:

MockKAnnotations.init(this)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 51 at r15 (raw file):

        // Act
        val vm = VoucherDialogViewModel(mockServiceConnectionManager, mockResources)

Is there any reason for not mocking the vm?

Code quote:

val vm = VoucherDialogViewModel(mockServiceConnectionManager, mockResources)

[albin-mullvad]

Reviewed 2 of 9 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @sabercodic)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 51 at r15 (raw file):

Previously, albin-mullvad wrote…

Is there any reason for not mocking the vm?

Mocking also means removing a lot of other mocked stuff, like for example: mockServiceConnectionManager & mockResources

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 45 at r16 (raw file):

    @Test
    fun testInsertInvalidVoucher() {

There should probably be two tests here, one for inserting a voucher with "valid" and one with "Invalid" format

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 63 at r16 (raw file):

        // Sets the TextField value
        composeTestRule.onNodeWithTag(VOUCHER_INPUT_TEST_TAG).performTextInput(DUMMY_VALID_VOUCHER)

This seem to differ from the test name testInsertInvalidVoucher

Code quote:

performTextInput(DUMMY_VALID_VOUCHER)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 83 at r16 (raw file):

                onDismiss = mockedClickHandler
            )
        }

I suggest moving this to Arrange. Same for the other test.

Code quote:

        composeTestRule.setContentWithTheme {
            RedeemVoucherDialogScreen(
                uiState = VoucherDialogUiState.INITIAL,
                onVoucherInputChange = {},
                onRedeem = {},
                onDismiss = mockedClickHandler
            )
        }

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 13 at r16 (raw file):

@RunWith(Parameterized::class)
class RedeemVoucherHelperTest(private val isValid: Boolean, private val voucher: String) {

I suggest renaming to VoucherRegexHelperParameterizedTest

Code quote:

RedeemVoucherHelperTest

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 31 at r16 (raw file):

                arrayOf(false, non_acceptable_inputs_for_voucher[0]),
                arrayOf(false, non_acceptable_inputs_for_voucher[1]),
                arrayOf(false, non_acceptable_inputs_for_voucher[2])

It's hard to get an overview regarding which vouchers and valid and not by looking at this? Can you structure it in a way where you can see the boolean next to the actual voucher? Perhaps also switching argument order and using a constant for the boolean would be nice to clarify that true/false actually means.

My suggestions would structure it like this:

 arrayOf("AAAA-AAAA-1111-2222", IS_ACCEPTED_FORMAT)
 // ...
 arrayOf("AAAA_BBBB_CCCC_DDDD", IS_UNACCEPTED_FORMAT)

Code quote:

                arrayOf(true, acceptable_inputs_for_voucher[0]),
                arrayOf(true, acceptable_inputs_for_voucher[1]),
                arrayOf(true, acceptable_inputs_for_voucher[2]),
                arrayOf(true, acceptable_inputs_for_voucher[3]),
                arrayOf(true, acceptable_inputs_for_voucher[4]),
                arrayOf(true, acceptable_inputs_for_voucher[5]),
                arrayOf(true, acceptable_inputs_for_voucher[6]),
                arrayOf(true, acceptable_inputs_for_voucher[7]),
                arrayOf(false, non_acceptable_inputs_for_voucher[0]),
                arrayOf(false, non_acceptable_inputs_for_voucher[1]),
                arrayOf(false, non_acceptable_inputs_for_voucher[2])

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 54 at r16 (raw file):

    @Test
    fun shouldReturnExpectedResultForVoucherCheck() {

I suggest renaming to testVoucherFormat

Code quote:

shouldReturnExpectedResultForVoucherCheck

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 45 at r16 (raw file):

    private val mockVoucherSubmissionOkResult: VoucherSubmissionResult.Ok =
        VoucherSubmissionResult.Ok(mockVoucherSubmission)

Should probably be moved to the test itself. Same for mockVoucherSubmissionErrorResult

Code quote:

    private val mockVoucherSubmissionOkResult: VoucherSubmissionResult.Ok =
        VoucherSubmissionResult.Ok(mockVoucherSubmission)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 87 at r16 (raw file):

    @Test
    fun testInsertValidVoucher() = runTest {

Why is this quite different from the testSubmitVoucher? I assume they should be almost identical

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 103 at r16 (raw file):

                VoucherDialogState.Default,
                uiStates.awaitItem().voucherViewModelState
            )

The Act and Assert can be easily separated here

Code quote:

            // Act, Assert
            viewModel.onRedeem(voucher)
            Assert.assertEquals(
                VoucherDialogState.Default,
                uiStates.awaitItem().voucherViewModelState
            )

[sabercodic]

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

a discussion (no related file):

Previously, albin-mullvad wrote…

As talked about offline, we should add parameterized tests for VoucherRegexHelper

Done

---

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 40 at r15 (raw file):

Previously, albin-mullvad wrote…

There's no @MockK annotations in this test, so this one can be skipped.

Done.

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 51 at r15 (raw file):

Previously, albin-mullvad wrote…

Is there any reason for not mocking the vm?

Done.

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 45 at r16 (raw file):

Previously, albin-mullvad wrote…

There should probably be two tests here, one for inserting a voucher with "valid" and one with "Invalid" format

Done.

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 63 at r16 (raw file):

Previously, albin-mullvad wrote…

This seem to differ from the test name testInsertInvalidVoucher

Done.

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 83 at r16 (raw file):

Previously, albin-mullvad wrote…

I suggest moving this to Arrange. Same for the other test.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 221 at r14 (raw file):

Previously, Rawa (David Göransson) wrote…

We do this in several places, and it seems dangerous. If we miss the argument we wouldn't do any validation.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 71 at r14 (raw file):

Previously, Rawa (David Göransson) wrote…

Yes, we should do this logic directly in the view model. We already have a voucherInput in this view model and we trim when the view edit that value. Not sure exactly why we need validation in UI layer since we will already have the trim, but if we want to notify if it is valid or not we can just include that into the viewstate. We already have the latest voucherInput value from when we combine into uiState

Ping me if I'm not clear or if you want to pair-program it togheter 👍

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 45 at r16 (raw file):

Previously, albin-mullvad wrote…

Should probably be moved to the test itself. Same for mockVoucherSubmissionErrorResult

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 103 at r16 (raw file):

Previously, albin-mullvad wrote…

The Act and Assert can be easily separated here

Done.

---

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

Previously, albin-mullvad wrote…

We should avoid making this function a string extension since it will bloat the string function list thoughout the project. Did you see my linked suggestion?

#5271 (review)

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 13 at r16 (raw file):

Previously, albin-mullvad wrote…

I suggest renaming to VoucherRegexHelperParameterizedTest

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 31 at r16 (raw file):

Previously, albin-mullvad wrote…

It's hard to get an overview regarding which vouchers and valid and not by looking at this? Can you structure it in a way where you can see the boolean next to the actual voucher? Perhaps also switching argument order and using a constant for the boolean would be nice to clarify that true/false actually means.

My suggestions would structure it like this:

 arrayOf("AAAA-AAAA-1111-2222", IS_ACCEPTED_FORMAT)
 // ...
 arrayOf("AAAA_BBBB_CCCC_DDDD", IS_UNACCEPTED_FORMAT)

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 54 at r16 (raw file):

Previously, albin-mullvad wrote…

I suggest renaming to testVoucherFormat

Done.

[sabercodic]

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

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 51 at r15 (raw file):

Previously, sabercodic wrote…

Done.

Done.

[Pururun]

Reviewed 5 of 5 files at r17, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @albin-mullvad)

[albin-mullvad]

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

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 26 at r17 (raw file):

    @get:Rule val composeTestRule = createComposeRule()

    private var uiState = VoucherDialogUiState.INITIAL

I suggest using this one directly in each test instead

Code quote:

VoucherDialogUiState.INITIAL

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 236 at r17 (raw file):

        onValueChanged = { input ->
            voucherInput = input
            onVoucherInputChange(input)

Should we really update two places?

Code quote:

            voucherInput = input
            onVoucherInputChange(input)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 35 at r17 (raw file):

    private val vmState = MutableStateFlow<VoucherDialogState>(VoucherDialogState.Default)
    private val voucherInput = MutableStateFlow(VoucherDialogUiState.INITIAL.voucherInput)

What's the reason for using the voucherInput string rather than the VoucherDialogUiState.INITIAL state here?

Code quote:

VoucherDialogUiState.INITIAL.voucherInput

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 45 at r16 (raw file):

Previously, sabercodic wrote…

Done.

I also mentioned mockVoucherSubmissionErrorResult so that we keep it consistent

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 31 at r16 (raw file):

Previously, sabercodic wrote…

Done.

As shown in my example above, can we also move the vouchers to top-level array to avoid two levels of arrays?

[sabercodic]

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

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/RedeemVoucherDialogTest.kt line 26 at r17 (raw file):

Previously, albin-mullvad wrote…

I suggest using this one directly in each test instead

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 236 at r17 (raw file):

Previously, albin-mullvad wrote…

Should we really update two places?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 35 at r17 (raw file):

Previously, albin-mullvad wrote…

What's the reason for using the voucherInput string rather than the VoucherDialogUiState.INITIAL state here?

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 45 at r16 (raw file):

Previously, albin-mullvad wrote…

I also mentioned mockVoucherSubmissionErrorResult so that we keep it consistent

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 87 at r16 (raw file):

Previously, albin-mullvad wrote…

Why is this quite different from the testSubmitVoucher? I assume they should be almost identical

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/utils/RedeemVoucherHelperTest.kt line 31 at r16 (raw file):

Previously, albin-mullvad wrote…

As shown in my example above, can we also move the vouchers to top-level array to avoid two levels of arrays?

Done.

[albin-mullvad]

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

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 52 at r18 (raw file):

        every { mockServiceConnectionContainer.connectionProxy } returns mockConnectionProxy

        serviceConnectionState.value

Clean up

Code quote:

serviceConnectionState.value

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 79 at r18 (raw file):

        coEvery { mockVoucherRedeemer.submit(voucher) } returns
            VoucherSubmissionResult.Ok(mockVoucherSubmission)
        // Act

Missing newline

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 93 at r18 (raw file):

        // Arrange
        val uiStates = viewModel.uiState

This can be skipped

Code quote:

val uiStates = viewModel.uiState

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 104 at r18 (raw file):

        // Assert
        Assert.assertTrue(uiStates.value.voucherViewModelState is VoucherDialogState.Default)

Shouldn't the VoucherSubmissionError.OtherError result in a error dialog state rather than the default state?

Also, we should probably await uiState updates here.

Code quote:

Assert.assertTrue(uiStates.value.voucherViewModelState is VoucherDialogState.Default)

[Rawa]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 63 at r18 (raw file):

                is VoucherSubmissionResult.Ok -> handleAddedTime(result.submission.timeAdded)
                is VoucherSubmissionResult.Error -> setError(result.error)
                else -> {

Generally I think we should avoid using else in when statements, it is better to be explicit. In this case if voucherRedeemer is null we will give VoucherDialogState.Default is this correct?

[sabercodic]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 63 at r18 (raw file):

Previously, Rawa (David Göransson) wrote…

Generally I think we should avoid using else in when statements, it is better to be explicit. In this case if voucherRedeemer is null we will give VoucherDialogState.Default is this correct?

This block of code did not change on this PR, I will refactor this part in different PR

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 52 at r18 (raw file):

Previously, albin-mullvad wrote…

Clean up

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 79 at r18 (raw file):

Previously, albin-mullvad wrote…

Missing newline

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 93 at r18 (raw file):

Previously, albin-mullvad wrote…

This can be skipped

Done.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModelTest.kt line 104 at r18 (raw file):

Previously, albin-mullvad wrote…

Shouldn't the VoucherSubmissionError.OtherError result in a error dialog state rather than the default state?

Also, we should probably await uiState updates here.

Done.

[albin-mullvad]

Nice, gj!

Reviewed 1 of 1 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

[Rawa]

Reviewable status: complete! all files reviewed, all discussions resolved

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 63 at r18 (raw file):

Previously, sabercodic wrote…

This block of code did not change on this PR, I will refactor this part in different PR

Ah, yes before it was lateinit so technically not nullable but the same type of error could happen. I'm fine with that we fix it in another PR, just make sure we don't miss to make a new ticket/PR about it

[Rawa]

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

[albin-mullvad]

Reviewable status: complete! all files reviewed, all discussions resolved

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 63 at r18 (raw file):

Previously, Rawa (David Göransson) wrote…

Ah, yes before it was lateinit so technically not nullable but the same type of error could happen. I'm fine with that we fix it in another PR, just make sure we don't miss to make a new ticket/PR about it

@sabercodic will you create and link an issue here?

[sabercodic]

Reviewable status: complete! all files reviewed, all discussions resolved

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 63 at r18 (raw file):

Previously, albin-mullvad wrote…

@sabercodic will you create and link an issue here?

https://linear.app/mullvad/issue/DROID-541/code-clean-up-for-conditional-statements

[albin-mullvad]

Reviewable status: complete! all files reviewed, all discussions resolved

[albin-mullvad]

Reviewed 2 of 2 files at r21, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved