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 toggling of NetP Notifications to iOS #2112

Merged
merged 25 commits into from
Oct 31, 2023

Conversation

graeme
Copy link
Contributor

@graeme graeme commented Oct 20, 2023

Task/Issue URL: https://app.asana.com/0/0/1205768725084797/f
BSK: duckduckgo/BrowserServicesKit#541

Description:

This adds the implementation of the VPN Notifications screen. It serves as a way to toggle NetP notifications on the iOS Network Extension and also to help prompt enabling notifications app-wide.

This is quite a design-heavy project, but there’s also use of an app-group shared UserDefaults store to share the toggle state between the status view, app side and the notifications presenter, extension side.

Steps to test this PR:

  1. Delete the iOS app and reinstall (to reset notifications settings)
  2. Launch and enable internal testing (Settings -> Debug -> Internal User)
  3. Go to the Network Protection screen (Settings -> Network Protection)
  4. Select VPN Notifications
  5. Check it matches these designs
  6. Press Turn on Notifications here and disallow them
  7. Press the button again which will take you to the iOS DuckDuckGo notifications settings. Enabled them.
  8. Go back to DuckDuckGo to the VPN Notifications screen and check they match these (other) designs
  9. Go back to the NetP Status view. Enabled NetP. Observe the notification
  10. Go to VPN Notifications one more time.
  11. Go back to the NetP Status view. Disable NeP and reenable it. Observe there is no notification.
  12. Extra: you can try the same with the test notifications from the Debug menu. They should also be affected by this toggle.

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@graeme graeme self-assigned this Oct 20, 2023
@graeme graeme requested a review from diegoreymendez October 20, 2023 15:47
@graeme graeme marked this pull request as ready for review October 20, 2023 15:50
.padding(.top, statusModel.error == nil ? 0 : -20)
.animation(.default, value: statusModel.shouldShowConnectionDetails)
.if(statusModel.animationsOn, transform: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The animation bug this fixes became a lot more noticeable with the addition of the new section. So I fixed it here.

@samsymons samsymons self-requested a review October 30, 2023 23:58
Comment on lines +25 to +31
static var networkProtectionGroupDefaults: UserDefaults {
let suiteName = "\(Global.groupIdPrefix).netp"
guard let defaults = UserDefaults(suiteName: suiteName) else {
fatalError("Failed to create netP UserDefaults")
}
return defaults
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I suspect I'll be using this for the widget - thanks!

Comment on lines +23 to +33
private static let notificationSettingsURL: URL? = {
let settingsString: String
if #available(iOS 16, *) {
settingsString = UIApplication.openNotificationSettingsURLString
} else if #available(iOS 15.4, *) {
settingsString = UIApplicationOpenNotificationSettingsURLString
} else {
settingsString = UIApplication.openSettingsURLString
}
return URL(string: settingsString)
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. We have some notification handling code that exists for the waitlist, but I don't think it's as complete as this - I'll keep this in mind for the next waitlist.

Copy link
Contributor

@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.

Looks great, and works as expected - nice one!

@graeme graeme merged commit ff096da into develop Oct 31, 2023
9 checks passed
@graeme graeme deleted the graeme/ios-vpn-notification-settings branch October 31, 2023 17:51
samsymons added a commit that referenced this pull request Nov 5, 2023
* develop:
  Alert user about abnormal app conditions (#2110)
  Add e2e test for email protection deep-links (#2123)
  Update to config v4 (#2114)
  Release 7.95.0 (#2122)
  Add toggling of NetP Notifications to iOS (#2112)
  Fix for deeplink crash when preparing webview preview (#2116)
  Integrates the latest DBP changes in BSK (#2108)
  Switch to next phase of new experiment for UA (#2118)
samsymons added a commit that referenced this pull request Nov 8, 2023
* develop:
  re-enable keyboard shortcuts (#2132)
  fix favorite launch with keyboard bug (#2131)
  Fix syncing empty favorites folders (#2121)
  kill variant when receiving atb update (#2130)
  Adding 'protectionsState' to breakage form submission (#2120)
  Release 7.96.0 (#2128)
  Update remote messaging production url (#2124)
  Alert user about abnormal app conditions (#2110)
  Add e2e test for email protection deep-links (#2123)
  Update to config v4 (#2114)
  Release 7.95.0 (#2122)
  Add toggling of NetP Notifications to iOS (#2112)
  Fix for deeplink crash when preparing webview preview (#2116)
  Integrates the latest DBP changes in BSK (#2108)
  Switch to next phase of new experiment for UA (#2118)
samsymons added a commit that referenced this pull request Nov 9, 2023
* develop:
  Release 7.95.0 (#2122)
  Add toggling of NetP Notifications to iOS (#2112)
  Fix for deeplink crash when preparing webview preview (#2116)
  Integrates the latest DBP changes in BSK (#2108)
  Switch to next phase of new experiment for UA (#2118)
  Update BSK with autofill 9.0.0 (#2103)
  Add an “Open VPN Settings” quick action (#2089)
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