Skip to content

Task/103/android phone auth change #369

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Task/103/android phone auth change #369

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e32362
added callback to let android know the verifationId in case the autom…
Apr 8, 2023
83e8481
fixed method call name mishap
Apr 8, 2023
1d5d740
fixed unresolved variable
Apr 13, 2023
74a77c9
Merge branch 'master' into task/103/AndroidPhoneAuthChange
AKnght Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions firebase-auth/src/androidMain/kotlin/dev/gitlive/firebase/auth/credentials.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ actual class PhoneAuthProvider(val android: com.google.firebase.auth.PhoneAuthPr
PhoneAuthProvider.OnVerificationStateChangedCallbacks() {

override fun onCodeSent(verificationId: String, forceResending: PhoneAuthProvider.ForceResendingToken) {
verificationProvider.onRecievedVerificationID(verificationId = verificationId)
verificationProvider.codeSent { android.verifyPhoneNumber(phoneNumber, verificationProvider.timeout, verificationProvider.unit, verificationProvider.activity, this, forceResending) }
}

Expand Down Expand Up @@ -123,6 +124,7 @@ actual interface PhoneVerificationProvider {
val activity: Activity
val timeout: Long
val unit: TimeUnit
fun onRecievedVerificationID(verificationID: String)
fun codeSent(triggerResend: (Unit) -> Unit)
Copy link
Author

Choose a reason for hiding this comment

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

I don't want to change a method signature. Also, less convoluted code is easier to read and maintain. Separation of concern. Separation of responsibility.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure about the last part but I have worked with many sdks and the smaller the pieces the better. Less siginture changes the better.

Either way this update is needed and had been needed for a very long time. If it doesn't happen soon I will have to shift into building and maintaining my own SDK for this which is what I am trying to avoid.

Copy link
Member

Choose a reason for hiding this comment

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

api should follow android sdk apart from the kotlin-first design principles defined in the readme, is this the case for this?
If you paste in links to the relevant firebase sdk documentation I would be happy to confirm it for you

Copy link
Author

Choose a reason for hiding this comment

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

override fun onCodeSent(
verificationId: String,
token: PhoneAuthProvider.ForceResendingToken
) {

https://github.com/firebase/snippets-android/blob/076236ee54e3b6c1bab0152c52fdb3ebc5c0640a/auth/app/src/main/java/com/google/firebase/quickstart/auth/kotlin/PhoneAuthActivity.kt#L39-L81

Line 68

Copy link
Author

Choose a reason for hiding this comment

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

Android applications need that ID or they cant complete the phone code auth.

Copy link
Author

Choose a reason for hiding this comment

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

Someone else may need this so I'll leave it open but I am pivoting away from this.

suspend fun getVerificationCode(): String
}
Expand Down