amplitude / unity-plugin

Official Amplitude Unity Plugin
https://developers.amplitude.com/docs/unity
MIT License
44 stars 34 forks source link

IDFA is null despite accepting prompt #65

Closed MartinGonzalez closed 3 years ago

MartinGonzalez commented 3 years ago

Unity: 2018.4.16 - 2019.4.16 Amplitude SDK: 2.1.0 Platform: Android & iOS

Hi! I'm testing idfa behaviour, to receive or not receive it in amplitude events based on user answer in the Tracking prompt. Despite of allowing trackign amplitude is sending null as idfa value

image

Is there any consideration to have? Am I missing something?

Thanks

EDIT: We are not using Amplitude custom solution from this page. We have our own implementation to prompt the tracking permission

I leave some other questions

Why there is no method like setIdfa(string) How should I call it if I have another implementation of how I use the prompt? What is the instance name I should pass to setIdfaBlock?

And seems not to work the CUstomIDFA.m

image

MartinGonzalez commented 3 years ago

After testing more, setIdfaBlockInternal is being called, but idfa still null in events properties. Idk if I'm passing the correct instanceName since in docs I could not find it or there is nothing related.

MartinGonzalez commented 3 years ago

After keep testing the issue was the instanceName. Since we do not name any instance by default I guess is "$default_instance". It would be better in doc to be more clear in this part, perhaps say that is not mandatory to send an instance name or create an overload. There are two ways to fix this

dantetam commented 3 years ago

Hello @MartinGonzalez, Thank you for the deep dive into our code. If you are using your own custom implementation, how are you integrating this into Amplitude SDK?

There is no method setIdfa(String) because of internal engineering considerations. I assume your custom solution will be in Objective C. Feel free to overwrite the code in setIdfaBlock(...) with your own code. To be clear, you may edit

void setIdfaBlockInternal(const char* instanceName) {
    AMPAdSupportBlock adSupportBlock = ...
    [[Amplitude instanceWithName:ToNSString(instanceName)] setAdSupportBlock:adSupportBlock];
}

where adSupportBlock is an Objective-C block that takes no arguments and returns an NSString*.

Thank you for your insight on the docs and I will go update that right now. If the instance name was the problem, were you able to set IDFA correctly?

Dante

MartinGonzalez commented 3 years ago

I share the actual working code

CustomIDFA.m

void setIdfaBlockInternalWith(const char* instanceName) {
    AMPAdSupportBlock adSupportBlock = ^{
        NSUUID *idfaUUID = [[ASIdentifierManager sharedManager] advertisingIdentifier];
        NSString *idfaString = [idfaUUID UUIDString];
        return idfaString;
    };

    NSString* string = [NSString stringWithFormat:@"%s" , instanceName];

    [[Amplitude instanceWithName:string] setAdSupportBlock:adSupportBlock];
}

void setIdfaBlockInternal(){
    setIdfaBlockInternalWith("");
}

CSharp

#if (UNITY_IPHONE || UNITY_TVOS)
    [DllImport ("__Internal")]
    private static extern void setIdfaBlockInternal();
#endif

void Start(){
#if (UNITY_IPHONE || UNITY_TVOS)
        setIdfaBlockInternal();
#endif
}
dantetam commented 3 years ago

This is a good improvement because it's consistent with the rest of the Unity API and other APIs. We have Amplitude.getInstance() and Amplitude.getInstance("..."). The latter function can take in null or "", which both default to Amplitude.getInstance(), which is the default instance. Just to be clear, the instanceName is an important parameter because it allows users to quickly "switch" between different settings.

MartinGonzalez commented 3 years ago

Also I'm testing that you need to call this setIdfaBlockInternal() after initializing Amplitude, instead the block will not work and will send the idfa as null even accepted, but if you kill the app and open it again then will send the idfa correctly

dantetam commented 3 years ago

Just to clarify action items: I have updated the documentation. I will update the code to allow the function call setIdfaBlock() (no arguments) as an improvement.

haoliu-amp commented 3 years ago

@dantetam Can we close this?

dantetam commented 3 years ago

@haoliu-amp This is just a small feature improvement for Unity. What should I do?