Skip to content

Conversation

@timacdonald
Copy link
Contributor

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
Copy link
Contributor Author

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 LKaemmerling merged commit 5d2f0fa into laravel-notification-channels:master Mar 13, 2018
@LKaemmerling
Copy link
Collaborator

Nice catch! Thank you!

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

@timacdonald
Copy link
Contributor Author

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 :)

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