Pm qa regression #3061
Pm qa regression #3061
Conversation
…the phase merge, fixed user redicrection for wrong pin, and user deactivation flow
📝 WalkthroughWalkthroughThis set of changes spans multiple areas of the application. The Sequence Diagram(s)sequenceDiagram
participant User
participant SignupFragment
participant ScrollView
participant RecoverySection
User->>SignupFragment: Open Signup Screen
SignupFragment->>ScrollView: Display Main Content (Scrollable)
SignupFragment->>RecoverySection: Display Recovery Section (Anchored to Bottom)
sequenceDiagram
participant User
participant ConnectIdPinFragment
participant DBHelper
User->>ConnectIdPinFragment: Forgot PIN during registration confirmation
ConnectIdPinFragment->>DBHelper: Set registration phase to CHANGE_PIN
ConnectIdPinFragment->>ConnectIdPinFragment: Navigate to Change PIN flow
sequenceDiagram
participant User
participant ConnectIdUserDeactivateOTPVerificationFragment
User->>ConnectIdUserDeactivateOTPVerificationFragment: Open OTP Verification
ConnectIdUserDeactivateOTPVerificationFragment->>ConnectIdUserDeactivateOTPVerificationFragment: Wait for manual OTP input
User->>ConnectIdUserDeactivateOTPVerificationFragment: Enter OTP
ConnectIdUserDeactivateOTPVerificationFragment->>ConnectIdUserDeactivateOTPVerificationFragment: Verify OTP and handle resend with timer
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 5
🧹 Nitpick comments (2)
app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (2)
3-21: Remove now-unused SMS-retriever imports and constants
SmsRetrieverClient,SmsRetriever,SMSBroadcastReceiver,SMSListener,IntentFilter,
Build, and the twoRECEIVER_*static imports are no longer referenced after the SMS-consent flow was deleted.
Keeping them around will raise “unused import” lint errors and slightly increase build time.-import android.content.IntentFilter; -import android.os.Build; -... -import com.google.android.gms.auth.api.phone.SmsRetriever; -import com.google.android.gms.auth.api.phone.SmsRetrieverClient; -... -import org.commcare.connect.SMSBroadcastReceiver; -import org.commcare.connect.SMSListener; -... -import static android.content.Context.RECEIVER_EXPORTED; -import static android.content.Context.RECEIVER_NOT_EXPORTED;Also applies to: 44-47
199-202: Handler starts every time the fragment gains focus – confirm this is intentional
startHandler()runs every 100 ms and is now invoked inonResume().
If the user navigates away and back repeatedly the callbacks will accumulate unlessstopHandler()is always reached. Current implementation callsstopHandler()inonPause(), which is fine, but please ensure no other exit path skipsonPause()(e.g. dialog overlays).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/res/layout/fragment_signup.xml(3 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/LoginActivity.java(3 hunks)app/src/org/commcare/connect/ConnectIDManager.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-80)
app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-80)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (5)
app/res/values/strings.xml (1)
468-468: New string resource for app selection added correctly.The new string resource "app_select_message" with value "Select an app:" has been properly added with the translatable attribute. This string will help improve the UI clarity for app selection interfaces.
app/src/org/commcare/connect/ConnectIDManager.java (1)
497-501: Method relocation without functional changes.The
wasAppLaunchedFromConnectmethod has been moved within the class but retains the same functionality. This appears to be a code organization change without affecting behavior.app/src/org/commcare/activities/LoginActivity.java (3)
122-122: New field added for Connect integration.Added a new boolean field
connectLaunchPerformedto track when a Connect launch has been completed. This appears to be part of the Connect feature integration.
143-143: Field initialization in onCreate.The
connectLaunchPerformedfield is properly initialized tofalsein theonCreatemethod, which is the appropriate lifecycle method for field initialization.
406-410: Commented Connect auto-login placeholder code.Added commented code to override login credentials with Connect auto-login functionality. This is intentionally left commented, likely as a placeholder for future implementation or feature toggling.
Note that the code contains a TODO comment, indicating this is preparing for future Connect integration. When this code is uncommented, it would allow Connect to override the password authentication flow.
| if (json.has(ConnectConstants.CONNECT_KEY_SECRET)) { | ||
| password = json.getString(ConnectConstants.CONNECT_KEY_SECRET); | ||
| } |
There was a problem hiding this comment.
Improved JSON parsing with null-safety check.
The code now properly checks if the JSON response contains the expected CONNECT_KEY_SECRET key before attempting to extract its value. This prevents potential JSONException if the key is missing from the response.
This change improves error handling in the code, preventing potential crashes when the server response doesn't contain the expected key.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java around lines 338 to 340, the code now correctly checks if the JSON object contains the key CONNECT_KEY_SECRET before accessing it. This prevents JSONException when the key is missing. Ensure that the condition uses json.has(ConnectConstants.CONNECT_KEY_SECRET) before calling json.getString to safely extract the password value only if the key exists.
| <LinearLayout | ||
| android:id="@+id/ll_recover" | ||
| android:layout_width="match_parent" | ||
| android:layout_gravity="bottom" | ||
| android:orientation="vertical" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| android:layout_height="wrap_content"> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
ll_recover is only bottom-constrained – add start/end constraints for RTL safety
Without start/end constraints the view relies on match_parent width, which is acceptable but discouraged because RTL mirroring can break the position. Add explicit constraints:
<LinearLayout
android:id="@+id/ll_recover"
android:layout_width="match_parent"
android:orientation="vertical"
app:layout_constraintBottom_toBottomOf="parent"
+ app:layout_constraintStart_toStartOf="parent"
+ app:layout_constraintEnd_toEndOf="parent"
android:layout_height="wrap_content">🤖 Prompt for AI Agents (early access)
In app/res/layout/fragment_signup.xml around lines 229 to 235, the LinearLayout with id ll_recover only has a bottom constraint and uses match_parent width, which can cause layout issues in RTL languages. Add explicit start and end constraints to this LinearLayout to ensure proper positioning and RTL safety, replacing or supplementing the match_parent width usage.
| <ScrollView | ||
| android:id="@+id/firstLayout" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="0dp" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| android:orientation="vertical"> | ||
|
|
There was a problem hiding this comment.
Missing vertical constraints causes ScrollView to collapse to 0 dp
android:layout_height="0dp" only works when BOTH top and bottom constraints are defined.
At the moment the ScrollView is constrained only on the start side, so it will measure to zero height and the entire main content becomes invisible.
<ScrollView
android:id="@+id/firstLayout"
android:layout_width="match_parent"
- android:layout_height="0dp"
- app:layout_constraintStart_toStartOf="parent"
- android:orientation="vertical">
+ android:layout_height="0dp"
+ app:layout_constraintStart_toStartOf="parent"
+ app:layout_constraintTop_toTopOf="parent"
+ app:layout_constraintBottom_toTopOf="@+id/ll_recover"
+ android:fillViewport="true">Notes
android:orientationis not a valid attribute forScrollView; usefillViewportif you need it to fill the available area.- If you leave the constraints as-is the screen will appear blank on most devices.
📝 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.
| <ScrollView | |
| android:id="@+id/firstLayout" | |
| android:layout_width="match_parent" | |
| android:layout_height="0dp" | |
| app:layout_constraintStart_toStartOf="parent" | |
| android:orientation="vertical"> | |
| <ScrollView | |
| android:id="@+id/firstLayout" | |
| android:layout_width="match_parent" | |
| android:layout_height="0dp" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintTop_toTopOf="parent" | |
| app:layout_constraintBottom_toTopOf="@+id/ll_recover" | |
| android:fillViewport="true"> |
🤖 Prompt for AI Agents (early access)
In app/res/layout/fragment_signup.xml around lines 11 to 17, the ScrollView has android:layout_height="0dp" but lacks vertical constraints, causing it to collapse to zero height. To fix this, add both top and bottom constraints to the ScrollView to define its vertical position. Also, remove the invalid android:orientation attribute and if needed, add android:fillViewport="true" to make the ScrollView fill the available space.
| ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN); | ||
| directions = navigateToConnectidPinSelf(ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN, user.getPrimaryPhone(), "", true, false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential NPE & duplicated phase handling after 3 failed PIN attempts
When the user exceeds the max failure count (forgot == true in this branch) you dereference user.getPrimaryPhone() without first verifying that user is non-null.
During early-registration flows the DB may not yet contain a record, which will crash the app.
In addition, the exact same phase update/navigation block already exists in the success-with-forgot branch (lines 446-449). Consider extracting this into a helper to avoid divergence.
-ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN);
-directions = navigateToConnectidPinSelf(ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN, user.getPrimaryPhone(), "", true, false);
+ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN);
+String primary = user != null ? user.getPrimaryPhone() : phone /* fall back */;
+directions = navigateToConnectidPinSelf(
+ ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN,
+ primary,
+ "",
+ true,
+ false);📝 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.
| ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN); | |
| directions = navigateToConnectidPinSelf(ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN, user.getPrimaryPhone(), "", true, false); | |
| } | |
| ConnectDatabaseHelper.setRegistrationPhase( | |
| getActivity(), | |
| ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN); | |
| String primary = user != null | |
| ? user.getPrimaryPhone() | |
| : phone; // fall back if user is null | |
| directions = navigateToConnectidPinSelf( | |
| ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN, | |
| primary, | |
| "", | |
| true, | |
| false); |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java around lines 457 to 459, avoid potential NullPointerException by checking if 'user' is non-null before calling 'user.getPrimaryPhone()'. Also, since the phase update and navigation code is duplicated in the success-with-forgot branch (lines 446-449), extract this repeated block into a helper method that takes necessary parameters and call it from both places to reduce code duplication and prevent divergence.
| if (json.has(ConnectConstants.CONNECT_KEY_SECRET)) { | ||
| password = json.getString(ConnectConstants.CONNECT_KEY_SECRET); | ||
| } |
There was a problem hiding this comment.
Storing OTP secret in a variable named password is misleading
if (json.has(ConnectConstants.CONNECT_KEY_SECRET)) {
password = json.getString(ConnectConstants.CONNECT_KEY_SECRET);
}password originally holds the user’s actual password; after this assignment it now stores the OTP secret, a different credential. This shadowing can confuse maintainers and lead to subtle bugs (e.g., accidental reuse of the secret where the real password is expected).
Recommendation:
- password = json.getString(ConnectConstants.CONNECT_KEY_SECRET);
+ otpSecret = json.getString(ConnectConstants.CONNECT_KEY_SECRET);
+ this.secret = otpSecret; // or introduce a dedicated fieldUpdate subsequent method calls (confirmUserDeactivation) accordingly.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java around lines 265 to 267, the variable named 'password' is being reused to store the OTP secret, which is misleading and can cause confusion. Rename this variable to something more descriptive like 'otpSecret' to clearly distinguish it from the actual password. Also, update all subsequent method calls such as 'confirmUserDeactivation' to use the new variable name instead of 'password' when passing the OTP secret.
…ssue, fixed dimagi logo background
Product Description
Cherry picked the commits based on regression testing in phase 2
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review