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

Send Notification based on the E-Mail instead of the OneSignal User ID #40

Conversation

LKaemmerling
Copy link
Collaborator

As wished from many parts and needed by myself, hear it comes.

After this PR it is possible to send a Notification Based on the User E-Mail.(https://documentation.onesignal.com/docs/email-syncing-and-targeting)

To use this feature, you must sync the E-Mail first with OneSignal and than change the Method "routeNotificationForOneSignal()" on your notifiable Model to something like this:

public function routeNotificationForOneSignal()
{
      return ['email' => '[email protected]'];
}

Because of the new index 'email' the package knows that it should use the email filter instead of the OneSignalUserId.

Actually it isn't possible to use an array with multiple E-Mails, because of a limitation from the OneSignal Api.

This PR keeps the BC and doesn't require a change to existing installations of the package.

@LKaemmerling
Copy link
Collaborator Author

Travis fails because of an error from packagist, looks like they had a temporary issue. (because the test fails on only one php version)

Lloople
Lloople previously requested changes Jan 4, 2018

namespace NotificationChannels\OneSignal\Test;

class NotifiableEMail
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this class be named NotifiableEmail instead of NotifiableEMai? 🤔

@Lloople Lloople dismissed their stale review January 5, 2018 00:21

Solved

@Lloople Lloople merged commit 9e87947 into laravel-notification-channels:master Jan 5, 2018
@LKaemmerling LKaemmerling deleted the implement-email-targeting branch January 5, 2018 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants