Ccct 1159 send request connectid verify firebase otp #3125
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThe changes introduce Firebase-based OTP authentication to the application. The Sequence Diagram(s)sequenceDiagram
participant User
participant Fragment as PersonalIdPhoneVerificationFragment
participant OtpManager
participant FirebaseAuthService
participant Firebase
participant BackendAPI
User->>Fragment: Request OTP
Fragment->>OtpManager: requestOtp(phoneNumber)
OtpManager->>FirebaseAuthService: requestOtp(phoneNumber)
FirebaseAuthService->>Firebase: Send OTP
Firebase-->>FirebaseAuthService: onCodeSent(verificationId)
FirebaseAuthService-->>Fragment: onCodeSent(verificationId)
User->>Fragment: Enter OTP and submit
Fragment->>OtpManager: submitOtp(otpCode)
OtpManager->>FirebaseAuthService: verifyOtp(otpCode)
FirebaseAuthService->>Firebase: Verify OTP
Firebase-->>FirebaseAuthService: onSuccess(FirebaseUser)
FirebaseAuthService-->>Fragment: onSuccess(FirebaseUser)
Fragment->>FirebaseUser: getIdToken()
FirebaseUser-->>Fragment: firebaseIdToken
Fragment->>BackendAPI: validateFirebaseIdToken(firebaseIdToken)
BackendAPI-->>Fragment: Validation result
Fragment-->>User: Show result / Navigate next
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)
75-82:⚠️ Potential issueMissing state restoration for
verificationId.The
verificationIdfield is not saved/restored during configuration changes. If the activity is recreated (e.g., screen rotation) after an OTP is sent but before verification, the user won't be able to verify the OTP asverificationIdwill be null.Add state persistence for
verificationId:@Override public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); if (savedInstanceState != null) { primaryPhone = savedInstanceState.getString(KEY_PHONE); + verificationId = savedInstanceState.getString("KEY_VERIFICATION_ID"); } initOtpComponents(); }Also update
onSaveInstanceState:@Override public void onSaveInstanceState(@NonNull Bundle outState) { super.onSaveInstanceState(outState); outState.putString(KEY_PHONE, primaryPhone); + if (verificationId != null) { + outState.putString("KEY_VERIFICATION_ID", verificationId); + } }
🧹 Nitpick comments (4)
app/src/org/commcare/connect/network/connectId/ApiService.java (1)
14-15: Consider adding parameter documentation.The method implementation follows established patterns correctly. However, consider adding JavaDoc to document the expected structure of the
firebaseIdTokenmap parameter (e.g., what keys are required).Example:
+ /** + * Validates a Firebase ID token with the backend service. + * @param firebaseIdToken Map containing the Firebase ID token (e.g., {"id_token": "token_value"}) + * @return Call for the validation response + */ @POST(ApiEndPoints.validateFirebaseIdToken) Call<ResponseBody> validateFirebaseIdToken(@Body Map<String, String> firebaseIdToken);app/src/org/commcare/utils/OtpAuthService.java (1)
1-25: Well-designed interface with room for improvement.The interface follows good design principles with clear documentation. However, consider enhancing it with:
- Error handling specification: How should implementations communicate failures?
- Input validation guidelines: What constitutes a valid phone number format?
- Async behavior clarification: Are these operations synchronous or asynchronous?
Consider adding more specific documentation:
/** * Interface defining the contract for OTP-based authentication operations. * <p> * Implementations of this interface are responsible for sending OTP codes to users - * and verifying the OTP codes they enter. + * and verifying the OTP codes they enter. All operations are asynchronous and + * results are communicated via callback mechanisms defined by the implementation. */ public interface OtpAuthService { /** * Sends an OTP to the specified phone number. * - * @param phoneNumber The recipient's phone number in a valid format + * @param phoneNumber The recipient's phone number in E.164 format (e.g., +1234567890) + * @throws IllegalArgumentException if phoneNumber is invalid */ void requestOtp(String phoneNumber); /** * Verifies the OTP code entered by the user. * - * @param code The OTP code to verify + * @param code The OTP code to verify (typically 6 digits) + * @throws IllegalArgumentException if code format is invalid */ void verifyOtp(String code);app/src/org/commcare/utils/FirebaseAuthService.java (1)
78-87: Improve verification ID validation.The current validation only checks for null or empty, but should also validate the format and provide better error context.
Apply this diff to enhance validation:
public void verifyOtp(String code) { - if (verificationId == null || verificationId.isEmpty()) { - callback.onFailure("Missing verification ID"); + if (verificationId == null || verificationId.trim().isEmpty()) { + callback.onFailure("Missing verification ID. Please request OTP first."); return; } + + if (code == null || code.trim().isEmpty()) { + callback.onFailure("OTP code cannot be empty"); + return; + }app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (1)
162-166: Consider validating phone number format.The phone number from arguments is used directly without format validation. Invalid formats could cause Firebase authentication to fail with unclear error messages.
Add phone number validation in
getPhoneNumberFromArguments:private void getPhoneNumberFromArguments() { if (getArguments() != null) { primaryPhone = PersonalIdPhoneVerificationFragmentArgs.fromBundle(getArguments()).getPrimaryPhone(); + // Validate phone number format (e.g., starts with +, contains only digits) + if (primaryPhone != null && !primaryPhone.matches("^\\+[1-9]\\d{1,14}$")) { + Logger.log("WARN", "Invalid phone number format: " + primaryPhone); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/build.gradle(1 hunks)app/src/org/commcare/connect/network/ApiPersonalId.java(1 hunks)app/src/org/commcare/connect/network/connectId/ApiEndPoints.java(1 hunks)app/src/org/commcare/connect/network/connectId/ApiService.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(6 hunks)app/src/org/commcare/utils/FirebaseAuthService.java(1 hunks)app/src/org/commcare/utils/OtpAuthService.java(1 hunks)app/src/org/commcare/utils/OtpManager.java(1 hunks)app/src/org/commcare/utils/OtpVerificationCallback.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/connect/network/connectId/ApiService.java (1)
app/src/org/commcare/connect/network/connectId/ApiEndPoints.java (1)
ApiEndPoints(3-36)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (5)
app/src/org/commcare/connect/network/connectId/ApiEndPoints.java (1)
8-8: LGTM!The new API endpoint constant follows the established naming convention and is properly placed within the existing structure.
app/src/org/commcare/utils/OtpVerificationCallback.java (1)
1-33: Well-designed callback interface for OTP operations.The interface is clean, well-documented, and follows good design principles with appropriate separation of concerns for the different OTP verification states.
app/src/org/commcare/utils/OtpManager.java (3)
22-24: Good defensive programming with null safety.The constructor properly validates the required dependency using
Objects.requireNonNullwhich will provide a clear error message if a null service is passed.
32-37: Effective input validation for phone number.The validation logic properly checks for null, empty, and whitespace-only phone numbers before delegating to the service, preventing unnecessary processing.
45-50: Consistent input validation for OTP code.The validation pattern matches the phone number validation, ensuring consistent error handling throughout the class.
| implementation "androidx.camera:camera-camera2:$cameraX_version" | ||
| implementation "androidx.camera:camera-lifecycle:$cameraX_version" | ||
| implementation 'com.google.android.gms:play-services-mlkit-face-detection:17.1.0' | ||
| implementation "com.google.firebase:firebase-auth:22.3.0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Firebase Auth dependency version and security.
Please confirm that Firebase Auth version 22.3.0 is current and free from security vulnerabilities.
🌐 Web query:
What is the latest version of Firebase Auth for Android and are there any known security vulnerabilities in version 22.3.0?
💡 Result:
As of May 23, 2025, the latest version of Firebase Authentication for Android is 23.2.0. This version was released on March 20, 2025, as part of Firebase Android BoM (Bill of Materials) version 33.11.0. (firebase.google.com)
Version 22.3.0 of Firebase Authentication for Android was released on November 16, 2023. (mvnrepository.com) There are no known security vulnerabilities associated with this version. However, it's important to note that Firebase Authentication version 22.3.0 is outdated, and using the latest version is recommended to benefit from the latest features, improvements, and security patches.
Citations:
- 1: https://firebase.google.com/support/release-notes/android?utm_source=openai
- 2: https://mvnrepository.com/artifact/com.google.firebase/firebase-auth/22.3.0?utm_source=openai
Upgrade Firebase Auth to latest version (23.2.0)
Firebase Auth 22.3.0 has no known security vulnerabilities but is outdated. Upgrading to 23.2.0 (released March 20, 2025) ensures you receive the latest features, improvements, and security patches.
• File: app/build.gradle
Line 168:
- implementation "com.google.firebase:firebase-auth:22.3.0"
+ implementation "com.google.firebase:firebase-auth:23.2.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| implementation "com.google.firebase:firebase-auth:22.3.0" | |
| implementation "com.google.firebase:firebase-auth:23.2.0" |
🤖 Prompt for AI Agents
In app/build.gradle at line 168, the Firebase Auth dependency is set to version
22.3.0, which is outdated. Update the version to 23.2.0 to ensure you have the
latest features, improvements, and security patches. Replace
"com.google.firebase:firebase-auth:22.3.0" with
"com.google.firebase:firebase-auth:23.2.0".
| public static void validateFirebaseIdToken(Context context, String firebaseIdToken, IApiCallback callback) { | ||
| HashMap<String, String> params = new HashMap<>(); | ||
| params.put("token", firebaseIdToken); | ||
| ApiService apiService = ApiClient.getClientApi(); | ||
| Call<ResponseBody> call = apiService.validateFirebaseIdToken(params); | ||
| callApi(context, call, callback); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the Firebase ID token parameter.
The method should validate the firebaseIdToken parameter before making the API call to prevent unnecessary network requests with invalid data.
Apply this diff to add input validation:
public static void validateFirebaseIdToken(Context context, String firebaseIdToken, IApiCallback callback) {
+ if (firebaseIdToken == null || firebaseIdToken.trim().isEmpty()) {
+ callback.processFailure(-1); // or appropriate error code
+ return;
+ }
HashMap<String, String> params = new HashMap<>();
params.put("token", firebaseIdToken);
ApiService apiService = ApiClient.getClientApi();
Call<ResponseBody> call = apiService.validateFirebaseIdToken(params);
callApi(context, call, callback);
}🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/ApiPersonalId.java around lines 296 to
302, the method validateFirebaseIdToken does not validate the firebaseIdToken
parameter before using it. Add a check at the start of the method to verify that
firebaseIdToken is not null or empty; if it is invalid, invoke the callback with
an error or return early to avoid making the API call with invalid data.
| PhoneAuthOptions options = PhoneAuthOptions.newBuilder(FirebaseAuth.getInstance()) | ||
| .setPhoneNumber(phoneNumber) // Phone number to verify | ||
| .setActivity(activity) // Activity for binding callback | ||
| .setCallbacks(new PhoneAuthProvider.OnVerificationStateChangedCallbacks() { | ||
|
|
||
| // Called when verification is completed automatically (instant/auto-retrieval) | ||
| @Override | ||
| public void onVerificationCompleted(@NonNull PhoneAuthCredential credential) { | ||
| signInWithCredential(credential); | ||
| } | ||
|
|
||
| // Called when verification process fails | ||
| @Override | ||
| public void onVerificationFailed(@NonNull FirebaseException e) { | ||
| callback.onFailure("Verification failed: " + e.getMessage()); | ||
| } | ||
|
|
||
| // Called when OTP has been sent successfully | ||
| @Override | ||
| public void onCodeSent(@NonNull String id, @NonNull PhoneAuthProvider.ForceResendingToken token) { | ||
| verificationId = id; // Store verification ID to use during verification | ||
| callback.onCodeSent(id); // Notify client | ||
| } | ||
| }) | ||
| .build(); | ||
|
|
||
| // Start phone number verification process | ||
| PhoneAuthProvider.verifyPhoneNumber(options); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout configuration for OTP verification.
Firebase PhoneAuth operations should include timeout configuration to prevent indefinite waiting and improve user experience.
Apply this diff to add timeout configuration:
PhoneAuthOptions options = PhoneAuthOptions.newBuilder(FirebaseAuth.getInstance())
.setPhoneNumber(phoneNumber) // Phone number to verify
- .setActivity(activity) // Activity for binding callback
+ .setActivity(activityRef.get()) // Activity for binding callback
+ .setTimeout(60L, java.util.concurrent.TimeUnit.SECONDS) // Timeout after 60 seconds
.setCallbacks(new PhoneAuthProvider.OnVerificationStateChangedCallbacks() {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java between lines 42 and 69,
the PhoneAuthOptions builder lacks a timeout configuration for the OTP
verification process. To fix this, add a timeout duration and time unit using
the setTimeout(long timeout, TimeUnit unit) method on the PhoneAuthOptions
builder before calling build(). This will ensure the verification process does
not wait indefinitely and improves user experience.
| private void signInWithCredential(PhoneAuthCredential credential) { | ||
| FirebaseAuth.getInstance().signInWithCredential(credential) | ||
| .addOnCompleteListener(activity, task -> { | ||
| if (task.isSuccessful()) { | ||
| // Authentication successful | ||
| FirebaseUser user = task.getResult().getUser(); | ||
| callback.onSuccess(user); | ||
| } else { | ||
| // Authentication failed | ||
| String error = (task.getException() != null) | ||
| ? task.getException().getMessage() | ||
| : "Unknown error"; | ||
| callback.onFailure("Sign-in failed: " + error); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Activity lifecycle in Firebase callback.
The Firebase authentication callback uses the Activity reference which may be null or destroyed when the callback executes.
Apply this diff to handle Activity lifecycle:
private void signInWithCredential(PhoneAuthCredential credential) {
+ Activity currentActivity = activityRef.get();
+ if (currentActivity == null || currentActivity.isDestroyed()) {
+ callback.onFailure("Activity no longer available");
+ return;
+ }
+
FirebaseAuth.getInstance().signInWithCredential(credential)
- .addOnCompleteListener(activity, task -> {
+ .addOnCompleteListener(currentActivity, task -> {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java around lines 95 to 110,
the Firebase authentication callback uses the Activity reference directly, which
may be null or destroyed when the callback executes. To fix this, check if the
activity is still valid and not finishing before proceeding with the callback
logic. If the activity is invalid, avoid calling the callback to prevent crashes
or leaks.
| private final Activity activity; // Activity context required for Firebase callbacks | ||
| private final OtpVerificationCallback callback; // Callback interface for OTP result events |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak risk with Activity reference.
Storing a direct reference to the Activity can cause memory leaks if the authentication process outlives the Activity lifecycle.
Consider using WeakReference<Activity> or pass the Activity context only when needed:
-private final Activity activity; // Activity context required for Firebase callbacks
+private final WeakReference<Activity> activityRef; // Weak reference to prevent memory leaks
private final OtpVerificationCallback callback; // Callback interface for OTP result eventsAnd update the constructor:
public FirebaseAuthService(Activity activity, OtpVerificationCallback callback) {
- this.activity = activity;
+ this.activityRef = new WeakReference<>(activity);
this.callback = callback;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final Activity activity; // Activity context required for Firebase callbacks | |
| private final OtpVerificationCallback callback; // Callback interface for OTP result events | |
| // Use a weak reference to the Activity to avoid leaking it if this service outlives the Activity | |
| - private final Activity activity; // Activity context required for Firebase callbacks | |
| + private final WeakReference<Activity> activityRef; // Weak reference to prevent memory leaks | |
| private final OtpVerificationCallback callback; // Callback interface for OTP result events | |
| public FirebaseAuthService(Activity activity, OtpVerificationCallback callback) { | |
| - this.activity = activity; | |
| + this.activityRef = new WeakReference<>(activity); | |
| this.callback = callback; | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java around lines 20 to 21,
the direct reference to Activity can cause memory leaks if the authentication
outlives the Activity lifecycle. Change the Activity field to a
WeakReference<Activity> to avoid holding a strong reference. Update the
constructor to accept an Activity and wrap it in a WeakReference. Modify all
usages of the Activity field to retrieve the Activity via get() on the
WeakReference and handle the case where it may be null.
| private void requestOtp() { | ||
| otpRequestTime = new DateTime(); | ||
| clearOtpError(); | ||
| // TODO: Call Firebase to request OTP | ||
| otpManager.requestOtp(primaryPhone); | ||
| } |
There was a problem hiding this comment.
Add null check for primaryPhone before requesting OTP.
The primaryPhone could be null if the fragment arguments are missing or incomplete, which could cause issues in the OTP request.
Add validation:
private void requestOtp() {
otpRequestTime = new DateTime();
clearOtpError();
+ if (primaryPhone == null || primaryPhone.isEmpty()) {
+ displayOtpError("Phone number is missing. Please go back and enter your phone number.");
+ return;
+ }
otpManager.requestOtp(primaryPhone);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void requestOtp() { | |
| otpRequestTime = new DateTime(); | |
| clearOtpError(); | |
| // TODO: Call Firebase to request OTP | |
| otpManager.requestOtp(primaryPhone); | |
| } | |
| private void requestOtp() { | |
| otpRequestTime = new DateTime(); | |
| clearOtpError(); | |
| if (primaryPhone == null || primaryPhone.isEmpty()) { | |
| displayOtpError("Phone number is missing. Please go back and enter your phone number."); | |
| return; | |
| } | |
| otpManager.requestOtp(primaryPhone); | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
around lines 280 to 284, add a null check for the variable primaryPhone before
calling otpManager.requestOtp(primaryPhone). If primaryPhone is null, avoid
making the OTP request to prevent potential errors. Implement a conditional
check that only calls requestOtp when primaryPhone is not null.
| private void verifyOtp() { | ||
| clearOtpError(); | ||
| String otpCode = binding.customOtpView.getOtpValue(); | ||
| // TODO: Call Firebase to verify OTP | ||
|
|
||
| if (verificationId == null) { | ||
| Toast.makeText(requireContext(), "Verification ID is missing. Send OTP first.", Toast.LENGTH_SHORT).show(); | ||
| return; | ||
| } | ||
|
|
||
| if (!otpCode.isEmpty()){ | ||
| otpManager.submitOtp(otpCode); | ||
| }else { | ||
| Toast.makeText(requireContext(), "Enter OTP", Toast.LENGTH_SHORT).show(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent error handling and improve code formatting.
The method uses Toast messages instead of the existing displayOtpError method, creating inconsistent user experience. Also, there's inconsistent spacing in the if statement.
Use consistent error handling:
private void verifyOtp() {
clearOtpError();
String otpCode = binding.customOtpView.getOtpValue();
if (verificationId == null) {
- Toast.makeText(requireContext(), "Verification ID is missing. Send OTP first.", Toast.LENGTH_SHORT).show();
+ displayOtpError("Please request a new verification code.");
return;
}
- if (!otpCode.isEmpty()){
+ if (!otpCode.isEmpty()) {
otpManager.submitOtp(otpCode);
- }else {
- Toast.makeText(requireContext(), "Enter OTP", Toast.LENGTH_SHORT).show();
+ } else {
+ displayOtpError("Please enter the verification code.");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void verifyOtp() { | |
| clearOtpError(); | |
| String otpCode = binding.customOtpView.getOtpValue(); | |
| // TODO: Call Firebase to verify OTP | |
| if (verificationId == null) { | |
| Toast.makeText(requireContext(), "Verification ID is missing. Send OTP first.", Toast.LENGTH_SHORT).show(); | |
| return; | |
| } | |
| if (!otpCode.isEmpty()){ | |
| otpManager.submitOtp(otpCode); | |
| }else { | |
| Toast.makeText(requireContext(), "Enter OTP", Toast.LENGTH_SHORT).show(); | |
| } | |
| } | |
| private void verifyOtp() { | |
| clearOtpError(); | |
| String otpCode = binding.customOtpView.getOtpValue(); | |
| if (verificationId == null) { | |
| displayOtpError("Please request a new verification code."); | |
| return; | |
| } | |
| if (!otpCode.isEmpty()) { | |
| otpManager.submitOtp(otpCode); | |
| } else { | |
| displayOtpError("Please enter the verification code."); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
around lines 286 to 300, replace the Toast messages used for error notifications
with calls to the existing displayOtpError method to maintain consistent error
handling. Also, fix the inconsistent spacing in the if-else statements by
ensuring proper spaces around parentheses and braces for better code
readability.
| private void validateFirebaseIdToken(String firebaseIdToken) { | ||
| ApiPersonalId.validateFirebaseIdToken(getActivity(),firebaseIdToken, new IApiCallback() { | ||
| @Override | ||
| public void processSuccess(int responseCode, InputStream responseData) { | ||
| navigateToNameEntry(); | ||
| } | ||
|
|
||
| @Override | ||
| public void processFailure(int responseCode) { | ||
| Logger.log("ERROR", String.format(Locale.getDefault(), "Failed: %d", responseCode)); | ||
| } | ||
|
|
||
| @Override | ||
| public void processNetworkFailure() { | ||
| ConnectNetworkHelper.showNetworkError(getActivity()); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenUnavailableError() { | ||
| ConnectNetworkHelper.handleTokenUnavailableException(requireActivity()); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenRequestDeniedError() { | ||
| ConnectNetworkHelper.handleTokenDeniedException(requireActivity()); | ||
| } | ||
|
|
||
| @Override | ||
| public void processOldApiError() { | ||
| ConnectNetworkHelper.showOutdatedApiError(getActivity()); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and user feedback for token validation failures.
The processFailure method only logs the error without informing the user. This could leave users confused when validation fails.
Add user feedback and fix formatting:
- ApiPersonalId.validateFirebaseIdToken(getActivity(),firebaseIdToken, new IApiCallback() {
+ ApiPersonalId.validateFirebaseIdToken(getActivity(), firebaseIdToken, new IApiCallback() {
@Override
public void processSuccess(int responseCode, InputStream responseData) {
navigateToNameEntry();
}
@Override
public void processFailure(int responseCode) {
Logger.log("ERROR", String.format(Locale.getDefault(), "Failed: %d", responseCode));
+ Toast.makeText(requireContext(), "Phone verification failed. Please try again.", Toast.LENGTH_LONG).show();
}Consider showing more specific error messages based on the response code (e.g., 401 for invalid token, 400 for malformed request).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void validateFirebaseIdToken(String firebaseIdToken) { | |
| ApiPersonalId.validateFirebaseIdToken(getActivity(),firebaseIdToken, new IApiCallback() { | |
| @Override | |
| public void processSuccess(int responseCode, InputStream responseData) { | |
| navigateToNameEntry(); | |
| } | |
| @Override | |
| public void processFailure(int responseCode) { | |
| Logger.log("ERROR", String.format(Locale.getDefault(), "Failed: %d", responseCode)); | |
| } | |
| @Override | |
| public void processNetworkFailure() { | |
| ConnectNetworkHelper.showNetworkError(getActivity()); | |
| } | |
| @Override | |
| public void processTokenUnavailableError() { | |
| ConnectNetworkHelper.handleTokenUnavailableException(requireActivity()); | |
| } | |
| @Override | |
| public void processTokenRequestDeniedError() { | |
| ConnectNetworkHelper.handleTokenDeniedException(requireActivity()); | |
| } | |
| @Override | |
| public void processOldApiError() { | |
| ConnectNetworkHelper.showOutdatedApiError(getActivity()); | |
| } | |
| }); | |
| } | |
| private void validateFirebaseIdToken(String firebaseIdToken) { | |
| - ApiPersonalId.validateFirebaseIdToken(getActivity(),firebaseIdToken, new IApiCallback() { | |
| + ApiPersonalId.validateFirebaseIdToken(getActivity(), firebaseIdToken, new IApiCallback() { | |
| @Override | |
| public void processSuccess(int responseCode, InputStream responseData) { | |
| navigateToNameEntry(); | |
| } | |
| @Override | |
| public void processFailure(int responseCode) { | |
| Logger.log("ERROR", String.format(Locale.getDefault(), "Failed: %d", responseCode)); | |
| + Toast.makeText(requireContext(), "Phone verification failed. Please try again.", Toast.LENGTH_LONG).show(); | |
| } | |
| @Override | |
| public void processNetworkFailure() { | |
| ConnectNetworkHelper.showNetworkError(getActivity()); | |
| } | |
| @Override | |
| public void processTokenUnavailableError() { | |
| ConnectNetworkHelper.handleTokenUnavailableException(requireActivity()); | |
| } | |
| @Override | |
| public void processTokenRequestDeniedError() { | |
| ConnectNetworkHelper.handleTokenDeniedException(requireActivity()); | |
| } | |
| @Override | |
| public void processOldApiError() { | |
| ConnectNetworkHelper.showOutdatedApiError(getActivity()); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
between lines 128 and 160, the processFailure method only logs the error without
providing user feedback. Enhance this method by adding user-visible error
messages based on the responseCode, such as showing a specific message for 401
(invalid token) and 400 (malformed request), using appropriate UI elements like
Toast or Dialog. Also, improve the log formatting for clarity.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
…est-connectid-verify-firebase-otp # Conflicts: # app/src/org/commcare/connect/network/ApiPersonalId.java # app/src/org/commcare/connect/network/connectId/ApiEndPoints.java
|
@jaypanchal-13 think this should target https://github.com/dimagi/commcare-android/pull/3113/files and not |
jaypanchal-13
changed the base branch from
master
to
CCCT-1157-sending-receiving-otp-firebase
…-verify-firebase-otp' into CCCT-1159-send-request-connectid-verify-firebase-otp # Conflicts: # app/src/org/commcare/utils/FirebaseAuthService.java # app/src/org/commcare/utils/OtpManager.java
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
…159-send-request-connectid-verify-firebase-otp # Conflicts: # app/src/org/commcare/connect/network/connectId/ApiEndPoints.java # app/src/org/commcare/connect/network/connectId/ApiService.java
|
@damagatchi retest this please |
app/src/org/commcare/connect/network/connectId/ApiEndPoints.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
| if (savedInstanceState != null) { | ||
| primaryPhone = savedInstanceState.getString(KEY_PHONE); | ||
| } |
There was a problem hiding this comment.
Yes for more clarification I created initOtpManager() which initialize otp callbacks and otpmanager
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
…159-send-request-connectid-verify-firebase-otp
app/src/org/commcare/connect/network/parser/FirebaseTokenValidationResponseParser.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException { | ||
| if (json.has("error")) { | ||
| sessionData.setSessionFailureCode(json.getString("error")); |
There was a problem hiding this comment.
I don't see this getting checked again to determine if it was a success/failure in the phone verification fragment. Am I missing something ?
There was a problem hiding this comment.
@shubham1g5 No it is not getting checked again but I created it because I am using createCallback() so that I have to not write different failure states of api
There was a problem hiding this comment.
but should not it get checked to decide success vs failure in your fragment ?
There was a problem hiding this comment.
for checking success and failure state validateFirebaseIdToken() is created in fragment
There was a problem hiding this comment.
can you hardcode an error here to test and make sure when there is an error we call the failure pathway in fragment and not success.
There was a problem hiding this comment.
Verified it in placing code in other screens failure is executing and not success
There was a problem hiding this comment.
I don't see anywhere thugh where we are directing to success vs failure based on getSessionFailureCode , Can you explain in detail with method call chain how is it working for you please.
There was a problem hiding this comment.
I have not pushed this testing code because it was just for testing purpose so you will not able to see it. getSessionFailureCode is not used to check success or failure state. FirebaseTokenValidationResponseParser is created just to use createCallback() so that I do not have to handle all the failure state(processFailure,processNetworkFailure etc) differently in fragment. And implemented this api /users/validate_firebase_id_token. I have created parser for it and tracked just failure state not success state. If I do not pass FirebaseTokenValidationResponseParser then it will through nullpointerexception so I created FirebaseTokenValidationResponseParser. Api is having response payload like
for failure
{
"error": "FAILED_VALIDATING_TOKEN"
}
for success
{
200
}
There was a problem hiding this comment.
i think its not necessary to create parser here you can just put the necessary check at the level of create callback because there is no data coming back from the api just 200 or not
There was a problem hiding this comment.
@pm-dimagi Yes as I want to show error msg in toast so I created FirebaseTokenValidationResponseParser to catch error msg
…159-send-request-connectid-verify-firebase-otp
…est-connectid-verify-firebase-otp
…est-connectid-verify-firebase-otp
| @Override | ||
| public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException { | ||
| if (json.has("error")) { | ||
| sessionData.setSessionFailureCode(json.getString("error")); |
There was a problem hiding this comment.
i think its not necessary to create parser here you can just put the necessary check at the level of create callback because there is no data coming back from the api just 200 or not
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
| // Handle validation errors | ||
| logNetworkError(response); | ||
| callback.processFailure(response.code()); | ||
| callback.processFailureWithParser(response.body().byteStream()); |
There was a problem hiding this comment.
when we are returning 1 failure what is the need of another one
There was a problem hiding this comment.
@pm-dimagi There is no parser to catch failure msg so created it
There was a problem hiding this comment.
but according to this doc there is no error filed in response have u asked this with charl
There was a problem hiding this comment.
@pm-dimagi Yes it is not mentioned in doc but I have confirmed it with Zandre.
|
@damagatchi retest this please |
…est-connectid-verify-firebase-otp # Conflicts: # app/res/values-fr/strings.xml # app/res/values-pt/strings.xml # app/res/values-sw/strings.xml # app/res/values-ti/strings.xml # app/res/values/strings.xml # app/src/org/commcare/connect/network/ApiPersonalId.java # app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java # app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java # app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java
|
@jaypanchal-13 @pm-dimagi I made some final changes here to keep things simple for now around error handling, specially I don't think we need multiple error parsers, so not trying to have one at the moment. |
Technical Summary
Added api call for validating firebase id token using
validate_firebase_id_tokenendpoints in PersonalIdPhoneVerificationFragment also added firebase otp implmentation in this PRhttps://dimagi.atlassian.net/browse/CCCT-1159
Feature Flag
new api added
Labels and Review