-
Notifications
You must be signed in to change notification settings - Fork 14
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 additonal error handling to DBP database and related classes #2384
Changes from all commits
fb20e20
b4e525d
7e9b11c
f582ffe
16b5efe
499b461
e4469ce
edb95e9
2a4d0cb
fa24c7e
3319885
6fb5b8a
40f3dff
dcd1756
70a518a
8dc1c2e
8a9f526
0022fc8
827f0af
8b6bc30
df2e53f
5cacc1f
676a402
8ff1bbb
0573d21
387fad0
839e06e
21262aa
468cd25
ff20faf
47f3f31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ public extension Notification.Name { | |
final class DBPHomeViewController: NSViewController { | ||
private var presentedWindowController: NSWindowController? | ||
private let dataBrokerProtectionManager: DataBrokerProtectionManager | ||
private let pixelHandler: EventMapping<DataBrokerProtectionPixels> = DataBrokerProtectionPixelsHandler() | ||
|
||
lazy var dataBrokerProtectionViewController: DataBrokerProtectionViewController = { | ||
let privacyConfigurationManager = PrivacyFeatures.contentBlocking.privacyConfigurationManager | ||
|
@@ -83,9 +84,14 @@ final class DBPHomeViewController: NSViewController { | |
attachDataBrokerContainerView() | ||
} | ||
|
||
if dataBrokerProtectionManager.dataManager.fetchProfile() != nil { | ||
let dbpDateStore = DefaultWaitlistActivationDateStore(source: .dbp) | ||
dbpDateStore.updateLastActiveDate() | ||
do { | ||
if try dataBrokerProtectionManager.dataManager.fetchProfile() != nil { | ||
let dbpDateStore = DefaultWaitlistActivationDateStore(source: .dbp) | ||
dbpDateStore.updateLastActiveDate() | ||
} | ||
samsymons marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} catch { | ||
os_log("DBPHomeViewController error: viewDidLoad, error: %{public}@", log: .error, error.localizedDescription) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably worth us renaming these log objects to something that specifies that they're DBP related at some point. At first glance I thought this wasn't using a log object at all and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah definitely, outside of the DBP package it's completely confusing |
||
pixelHandler.fire(.generalError(error: error, functionOccurredIn: "DBPHomeViewController.viewDidLoad")) | ||
} | ||
} | ||
|
||
|
@@ -146,6 +152,11 @@ public class DataBrokerProtectionPixelsHandler: EventMapping<DataBrokerProtectio | |
switch event { | ||
case .error(let error, _): | ||
Pixel.fire(.debug(event: .pixelKitEvent(event), error: error)) | ||
case .generalError(let error, _), | ||
.secureVaultInitError(let error), | ||
.secureVaultError(let error): | ||
// We can't use .debug directly because it modifies the pixel name and clobbers the params | ||
Pixel.fire(.pixelKitEvent(DebugEvent(event, error: error))) | ||
case .ipcServerOptOutAllBrokersCompletion(error: let error), | ||
.ipcServerScanAllBrokersCompletion(error: let error), | ||
.ipcServerRunQueuedOperationsCompletion(error: let error): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,11 @@ struct DataBrokerProtectionFeatureDisabler: DataBrokerProtectionFeatureDisabling | |
|
||
scheduler.disableLoginItem() | ||
|
||
dataManager.removeAllData() | ||
do { | ||
try dataManager.removeAllData() | ||
} catch { | ||
os_log("DataBrokerProtectionFeatureDisabler error: disableAndDelete, error: %{public}@", log: .error, error.localizedDescription) | ||
} | ||
Comment on lines
+49
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forget, do we need a pixel here? (feels like if we started failing to remove data broker data, that could be a big deal and we would definitely want to know about it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have one for if removing all data failures certainly. The actual method that is called that can fail here is database.deleteProfileData, which will fire thusly:
So then I think it comes down to two questions:
|
||
|
||
DataBrokerProtectionLoginItemPixels.fire(pixel: .dataBrokerDisableAndDeleteDaily, frequency: .dailyOnly) | ||
NotificationCenter.default.post(name: .dbpWasDisabled, object: nil) | ||
|
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.
Ah this again, Brindy has a task to fix this - we can leave it this way until that happens tbh.