Freemium PIR: Prerequisite Checks and Scheduled Scans #3178

aataraxiaa · 2024-08-30T13:48:01Z

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

Description: This PR (to a feature branch) implements two Freemium PIR aspects:

When checking PIR prerequisites (see GateKeeper.. AgentStopper… types), it accounts for Freemium PIR users
When running scheduled operations, it only performs scans, and not opt-outs

These aspects are implemented by:

Utilizing the FreemiumPIRUserState type to check freemium state (if user has onboarded to freemium, and has no subscription, they are freemium)
Checking this state in our gatekeeper and stopper types
Adding a new method to our queue manager to run scheduled scans (as opposed to scheduled scans AND opt-outs)
Adding a new .scheduledScan operation type (and renaming an existing one)
Adding tests

Testing Prerequisites

Sign out of Privacy Pro (Settings -> PP -> Remove from this device)
Ensure you are an internal user
Set Freemium PIR State TRUE (Debug Menu -> Freemium -> Set Freemium PIR Onboarded State TRUE)
Set breakpoint # 1 at DataBrokerProtectionQueueManager line 141
Set breakpoint # 2 at DataBrokerProtectionQueueManager line 150

Steps to test this PR:
Test 1 - Checking initial scan works

Launch Freemium PIR by going to the More Options top right menu, and selecting “Personal Information Scan”
Fill in profile details, perform a scan
Attach debugger to background process while scan is running
Ensure it completes

Test 1 - Checking background scan does not perform opt-outs

Launch background scheduled operation via Debug Menu -> PIR -> Operations…-> Run Queued operations
Ensure breakpoint # 1 is NOT hit
Ensure breakpoint # 2 is hit
After a while, also check the Debug DB browser events table, and make sure there are no opt-out events

Definition of Done:

Does this PR satisfy our Definition of Done?

—

Internal references:

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

…date some mocks.

github-actions · 2024-08-30T13:49:37Z

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

Generated by 🚫 dangerJS against 9aa5171

aataraxiaa · 2024-08-30T13:59:03Z

DuckDuckGo/DBP/DataBrokerProtectionFeatureGatekeeper.swift

    func disableAndDeleteForAllUsers()
-    func isPrivacyProEnabled() -> Bool


Methods unused, removing.

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

THISISDINOSAUR

Mostly looks great, love some of the tangental improvements to the code base this includes. Just one thing, the description says "Adding debug menu commands and the associated hierarchy of XPC calls" but I don't see the xpc calls, I might be missing it though

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

...taBrokerProtection/Sources/DataBrokerProtection/Utils/DataBrokerProtectionAgentStopper.swift

...es/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerExecutionConfigTests.swift

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

.../DataBrokerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerExecutionConfig.swift

THISISDINOSAUR · 2024-09-02T16:14:05Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

@@ -194,23 +222,23 @@ extension DataBrokerProtectionAgentManager: DataBrokerProtectionAgentAppEvents {

        userNotificationService.requestNotificationPermission()
        fireMonitoringPixels()
-        queueManager.startImmediateOperationsIfPermitted(showWebView: false, operationDependencies: operationDependencies) { [weak self] errors in
+        queueManager.startImmediateScanOperationsIfPermitted(showWebView: false, operationDependencies: operationDependencies) { [weak self] errors in


We discussed over MM, but posting here for prosperity:

Previously (i.e. prior to this PR), things higher up the stack (agent manager, queueManager) would tell stuff lower (e.g. DataBrokerOperation) to run "immediate operations" when a profile is saved. I.e. it was non specific about what immediate operations included

Now, they explicitly tell it only to do scans (and are now specific for the kinds of scans)

This is enforced through DataBrokerOperation.filterAndSortOperationsData()

Ignoring freemium, this actually represents mostly no change in practice for subscribed users, because for manual scans we never actually ran opt outs immedtiely anyway (which I think might actually be an implementation mistake, but seems to be the observed behavior)

So now instead AgentManager is explicit about what immediate operations includes, which I think is an improvement and where the decision should be made.

But the main reason I mention it is because I think that original behavior is a bug and we do want opt outs to run immediately after the scans, so it's worth discussing if we want to codify it like this.

I think we do, because of how I think we should fix that bug, which is on the completion for the manual scans, in agent manager, call the thing on queuemanager, rather than waiting for the scheduler to do it. But there's a slight asterisk of we'd need to move something around in how the QueueManager handles completions. atm it would break some stuff cos it calls the completion before clearing errors

The alternative approach is to somehow add the opt outs to the queue as we make them. If it was easy to do I'd seriously consider doing it, but as it would be much much harder and be a big increase in implementation complexity lower down the stack solely for making AgentManager slightly clearer.

Thanks for capturing this here @THISISDINOSAUR . You set a really good standard for prosperity, it’s great.

aataraxiaa · 2024-09-02T16:47:31Z

Mostly looks great, love some of the tangental improvements to the code base this includes. Just one thing, the description says "Adding debug menu commands and the associated hierarchy of XPC calls" but I don't see the xpc calls, I might be missing it though

Thanks for the review @THISISDINOSAUR , feedback addressed (commit has mostly renaming changes). Apologies on the inaccurate description - I had a change included that I removed before opening the PR, but I forgot to remove that line. Description updated!

THISISDINOSAUR

LGTM! I have opt out jobs in my DB (but no actual signs of them being run), I assume this is expected?

IMO this isn't ideal, but then the way we create those jobs I think is not ideal anyway and well beyond the scope of this PR to change

aataraxiaa · 2024-09-03T07:31:31Z

LGTM! I have opt out jobs in my DB (but no actual signs of them being run), I assume this is expected?

IMO this isn't ideal, but then the way we create those jobs I think is not ideal anyway and well beyond the scope of this PR to change

Thanks @THISISDINOSAUR . Yes, we still create opt-out jobs as if a user subscribes we want to then run them. However, when the user is Freemium, they are not run.

@miasma13

…ountManager (#3197) Task/Issue URL: https://app.asana.com/0/1201621853593513/1208197280287450/f CC: @miasma13 **Description**: As an outcome of [this](#3178 (comment)) discussion, I have refactored `FreemiumPIRUserStateManager` to remove it’s dependency on `AccountManager`. This includes the removal of the `isActiveUser` computed property.

Task/Issue URL: https://app.asana.com/0/1206488453854252/1208052452067290/f **Description**: This PR (_to a feature branch_) implements two Freemium PIR aspects: 1. When checking PIR prerequisites (see `GateKeeper..` `AgentStopper…` types), it accounts for Freemium PIR users 2. When running scheduled operations, it only performs scans, and not opt-outs

@miasma13

…ountManager (#3197) Task/Issue URL: https://app.asana.com/0/1201621853593513/1208197280287450/f CC: @miasma13 **Description**: As an outcome of [this](#3178 (comment)) discussion, I have refactored `FreemiumPIRUserStateManager` to remove it’s dependency on `AccountManager`. This includes the removal of the `isActiveUser` computed property.

Task/Issue URL: https://app.asana.com/0/1206488453854252/1208052452067290/f **Description**: This PR (_to a feature branch_) implements two Freemium PIR aspects: 1. When checking PIR prerequisites (see `GateKeeper..` `AgentStopper…` types), it accounts for Freemium PIR users 2. When running scheduled operations, it only performs scans, and not opt-outs

@miasma13

…ountManager (#3197) Task/Issue URL: https://app.asana.com/0/1201621853593513/1208197280287450/f CC: @miasma13 **Description**: As an outcome of [this](#3178 (comment)) discussion, I have refactored `FreemiumPIRUserStateManager` to remove it’s dependency on `AccountManager`. This includes the removal of the `isActiveUser` computed property.

Task/Issue URL: https://app.asana.com/0/1206488453854252/1208052452067290/f **Description**: This PR (_to a feature branch_) implements two Freemium PIR aspects: 1. When checking PIR prerequisites (see `GateKeeper..` `AgentStopper…` types), it accounts for Freemium PIR users 2. When running scheduled operations, it only performs scans, and not opt-outs

@miasma13

…ountManager (#3197) Task/Issue URL: https://app.asana.com/0/1201621853593513/1208197280287450/f CC: @miasma13 **Description**: As an outcome of [this](#3178 (comment)) discussion, I have refactored `FreemiumPIRUserStateManager` to remove it’s dependency on `AccountManager`. This includes the removal of the `isActiveUser` computed property.

aataraxiaa added 7 commits August 30, 2024 12:03

Update Gatekeeper and AgentStopper to consider Freemium user

f7af82f

Inject dependencies and update mock

af731da

Add new scheduledScan operation type, rename scan to manualScan

7389650

Distinguish between freemium and subscription scheduled operations

62b0dcd

Add AgentStopper and Gatekeeper logic and tests

1cfef9b

Add debug menu command for scheduled scans, update call hierarchy. Up…

9839a83

…date some mocks.

Add some more DataBrokerProtectionQueueManagerTests tests

8d17694

aataraxiaa added 2 commits August 30, 2024 15:24

Add Freemium debug menu, remove PIR debug menu items, fix Swiftlint

b204562

Fix mock

a6a7040

aataraxiaa commented Aug 30, 2024

View reviewed changes

aataraxiaa marked this pull request as ready for review August 30, 2024 14:40

aataraxiaa requested a review from THISISDINOSAUR August 30, 2024 14:47

Improve Stopper logic and add more tests

c554af9

aataraxiaa commented Sep 1, 2024

View reviewed changes

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift Outdated Show resolved Hide resolved

THISISDINOSAUR reviewed Sep 2, 2024

View reviewed changes

PR Feedback (Mostly renaming)

9aa5171

THISISDINOSAUR approved these changes Sep 2, 2024

View reviewed changes

aataraxiaa merged commit 83b54ff into pete/feature/pir-freemium Sep 3, 2024
17 of 18 checks passed

aataraxiaa deleted the pete/feature/pir-freemium-scan-behavior branch September 3, 2024 07:50

aataraxiaa restored the pete/feature/pir-freemium-scan-behavior branch September 3, 2024 12:22

aataraxiaa deleted the pete/feature/pir-freemium-scan-behavior branch September 3, 2024 12:23

aataraxiaa mentioned this pull request Sep 3, 2024

Freemium PIR: Refactor Freemium PIR State to Remove Dependency on AccountManager #3197

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Freemium PIR: Prerequisite Checks and Scheduled Scans #3178

Freemium PIR: Prerequisite Checks and Scheduled Scans #3178

aataraxiaa commented Aug 30, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024 •

edited

Loading

aataraxiaa Aug 30, 2024

THISISDINOSAUR left a comment

THISISDINOSAUR Sep 2, 2024

aataraxiaa Sep 2, 2024

aataraxiaa commented Sep 2, 2024

THISISDINOSAUR left a comment

aataraxiaa commented Sep 3, 2024

		func disableAndDeleteForAllUsers()
		func isPrivacyProEnabled() -> Bool

Freemium PIR: Prerequisite Checks and Scheduled Scans #3178

Freemium PIR: Prerequisite Checks and Scheduled Scans #3178

Conversation

aataraxiaa commented Aug 30, 2024 • edited Loading

Internal references:

github-actions bot commented Aug 30, 2024 • edited Loading

aataraxiaa Aug 30, 2024

Choose a reason for hiding this comment

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

THISISDINOSAUR Sep 2, 2024

Choose a reason for hiding this comment

aataraxiaa Sep 2, 2024

Choose a reason for hiding this comment

aataraxiaa commented Sep 2, 2024

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

aataraxiaa commented Sep 3, 2024

aataraxiaa commented Aug 30, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024 •

edited

Loading