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

Add filter for ownership and providers #5506

Merged
merged 7 commits into from
Dec 4, 2023

Conversation

MaryamShaghaghi
Copy link
Contributor

@MaryamShaghaghi MaryamShaghaghi commented Nov 27, 2023

In this screen, users can select ownership and providers to customize their experience. The selected preferences take effect in the 'Select Location' screen upon clicking 'Apply'.


This change is Reviewable

@Pururun Pururun added the Android Issues related to Android label Nov 27, 2023
Copy link
Contributor

@Pururun Pururun 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 43 of 43 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Rawa Rawa 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 r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

:lgtm:

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.

Good job! 🌟 I just have a few small comments.

Reviewed 40 of 43 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @MaryamShaghaghi)


android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt line 80 at r3 (raw file):

        composeTestRule.apply {
            onNodeWithText("Ownership").performClick()
            onNodeWithText("Mullvad owned only").performClick()

This should probably be assertExists() which is used in for example testIsRentedCellShowing?

Code quote:

performClick()

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt line 167 at r3 (raw file):

                Provider("techfutures", false),
                Provider("Tzulo", false),
                Provider("xtom", false)

I think it would be better to not use real provider names here, but we can probably address that later.

Code quote:

                Provider("31173", true),
                Provider("100TB", false),
                Provider("Blix", true),
                Provider("Creanova", true),
                Provider("DataPacket", false),
                Provider("HostRoyale", false),
                Provider("hostuniversal", false),
                Provider("iRegister", false),
                Provider("M247", false),
                Provider("Makonix", false),
                Provider("PrivateLayer", false),
                Provider("ptisp", false),
                Provider("Qnax", false),
                Provider("Quadranet", false),
                Provider("techfutures", false),
                Provider("Tzulo", false),
                Provider("xtom", false)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 177 at r3 (raw file):

    }

    fun openFilter() {

Is there a reason for including this function in the activity? Seems like it could be part of the fragment 🤔

Code quote:

openFilter

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/FilterViewModelTest.kt line 34 at r3 (raw file):

    private val selectedOwnership =
        MutableStateFlow<Constraint<Ownership>>(Constraint.Only(Ownership.MullvadOwned))
    private val mockAllProviders =

This name is a bit misleading since it's not a mock. I suggest renaming to something like dummyListOfAllProviders

Code quote:

mockAllProviders

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

    val dialogIconSize: Dp = 48.dp,
    val expandableCellChevronSize: Dp = 30.dp,
    val filterTittlePadding: Dp = 4.dp,

nit: typo

Code quote:

tt

Copy link
Contributor Author

@MaryamShaghaghi MaryamShaghaghi 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, 3 unresolved discussions (waiting on @albin-mullvad)


android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt line 80 at r3 (raw file):

Previously, albin-mullvad wrote…

This should probably be assertExists() which is used in for example testIsRentedCellShowing?

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 177 at r3 (raw file):

Previously, albin-mullvad wrote…

Is there a reason for including this function in the activity? Seems like it could be part of the fragment 🤔

Done.


android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/FilterViewModelTest.kt line 34 at r3 (raw file):

Previously, albin-mullvad wrote…

This name is a bit misleading since it's not a mock. I suggest renaming to something like dummyListOfAllProviders

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 6 of 6 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

MaryamShaghaghi and others added 5 commits December 4, 2023 11:30
Co-Authored-By: Boban Sijuk <[email protected]>
Co-Authored-By: Boban Sijuk <[email protected]>
Co-Authored-By: Boban Sijuk <[email protected]>
Copy link
Contributor

@Pururun Pururun 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 r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun merged commit e119d9b into mullvad:main Dec 4, 2023
17 checks passed
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.

4 participants