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

Implemented setAuthStateChanged for iOS and added getCurrentUser (iOS and Android) #54

Merged
merged 4 commits into from Mar 9, 2019

Conversation

JavierPAYTEF
Copy link
Contributor

This implements the missing method setAuthStateChanged in the iOS version of the plugin.
It also adds the method getCurrentUser() to request the current user in the Firebase Auth instance.

@JavierPAYTEF
Copy link
Contributor Author

Hi, please let me know if you need anything, I made this because of the issues I had in #48. This pull request is in case you feel it's worth merging.

@chemerisuk
Copy link
Owner

Hi @JavierPAYTEF, thank you for the PR. The changes look good - just make some adjustments according to my inline comments.

@JavierPAYTEF
Copy link
Contributor Author

Hi @chemerisuk, thanks for the reply. No problem about the changes. Although this is the first time I do a PR so I'm not very familiar with the process. Where can I see your comments?

private void getCurrentUser(final CallbackContext callbackContext) {
FirebaseUser user = firebaseAuth.getCurrentUser();
if (user == null) {
callbackContext.error("User is not authorized");
Copy link
Owner

Choose a reason for hiding this comment

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

Return null is user is not signed in

callbackContext.sendPluginResult(new PluginResult(PluginResult.Status.OK, (String)null))

@@ -209,6 +249,9 @@ - (CDVPluginResult*) createAuthResult:(FIRAuthDataResult*)result withError:(NSEr
}

- (NSDictionary*)userToDictionary:(FIRUser *)user {
if (!user) {
return @{};
Copy link
Owner

Choose a reason for hiding this comment

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

Return null is user is not signed in

return nil;

@@ -11,6 +17,17 @@ - (void)pluginInitialize {
}
}

- (void)getCurrentUser:(CDVInvokedUrlCommand *)command {
FIRUser *user = [FIRAuth auth].currentUser;
if (user) {
Copy link
Owner

Choose a reason for hiding this comment

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

Just return nil if user is not signed in - you don't need the if block, just leave lines 23 + 24

@chemerisuk chemerisuk merged commit d2d99e2 into chemerisuk:master Mar 9, 2019
@chemerisuk
Copy link
Owner

@JavierPAYTEF well done, merged!

@chemerisuk
Copy link
Owner

@JavierPAYTEF I did some refactoring too. Please let me know if v1.1.0 works for you.

@JavierPAYTEF
Copy link
Contributor Author

Nice! I'll test it better on monday and let you know if I see anything wrong. Thanks for the plugin

@JavierPAYTEF
Copy link
Contributor Author

Hi @chemerisuk I just realized of a small issue described here #22
The iOS version is overriding the listener but the Android version needs a small change to do it. From this:

    @CordovaMethod
    private void setAuthStateChanged(boolean disable, CallbackContext callbackContext) {
        this.authStateCallback = disable ? null : callbackContext;

        if (disable) {
            firebaseAuth.removeAuthStateListener(this);
        } else {
            firebaseAuth.addAuthStateListener(this);
        }
    }

To this:

    @CordovaMethod
    private void setAuthStateChanged(boolean disable, CallbackContext callbackContext) {
        if (this.authStateCallback != null) {
            this.authStateCallback = null;
            firebaseAuth.removeAuthStateListener(this);
        }
        if (!disable) {
            this.authStateCallback = callbackContext;
            firebaseAuth.addAuthStateListener(this);
        }
    }

To be honest, I haven't tested what happens if you add the same listener multiple times, probably not something good.

@chemerisuk
Copy link
Owner

@JavierPAYTEF me too, but to be safe I agree with what you suggest.

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

Successfully merging this pull request may close these issues.

None yet

3 participants