Resolved OTP issue #2923
Resolved OTP issue #2923
Conversation
Solve OTP issue
📝 WalkthroughWalkthroughThe changes in the Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Outside diff range and nitpick comments (2)
app/src/org/commcare/views/connect/CustomOtpView.java (2)
100-112: Consider additional keyboard event handlingThe deletion handling looks good, but consider these edge cases:
- Hardware keyboard events
- Different Android keyboard implementations
- Input Method Editor (IME) specific events
Consider adding a comment explaining the deletion behavior and testing with different keyboard types.
Line range hint
1-241: Consider security implications of OTP handlingWhile the implementation improvements are good, consider these security aspects:
- Add input sanitization to prevent injection attacks
- Implement clipboard handling policy for sensitive OTP data
- Consider auto-clear functionality after a timeout
- Add visual feedback for remaining time if OTP is time-sensitive
Would you like assistance in implementing any of these security enhancements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/views/connect/CustomOtpView.java(5 hunks)
🔇 Additional comments (1)
app/src/org/commcare/views/connect/CustomOtpView.java (1)
83-83: Document the rationale for removing the input length filter
While removing the length filter enables more flexible input handling (e.g., paste operations), this change should be documented to explain the design decision and potential implications.
Let's check if this change is consistent across the codebase:
✅ Verification successful
Removing input length filter is intentional and safe
The removal of the input length filter in CustomOtpView is consistent with the codebase as:
- The DecimalWidget's length filter (15 chars) serves a different purpose for decimal input
- The CustomOtpView still maintains input integrity through:
- InputType.TYPE_CLASS_NUMBER restriction
- TextWatcher implementation for handling input validation
- No other OTP implementations are affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar OTP input implementations that might need the same update
rg -l "InputFilter\.LengthFilter.*1.*" --type java
Length of output: 160
Script:
#!/bin/bash
# Let's check the context of LengthFilter usage in DecimalWidget.java
rg "InputFilter\.LengthFilter.*1.*" -B 2 -A 2 app/src/org/commcare/views/widgets/DecimalWidget.java
# Also check if there are any other OTP-related files
rg -l "OTP|otp" --type java
Length of output: 875
Script:
#!/bin/bash
# Let's check how CustomOtpView is used in other OTP-related files
rg "CustomOtpView" -B 2 -A 2 app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java
# Also check the implementation details around the commented line
rg "setFilters|TextWatcher|addTextChangedListener" -B 2 -A 2 app/src/org/commcare/views/connect/CustomOtpView.java
Length of output: 926
| private boolean isSelfTriggered = false; | ||
|
|
There was a problem hiding this comment.
Ensure thread safety of the isSelfTriggered flag
The isSelfTriggered flag could potentially cause issues in multi-threaded scenarios. Consider using AtomicBoolean or synchronization.
- private boolean isSelfTriggered = false;
+ private final AtomicBoolean isSelfTriggered = new AtomicBoolean(false);Also applies to: 123-124
| } else if (s.length() > 1) { | ||
| // If more than one character, replace with the latest input | ||
| editText.setText(String.valueOf(s.charAt(s.length() - 1))); | ||
| // editText.setSelection(1); // Move cursor to the end |
There was a problem hiding this comment.
Add input validation for numeric input
Since InputType.TYPE_CLASS_NUMBER is set, we should validate that the input is actually numeric to prevent potential crashes.
} else if (s.length() > 1) {
- editText.setText(String.valueOf(s.charAt(s.length() - 1)));
+ char lastChar = s.charAt(s.length() - 1);
+ if (Character.isDigit(lastChar)) {
+ editText.setText(String.valueOf(lastChar));
+ } else {
+ editText.setText("");
+ }📝 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.
| } else if (s.length() > 1) { | |
| // If more than one character, replace with the latest input | |
| editText.setText(String.valueOf(s.charAt(s.length() - 1))); | |
| // editText.setSelection(1); // Move cursor to the end | |
| } else if (s.length() > 1) { | |
| // If more than one character, replace with the latest input | |
| char lastChar = s.charAt(s.length() - 1); | |
| if (Character.isDigit(lastChar)) { | |
| editText.setText(String.valueOf(lastChar)); | |
| } else { | |
| editText.setText(""); | |
| } | |
| // editText.setSelection(1); // Move cursor to the end |
| editText.setText(""); | ||
| } | ||
| editText.clearFocus(); // Ensure no field remains focused | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance setOtp method with validation and notification
Consider these improvements:
- Validate that input contains only digits
- Notify listeners when OTP is set programmatically
public void setOtp(String otp) {
if (otp.length() > digitCount) {
throw new IllegalArgumentException("OTP length exceeds the digit count");
}
+ if (!otp.matches("\\d*")) {
+ throw new IllegalArgumentException("OTP must contain only digits");
+ }
for (int i = 0; i < digitCount; i++) {
EditText editText = (EditText) getChildAt(i);
if (i < otp.length()) {
editText.setText(String.valueOf(otp.charAt(i)));
} else {
editText.setText("");
}
editText.clearFocus();
}
+ // Notify listeners
+ if (otpChangedListener != null) {
+ otpChangedListener.onOtpChanged(otp);
+ }
+ if (otp.length() == digitCount && otpCompleteListener != null) {
+ otpCompleteListener.onOtpComplete(otp);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Solve OTP issue
Summary
Feature Flag
Product Description
PR Checklist
Automated test coverage
Safety story