Edujugon / PushNotification

PHP and Laravel Package to send push notifications to Android and IOS devices.
MIT License
478 stars 159 forks source link

To handle FCM notification in backgrounded Android applications #70

Closed alvibd closed 5 years ago

alvibd commented 5 years ago

Added click_action for android and category for iOS in order to set action when the status bar notification is clicked. Moreover, changed GcmChannel buildData method to send data without the 'notifications' parameter in order to solve the problem mentioned in the following article: How to handle FCM notification in backgrounded Android applications

Edujugon commented 5 years ago

Hi @alvibd ,

Thanks for taking time on creating this PR. Really appreciate :)

You mentioned that you "changed GcmChannel buildData method to send data without the 'notifications' parameter". The package already sends extra payload under data key. You just have to add that extra payload via the extramethod from the Message class.

Something like:

public function toFcm($notifiable)
{
    return (new PushMessage)
        ->title('Hello world')
        ->body('yeah, it is me')
        ->extra([
            'key' => 'my payload'
        ]);
}

Is there anything I'm missing?

Besides that, I saw you add category for APN and click_actionfor GCM/FCM channels.

alvibd commented 5 years ago

Hi @Edujugon , I am not a subtle speaker, so bear with me on this please. I am working for an NGO, and as you can see I am not using pusher as at the moment we are on a budget. So, I am building my own notification system with database channel and your package's fcm channel.

My android developer colleagues want to use push notification to trigger loading the activities from the status bar. Although this can be achieved with the click_action on android, they told me that some parameters such as my jwt auth tokens cannot be passed from the "notification" key in push notification when the app is on the background.

They want to catch an event called "onMessageReceive" to catch the information on the "data" key in fcm push notification. This works fine when the app is running on the foreground, however, when the app is in the background, this "onMessageReceive" event isn't triggered and the "data" payload isn't received if the "notification" key exists in the push notification. I found in your "buildData" method in the "GcmChannel" it is not possible to omit the "notification" key.

/**
     * {@inheritdoc}
     */
    protected function buildData(PushMessage $message)
    {
        $data = [
            'notification' => [
                'title' => $message->title,
                'body' => $message->body,
                'sound' => $message->sound,
            ],
        ];

        if (! empty($message->extra)) {
            $data['data'] = $message->extra;
        }

        return $data;
    }

Even though I have created my own channel extending yout "FcmChannel" and overriding your "buildData" method. I thought it would be good idea to incorporate the changes I have made to your package, in order to completely omit the "notification" key from the push notification for what I am trying to achieve as I have seen quite a few people who have faced this problem on the internet. Moreover, I chose your package because I have seen it being maintained all the time, so If anytime google decides to change their fcm system my system will still be updated with just a simple composer update.

If you need more information on the matter kindly check the link I have provided in the PR description. Also while studying about this I found out about click_action and category for android and iOS respectively. As they help to take the app user to a certain 'activity' of the app instead of just launching the app and taking the user to the main activity when the status bar of push notification is tapped.

This would also be my first contribution to any git open source project, So I do apologize in advance if you find any lacking in my work, furthermore please do point out mistakes if you find any to improve my work and thank you for taking the time to read this. Enjoy your day :)

Edujugon commented 5 years ago

Hi @alvibd ,

Thanks for the details, now I understand exactly your point. It's actually a good feature to add to the package, allowing users to set the type of the message they want to send.

I'm thinking of adding a more explicit way to handle that. Say that you are defining the message you want to send on toFcm method and you can do something like this.

// For those that want to send only a data message type
public function toFcm($notifiable)
{
    return (new PushMessage)
        ->data([
            'key' => 'my payload'
        ])
       ->asDataMessageType();
}
//  For those that want to send payload inside a notification message type.
// Current behaviour
public function toFcm($notifiable)
{
    return (new PushMessage)
        ->title('Hello world')
        ->body('yeah, it is me')
        ->data([
            'key' => 'my payload'
        ]);
}

//  For those that want to send only payload but still want to send a notification message type.
// They may want to retrieve the data from the extras of the intent
public function toFcm($notifiable)
{
    return (new PushMessage)
        ->data([
            'key' => 'my payload'
        ]);
}

// Same result as above the one.
// Just for API consistency
public function toFcm($notifiable)
{
    return (new PushMessage)
        ->data([
            'key' => 'my payload'
        ])
       ->asNotificationMessageType();
}

So basically you would be able to set the type of the message you want to send, no matter the other fields. And the default behaviour would be to send a notification message type.

Moreover, I would also think of creating independent PushMessage classes per provider (APN/FCM) since looks like they will hold different options..

It still requires more thought, but it could be an interesting way of adding that new feature.

What are your thoughts?

alvibd commented 5 years ago

Hello @Edujugon , I had in mind the existing users of your package, wouldn't changing it mean your existing users have to update their code? However, I do agree with you. It is a cleaner solution.

I think we can take an approach like the withAndroid() method in this package

https://github.com/laravel-notification-channels/pusher-push-notifications/blob/master/src/PusherMessage.php

let me know your thoughts.

Edujugon commented 5 years ago

Hi @alvibd , Yeah, it would be a breaking change so the current users would have to adjust their code just a bit. But I guess it's worth to make the code cleaner and easy to maintain in the future.

Anyway, since this requires a bit extra thought and it will take me some time since I'm currently quite busy, I guess we can do a first step with your initial approach and when we have the new version, just release it.

alvibd commented 5 years ago

Hello @Edujugon , Thank you for the opportunity to learn from you. I'd see if I can help you out somehow. :)

Edujugon commented 5 years ago

No, thank you @alvibd for opening this PR and want to improve the package.

Regards

Edujugon commented 5 years ago

Thanks @alvibd for your contribution :)

alvibd commented 5 years ago

@Edujugon It was a pleasure :)