Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[firebase_auth] Ability to choose between sign-in or link on auto verification of phone number on Android #973

Closed
wants to merge 6 commits into from
Closed

[firebase_auth] Ability to choose between sign-in or link on auto verification of phone number on Android #973

wants to merge 6 commits into from

Conversation

cqhchan
Copy link

@cqhchan cqhchan commented Dec 8, 2018

  • Fixed Android Link with credentials bug.
  • Added in ability to choose between sign-in or link on auto verification of phone number

@cqhchan
- Fixed Android Link with credentials bug.
79409ab 
- Added in ability to choose between sign-in or link on auto verification of phone number
ened
ened reviewed Dec 8, 2018
View reviewed changes
packages/firebase_auth/lib/src/firebase_auth.dart Outdated
@required PhoneVerificationFailed verificationFailed,
@required PhoneCodeSent codeSent,
@required PhoneCodeAutoRetrievalTimeout codeAutoRetrievalTimeout,
bool linkCredentials = false}) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add back the comma (,) at the end of the array setup and reformat?

.../firebase_auth/android/src/main/java/io/flutter/plugins/firebaseauth/FirebaseAuthPlugin.java Outdated
if (task.isSuccessful()) {
Map<String, Object> arguments = new HashMap<>();
arguments.put("handle", handle);
channel.invokeMethod("phoneVerificationCompleted", arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the 2 onComplete handlers be merged?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @ened completed the requested changes

.../firebase_auth/android/src/main/java/io/flutter/plugins/firebaseauth/FirebaseAuthPlugin.java Outdated Show resolved Hide resolved
@cqhchan
Copy link
Author

cqhchan commented Dec 10, 2018

@ened completed the requested changes

This was referenced Jan 18, 2019
Closed
Closed
@cyanglaz cyanglaz changed the title With issue with firebase_auth linkWithCredentials on Android [firebase_auth]With issue with firebase_auth linkWithCredentials on Android Feb 22, 2019
@cqhchan
Copy link
Author

cqhchan commented Feb 28, 2019

@tolotrasamuel

firebase_auth: git: url: git://github.com/cqhchan/plugins.git path: packages/firebase_auth

Insert this into pubspec.yaml to use it.

https://github.com/cqhchan/plugins

This makes use of credentials implementation in #928
I fixed a few bugs with their implementation and added a bool value in
Future<void> verifyPhoneNumber(...)
to allow you to differentiate between verifyPhoneNumber for Login or For linking credentials with existing account. This addition bool value only affects specific android devices where they have auto verification.

@kroikie kroikie self-assigned this Mar 21, 2019
@collinjackson collinjackson self-requested a review March 22, 2019 12:40
@collinjackson
Copy link
Contributor

Thanks for the contribution!

We've fixed the linkWithCredential issue (#1333).

I'd love to get the other parts of your change, but I think we need an iOS implementation. Before we land this I'd ideally like to figure out how to write integration tests for phone auth so that we know the functionality is working on all platforms.

@kroikie kroikie assigned collinjackson and unassigned kroikie Mar 25, 2019
@collinjackson
Copy link
Contributor

@cqhchan -- Please let me know if you'd like to implement the iOS side of the pull request or if you want to abandon this PR. Thanks!

@cqhchan
Copy link
Author

cqhchan commented Apr 9, 2019

hi @collinjackson , Im not too sure what IOS side u are referring to. The bug only appeared in Android. Regarding the additional feature to enable auto phone number verification, again it is an Android only feature in selected Android phones that support it

@collinjackson collinjackson changed the title [firebase_auth]With issue with firebase_auth linkWithCredentials on Android [firebase_auth] Ability to choose between sign-in or link on auto verification of phone number Apr 9, 2019
@collinjackson collinjackson changed the title [firebase_auth] Ability to choose between sign-in or link on auto verification of phone number [firebase_auth] Ability to choose between sign-in or link on auto verification of phone number on Android Apr 9, 2019
collinjackson
collinjackson reviewed Apr 9, 2019
View reviewed changes
Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

OK, you're right -- this is an Android-only feature.

.../firebase_auth/android/src/main/java/io/flutter/plugins/firebaseauth/FirebaseAuthPlugin.java

@Override
public void onComplete(@NonNull Task<AuthResult> task) {
if (task.isSuccessful()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we should report errors back to the Dart side rather than swallowing them

@creativecreatorormaybenot
Copy link
Contributor

@collinjackson Is this going to be merged soon then? (considering the comment)

@creativecreatorormaybenot creativecreatorormaybenot mentioned this pull request May 5, 2019
Merged
@collinjackson
Copy link
Contributor

The API design in #1568 seems preferable to me, so I'm closing this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@collinjackson collinjackson collinjackson left review comments

@ened ened ened left review comments

Assignees

@collinjackson collinjackson

Labels
bugfix cla: yes flutterfire
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cqhchan @collinjackson @creativecreatorormaybenot @ened @tolotrasamuel @kroikie @cyanglaz @googlebot