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

[Bug] iOS - requestPermission fallbackToSettings fix #1729

Merged

Conversation

aharonphytech
Copy link
Contributor

@aharonphytech aharonphytech commented Aug 12, 2024

Description

One Line Summary

The PR fixes bug in iOS in requestPermission method where the fallbackToSettings parameter has no effect.

Details

Motivation

In the Objective-C code, the fallbackToSettings parameter is of type BOOL. However, in JavaScript, boolean values (true and false) are passed as NSNumber objects when bridged to Objective-C. The __NSCFBoolean type seen in the lldb debugger is actually an NSNumber instance representing a boolean value. This leads to a type mismatch, causing the fallbackToSettings parameter to always be treated as truthy in the if statement (because NSNumber is a non-null object and will always be truthy, no matter which value it encapsulates), resulting in an unintended opening of the Open Settings prompt.

Scope

This change only affects the requestNotificationPermission method in the iOS SDK. It does not alter the behavior of any other methods or functionalities.

Other

I fixed the problem by changing how the fallbackToSettings parameter is handled in the Objective-C method. The issue arose because the fallbackToSettings parameter was being passed from JavaScript as an NSNumber (which is how boolean values are bridged between JavaScript and Objective-C), but the method in Objective-C was expecting a BOOL.

In Objective-C, a BOOL is a primitive type, whereas NSNumber is an object that can encapsulate a boolean value. Due to this mismatch, the condition if (fallbackToSettings) was always evaluating to true, because an NSNumber object is always non-null and thus truthy, regardless of whether it represents true or false.

To fix this, I ensured that the value passed as fallbackToSettings is properly unboxed from the NSNumber object to a BOOL. I did this by using [fallbackToSettings boolValue], which correctly interprets the NSNumber's encapsulated value as a BOOL. This way, the condition will properly evaluate to true or false based on the actual value passed from JavaScript.

Testing

Unit testing

None. This change addresses a specific issue in the interaction between JavaScript and Objective-C, which is difficult to cover with unit tests. Manual testing on actual devices provided sufficient validation for this fix.

Manual testing

  • iPhone 15 Pro running iOS 17.5.1

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
  • Outcomes
  • Sessions
  • In-App Messaging
  • 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

@aharonphytech aharonphytech changed the title [Bug] NSNumber boolean representation in fallbackToSettings parameter [Bug] iOS - requestPermission fallbackToSettings fix Aug 12, 2024
@nan-li nan-li self-requested a review August 15, 2024 21:20
@nan-li
Copy link
Contributor

nan-li commented Aug 15, 2024

Hi @aharonphytech thank you for submitting this PR!

@jennantilla jennantilla self-requested a review August 15, 2024 22:23
@jennantilla jennantilla merged commit 012f6de into OneSignal:main Aug 15, 2024
4 checks passed
@jennantilla jennantilla mentioned this pull request Aug 15, 2024
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