Pm qa 7662 #3058
Pm qa 7662 #3058
Conversation
📝 WalkthroughWalkthroughThis set of changes spans UI layout adjustments, string resource updates, and significant refactoring in the Connect ID management code. The Sequence Diagram(s)sequenceDiagram
participant User
participant LoginActivity
participant ConnectManager
participant ConnectIDManager
User->>LoginActivity: Launch app (possibly via Connect)
LoginActivity->>ConnectManager: wasAppLaunchedFromConnect(appId)
ConnectManager->>ConnectManager: Check primedAppIdForAutoLogin
ConnectManager->>LoginActivity: Return boolean
Note over LoginActivity: Proceed based on result
sequenceDiagram
participant User
participant ConnectActivity
participant ConnectIDManager
User->>ConnectActivity: Complete activity (e.g., PIN/OTP flow)
ConnectActivity->>ConnectIDManager: handleFinishedActivity(this, resultCode)
ConnectIDManager->>ConnectActivity: Handle result, update state
Possibly related PRs
✨ 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:
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: 2
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (1)
5-6: Removed SMS broadcast receiver functionality.The SMS broadcast receiver implementation has been removed from this fragment, as indicated by the added imports but removed usage. This simplifies the code but users will now need to manually enter verification codes.
Note: These imports aren't being used and could be removed.
-import android.os.Build; -import static android.content.Context.RECEIVER_EXPORTED; -import static android.content.Context.RECEIVER_NOT_EXPORTED;Also applies to: 45-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/res/layout/fragment_signup.xml(3 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/LoginActivity.java(2 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(1 hunks)app/src/org/commcare/connect/ConnectIDManager.java(0 hunks)app/src/org/commcare/connect/ConnectManager.java(4 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(4 hunks)
💤 Files with no reviewable changes (1)
- app/src/org/commcare/connect/ConnectIDManager.java
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-84)
app/src/org/commcare/activities/LoginActivity.java (1)
app/src/org/commcare/connect/ConnectManager.java (1)
ConnectManager(64-584)
app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-84)
🔇 Additional comments (13)
app/src/org/commcare/activities/LoginActivity.java (2)
40-40: Added ConnectManager import for refactored method access.This import is needed to access the static
wasAppLaunchedFromConnectmethod that was previously in ConnectIDManager.
145-145:Details
✅ Verification successful
Method call updated to use ConnectManager instead of ConnectIDManager.
This change reflects a refactoring where the static method
wasAppLaunchedFromConnectwas moved from ConnectIDManager to ConnectManager, centralizing static Connect-related utility methods.
🏁 Script executed:
#!/bin/bash # Verify that wasAppLaunchedFromConnect is now defined in ConnectManager and not in ConnectIDManager echo "Searching for wasAppLaunchedFromConnect method in ConnectManager..." rg "public static boolean wasAppLaunchedFromConnect" app/src/org/commcare/connect/ConnectManager.java echo "Confirming method is no longer in ConnectIDManager..." rg "wasAppLaunchedFromConnect" app/src/org/commcare/connect/ConnectIDManager.javaLength of output: 515
Static utility method refactoring verified
The call in LoginActivity.java now correctly usesConnectManager.wasAppLaunchedFromConnect(presetAppId). Verification confirms the method is defined only in ConnectManager and removed from ConnectIDManager.• Defined in:
- app/src/org/commcare/connect/ConnectManager.java
public static boolean wasAppLaunchedFromConnect(String appId) { … }• No occurrences in:
- app/src/org/commcare/connect/ConnectIDManager.java
app/res/values/strings.xml (1)
409-409: Reduced the number of underscores in the OR separator.The string resource
choice_orwas updated to reduce the number of underscores from 13 to 11 on each side of "OR", likely for better visual formatting in UI layouts.app/src/org/commcare/activities/connect/ConnectActivity.java (1)
146-146:Details
✅ Verification successful
Updated ConnectID activity result handling logic.
The method call has been changed from a static call on
ConnectManagerto an instance method onConnectIDManager, and the parameter list has been simplified to only pass the context and result code.
🏁 Script executed:
#!/bin/bash # Verify that handleFinishedActivity is now defined in ConnectIDManager and not in ConnectManager echo "Searching for handleFinishedActivity method in ConnectIDManager..." rg "handleFinishedActivity.*resultCode" app/src/org/commcare/connect/ConnectIDManager.java echo "Confirming method is no longer in ConnectManager as a static method..." rg "static.*handleFinishedActivity" app/src/org/commcare/connect/ConnectManager.javaLength of output: 560
Approved: ConnectID activity result handling logic updated
Verified that
handleFinishedActivity(CommCareActivity<?> activity, int resultCode)is now an instance method inConnectIDManagerand that the static version has been removed fromConnectManager. All good to merge.app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (1)
457-458: Added database state update before navigation, improving flow consistency.This change ensures the registration phase is properly updated in the database before navigating when a user forgets their PIN during confirmation. This fixes a potential state inconsistency by aligning the database state with the UI navigation flow.
app/res/layout/fragment_signup.xml (3)
3-4: Improved layout using ConstraintLayout instead of FrameLayout.Replacing FrameLayout with ConstraintLayout provides better positioning control for the UI elements, which is particularly important for responsive design across different screen sizes.
Also applies to: 272-272
11-17: Added ScrollView to ensure content scrollability.The addition of ScrollView wrapping the main content allows users to scroll through the signup form on smaller screens while maintaining the layout structure. This improves usability on devices with limited screen real estate.
Also applies to: 226-227
229-234: Fixed recovery section positioning at the bottom of the screen.Using constraint positioning ensures the recovery section stays at the bottom of the screen regardless of the content above it, providing a consistent UI layout and better user experience.
app/src/org/commcare/connect/ConnectManager.java (4)
78-78: Removed redundant enum and delegated status management to ConnectIDManager.Replaced the internal ConnectIdStatus enum with ConnectIDManager.ConnectIdStatus, reducing code duplication and improving organization by centralizing status management in ConnectIDManager.
81-81: Changed primedAppIdForAutoLogin to static.Making this field static is appropriate since it's used as a global flag for application-wide auto-login functionality.
99-103: Updated status checks to use the external enum.Updated references to the ConnectIdStatus enum to use the one from ConnectIDManager, ensuring consistent status handling throughout the codebase.
165-169: Added wasAppLaunchedFromConnect method.This method checks if the app was launched from Connect by comparing the given appId with the stored primedAppIdForAutoLogin, and then clears the flag. This centralizes the app launch detection logic in ConnectManager.
app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (1)
271-273: Improved JSON parsing robustness.Added a null check for the CONNECT_KEY_SECRET key before extracting it from the JSON response, preventing potential NullPointerExceptions if the key is missing.
| if (json.has(ConnectConstants.CONNECT_KEY_SECRET)) { | ||
| password = json.getString(ConnectConstants.CONNECT_KEY_SECRET); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Added null-safety check for JSON parsing.
The code now checks if the key exists before attempting to extract the value, making the JSON parsing more robust against unexpected server responses that might not include the secret field.
This defensive programming approach prevents potential JSONException that could occur when a key is missing from the JSON response. This kind of defensive check should be applied consistently across all JSON parsing in the codebase.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java around lines 339 to 341, the code correctly checks for the presence of the key before accessing it in JSON parsing to avoid JSONException. Review all other JSON parsing instances in this file and add similar null-safety checks by verifying the key exists with json.has(key) before calling json.getString(key) or similar methods to ensure consistent defensive programming and prevent runtime exceptions.
| primaryPhone = ConnectIdUserDeactivateOTPVerificationFragmentArgs.fromBundle(getArguments()).getPrimaryPhone(); | ||
| username = ConnectIdUserDeactivateOTPVerificationFragmentArgs.fromBundle(getArguments()).getUsername(); | ||
| password = ConnectIdUserDeactivateOTPVerificationFragmentArgs.fromBundle(getArguments()).getPassword(); |
There was a problem hiding this comment.
Fixed argument extraction using the correct fragment args class.
Updated to use ConnectIdUserDeactivateOTPVerificationFragmentArgs instead of ConnectIdPhoneVerificationFragmentArgs, ensuring proper argument extraction for this specific fragment.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java around lines 101 to 103, the argument extraction was using the wrong fragment args class. Replace ConnectIdPhoneVerificationFragmentArgs with ConnectIdUserDeactivateOTPVerificationFragmentArgs to correctly extract primaryPhone, username, and password arguments specific to this fragment.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
@OrangeAndGreen I am not sure around the changes that are changing the fragment na directions and will need to you to verify these.
| public void onStart() { | ||
| super.onStart(); | ||
| registerBrodcastReciever(); | ||
|
|
||
| } |
There was a problem hiding this comment.
remove the whole thing.
| super.onPause(); | ||
| try { | ||
| requireActivity().unregisterReceiver(smsBroadcastReceiver); | ||
| stopHandler(); |
There was a problem hiding this comment.
a bit weird that we are calling stop in onPause and starting this in onCreate. Think we should be starting it in onResume if we want the timer to be active only when the screen is visible to the user.
Product Description
Fixed QA bugs on regression testing
Technical Summary
https://dimagi.atlassian.net/browse/QA-7662
Feature Flag
Safety Assurance
Safety story
I have checked the all the flows on deactivation and recovery flow
Automated test coverage
QA Plan
Labels and Review