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

[User Model] Rename Events & API Changes #1524

Merged
merged 30 commits into from
Jul 24, 2023

Conversation

jennantilla
Copy link
Contributor

@jennantilla jennantilla commented Jun 16, 2023

Description

One Line Summary

This is a large PR that includes changes to events and public APIs across multiple features in the SDK to reflect the updates in the native iOS and Android 5.0.0-beta4 SDKs.

Privacy

  • Rename RequiresPrivacyConsent and PrivacyConsent to ConsentRequired and ConsentGiven
  • Remove public getters for these 2 properties

Live Activities

  • Add LiveActivities namespace with enter and exit methods.

PermissionNative

  • Added PermissionNative property to the Notifications namespace & update Subscription model to return a new enum type, OSNotificationPermission, that is the notification permission status of the device. The status can be NotDetermined, Denied, Authorized, and in addition for iOS devices - Provisional, Ephemeral.

In App Messages

  • Support adding multiple In-App Message click/lifecycle listeners, and removal
  • Updated InAppMessages handlers to be invoked with addEventListener method (see below)
  • Separate out each In-App lifecycle event so they can be called separately
    OneSignal.InAppMessages.addEventListener('click', (event) =>{
      console.log('OneSignal IAM clicked:', event);
    });
    OneSignal.InAppMessages.addEventListener('willDisplay', (event) =>{
      console.log('OneSignal: will display IAM: ', event);
    });

    OneSignal.InAppMessages.addEventListener('didDisplay', (event) =>{
      console.log('OneSignal: did display IAM: ', event);
    });

    OneSignal.InAppMessages.addEventListener('willDismiss', (event) =>{
      console.log('OneSignal: will dismiss IAM: ', event);
    });

    OneSignal.InAppMessages.addEventListener('didDismiss', (event) =>{
      console.log('OneSignal: did dismiss IAM: ', event);
    });
  • Updated trigger value to be string instead of object and updated method signatures AddTrigger(string key, string value) and AddTriggers(Dictionary<string, string> triggers)

Notifications

  • Support adding multiple notification click listeners, and removal
  • Create notification event types and update Notifications handlers to be invoked with addEventListener method (see below):
    OneSignal.Notifications.addEventListener('click', (event) => {
      console.log('OneSignal: notification clicked:', event);
    });
    OneSignal.Notifications.addEventListener('foregroundWillDisplay', (event) => {
      event.preventDefault();
      // async work
      event.getNotifications.display();
};

Push Subscription

  • Update iOSonOSPushSubscriptionChange with OSPushSubscriptionState state parameter of previous and current

Details

Motivation

Update to updated user model API

Scope

In Live Activities, App Messages, Notifications, Subscriptions, Permissions, Privacy Consent

Testing

Manual testing

Tested naming updates on Samsung Galaxy S21 & Pixel 4 running Android 13 / 6th gen iPad running 16.4.

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
    • 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

@jennantilla jennantilla requested a review from nan-li June 16, 2023 02:25
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

A few points of feedback:

  • Left a comment on the migration guide in-line
  • Omit new files under android/.gradle and android./idea that were added i this PR
  • I see your other PRs covered the lint and other renaming

MIGRATION_GUIDE.md Show resolved Hide resolved
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.

Sorry there are so many comments. I didn't want to lose anything as I went through this PR.
I'm not going to request changes as your two other PRs build off this, especially the prettier PR is pretty hefty and touches all the files. We can look at making any changes in a later PR.

OR, we can always toss the current prettier PR changes and rerun prettier, and cherry pick the migration guide fixes, in order to add additional changes to this PR.

There are some extra files committed and usually the ones that sound like .gradle or .idea are extraneous files.

src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
ios/RCTOneSignal/RCTOneSignal.m Show resolved Hide resolved
ios/RCTOneSignal/RCTOneSignalEventEmitter.m Show resolved Hide resolved
ios/RCTOneSignal/RCTOneSignalEventEmitter.m Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@jennantilla
Copy link
Contributor Author

jennantilla commented Jul 17, 2023

@nan-li as disccussed, for a cleaner merge, all changes requested have been incorporated in this branch. Commit addressing each comment linked above. Thanks for the feedback!

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.

Approving as changes are addressed in following PR #1532

@nan-li nan-li requested a review from jkasten2 July 21, 2023 22:57
@jennantilla jennantilla merged commit be6676e into major_release_5.0.0 Jul 24, 2023
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