Skip to content

Conversation

@LKaemmerling
Copy link
Collaborator

see #47 for details

When this is merged, Version 1.2.0 should be released

@timacdonald
Copy link
Contributor

timacdonald commented Feb 20, 2018

I was just about to do a PR for this! Awesome!

I think that the logic change is breaking though. The only way that include_player_ids gets past the logic now is if it is not an array - however you will find this in the readme:

You can either return a single player-id, or if you want to notify multiple player IDs just return an array containing all IDs.

I just PR'd a test for this also which this PR fails.

My implementation of this (as I said I was about to PR haha) goes like this:

public function send($notifiable, Notification $notification)
{
    if (! $targeting = $notifiable->routeNotificationFor('OneSignal')) {
        return;
    }

    $payload = $this->addTargetingToPayload(
        $notification->toOneSignal($notifiable)->toArray(), $targeting
    );

    /** @var ResponseInterface $response */
    $response = $this->oneSignal->sendNotificationCustom($payload);

    if ($response->getStatusCode() !== 200) {
        throw CouldNotSendNotification::serviceRespondedWithAnError($response);
    }

    return $response;
}

protected function addTargetingToPayload($payload, $targeting)
{
    if ($this->isTargetingEmail($targeting)) {
        $payload['filters'] = collect([['field' => 'email', 'value' => $targeting['email']]]);
    } elseif ($this->isTargetingTags($targeting)) {
        $payload['tags'] = collect([$targeting['tags']]);
    } else {
        $payload['include_player_ids'] = collect($targeting);
    }

    return $payload;
}

protected function isTargetingEmail($targeting)
{
    return is_array($targeting) && array_key_exists('email', $targeting);
}

protected function isTargetingTags($targeting)
{
    return is_array($targeting) && array_key_exists('tags', $targeting);
}

This makes it much easier for devs to extend in the future to add more targeting options that might not yet be implemented in the package - by extending and overriding addTargetingToPayload rather than having re-write everything in the send method manually.

Hope I haven't overstepped the mark on this one - keen for this functionality to appear in the package either way! And thank you for maintaining this package!

Note: I only started playing with this package today - so I might be off in my assumptions and its functionality - sorry if that is the case.

@timacdonald timacdonald mentioned this pull request Feb 28, 2018
@LKaemmerling
Copy link
Collaborator Author

@Lloople can you review this? After this @timacdonald can you short rebase the #50 PR?

if (array_key_exists('email', $userIds)) {
$payload['filters'] = collect([['field' => 'email', 'value' => $userIds['email']]]);
} else {
if (array_key_exists('tags', $userIds)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can change this into elseif? the rest of the code looks good 🙂

@Lloople Lloople merged commit bdc50ed into master Feb 28, 2018
@LKaemmerling LKaemmerling deleted the implement-targetting-per-tags branch February 28, 2018 19:38
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.

4 participants