-
Notifications
You must be signed in to change notification settings - Fork 10
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
Additional PIR opt out confirmation pixels #3119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @THISISDINOSAUR. Changes look great, made one suggestion. Tested and DB is migrated and pixel fired after changing code locally. 👍🏼
let preferredRunDate: Date? | ||
let historyEvents: [HistoryEvent] | ||
let lastRunDate: Date? | ||
|
||
// This was added in a later DB migration (V4), so will be nil for older entries submitted before the migration | ||
let submittedSuccessfullyDate: Date? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏼
- Update the DB to indicate which pixels have been newly fired | ||
|
||
Because submittedSuccessfullyDate will be nil for data that existed before the migration | ||
the pixels won't fire for old data, which is the behaviour we want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice documentation 👍🏼
forBrokerId: optOutJob.brokerId, | ||
profileQueryId: optOutJob.profileQueryId, | ||
extractedProfileId: extractedProfileID) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made an attempt locally at reducing duplication here, but due to the slight differences at numerous points, it was not much of an improvement, so I think fine as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this duplication bothers me but I couldn't find an alternative that I was happier with
throw DataBrokerProtectionDatabaseErrors.elementNotFound | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could reduce duplication here with a helper method:
func updateOptOutField<T>(_ fieldUpdate: @escaping (inout OptOutDB, T) -> Void,
value: T,
forBrokerId brokerId: Int64,
profileQueryId: Int64,
extractedProfileId: Int64) throws {
try db.write { db in
if var optOut = try OptOutDB.fetchOne(db, key: [
OptOutDB.Columns.brokerId.name: brokerId,
OptOutDB.Columns.profileQueryId.name: profileQueryId,
OptOutDB.Columns.extractedProfileId.name: extractedProfileId]) {
fieldUpdate(&optOut, value)
try optOut.update(db)
} else {
throw DataBrokerProtectionDatabaseErrors.elementNotFound
}
}
}
```
Which could then use like:
```
func updateSubmittedSuccessfullyDate(_ date: Date?,
forBrokerId brokerId: Int64,
profileQueryId: Int64,
extractedProfileId: Int64) throws {
try updateOptOutField({ $0.submittedSuccessfullyDate = $1 }, value: date, forBrokerId: brokerId, profileQueryId: profileQueryId, extractedProfileId: extractedProfileId)
}
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on this since it's a departure from what's there already, but I certainly have existing gripes with what's there already. My problem I think is I don't have a clear direction in mind for this class
If I make this change I think I want to change some more changes that depart from the rest of the class, at least change the function names to clearly mention optOuts. I think currently I'm leaning towards making those changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay changed my mind on the names, if I change these names I think I should change all of them, and at that point I a) don't think I should do it now, but b) think it should be a separate PR.
So I'm gonna go ahead and just reduce the duplication, but will do for the existing opt out fields too
Thanks a lot for providing the snippet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @THISISDINOSAUR. Agree on reasoning re: name changes. LGTM!
…iption * main: (25 commits) Bump version to 1.103.0 (249) Bump version to 1.103.0 (248) Resolving automatic update edge cases (#3142) PIR Time-Based Pixel: 24 Opt-Out Request Success Rate (#2942) Freemium Local Package and Freemium State Implementation (#3118) Bump rexml from 3.3.3 to 3.3.6 (#3141) Filter out inaccessible tasks from release (#3133) Bump rexml from 3.2.9 to 3.3.3 (#3139) Bump version to 1.103.0 (247) Specify secrets for pr.yml where called from another workflow (#3137) Fix secrets usage in CI workflows (#3134) Adding info about how we improve our products to the README.md (#3135) Add Freemium PIR Feature Flag (#3129) Release notes page loading fix + Adding close button to the upgrade notification (#3094) App Configuration app group (#3132) Use code signing in CI tests workflows (#3125) 'track' rephrased to 'anonymously track' (#3121) Additional PIR opt out confirmation pixels (#3119) Logging refactoring #1 Subscription and Content Blocking (#3091) Update RemoteMessagingDebugMenu to work on a private-queue context (#3120) ...
Task/Issue URL: https://app.asana.com/0/0/1207733131726022/f
Tech Design URL:
CC:
Description:
I've linked to the implementation task above, but more useful is the proposal task here: https://app.asana.com/0/0/1207999607996551/f
Adds new pixels that indicate how long it takes us to confirm successful opt outs
This involves adding new fields to the DB, so the migration also needs to be tested
Also moves monitoring pixel firing up the stack to the agent manager where IMO it belongs, so special attention needs to be made that nothing is broken there.
Steps to test this PR:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation