Pm device compatiblity #3168
Pm device compatiblity #3168
Conversation
Handling a JSON Exception properly.
📝 WalkthroughWalkthroughThe changes introduce a device compatibility check in the Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant Device
participant MessageScreen
User->>PersonalIdPhoneFragment: Fragment created (onCreateView)
PersonalIdPhoneFragment->>Device: Check Android version
alt Device SDK < 24
PersonalIdPhoneFragment->>MessageScreen: Navigate with incompatibility error message
MessageScreen-->>User: Show error and block further interaction
else Device SDK >= 24
PersonalIdPhoneFragment->>PersonalIdPhoneFragment: Continue normal initialization
end
Possibly related PRs
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: 0
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
80-86:Details
❓ Verification inconclusive
Fix method name typo and verify API level requirement.
The compatibility check logic is sound, but there's a spelling error in the method name.
Apply this diff to fix the method name:
- private void checkDeviceCompability() { + private void checkDeviceCompatibility() {Also update the method call on line 61:
- checkDeviceCompability(); + checkDeviceCompatibility();Please verify that Android N (API 24) is the correct minimum requirement for the Personal ID feature.
🏁 Script executed:
#!/bin/bash # Description: Search for other Android version checks in the codebase to verify consistency # Search for Build.VERSION_CODES usage in PersonalID related files rg -A 3 -B 3 "Build\.VERSION_CODES\." --type java | grep -E "(PersonalId|Connect|Biometric|KeyStore)" # Search for API level requirements in PersonalID context rg -A 5 -B 5 "@RequiresApi|targetSdkVersion|minSdkVersion" --type java --type xmlLength of output: 11487
Rename method for clarity and confirm API 24 baseline
We should correct the typo in the method name and update its invocation. Also, double-check that blocking devices below API 24 is intentional, since there’s no other fallback or annotation enforcing API 24 for Personal ID and no manifest/Gradle minSdkVersion entry for this flow.
In
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(around lines 61 & 80):- private void checkDeviceCompability() { + private void checkDeviceCompatibility() { if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) { navigateToMessageDisplay( getString(R.string.device_incompatible_version_error), false); }Update the invocation (line 61):
- checkDeviceCompability(); + checkDeviceCompatibility();Please verify that Android N (API 24) is indeed the correct minimum requirement for the Personal ID feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
app/src/org/commcare/android/security/AesKeyStoreHandler.kt (1)
Build(15-54)
🔇 Additional comments (4)
app/res/values/strings.xml (1)
938-938: LGTM! Clear and user-friendly error message.The string resource provides appropriate guidance to users about device compatibility requirements.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (2)
5-5: LGTM! Appropriate import for device compatibility check.The Build import is correctly added to support SDK version checking.
61-61: Good placement of compatibility check.Calling the device compatibility check early in
onCreateViewbefore ViewModel initialization is the right approach to prevent further processing on incompatible devices.app/src/org/commcare/connect/PersonalIdManager.java (1)
510-514:Details
❓ Verification inconclusive
Verify the impact of escalating JSON parsing errors to runtime exceptions.
The change in exception handling behavior is significant - JSONException will now crash the application instead of being logged. This could affect application stability if JSON parsing errors are recoverable.
Please confirm this behavior change is intentional and verify the impact on error handling:
Consider whether JSON parsing errors should be handled more gracefully, especially for network responses that might be malformed due to temporary server issues.
🏁 Script executed:
#!/bin/bash # Description: Search for other JSONException handling patterns in the codebase # Search for other JSONException catch blocks to check consistency rg -A 5 -B 2 "catch.*JSONException" --type java # Search for usage of the getRemoteDbPassphrase method to understand call sites ast-grep --pattern $'getRemoteDbPassphrase($_, $_)' # Search for other RuntimeException throws to see if this pattern is used elsewhere rg -A 3 -B 2 "throw new RuntimeException" --type javaLength of output: 68484
Verify consistent error handling for JSON parsing in Connect module.
The new catch in PersonalIdManager.java now throws a RuntimeException on JSONException, causing a crash. While this matches ApiPersonalId.java’s approach, it diverges from PersonalIdApiHandler.java, which logs parse errors and invokes an onFailure callback. Please confirm this escalation is intentional and acceptable for application stability.
• app/src/org/commcare/connect/PersonalIdManager.java: catch JSONException → throw RuntimeException
• app/src/org/commcare/connect/network/ApiPersonalId.java: same unchecked‐throw pattern
• app/src/org/commcare/connect/network/PersonalIdApiHandler.java: logs and returns an error code on JSONException
|
@shubham1g5 updated the pr as discussed on slack |
| return true; | ||
| } | ||
|
|
||
| private boolean checkDeviceCompability() { |
|
@damagatchi retest this please |
Product Description
Check the version of the app and set the min version for personal iD is android N
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review