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

🔥 [🐛] setUserId(null) does not work #4931

Closed
1 of 9 tasks
anuragraina opened this issue Feb 19, 2021 · 18 comments · Fixed by #6004
Closed
1 of 9 tasks

🔥 [🐛] setUserId(null) does not work #4931

anuragraina opened this issue Feb 19, 2021 · 18 comments · Fixed by #6004
Labels
help: good-first-issue Issues that are non-critical issues & well defined for first time contributors. help: needs-triage Issue needs additional investigation/triaging. platform: ios plugin: authentication Firebase Authentication type: bug New bug report Workflow: Needs Review Pending feedback or review from a maintainer.

Comments

@anuragraina
Copy link

Issue

await analytics().setUserId('Some id'); works perfect.

But the issue arises when I set

  • await analytics().setUserId(null); or
  • await analytics().setUserProperty('some_property', null);

This does not work and the same old id and property is shown in events.

This comment mentions the issue exactly.


Project Files

Javascript

Click To Expand

package.json:

# N/A

firebase.json for react-native-firebase v6:

# N/A

iOS

Click To Expand

ios/Podfile:

  • I'm not using Pods
  • I'm using Pods and my Podfile looks like:
# N/A

AppDelegate.m:

// N/A


Android

Click To Expand

Have you converted to AndroidX?

  • my application is an AndroidX application?
  • I am using android/gradle.settings jetifier=true for Android compatibility?
  • I am using the NPM package jetifier for react-native compatibility?

android/build.gradle:

// N/A

android/app/build.gradle:

// N/A

android/settings.gradle:

// N/A

MainApplication.java:

// N/A

AndroidManifest.xml:

<!-- N/A -->


Environment

Click To Expand

react-native info output:

 OUTPUT GOES HERE
  • Platform that you're experiencing the issue on:
    • iOS
    • Android
    • iOS but have not tested behavior on Android
    • [x ] Android but have not tested behavior on iOS
    • Both
  • react-native-firebase version you're using that has this issue:
    • 10.6.4
  • Firebase module(s) you're using that has the issue:
    • analytics
  • Are you using TypeScript?
    • Y & 3.8.3


@anuragraina anuragraina added help: needs-triage Issue needs additional investigation/triaging. type: bug New bug report labels Feb 19, 2021
@anuragraina anuragraina changed the title 🔥 setUserId(null) does not work 🔥 [🐛] setUserId(null) does not work Feb 19, 2021
@mikehardy
Copy link
Collaborator

Focusing on setUserId: I don't see where in the API that we attempt to mirror exactly (the Firebase JS API) setting a null is supposed to do anything?

https://firebase.google.com/docs/reference/js/firebase.analytics.Analytics#setuserid

...but that could be a documentation issue, to travel deeper you may follow the information "Use gtag 'config' command to set 'user_id'" and see that even in the deeper information there is no mention of clearing a value

https://developers.google.com/analytics/devguides/collection/ga4/persistent-values

I do see how the native APIs allow a null to unset per their documentation:

https://firebase.google.com/docs/reference/android/com/google/firebase/analytics/FirebaseAnalytics#public-void-setuserid-string-id

https://firebase.google.com/docs/reference/ios/firebaseanalytics/api/reference/Classes/FIRAnalytics#+setuserid:

...widening scope back out to look at User Properties, the native SDKs also seem to indicate that clearing is possible (though the JS API is again quiet on the subject)

https://firebase.google.com/docs/reference/ios/firebaseanalytics/api/reference/Classes/FIRAnalytics#+setuserpropertystring:forname:

https://firebase.google.com/docs/reference/android/com/google/firebase/analytics/FirebaseAnalytics#setUserProperty(java.lang.String,%20java.lang.String)

So: it looks like it's possible

Have you examined in the code here (you can just reach right into node_modules @react-native-firebase/analytics directory... to see if the javascript (in the 'lib' sub directory) is passing the nulls down to native and/or if the ios / android sub-directories' native code is calling the firebase-ios-sdk / firebase-android-sdk APIs correctly to pass null in and clear things?

That's the next step, and if there is anything to repair there I would happily merge a PR if you posted one up!

@mikehardy
Copy link
Collaborator

Actually a quick scan of the code indicates it is allowing nulls through:

if (!isNull(id) && !isString(id)) {

I believe the native code then will handle the null correctly if it is allowed to pass, calling the firebase SDKs correctly:

Task<Void> setUserId(String id) {
return Tasks.call(() -> {
FirebaseAnalytics.getInstance(getContext()).setUserId(id);
return null;
});
}
Task<Void> setUserProperty(String name, String value) {
return Tasks.call(() -> {
FirebaseAnalytics.getInstance(getContext()).setUserProperty(name, value);
return null;
});
}
Task<Void> setUserProperties(Bundle properties) {
return Tasks.call(() -> {
Set<String> bundleKeys = properties.keySet();
FirebaseAnalytics firebaseAnalytics = FirebaseAnalytics.getInstance(getContext());
for (String bundleKey : bundleKeys) {
firebaseAnalytics.setUserProperty(bundleKey, (String) properties.get(bundleKey));
}
return null;
});
}

RCT_EXPORT_METHOD(setUserId:
(NSString *) id
resolver:
(RCTPromiseResolveBlock) resolve
rejecter:
(RCTPromiseRejectBlock) reject) {
@try {
[FIRAnalytics setUserID:id];
} @catch (NSException *exception) {
return [RNFBSharedUtils rejectPromiseWithExceptionDict:reject exception:exception];
}
return resolve([NSNull null]);
}
RCT_EXPORT_METHOD(setUserProperty:
(NSString *) name
value:
(NSString *) value
resolver:
(RCTPromiseResolveBlock) resolve
rejecter:
(RCTPromiseRejectBlock) reject) {
@try {
[FIRAnalytics setUserPropertyString:value forName:name];
} @catch (NSException *exception) {
return [RNFBSharedUtils rejectPromiseWithExceptionDict:reject exception:exception];
}
return resolve([NSNull null]);
}
RCT_EXPORT_METHOD(setUserProperties:
(NSDictionary *) properties
resolver:
(RCTPromiseResolveBlock) resolve
rejecter:
(RCTPromiseRejectBlock) reject) {
@try {
[properties enumerateKeysAndObjectsUsingBlock:^(id key, id value, BOOL *stop) {
[FIRAnalytics setUserPropertyString:value forName:key];
}];
} @catch (NSException *exception) {
return [RNFBSharedUtils rejectPromiseWithExceptionDict:reject exception:exception];
}
return resolve([NSNull null]);
}

The tests even appear to be specifically checking to make sure nulls are usable

describe('setUserId()', function () {
it('allows a null values to be set', async function () {
await firebase.analytics().setUserId(null);
});
it('accepts string values', async function () {
await firebase.analytics().setUserId('rn-firebase');
});
});
describe('setUserProperty()', function () {
it('allows a null values to be set', async function () {
await firebase.analytics().setUserProperty('invertase', null);
});
it('accepts string values', async function () {
await firebase.analytics().setUserProperty('invertase2', 'rn-firebase');
});
});
describe('setUserProperties()', function () {
it('allows null values to be set', async function () {
await firebase.analytics().setUserProperties({ invertase2: null });
});
it('accepts string values', async function () {
await firebase.analytics().setUserProperties({ invertase3: 'rn-firebase' });
});
});

So I'm not sure what the problem is. I would definitely instrument the react-native-firebase java or objective-c code (take your pick) to log out the parameters we send to the native APIs when you send a null in and I bet we are doing the right thing here.

At which point it becomes an upstream bug and I'd advise trying the same test but without using react-native-firebase - you have to use the firebase SDKs directly and give the google firebase team a clean reproduction for them to triage an issue. They provide quickstarts that make that easy though: https://github.com/firebase/quickstart-android/tree/master/analytics / https://github.com/firebase/quickstart-ios/tree/master/analytics

@mikehardy mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Feb 19, 2021
@mikehardy
Copy link
Collaborator

Hi @anuragraina - just curious if you got a chance to put some printouts in the javascript -> native code to see whether react-native-firebase is sending in the nulls to clear things correctly and move this forward

@louiszawadzki
Copy link

louiszawadzki commented Jun 7, 2021

I had the same issue with setUserProperty and did some debugging, so maybe it can help :)

When trying to call `setUserProperty('usa_code', null), my XCode console would output:

User property must be NSString. Value: (nil)

After some debugging, I found out that value was casted as NSNull instead of the expected nil - and editing RNFBAnalyticsModule.m to pass nil instead of value every time worked. Now Xcode would output:

User property removed. Name: usa_code

Here is my patch if someone needs it, I am going to open a PR soon:

diff --git a/node_modules/@react-native-firebase/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m b/node_modules/@react-native-firebase/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m
index 8339172..621d876 100644
--- a/node_modules/@react-native-firebase/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m
+++ b/node_modules/@react-native-firebase/analytics/ios/RNFBAnalytics/RNFBAnalyticsModule.m
@@ -21,6 +21,7 @@
 #import "RNFBAnalyticsModule.h"
 #import <RNFBApp/RNFBSharedUtils.h>
 
+#define NULL_TO_NIL(obj) ({ __typeof__ (obj) __obj = (obj); __obj == [NSNull null] ? nil : obj; })
 
 @implementation RNFBAnalyticsModule
 #pragma mark -
@@ -74,7 +75,7 @@ - (dispatch_queue_t)methodQueue {
         rejecter:
         (RCTPromiseRejectBlock) reject) {
     @try {
-      [FIRAnalytics setUserID:id];
+      [FIRAnalytics setUserID:NULL_TO_NIL(id)];
     } @catch (NSException *exception) {
       return [RNFBSharedUtils rejectPromiseWithExceptionDict:reject exception:exception];
     }
@@ -90,7 +91,7 @@ - (dispatch_queue_t)methodQueue {
         rejecter:
         (RCTPromiseRejectBlock) reject) {
     @try {
-      [FIRAnalytics setUserPropertyString:value forName:name];
+      [FIRAnalytics setUserPropertyString:NULL_TO_NIL(value) forName:name];
     } @catch (NSException *exception) {
       return [RNFBSharedUtils rejectPromiseWithExceptionDict:reject exception:exception];
     }
@@ -103,9 +104,10 @@ - (dispatch_queue_t)methodQueue {
         (RCTPromiseResolveBlock) resolve
         rejecter:
         (RCTPromiseRejectBlock) reject) {
+    
     @try {
       [properties enumerateKeysAndObjectsUsingBlock:^(id key, id value, BOOL *stop) {
-        [FIRAnalytics setUserPropertyString:value forName:key];
+        [FIRAnalytics setUserPropertyString:NULL_TO_NIL(value) forName:key];
       }];
     } @catch (NSException *exception) {
       return [RNFBSharedUtils rejectPromiseWithExceptionDict:reject exception:exception];

EDIT: Even though Xcode says everything is fine, I still don't see the updates in the Firebase DebugView. Maybe it has to do with some cache (when I have to change a variable, I have to put the app in background for the change to be sent).
Apparently this is a problem with the DebugView: firebase/firebase-ios-sdk#3230

@mikehardy
Copy link
Collaborator

mikehardy commented Jun 7, 2021

Oh that's fantastic - looking forward to the PR, will check it and if everything looks good will be happy to merge! This one has been a sore point here. Probably a similar issue with Java (just not sending null in the right way? or perhaps it's working on Java but we just can't tell owing to DebugView oddity as you mention)

@louiszawadzki
Copy link

Java seemed to work for me (I didn't get any error log). I will get my hands on a real Android device later this week to try and see if it appears as expected in the reports :)

As for the PR I'll try to open it by the end of the week if it really works :)

@mikehardy
Copy link
Collaborator

I'm going to reopen this and tag it up for review just in case since the patch exists in a comment at least

@mikehardy mikehardy reopened this Jun 8, 2021
@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. platform: ios plugin: authentication Firebase Authentication labels Jun 8, 2021
@stale
Copy link

stale bot commented Jul 13, 2021

Hello 👋, to help manage issues we automatically close stale issues.
This issue has been automatically marked as stale because it has not had activity for quite some time. Has this issue been fixed, or does it still require the community's attention?

This issue will be closed in 15 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the Type: Stale Issue has become stale - automatically added by Stale bot label Jul 13, 2021
@stale
Copy link

stale bot commented Aug 3, 2021

Closing this issue after a prolonged period of inactivity. If this is still present in the latest release, please feel free to create a new issue with up-to-date information.

@stale stale bot closed this as completed Aug 3, 2021
@ghost
Copy link

ghost commented Nov 24, 2021

Hey @mikehardy, we are facing the same issue. Calling setUserId(null) seems to do nothing.
We have tested it on both, Android and iOS and they are not working properly.
Is there any upcoming release that will fix this? Or should it be fixed already?

@mikehardy
Copy link
Collaborator

Sorry I don't have solid info but just replying since it was a direct question: there's no current work in this area, so if it is not working with current (13.0.1 IIRC), it will continue not working until it receives attention - apologies

@ghost
Copy link

ghost commented Nov 25, 2021

Unfortunately, we are not in the latest version, I'll try to upgrade and give you feedback about it. Thanks for the quick response.

@ghost
Copy link

ghost commented Nov 26, 2021

It's confirmed, the latest version doesn't work too. Is there any chance to give attention to this ASAP? It may be impacting others too.

@mikehardy
Copy link
Collaborator

Sorry @emilioheinz this is/was in my queue but obviously I haven't had time to look at it yet, apologies. I can reopen but if a PR doesn't come from someone in the community rolling up their sleeves and investigating a) is it here or native SDKs? and b) posting PR here or logging upstream, there may not be activity for a bit sadly

@mikehardy mikehardy reopened this Dec 2, 2021
@stale stale bot removed the Type: Stale Issue has become stale - automatically added by Stale bot label Dec 2, 2021
@mikehardy mikehardy added help: good-first-issue Issues that are non-critical issues & well defined for first time contributors. and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Dec 2, 2021
@ghost
Copy link

ghost commented Dec 2, 2021

I'll try to take a look at this in some weeks and update it here with my progress. If anyone falls into this issue and wants to work on it, just do it, it will be really helpful.

@bonesyblue
Copy link
Contributor

Hi all 👋 I also encountered this issue in an app I am working on so I have created a PR: #6004 to fix this issue. Would love to hear your feedback on this!

mikehardy pushed a commit that referenced this issue Jan 10, 2022
@Nicolakacha
Copy link

I'm using 14.5.0 and this still happen on Android. Anyone still encounter the same issue?

@dgreasi
Copy link

dgreasi commented May 14, 2024

Happening to me too in debug mode for iOS. I have the following dependencies:

"@react-native-firebase/analytics": "^19.2.2",
"@react-native-firebase/app": "^19.2.2",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help: good-first-issue Issues that are non-critical issues & well defined for first time contributors. help: needs-triage Issue needs additional investigation/triaging. platform: ios plugin: authentication Firebase Authentication type: bug New bug report Workflow: Needs Review Pending feedback or review from a maintainer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants