Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Update startGetLocation to only run if location is shared #1942

Merged
merged 7 commits into from
Dec 28, 2023

Conversation

jennantilla
Copy link
Contributor

@jennantilla jennantilla commented Dec 13, 2023

Description

One Line Summary

Update startGetLocation to only run if location is shared

Details

Motivation

In #1910 it was reported that when OneSignal.Location.setShared(false) but location permission is granted through a custom UI, OneSignal is still accessing and recording the device's location. Adding a conditional before startGetLocation to check whether isShared is true will prevent OneSignal from accessing the device's location when setShared is false.

Testing

Manual testing

The following steps were followed on and Android 14 emulator to recreate and test the fix:

  1. Add native code to request/grant location permission.
  2. Set OneSignal.Location.setShared(false)
  3. Restart app and observe that location is shared in OneSignal dashboard
  4. Add conditional to startGetLocation
  5. On a new device, request location permission natively
  6. Restart app and observe that location is not shared in OneSignal dashboard

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • Location
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

try {
if (!_locationController.start()) {
Logging.warn("LocationManager.startGetLocation: not possible, no location dependency found")
if (isShared) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend returning early instead if isShared is false, this keeps the code from getting to nested.

@nan-li
Copy link
Contributor

nan-li commented Dec 13, 2023

On every cold start, isShared defaults to false.

General question as I am not sure: what is the expected behavior when the app developer wants location to be shared with OneSignal?

They must set Location.isShared = true whenever they initialize OneSignal? Or is it expected to be a cached setting that persists across cold starts?

Also, from our docs: "Whether the location is currently shared with OneSignal. (defaults to true if your app has location permission)" suggests the default is true unless explicitly set to false. I think this PR may change that behavior? I did not test, but do you know what it looks like when you tested the scenario that shares location with OneSignal?

@jennantilla
Copy link
Contributor Author

@nan-li really good insight! While I did test the scenarios of setting setShared explicitly to true as well as omitting the method completely (it appears everything behaves as expected), I did not account for the fact that if setShared is set true outside of initialization (e.g., on a button click) that the value will not persist to a cold start.

I'm also not sure what the expected behavior for the app developer should be, but it appears that the current behavior is that setShared should be set whenever they initialize OneSignal.

I'll think about this one some more!

@jennantilla
Copy link
Contributor Author

@nan-li I've pushed two additional commit that account for the following requirements:

  • Update default value for isShared to false and to be accessed via preferences.
  • Allow for mid-Session updates by calling onLocationPermissionChange when set method is called (I'm not sure if there is a better way to do this or if I'm missing any gotchas).

Insight appreciated!

Since isShared now defaults to false, remove early return and add logging to update isShared to true.
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, but everything looks like they are working as expected!

- Change log to warn vs. error
- Access and save to constant PREFS_OS_LOCATION_SHARED
@jennantilla jennantilla merged commit c5f48ad into user-model/main Dec 28, 2023
2 of 3 checks passed
@jennantilla jennantilla deleted the fix/location_sharing branch December 28, 2023 18:35
@jennantilla jennantilla mentioned this pull request Dec 28, 2023
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
Fix: Update startGetLocation to only run if location is shared
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
Fix: Update startGetLocation to only run if location is shared
jinliu9508 pushed a commit that referenced this pull request Feb 6, 2024
Fix: Update startGetLocation to only run if location is shared
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants