Implement waitlistBetaActive logic (without actually resetting) #1562

diegoreymendez · 2023-08-29T21:34:27Z

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

Related PRs

BSK PR: duckduckgo/BrowserServicesKit#483
iOS PR: duckduckgo/iOS#1967

Description

Adds logic to handle the remote waitlistBetaActive subfeature, and to reset Network Protection once the beta is no longer active.

Tests:

Before testing you'll want to apply the change suggested in this comment: https://github.com/duckduckgo/macos-browser/pull/1562/files#r1313326009

Filter your Xcode console by "NetP Debug"

Test 1: Easter Egg User + Waitlist Ended

In this test we're going to check that easter egg users have Network Protection enabled

Restart Network Protection
Enable NetP using an easter egg code.
Set the Waitlist Enabled override to ON and the Waitlist Active override to OFF.
Make sure Network Protection is visible in the more menu.
Make sure there are no messages in the Xcode console (using the filter from above).

Test 2: Waitlist User + Waitlist Ongoing

In this test we're going to check that Waitlist users can see Network Protection when the waitlist is enabled.

Change this line to return true.
Restart Network Protection
Set the Waitlist Enabled override to ON and the Waitlist Active override to ON.
Make sure Network Protection is visible in the more menu.
Make sure there are no messages in the Xcode console (using the filter from above).

Test 3: Waitlist User + Waitlist Ended

In this test we're going to check that Waitlist users can see Network Protection when the waitlist is enabled.

Change this line to return true.
Restart Network Protection
Set the Waitlist Enabled override to ON and the Waitlist Active override to OFF.
Make sure Network Protection is NOT visible in the more menu.
Check that you have a message in the console stating NetP was disabled.

Internal references:

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

…itlist-beta-disable-feature-flag-2

...etworkProtection/AppAndExtensionAndAgentTargets/UserDefaults+NetworkProtectionWaitlist.swift

diegoreymendez · 2023-08-29T22:27:38Z

...etworkProtection/AppAndExtensionAndAgentTargets/UserDefaults+NetworkProtectionWaitlist.swift

+protocol WaitlistBetaOverriding {
+    var waitlistActive: WaitlistOverride { get }
+    var waitlistEnabled: WaitlistOverride { get }
+}


Protocol for obtaining the overrides in a way that we can mock this easily for testing.

diegoreymendez · 2023-08-29T22:28:15Z

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionWaitlistMenu.swift

+
+    // MARK: - UI Additions
+
+    private func waitlistOFFAlert() -> NSAlert {


Overriding the subfeatures to OFF will be "dangerous" to some degree when we start disabling NetP.

diegoreymendez · 2023-08-29T22:28:57Z

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift

+    private var isWaitlistEnabled: Bool {
+        switch featureOverrides.waitlistEnabled {
+        case .useRemoteValue:
+            return privacyConfigurationManager.privacyConfig.isSubfeatureEnabled(NetworkProtectionSubfeature.waitlist)
+        case .on:
+            return true
+        case .off:
+            return false
+        }
+    }


This is the gist of the overriding mechanism I'm proposing, where the client app takes care of overriding the flags.

I like it 👍

diegoreymendez · 2023-08-29T22:29:41Z

...etworkProtection/AppAndExtensionAndAgentTargets/UserDefaults+NetworkProtectionWaitlist.swift

+    case on
+    case off
+
+    static let `default`: WaitlistOverride = .on


This default should change to .remote once we start the waitlist in our backend.

diegoreymendez · 2023-08-29T22:30:41Z

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift

+    private func disableNetworkProtectionForExternalUsers() {
+#if DEBUG
+        if internalUserDecider.isInternalUser {
+            print("NetP Debug: Internal user detected. Network Protection is still enabled.")
+        } else {
+            print("NetP Debug: Network Protection was disabled.")
+        }
+#endif
+    }
+}


No deleting anything yet to avoid breakage. I think we should review the exact conditions that would trigger this.

I'm going to open a PR to turn the test feature flag on, so that this will never technically run for users for a few months. That gives us time to decide how to handle internal users properly.

Follow up to my last comment, it's important to note that we should only include this when the bundled version of the config in the app includes the enabled flag. Otherwise, any F&F users who launch the browser will have NetP state removed, because the remote config won't have been fetched yet.

We could probably merge the privacy config PR on Friday, and then update the embedded copy of the config file as a part of this PR to solve that.

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift

DuckDuckGo/Menus/MainMenu.storyboard

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift

samsymons

LGTM! I'll refine the conditions and wire up the waitlist state as a part of my PR.

github-actions · 2023-09-01T19:31:17Z

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

Generated by 🚫 dangerJS against d72eac0

Task/Issue URL: https://app.asana.com/0/0/1205381133187308/f iOS PR: duckduckgo/iOS#1967 macOS PR: duckduckgo/macos-browser#1562 What kind of version bump will this require?: Minor ## Description Adds the waitlist remote flags. --------- * Add the Network Protection feature and waitlist subfeature. * Rename NetworkProtectionFeatureVisibility to NetworkProtectionFeatureActivation. * Hardcode NetP on. * Adds the waitlistBetaActive remote subfeature * Removes an override --------- Co-authored-by: Sam Symons <[email protected]>

Task/Issue URL: https://app.asana.com/0/0/1205381133187310/f ## Related PRs BSK PR: duckduckgo/BrowserServicesKit#483 macOS PR: duckduckgo/macos-browser#1562 ## Description Integrates some of the latest changes from BSK. -------- Co-authored-by: Sam Symons <[email protected]>

# By Diego Rey Mendez # Via GitHub * develop: Implement waitlistBetaActive logic (without actually resetting) (#1562) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

# By Alexey Martemyanov (5) and others # Via GitHub (1) and Tomas Strba (1) * develop: DBP Debug UI (#1587) Bump version to 1.55.0 (57) Update embedded files Auto start DBP scheduler (#1586) Add broker updater mechanism (#1580) Update develop with Bitwarden fix (#1585) fix outlook popups loading; set popup window mode to match Safari (#1564) remove AppActivityMonitor and related unused code (#1582) Popovers positioning (#1574) Version 1.54.1 xcode 15 synthesized color constants fixes (#1583) Fixes issue with password saving when using Bitwarden (#1584) Remove the incremental test pixel sender (#1578) Implement waitlistBetaActive logic (without actually resetting) (#1562) Lock Form fill when autofill is locked (#1559) show alert when user tries to open file in AppStore version (#1494) DBP UI Middleware (#1553) Dashboard & Profile Creation (#1567) roll out the bookmarks bar experiment (#1572) Open duck player for youtube same-document navigation (#1560) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Common/Localizables/UserText.swift # DuckDuckGo/NavigationBar/PinningManager.swift # DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift # DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift # UnitTests/Menus/MoreOptionsMenuTests.swift

diegoreymendez added 4 commits August 29, 2023 11:46

Adds a basic NetP waitlist beta ended override

2af200c

WIP

64c1ceb

Improves the override menu for waitlistBetaActive

8d9254f

Merge branch 'diego/waitlist-beta-disable-feature-flag' into diego/wa…

68383c2

…itlist-beta-disable-feature-flag-2

diegoreymendez self-assigned this Aug 29, 2023

diegoreymendez mentioned this pull request Aug 29, 2023

Adds the networkProtection remote subfeatures duckduckgo/BrowserServicesKit#483

Merged

diegoreymendez added 3 commits August 29, 2023 23:37

Updates BSK to point at the commit

ebc8014

Adds a file that was missing by mistake

ae6f9cf

Adds waitlist enabled

79cf36b

diegoreymendez commented Aug 29, 2023

View reviewed changes

...etworkProtection/AppAndExtensionAndAgentTargets/UserDefaults+NetworkProtectionWaitlist.swift Outdated Show resolved Hide resolved

diegoreymendez commented Aug 29, 2023

View reviewed changes

diegoreymendez changed the title ~~Diego/waitlist beta disable feature flag 2~~ Implement waitlistBetaActive logic (without actually resetting) Aug 29, 2023

diegoreymendez marked this pull request as ready for review August 29, 2023 22:31

diegoreymendez requested a review from samsymons August 29, 2023 22:31

diegoreymendez added 3 commits August 30, 2023 01:59

Applies some small fixes

0a87930

Removes a menu item added by mistake

2b831f8

Fixes some issues introduced by mistake

954ecc1

diegoreymendez commented Aug 30, 2023

View reviewed changes

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift Outdated Show resolved Hide resolved

diegoreymendez mentioned this pull request Aug 30, 2023

Integrates some of the latest BSK changes duckduckgo/iOS#1967

Merged

samsymons reviewed Sep 1, 2023

View reviewed changes

DuckDuckGo/Menus/MainMenu.storyboard Outdated Show resolved Hide resolved

samsymons reviewed Sep 1, 2023

View reviewed changes

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift Outdated Show resolved Hide resolved

Makes some review changes

1cc9c7b

diegoreymendez commented Sep 1, 2023

View reviewed changes

DuckDuckGo/Waitlist/NetworkProtectionFeatureVisibility.swift Outdated Show resolved Hide resolved

diegoreymendez added 3 commits September 1, 2023 19:41

Cleans up some code based on review discussion

5c541d1

Rolls back some unintentional changes

13afd27

Merges the latest from develop

80c418b

diegoreymendez added 3 commits September 1, 2023 19:48

Updates BSK

6482aef

Some additional cleanup

718ad38

Updates BSK

d191fa5

samsymons approved these changes Sep 1, 2023

View reviewed changes

Fixes the unit tests and some conditions

6627fd4

Updates BSK to 75.3.1

d72eac0

diegoreymendez merged commit 3cbb529 into develop Sep 3, 2023

diegoreymendez deleted the diego/waitlist-beta-disable-feature-flag-2 branch September 3, 2023 06:26


		// MARK: - UI Additions

		private func waitlistOFFAlert() -> NSAlert {

Implement waitlistBetaActive logic (without actually resetting) #1562

Implement waitlistBetaActive logic (without actually resetting) #1562

Uh oh!

Conversation

diegoreymendez commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related PRs

Description

Tests:

Test 1: Easter Egg User + Waitlist Ended

Test 2: Waitlist User + Waitlist Ongoing

Test 3: Waitlist User + Waitlist Ended

Internal references:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samsymons left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

diegoreymendez commented Aug 29, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading