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

Omit misleading fatal-level log for wrappers #1468

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Aug 6, 2024

Description

One Line Summary

Omit misleading fatal-level log for wrappers that always happens and is alarming, and is regularly reported by SDK users.

Details

Log Fatal log for null app ID only on native SDK

  • Wrapper SDKs will automatically initialize with null app ID.
  • On wrappers, this fatal log is very misleading, so only log if this happens on the native SDK.
  • Most wrappers also have type signatures indicating app ID should be non-null for the public initialize API
  • Note all wrappers except Flutter set the wrapper sdkType before initializing with null app ID. Requires Flutter update.

Remove setLaunchOptions from OneSignal header

  • This was added to the OneSignalFramework header file to expose to wrappers.
  • However, wrappers do not actually use this because they must initialize the SDK (not sufficient to only set launch options) so cold start click listeners can be triggered.
  • Remove from header so it is not misleading, which suggests wrappers behave differently than how they are.

Motivation

Wrapper SDK users regularly report the alarming FATAL: OneSignal AppId: (null) - AppId is null or format is invalid, stopping initialization, and it is misleading, and hides the real issue they have, if any.

Scope

An error log

Testing

Unit testing

Manual testing

Iphone 13 on iOS 17.5.1

  • Tested setting and not setting a OneSignalWrapper.sdkType before calling init with null app ID and seeing logs

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • 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
  • [ x 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

* Wrapper SDKs will automatically initialize with null app ID.
* On wrappers, this fatal log is very misleading, so only log if this happens on the native SDK.
* Most wrappers also have type signatures indicating app ID should be non-null for the public initialize API
* This was added to the OneSignalFramework header file to expose to wrappers.
* However, wrappers do not actually use this because they must initialize the SDK (not sufficient to only set launch options) so cold start click listeners can be triggered.
* Remove from header so it is not misleading, which suggests it is used.
@nan-li nan-li merged commit cf90a80 into main Aug 9, 2024
4 checks passed
@nan-li nan-li deleted the feat/fix_misleading_error_log_for_wrappers branch August 9, 2024 20:24
nan-li added a commit to OneSignal/OneSignal-Flutter-SDK that referenced this pull request Aug 9, 2024
* Set the wrapper SDK type first so when initialize is called, the native SDK can check if the SDK is a wrapper type in order to conditionally omit a misleading fatal log
* See OneSignal/OneSignal-iOS-SDK#1468
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.

2 participants