laravel-notification-channels / onesignal

OneSignal notifications channel for Laravel
MIT License
283 stars 119 forks source link

Allow an empty subject #56

Closed timacdonald closed 6 years ago

timacdonald commented 6 years ago

The only required parameter of the OneSignalMessage should be the body / contents. However, due to how the subject was previously added in the toArray() method, OneSignal would throw an error due to an empty subject.

Let's see some code.

public function toOneSignal($notifiable)
{
    return OneSignalMessage::create()
        ->body($this->content($notifiable))
        ->setParameter('ios_badgeCount', 1)
        ->setParameter('mutable_content', true)
        ->setParameter('ios_badgeType', 'Increase')
        ->setParameter('ios_category', $this->iosCategory($notifiable))
        ->setParameter('android_group', $this->androidGroup($notifiable));
}

Here you can see I am not setting the subject. Now let's look at the toArray() method:

public function toArray()
{
    $message = [
        'contents' => ['en' => $this->body],
        'headings' => ['en' => $this->subject],
        // "headings" => ["en" => null]

OneSignal throws an error if you provide an empty value for the subject as above:

[2018-03-13 02:36:57] local.ERROR: Client error: `POST https://onesignal.com/api/v1/notifications` resulted in a `400 Bad Request` response:
{"errors":["Notification headings must not be null for any languages."]}
 {"exception":"[object] (GuzzleHttp\\Exception\\ClientException(code: 400): Client error: `POST https://onesignal.com/api/v1/notifications` resulted in a `400 Bad Request` response:
{\"errors\":[\"Notification headings must not be null for any languages.\"]}
 at /Users/tim/Documents/Gits/APP_NAME/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:113)
[stacktrace]
...

This PR implements a new method called subjectToArray() which will only return the language / value key pair if there is a subject set - otherwise an empty array is returned. This fixes the above OneSignal error.

timacdonald commented 6 years ago

Also, sorry about the PR spam - I've just been working a lot with this today and came across a few things - thanks for maintaining the package!

LKaemmerling commented 6 years ago

Nice catch! Thank you!

OT: Why would someone doesn't set a subject for a notification?

timacdonald commented 6 years ago

My current client wanted to ditch the subject and just have what content of the chat post was - so had to work out how to remove it :)