Passed location to start configuration call #3201
Conversation
📝 WalkthroughWalkthroughThis change introduces location permission handling and GPS location acquisition to the Personal ID phone fragment. A new string resource for location permission denial is added. The Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant Permissions
participant CommCareLocationController
participant System
User->>PersonalIdPhoneFragment: Opens fragment
PersonalIdPhoneFragment->>Permissions: Check location permission
alt Permission not granted
PersonalIdPhoneFragment->>Permissions: Should show rationale?
alt Show rationale
PersonalIdPhoneFragment->>User: Show permission error screen
else Request permission
PersonalIdPhoneFragment->>System: Request location permission
System-->>PersonalIdPhoneFragment: Permission granted/denied
end
else Permission granted
PersonalIdPhoneFragment->>CommCareLocationController: Start location updates
CommCareLocationController-->>PersonalIdPhoneFragment: onLocationResult (with location)
end
PersonalIdPhoneFragment->>User: Enable "Continue" if phone, consent, and location are valid
User->>PersonalIdPhoneFragment: Tap "Continue"
PersonalIdPhoneFragment->>System: Send configuration request (includes location)
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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: 3
🔭 Outside diff range comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
172-183: Button never re-enables after location arrives
updateContinueButtonState()gates onlocation != null, but the method is never invoked from the location callback.
Result: user enters valid phone + consent, GPS fix arrives, button stays disabled.@Override public void onLocationResult(@NonNull Location result) { location = result; locationController.stop(); + updateContinueButtonState(); // refresh UI }
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
270-296: Permission result logic – simplify and remove duplicated checks
allPermissionsGrantedcan be derived directly from theresultmap; no need to call back intoPermissions. This reduces race conditions on slight timing differences in the OS.-boolean allPermissionsGranted = !Permissions.missingAppPermission(requireActivity(), REQUIRED_PERMISSIONS); +boolean allPermissionsGranted = result.values().stream().allMatch(Boolean::booleanValue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(11 hunks)app/src/org/commcare/utils/Permissions.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
app/src/org/commcare/connect/ConnectConstants.java (1)
54-55: Constant looks fine – consider adding brief JavaDocThe new constant follows the existing naming and offset scheme; no functional issues spotted. A one-liner JavaDoc would make its intent searchable.
app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (1)
148-151: Order of operations may hide system-settings screen
navigateUp()first pops the fragment; immediately afterwardsGeoUtils.goToProperLocationSettingsScreen()starts an external Activity.
If the host Activity is finishing or the NavController is in an intermediate state, the settings intent might be dropped.-NavHostFragment.findNavController(this).navigateUp(); -GeoUtils.goToProperLocationSettingsScreen(activity); +GeoUtils.goToProperLocationSettingsScreen(activity); +NavHostFragment.findNavController(this).navigateUp();Reversing ensures the intent is fired before the nav stack mutates.
app/src/org/commcare/utils/Permissions.java (1)
64-78: Now acceptsActivity– ensure callers updatedMethod signatures were widened to
Activity, butacquireAllAppPermissionsstill demandsAppCompatActivity.
Double-check every caller; mixing types may cause accidental down-casts.
| private void checkPermission() { | ||
| if (Permissions.missingAppPermission(requireActivity(), REQUIRED_PERMISSIONS)) { | ||
| if (shouldShowPermissionRationale(requireActivity(), REQUIRED_PERMISSIONS)) { | ||
| navigateToPermissionErrorMessageDisplay(); | ||
| } else { | ||
| locationPermissionLauncher.launch(REQUIRED_PERMISSIONS); | ||
| } | ||
| } else { | ||
| locationController.start(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Infinite permission prompt loop risk
Inside checkPermission() a denial without “don’t ask again” shows the error screen instead of re-prompting, but inside registerLocationPermissionLauncher you recursively call launch() after the first denial.
This can loop indefinitely and blocks navigation.
Consider:
-else {
- locationPermissionLauncher.launch(REQUIRED_PERMISSIONS); // Retry asking
-}
+else if (shouldShowPermissionRationale(requireActivity(), REQUIRED_PERMISSIONS)) {
+ navigateToPermissionErrorMessageDisplay();
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java around
lines 258 to 268, the checkPermission method risks an infinite loop by
recursively calling locationPermissionLauncher.launch() after a denial without
"don't ask again". To fix this, refactor the permission request flow to avoid
recursive launches by handling the denial case gracefully—either by showing the
rationale or error screen without immediately re-launching the permission
prompt. Ensure that after a denial, the app does not automatically re-prompt
permissions but waits for user action.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
|
@coderabbitai 829a449 can you review this |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (2)
213-217: Possible NPE whenlocationis null
Same issue flagged earlier—GeoUtils.locationToString(location)will crash if a fix was never obtained. Guard with a null-check or fallback string.
294-299: Infinite permission-prompt loop still presentThe launcher recursively calls
launch()after a standard denial, recreating the loop noted in the previous review. Apply the earlier fix: only retry whenshouldShowPermissionRationalereturns true; otherwise show the error screen.
🧹 Nitpick comments (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
255-256: Unimplemented interface methods
requestNeededPermissionsis empty although the fragment declaresRuntimePermissionRequester. Either implement the logic or remove the interface to avoid dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/res/values/strings.xml
| enableContinueButton(isValidPhone && isConsentChecked && location!=null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
UX blocker – “Continue” button stays disabled until a fix is acquired
enableContinueButton(isValidPhone && isConsentChecked && location != null);
If the device takes a long time (or never) to obtain a location, users are effectively stuck even though phone + consent are valid. Consider making location optional or add a timeout / “Skip” fallback so that the flow is not blocked.
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java around
lines 180 to 181, the "Continue" button is disabled until a location fix is
acquired, blocking users if location is delayed or unavailable. Modify the logic
in enableContinueButton to make location optional or implement a timeout
mechanism that enables the button after a set period even if location is null.
Alternatively, add a "Skip" option to allow users to proceed without location,
ensuring the flow is not blocked when phone and consent are valid.
| @Override | ||
| public void onLocationResult(@NonNull Location result) { | ||
| location = result; | ||
| locationController.stop(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing UI refresh after location arrives
onLocationResult stores the location and stops updates but never re-evaluates the button state. The button therefore remains disabled even though location is now non-null (see check in updateContinueButtonState). Add a call:
public void onLocationResult(@NonNull Location result) {
location = result;
+ updateContinueButtonState(); // re-enable “Continue” when possible
locationController.stop();
}📝 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.
| @Override | |
| public void onLocationResult(@NonNull Location result) { | |
| location = result; | |
| locationController.stop(); | |
| } | |
| @Override | |
| public void onLocationResult(@NonNull Location result) { | |
| location = result; | |
| updateContinueButtonState(); // re-enable “Continue” when possible | |
| locationController.stop(); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java around
lines 228 to 233, the onLocationResult method updates the location and stops
location updates but does not refresh the UI state. To fix this, add a call to
updateContinueButtonState() at the end of onLocationResult to re-evaluate and
update the button state based on the new location value.
| private static final String[] REQUIRED_PERMISSIONS = new String[]{ | ||
| Manifest.permission.ACCESS_COARSE_LOCATION | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Requesting only ACCESS_COARSE_LOCATION may be insufficient for GPS coordinates
The fragment later calls GeoUtils.locationToString(location) and expects a GPS fix. Relying solely on ACCESS_COARSE_LOCATION can return cell / Wi-Fi based positions with low accuracy or even null when coarse providers are disabled.
Consider requesting ACCESS_FINE_LOCATION (or both) so that the location controller can fall back to GPS when needed.
-private static final String[] REQUIRED_PERMISSIONS = new String[]{
- Manifest.permission.ACCESS_COARSE_LOCATION
-};
+private static final String[] REQUIRED_PERMISSIONS = new String[]{
+ Manifest.permission.ACCESS_COARSE_LOCATION,
+ Manifest.permission.ACCESS_FINE_LOCATION
+};📝 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 static final String[] REQUIRED_PERMISSIONS = new String[]{ | |
| Manifest.permission.ACCESS_COARSE_LOCATION | |
| }; | |
| private static final String[] REQUIRED_PERMISSIONS = new String[]{ | |
| Manifest.permission.ACCESS_COARSE_LOCATION, | |
| Manifest.permission.ACCESS_FINE_LOCATION | |
| }; |
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java around
lines 70 to 72, the code only requests ACCESS_COARSE_LOCATION permission, which
may not provide accurate GPS coordinates needed by
GeoUtils.locationToString(location). Update the REQUIRED_PERMISSIONS array to
include ACCESS_FINE_LOCATION as well, ensuring the app can access fine location
data for better accuracy and fallback to GPS when necessary.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
| public void onLocationRequestFailure(@NonNull Failure failure) { | ||
| if (failure instanceof CommCareLocationListener.Failure.ApiException) { | ||
| Exception exception = ((CommCareLocationListener.Failure.ApiException)failure).getException(); | ||
| if (exception instanceof ResolvableApiException) { |
There was a problem hiding this comment.
shubham-> abstract as handleResolvableLocationException and resue.
There was a problem hiding this comment.
375ea98 removed this due to negative button handling
There was a problem hiding this comment.
think we should directly be calling exception.startResolutionForResult(this, LOCATION_SETTING_REQ) for ResolvableApiException . Also there is lot of common code for this method onLocationRequestFailure in different places in code and we should be abstracting it better and resusing it in all other places as well.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
@pm-dimagi This is true but it does not mean that we should show the permission dialog twice in a row on user clicking "Don't allow", this is not expected but result of how we are coding it (permission check getting called after user denies the request). If you look at other parts of code asking location permission, this is not the behaviour you would see there on UI. |
|
@shubham1g5 i have updated the videos with new flow |
Thanks, we should remove the old videos from there as they are no longer relevant. |
|
Other comments based on videos - Video1: User should not get "Process Failed" message on clicking Video2: -Change "Ok" button to say
|
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
| public void onLocationResult(@NonNull Location result) { | ||
| location = result; | ||
| updateContinueButtonState(); | ||
| locationController.stop(); |
There was a problem hiding this comment.
does calling stop twice on locationController doesn't cause a problem ?
There was a problem hiding this comment.
nope it dosent create issue but then also i handled it
1a5d890
|
| @Override | ||
| public void onDestroyView() { | ||
| super.onDestroyView(); | ||
| if (locationController != null) { |
There was a problem hiding this comment.
this will never be null here.
| @Override | ||
| public void onPause() { | ||
| super.onPause(); | ||
| if (locationController != null) { |
There was a problem hiding this comment.
null check is not required.
| public void requestNeededPermissions(int requestCode) { | ||
| } | ||
|
|
||
| private void checkLocationPermission() { |
There was a problem hiding this comment.
It seems like locationController itself checks for the locations permission and if not given calls missingPermissions() callback method. As such the logic to request permisssion should be handled instead in missingPermissions() method to align with our location framework better
| @Override | ||
| public void onResume() { | ||
| super.onResume(); | ||
| checkLocationPermission(); |
There was a problem hiding this comment.
we should instead call locationController.start(); directly here as the location controller itself check for permissions. Take a look at code in CommCareFusedLocationController
| public void onLocationRequestFailure(@NonNull Failure failure) { | ||
| if (failure instanceof CommCareLocationListener.Failure.ApiException) { | ||
| Exception exception = ((CommCareLocationListener.Failure.ApiException)failure).getException(); | ||
| if (exception instanceof ResolvableApiException) { |
There was a problem hiding this comment.
think we should directly be calling exception.startResolutionForResult(this, LOCATION_SETTING_REQ) for ResolvableApiException . Also there is lot of common code for this method onLocationRequestFailure in different places in code and we should be abstracting it better and resusing it in all other places as well.
| } | ||
|
|
||
| @Override | ||
| public void requestNeededPermissions(int requestCode) { |
There was a problem hiding this comment.
no reason to implement RuntimePermissionRequester for this class if we are not using requestNeededPermissions
| @@ -403,5 +385,10 @@ private void navigateToPermissionErrorMessageDisplay(int errorMeesage, int butto | |||
|
|
|||
| @Override | |||
| public void missingPermissions() { | |||
| if (Permissions.missingAppPermission(requireActivity(), REQUIRED_PERMISSIONS)) { | |||
There was a problem hiding this comment.
this is always true here and we should not need to check again.
| new LocationRequestFailureHandler.LocationResolutionCallback() { | ||
| @Override | ||
| public void onResolvableException(ResolvableApiException exception) { | ||
| handleNoLocationServiceProviders(); |
There was a problem hiding this comment.
looking at other implementation of this method in GeoPointActivity , it seems like we should not be calling handleNoLocationServiceProviders from onResolvableException but onNonResolvableFailure instead!
…removed unused import
…nto pm_location_fix_duplicate # Conflicts: # app/res/values/strings.xml # app/src/org/commcare/connect/ConnectConstants.java # app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java # app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
| exception.getResolution()).build(); | ||
| resolutionLauncher.launch(request); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
remove e.printStackTrace()
Product Description
This is the duplicate PR of this, its due to by mistake i have taken the pull of feature/connect in this because earlier i was working on phase 4 sorry for the mistake , i have taken care of all shubham comment in this attaching the video for reference
Screen_Recording_20250620_021956_Settings.mp4
Screen_Recording_20250620_022347_CommCare.Debug.mp4
Shubham asked behaviour:->
Behavior when we don't have location permission and on showing location dialogue user denies the permission
Behavior when we don't have location permission and on showing location dialogue user accepts the permission
We already have location permission but location is turned off in device settings
We already have location permission but device is not able to get location due to no network and no gps.
One thing to add more the dialog for asking the location is asked two times because it is general behaviour of the permission because
->When the user first denies a permission (taps "Don't Allow"), but does NOT check “Don’t ask again,” the system returns true for shouldShowRequestPermissionRationale().This is Android’s way of saying: “You should show the user an explanation before asking again.”
some of the method is empty because we are checking the permission at on resume
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review