berkayk / laravel-onesignal

OneSignal Push Notifications for Laravel
505 stars 175 forks source link

Add two more methods, use new array notation #55

Closed mkwsra closed 6 years ago

mkwsra commented 7 years ago

I have added two new methods sendNotificationToUserByEmail & sendNotificationToFiltersToSegment

mkwsra commented 6 years ago

@berkayk It's really strange that you did not even make any comment regarding it or merge it?

Can you please let me know what's exactly going on? I hope that you just accept it pls! or correct it for me if you can and if there is anything that is incomplete.

Maykonn commented 6 years ago

@mkwsra:

1) Revert the private properties to protected again(to keep the compatibility),

2) Revert the file structure, e.g: Revert things like this:

public function sendNotificationToUser(
        $message,
        $userId,
        $url = null,
        $data = null,
        $buttons = null,
        $schedule = null
    ) {
// ....

To:

public function sendNotificationToUser($message, $userId, $url = null, $data = null, $buttons = null, $schedule = null) {
// ...

The { on methods you can keep in new line,

3) The requiresAuth method make senses but testCredentials is testing nothing, remove it.

berkayk commented 6 years ago

Closing this because it is already resolved by another pull request.