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

Add global data collection switch. #1219

Merged
merged 4 commits into from May 14, 2018
Merged

Conversation

@ryanwilson
Copy link
Member

@ryanwilson ryanwilson commented May 3, 2018

NOTE: This shouldn't be public until all SDKs conform to the setting.

This allows developers to enable/disable all collection at compile time and override it at runtime.

return YES;

// If none of above exists, we default to the global switch that comes from FIRApp.
return self.isGlobalAutomaticDataCollectionEnabled;
Copy link
Contributor

@chliangGoogle chliangGoogle May 9, 2018

Choose a reason for hiding this comment

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

Before we make sure it's default true if no special action happened on the user side, so we make sure we are not breaking app's existing behavior on fetching instanceID, and other activities.

Is this global boolean value also serves the same purpose, that if developer doesn't do anything, it will be default ture?

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Yep, if nothing is set in the plist or at runtime, the default is true and we have unit tests to confirm that.

@@ -227,6 +232,7 @@ - (void)deleteApp:(FIRAppVoidBoolCallback)completion {
if (sAllApps && sAllApps[self.name]) {
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

self.name can be nil. We should check it before accessing the dictionary.

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

We throw an exception if a user tries to configure the app with a nil or empty name, which should catch this. If you're still concerned, we can add it in a separate PR since it's unrelated to this change.

Choose a reason for hiding this comment

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

ok. I think we should be careful of accessing dictionary when the provided key is nil. I'm concerned that self.name might be nil at some point during the entire app life cycle.

NSNumber *collectionEnabledPlistValue = [[self class] readDataCollectionSwitchFromPlist];
if (collectionEnabledPlistValue) {
return [collectionEnabledPlistValue boolValue];
} else {
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

nit: no need for else

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

#pragma mark - Reading From Plist & User Defaults

/**
* A wrapper to clear the data collection switch from the standard NSUserDefaults for easier testing
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

nit: Clears the data collection ...

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

}

/**
* A wrapper to read the data collection switch from the standard NSUserDefaults for easier testing
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

nit: Reads the data collection...
Please remove "A wrapper to" here and below

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

id collectionEnabledDefaultsObject = [[NSUserDefaults standardUserDefaults] objectForKey:key];
if ([collectionEnabledDefaultsObject isKindOfClass:[NSNumber class]]) {
return collectionEnabledDefaultsObject;
} else {
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

nit: no need for else

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
// Read the data from the `Info.plist`, only assign it if it's there and an NSNumber.
id plistValue = [[NSBundle mainBundle] objectForInfoDictionaryKey:kFIRGlobalAppDataCollectionEnabledPlistKey];
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

ident +4 from previous line

Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Not sure what happened here but I think this was out of date - it's split onto two lines now, even indenting +4 leaves it over 100 chars.

* your app's Info.plist. This value is persisted across runs of the app so that it
* can be set once when users have consented to collection.
*/
@property(nonatomic, readwrite, getter=isAutomaticDataCollectionEnabled) BOOL automaticDataCollectionEnabled;
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

nit: ident +4

/**
* Gets or sets whether automatic data collection is enabled for all products.
* Defaults to YES unless FirebaseAutomaticDataCollectionEnabled is set to NO in
* your app's Info.plist. This value is persisted across runs of the app so that it
Copy link

@baolocdo baolocdo May 9, 2018

Choose a reason for hiding this comment

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

nit: fits previous line. Please fill the whole row up to 100 chars per line.

Copy link
Member Author

@ryanwilson ryanwilson left a comment

Addressed the comments, @baolocdo there were a few you added on an old iteration, apologies if I sent you the wrong link. Everything should be properly formatted now.

@@ -227,6 +232,7 @@ - (void)deleteApp:(FIRAppVoidBoolCallback)completion {
if (sAllApps && sAllApps[self.name]) {
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

We throw an exception if a user tries to configure the app with a nil or empty name, which should catch this. If you're still concerned, we can add it in a separate PR since it's unrelated to this change.

NSNumber *collectionEnabledPlistValue = [[self class] readDataCollectionSwitchFromPlist];
if (collectionEnabledPlistValue) {
return [collectionEnabledPlistValue boolValue];
} else {
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

#pragma mark - Reading From Plist & User Defaults

/**
* A wrapper to clear the data collection switch from the standard NSUserDefaults for easier testing
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

}

/**
* A wrapper to read the data collection switch from the standard NSUserDefaults for easier testing
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

id collectionEnabledDefaultsObject = [[NSUserDefaults standardUserDefaults] objectForKey:key];
if ([collectionEnabledDefaultsObject isKindOfClass:[NSNumber class]]) {
return collectionEnabledDefaultsObject;
} else {
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Done.

return YES;

// If none of above exists, we default to the global switch that comes from FIRApp.
return self.isGlobalAutomaticDataCollectionEnabled;
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Yep, if nothing is set in the plist or at runtime, the default is true and we have unit tests to confirm that.

static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
// Read the data from the `Info.plist`, only assign it if it's there and an NSNumber.
id plistValue = [[NSBundle mainBundle] objectForInfoDictionaryKey:kFIRGlobalAppDataCollectionEnabledPlistKey];
Copy link
Member Author

@ryanwilson ryanwilson May 10, 2018

Choose a reason for hiding this comment

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

Not sure what happened here but I think this was out of date - it's split onto two lines now, even indenting +4 leaves it over 100 chars.

@ryanwilson ryanwilson merged commit 08f447c into master May 14, 2018
2 checks passed
@ryanwilson ryanwilson deleted the rw-data-collection-switch branch May 18, 2018
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this issue Jun 6, 2018
* Addition of global data collection switch.
* Added Messaging conformance to data switch.
  Also formatted code.
* Move data collection flag internal until all SDKs conform to it.
* Formatting in response to code review.
@firebase firebase locked and limited conversation to collaborators Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants