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

Location sharing with OneSignal now defaults to false #1352

Merged
merged 3 commits into from
Jan 5, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jan 3, 2024

Description

One Line Summary

Location sharing with OneSignal now defaults to false regardless of device permissions, so developers must explicitly set sharing to true.

Details

Motivation

It may be a poor developer experience to be opted-in by default. Previously, as long as device has location permission, we shared location with the OneSignal server. Now, sharing must be explicitly enabled. This aligns with recent similar Android behavior changes OneSignal/OneSignal-Android-SDK#1942.

Scope

⚠️ Behavior changes:

  • Developers must now explicitly call OneSignal.Location.isShared = true (Swift) or [OneSignal.Location setShared:true] to share location with OneSignal regardless of permission or prompting for location.

Fix location module bug

  • Use constants for Location and IAM module classes that are used with NSClassFromString: OneSignalLocationManager and OneSignalInAppMessages
  • These hardcoded classes can be easily missed. When the class OneSignalLocation was renamed to OneSignalLocationManager, some of the strings were missed in NSClassFromString calls. One consequence of this is the location module's start method never ran and location not shared.

Testing

Unit testing

None

Manual testing

iPhone 13 on iOS 17.1.2

  • Enable and then request permission → shares location when both steps are done
  • Request permission and then enable → shares location when both steps are done

Something that is not working is mid-session disabling and enabling doesn't drive location sharing until next cold start.

  1. Already sharing location
  2. Disable sharing - location stops being tracked and shared
  3. Enable sharing - location sharing doesn't start
  4. New cold start - location is shared

Affected code checklist

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

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • 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.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

* Location sharing with OneSignal will default to NO, regardless of prompting or location permission on the device
Details:
* Use constants for the 2 class names for the Location and IAM modules that are used by NSClassFromString: `OneSignalLocationManager` and `OneSignalInAppMessages`
Motivation:
* These can be easily missed. When the class `OneSignalLocation` was renamed to `OneSignalLocationManager`, some of the hardcoded strings were missed in `NSClassFromString` calls.
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Should we update the migration guide to call out the new default as well?

@nan-li
Copy link
Contributor Author

nan-li commented Jan 3, 2024

Should we update the migration guide to call out the new default as well?

Done. We will also need to update our docs calling this out for v5.1.0 and above.

@emawby emawby self-requested a review January 3, 2024 21:55
Copy link
Contributor

@jennantilla jennantilla left a comment

Choose a reason for hiding this comment

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

Looks great! Confirmed that isShared now defaults to false.

@nan-li nan-li changed the title Location sharing with OneSignal now defaults to false ⚠️ Location sharing with OneSignal now defaults to false Jan 5, 2024
@nan-li nan-li changed the title ⚠️ Location sharing with OneSignal now defaults to false Location sharing with OneSignal now defaults to false Jan 5, 2024
@nan-li nan-li merged commit a9a0e54 into main Jan 5, 2024
4 checks passed
@nan-li nan-li deleted the v5_location_sharing branch January 5, 2024 19:08
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.

None yet

3 participants