FIX: Ensure permission denied in geolocation.getCurrentPosition rejects the Promise

[Jyrno42]

fixes #22535

Moves the permission request logic on Android M and above to Native side to mimic IOS behavior.

Changelog:

[Android] [Fixed] - Ensure permission denied in geolocation.getCurrentPosition rejects the promise

Test Plan:

Verified manually with a test project which requests location via Geolocation service. If the permission request is denied by the user the promise is rejected as well.

https://github.com/Jyrno42/rn-geoloctest

[Jyrno42]

The IOS test failure is not releated to this PR.

[matthargett]

can you extract these inline Callback ctors to named variables so the code is a bit easier to follow?

[Jyrno42]

Done. I will squash the commits once they have been approved.

[Jyrno42]

I also updated the test project to use the latest commit.

https://github.com/Jyrno42/rn-geoloctest

[pull-bot]

	Warnings
⚠️	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
⚠️	📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS

[Jyrno42]

Rebased and squashed

[Jyrno42]

@hramos @matthargett Is there anything else I need to do to get this merged?

[cpojer]

Unfortunately this PR would currently break code at FB. Could you instead create a new native method with this functionality, and keep the old native method around unchanged and then change your JS code to something like:

if (RCTLocationObserver.newMethod) {
  // handle the new method
} else {
  // Old code
}

Then I can land this code, and we can remove the old code soon.

[Jyrno42]

@cpojer

I can, however would like some clarification. So I keep LocationModule.getCurrentPosition as is and move the new logic to something like LocationModule.getCurrentPositionAndRequestPermissions.

and on the JS side getCurrentPosition becomes this:

getCurrentPosition: async function(
    geo_success: Function,
    geo_error?: Function,
    geo_options?: GeoOptions,
) {
    invariant(
        typeof geo_success === 'function',
        'Must provide a valid geo_success callback.',
    );

    if (LocationModule.getCurrentPositionAndRequestPermissions) {
        // Permission checks/requests are done on the native side
        RCTLocationObserver.getCurrentPosition(
            geo_options || {},
            geo_success,
            geo_error || logError,
        );
    } else {
        let hasPermission = true;
        // Supports Android's new permission model. For Android older devices,
        // it's always on.
        if (Platform.OS === 'android' && Platform.Version >= 23) {
            hasPermission = await PermissionsAndroid.check(
                PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION,
            );
            if (!hasPermission) {
                const status = await PermissionsAndroid.request(
                PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION,
                );
                hasPermission = status === PermissionsAndroid.RESULTS.GRANTED;
            }
        }
        if (hasPermission) {
            RCTLocationObserver.getCurrentPosition(
                geo_options || {},
                geo_success,
                geo_error || logError,
            );
        }
    }
}

Do I understand you correctly?

Also, I am wondering do you just strip the new native method during build in FB, or do you depend directly on the old native method not it's js proxy?

[cpojer]

Exactly, that should work for now :) We can then remove the old code in a couple of weeks.

[cpojer]

Hey @Jyrno42,

because of App rejections around Geolocation usage we decided to extract Geolocation into https://github.com/react-native-community/react-native-geolocation. You can ship your pull request in that repo, in the original form without the changes that I required as soon as the code appears there. This should be less work for you and allow you to move faster :)

[Jyrno42]

I was just going to push it but ok :)

[Jyrno42]

Filed as michalchudziak/react-native-geolocation#1