Skip to content
This repository has been archived by the owner on Apr 2, 2021. It is now read-only.

Expose API for CCPA compliance and iOS 14 ATE #804

Merged
merged 5 commits into from Nov 23, 2020

Conversation

dreamolight
Copy link
Contributor

What is this commit about:

  • Updated FB iOS SDK to v8.0 which has the necessary updates for iOS 14
  • Updated FB Android SDK to v7.1.0+ which the API for CCPA compliance
  • Provided setDataProcessingOptions API for CCPA compliance for both iOS and Android
  • Provided access to getAdvertiserTrackingEnabled and setAdvertiserTrackingEnabled which are necessary for iOS 14

Test Plan:

  1. Create sample project
  2. Integrate the SDK
  3. Add below code to verify the new API works
<Button
            title="ATE"
            onPress={() =>
            Settings.setAdvertiserTrackingEnabled(true).then( (result) => {
              console.log("is success: " + result);
              Settings.getAdvertiserTrackingEnabled().then((result) => {
                console.log("ATE: " + result);
                Settings.setAdvertiserTrackingEnabled(false).then( (result) => {
                console.log("is success: " + result);
                Settings.getAdvertiserTrackingEnabled().then((result) => {
                  console.log("ATE: " + result)
                });
              })
              });
            })
          }/>
          <Button
            title="DPO"
            onPress={() =>
            Settings.setDataProcessingOptions(["a", "b"])
          }/>


#pragma mark - React Native Methods

RCT_EXPORT_METHOD(getAdvertiserTrackingEnabled:(RCTResponseSenderBlock)callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the promise API instead to avoid having to wrap those in a promise in JS. See https://github.com/facebook/react-native-fbsdk/blob/master/ios/RCTFBSDK/share/RCTFBSDKShareDialog.m#L63 /

* For iOS only, get AdvertiserTrackingEnabled status.
* @platform ios
*/
getAdvertiserTrackingEnabled(): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a platform check here and no-op / warn on Android instead of crashing?

import com.facebook.react.bridge.BaseJavaModule;
import com.facebook.react.bridge.ReactMethod;

public class FBSettingsModule extends BaseJavaModule {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the @ReactModule / NAME const here like https://github.com/facebook/react-native-fbsdk/blob/master/android/src/main/java/com/facebook/reactnative/androidsdk/FBAccessTokenModule.java#L40. This is useful when using the new turbomodules infra.

@janicduplessis
Copy link
Contributor

Can you also run pod install in the example project, it should fix the CI error https://github.com/facebook/react-native-fbsdk/pull/804/checks?check_run_id=1355282567

@dreamolight
Copy link
Contributor Author

Will address comments and update the PR.

@@ -14,17 +14,17 @@ Pod::Spec.new do |s|
s.dependency 'React'

s.subspec 'Core' do |ss|
ss.dependency 'FBSDKCoreKit', '~> 7.0'
ss.dependency 'FBSDKCoreKit', '~> 8.0'

Choose a reason for hiding this comment

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

@janicduplessis these can go to 8.1 now, but I believe if you don't also update example/ios/Podfile.lock (or delete it) then CI will fail? It did on #811 but otherwise this is obviously a more interesting PR as it exposes functionality, not just bumping versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 8.1

@Stafox
Copy link

Stafox commented Nov 12, 2020

Without this PR there is no possibility to promote an app on facebook for ios14.
Any thoughts when it can be merged?

@dreamolight
Copy link
Contributor Author

Hi @janicduplessis , could you review again and let me know if further change needed?

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Nice, just made a small change to use Promise.resolve instead of new Promise(...)

@janicduplessis janicduplessis merged commit c4479dd into facebookarchive:master Nov 23, 2020
@mikehardy
Copy link

Nice! @janicduplessis + @dreamolight --> 🏆

@eladgel
Copy link

eladgel commented Nov 23, 2020

@mikehardy can this be used by changing the package.json to work with master? or should we wait for an official release?

@mikehardy
Copy link

@mikehardy can this be used by changing the package.json to work with master? or should we wait for an official release?

Pretty sure we need a release or you need to craft patch-package compatible patches

@eladgel
Copy link

eladgel commented Nov 23, 2020

Thank you!

@janicduplessis
Copy link
Contributor

I just published 3.0.0

@dreamolight
Copy link
Contributor Author

Thanks @janicduplessis, that's really nice! Also thanks @mikehardy for pointing out the latest iOS version!

@sbarbour-93
Copy link

Could someone on this thread clarify if these changes have resolved issue #799? I've just upgraded to 3.0.0 and tested with my Android app, the FB profile picture (which now requires the access token as part of the request) is still defaulting to the silhouette for me. Just want to be sure I'm not missing something before I continue to look into this.

@Stafox
Copy link

Stafox commented Nov 26, 2020

Could someone on this thread clarify if these changes have resolved issue #799? I've just upgraded to 3.0.0 and tested with my Android app, the FB profile picture (which now requires the access token as part of the request) is still defaulting to the silhouette for me. Just want to be sure I'm not missing something before I continue to look into this.

As you can see this PR provides integration with FB SDK >= 7.1 for Android.
Make sure you've FB SDK 8.x installed as dependency.

@sbarbour-93
Copy link

Could someone on this thread clarify if these changes have resolved issue #799? I've just upgraded to 3.0.0 and tested with my Android app, the FB profile picture (which now requires the access token as part of the request) is still defaulting to the silhouette for me. Just want to be sure I'm not missing something before I continue to look into this.

As you can see this PR provides integration with FB SDK >= 7.1 for Android.
Make sure you've FB SDK 8.x installed as dependency.

Yeah i've done this. It's pulling in 8.0.0 on Android and i'm still seeing the default silhouette instead of the user's profile picture. Is there any reference as to how we should pass the access token in our login request using this lib so that the user's profile picture is returned as it was prior to these updates from FB on how the profile picture is requested?

@Stafox
Copy link

Stafox commented Dec 2, 2020

have no ideas. Will be more effective to create an issue in android sdk https://github.com/facebook/facebook-android-sdk.

@sbarbour-93
Copy link

I've tried this on the iOS platform and have the same issue. I'm assuming the access token isn't being passed into configuration of the native library which is resulting in the default silhouette being returned. Would you rather I raise a separate issue for this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants