Skip to content

Send notifications to multiple interests #87

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dees040
Copy link

@dees040 dees040 commented Mar 18, 2022

The alymosul/exponent-server-sdk-php package supports sending a notification to multiple interests, however, this package forces you to only return one interest using the routeNotificationForExpoPushNotifications() method. I've changed this so that the developer can choose themselves if they want to return multiple interests or not. For me this is particularly useful because I've made a team notifiable and in the team routeNotificationForExpoPushNotifications() I can now return all intersets of users in that team.

Small suggestion

I personally would maybe change the way the routeNotificationForExpoPushNotifications() method works. Instead of returning interests, I maybe would allow developers to return actual tokens. This optionally gives them more flexibility to decide which token needs to be used when sending notifications to a notifiable model. If no routeNotificationForExpoPushNotifications() exists, you obviously would keep the current implementation. This would mean that the Expo class also need to change. Maybe a $expo->notifyDirectly(array $recipients, array $data, bool $debug); (maybe not the best name, just a quick thought).

The notify() method itself could use that method as well:

if (count($interests) == 0) {
    throw new ExpoException('Interests array must not be empty.');
}

// Gets the expo tokens for the interests
$recipients = $this->registrar->getInterests($interests);

return $this->notifyDirectly($recipients, $data, $debug);

Again, this would be useful for me. In my project I've my own API implementation for saving and managing tokens of a user because I need that to support multiple token types (Firebase and APN). Currently the package doesn't give me a lot of flexibility (which I get) and I need to override I few things in the service provider to accomplish this. This way instead of overriding service providers or creating your own expo repository you can just handle the token retrievement via the models which is also what I like about the notification system of Laravel 😄

What do you think about this idea? I could make a PR as a proof of concept.

@Alymosul
Copy link
Owner

Alymosul commented Apr 4, 2022

As far as I remember routeNotificationFor* methods have to return a notifiable object according to Laravel code design. Also tbh honest I'm kinda lost and don't get your idea 100% so I would suggest if you'd like you can submit the changes here in the PR and we can review it

@dees040
Copy link
Author

dees040 commented Apr 6, 2022

Hey @Alymosul, thanks for your reply!

What if the user has multiple devices. Currently we can only send a notification to one device. The Firebase and APN channels also support this feature. About the suggestion, I'll make another PR so that we can discuss this.

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.

4 participants