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

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

Conversation

aataraxiaa
Copy link
Contributor

@aataraxiaa aataraxiaa commented Sep 3, 2024

Task/Issue URL: https://app.asana.com/0/1201621853593513/1208197280287450/f
CC: @miasma13

Description: As an outcome of this discussion, I have refactored FreemiumPIRUserStateManager to remove it’s dependency on AccountManager. This includes the removal of the isActiveUser computed property. The main reason for this change is as follows:

  • In the app we rely on a single instance of AccountManager which is used throughout the app and accessed via the “singleton” Subscription instance, which is a property on the app delegate. By injecting an instance of AccountManager into FreemiumPIRUserStateManager, we introduce a new possible point of error. Injecting a new instance of AccountManager into FreemiumPIRUserStateManager, as opposed to the desired single app instance would result in error. This is particularly concerning when it comes to how we make decisions about running PIR opt-outs or not.

  • By removing this injection of AccountManager, clients of FreemiumPIRUserStateManager are less likely to inject an incorrect instance of AccountManager. In places where FreemiumPIRUserStateManager is used to make decisions, we usually have an instance of Subscription or AccountManager defined locally, and so it can be used in combination with FreemiumPIRUserStateManager to make decisions.

  • The Freemium package is now simpler, with no dependencies, and simply manages stored Freemium state.

Testing Prerequisites

  1. Sign out of Privacy Pro (Settings -> PP -> Remove from this device)
  2. Ensure you are an internal user
  3. Set Freemium PIR State TRUE (Debug Menu -> Freemium -> Set Freemium PIR Onboarded State TRUE)
  4. Set breakpoint # 1 at DataBrokerProtectionAgentManager line 205
  5. Set breakpoint # 2 at DataBrokerProtectionAgentManager line 207

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

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

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

  1. Launch background scheduled operation via Debug Menu -> PIR -> Operations…-> Run Queued operations
  2. Ensure breakpoint # 1 is NOT hit
  3. Ensure breakpoint # 2 is hit

Test 3 - Checking background scan with Subscription DOES perform opt-outs

  1. Sign into Privacy Pro (using your already subscribed email if you have one)
  2. Launch background scheduled operation via Debug Menu -> PIR -> Operations…-> Run Queued operations
  3. Ensure breakpoint # 1 is hit
  4. Ensure breakpoint # 2 is NOT hit

Definition of Done:

Internal references:

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

Copy link
Contributor

@THISISDINOSAUR THISISDINOSAUR left a comment

Choose a reason for hiding this comment

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

LGTM!

....
....
....look I'm just as shocked as you are I have nothing else to add

@aataraxiaa
Copy link
Contributor Author

LGTM!

.... .... ....look I'm just as shocked as you are I have nothing else to add

😄 I CMD-R’d a few times until I accepted reality. Thanks for the review. I just updated one test expectation (which was incorrect and failing), and now merging to feature branch.

@aataraxiaa aataraxiaa merged commit 14a2386 into pete/feature/pir-freemium Sep 4, 2024
15 of 18 checks passed
@aataraxiaa aataraxiaa deleted the pete/feature/pir-freemium-state-improvement branch September 4, 2024 08:54
aataraxiaa added a commit that referenced this pull request Sep 10, 2024
…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 a commit that referenced this pull request Sep 18, 2024
…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 a commit that referenced this pull request Oct 8, 2024
…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.
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