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

Fix Disconnect/re-connect from notification when both VPN and AppTP are enabled #5154

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

aitorvs
Copy link
Collaborator

@aitorvs aitorvs commented Oct 18, 2024

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

Description

Fix bug when both AppTP and VPN are enabled and user disconnects from the notification.
When that happens two notifications are showed with CTAs to re-enable AppTP and VPN resp.
Expected:

  • AppTP enable enables just ApptP
  • VPN enable just enables VPN

Actual

  • either ApptP or VPN enable enable both

Steps to test this PR

Test Fix

  • Enable both AppTP and VPN
  • In the combined AppTP/VPN notification press Disconnect
  • Verify two notifications, for AppTP and VPN are shown
  • Click on AppTP notification, enable CTA
  • verify only AppTP is enabled
  • Click on VPN notification, enable CTA
  • verify only VPN is enabled
  • repeat this test in the inverse order, before VPN then AppTP

Test combined notification

  • After doing Test Fix above
  • open apps with trackers
  • verify AppTP blocks trackers and combined notification works as expected
  • smoke test

Test AppTP

  • smoke test AppTP only

Test VPN

  • smoke test VPN only

Copy link
Collaborator Author

aitorvs commented Oct 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @aitorvs and the rest of your teammates on Graphite Graphite

@karlenDimla
Copy link
Contributor

@aitorvs when testing this PR I encountered this issue:

  • After repeating the test steps in Text fix, at some point I get to a state where after clicking on Disconnect, the combined enabled notif doesn’t disappear (x) , VPN is disabled (ok), VPN and AppTP disabled notifs are shown (ok) BUT nothing on the actions work or do anything (x).

@aitorvs aitorvs force-pushed the fix/aitor/vpn/disable-reenable-vpn-apptp-fix branch from 5f36fbf to a99ebfe Compare October 21, 2024 17:34
@@ -38,6 +40,10 @@ import logcat.logcat
class VpnActionReceiver : BroadcastReceiver() {
@Inject lateinit var vpn: Vpn

@Inject lateinit var networkProtectionState: NetworkProtectionState
Copy link
Contributor

Choose a reason for hiding this comment

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

NIt: Can be removed as they are not used anymore in this class right?

Copy link
Contributor

@karlenDimla karlenDimla left a comment

Choose a reason for hiding this comment

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

Tested and worked as expected. One NIT which will be good to remove before merging

@aitorvs aitorvs merged commit a0f65ad into develop Oct 23, 2024
8 checks passed
@aitorvs aitorvs deleted the fix/aitor/vpn/disable-reenable-vpn-apptp-fix branch October 23, 2024 11:36
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