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

Implement VPN App Exclusions management view #3800

Merged
merged 33 commits into from
Feb 3, 2025

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Jan 29, 2025

Task/Issue URL: https://app.asana.com/0/0/1209150117333886/f

Description

Adds app exclusions management UI, effectively enabling app exclusions behind the feature flag.

Translations

Job is here.

Known limitations

  1. Chrome requires excluding it's helper app too, which is currently not automatically handled.

Testing

Some basic testing expectations:

  • The app store build should never show exclusions since they're disabled
  • When the app exclusions feature flag is disabled (which is the current default), you should see the same exclusions UI that's in our release build.
  • When the app exclusions feature flag is enabled (through Debug Menu > Feature Flag Overrides) you should see the new exclusions UI.

Test app exclusions work

  1. Enable the app exclusions feature flag override
  2. Go into Settings > VPN > App Exclusions and add an app to exclude
  3. Start the VPN
  4. Make sure the app traffic is excluded (if using a browser you can check you IP address).

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Copy link
Contributor

github-actions bot commented Jan 29, 2025

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 62d13ae

import PackageDescription

let package = Package(
name: "AppInfoRetriever",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I'm adding some code to retrieve information from 3rd party apps living in /Applications, I figured it would be good to modularize it into a Swift Package.

@diegoreymendez diegoreymendez self-assigned this Jan 31, 2025
@diegoreymendez diegoreymendez marked this pull request as ready for review January 31, 2025 12:43
value > 0 ? String(value) : "None"
value > 0 ? String(value) : UserText.vpnNoExclusionsFoundText
Copy link
Collaborator

Choose a reason for hiding this comment

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

One note on this, it didn't update automatically when I excluded an app, so it was telling me I had nothing excluded until I closed and re-opened the settings UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try to get proper refresh fixed before merging this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


}

public class AppInfoRetriever: AppInfoRetrieveing {
Copy link
Collaborator

@samsymons samsymons Feb 3, 2025

Choose a reason for hiding this comment

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

Neat – we could possibly reuse this for the data import feature, which does some app info lookup of its own but in a non-generic way.

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Only issue I had with this was that the string showing the number of excluded apps didn't refresh after changing my list of exclusions. Also, the apps I excluded sometimes had to be restarted for it to take effect, but this is nothing new. Otherwise it worked really well, no issues!

@diegoreymendez
Copy link
Contributor Author

Addressed the refresh issue and no other feedback pending so I'll go ahead and merge. Thanks!

@diegoreymendez diegoreymendez merged commit 877a0f7 into main Feb 3, 2025
24 checks passed
@diegoreymendez diegoreymendez deleted the diego/vpn-app-exclusions-dialogue branch February 3, 2025 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants