django-push-notifications
django-push-notifications copied to clipboard
jazzband
WIP: Do not grab FCM notification keys from `extra` kwargs
Currently: As a convenience, it appears that this package will grab keys and values from the extra dict that normally belong in the FCM notification payload.
Problem: Some android app frameworks (namely phonegap/cordova) require that you send the notification data inside the extra data payload and not the main notification payload (see https://github.com/phonegap/phonegap-plugin-push/blob/master/docs/PAYLOAD.md#notification-vs-data-payloads). The existing code prevents sending keys such as title and body in that additional data payload.
Suggested Change: Simply omit pulling the reserved keywords from the extra dict.
I have no yet updated the tests to reflect the change in behavior because I wanted to see if this was an acceptable change or not.
It is not required to send the notification in the data dict, so I don't think the fix is to remove functionality because some Android frameworks do not support FCM properly.
I understand it is an edge case, but currently it is a requirement for users of phonegap if they want to handle notification callbacks on Android.
Essentially, you need to skip the OS handling of the notification, the app reads in the data and recreates the OS level notification locally so when the user opens it the app's callback is trigger. In order for that to happen, the notification block must be empty and the attributes passed through the extra/data block.
Ultimately, the point is that if I'm passing key/value pairs into extra, I expect it to end up in the extra payload, rather than the notification payload.
Is the current behavior just a convenience, so a user can build a single dict and pass it in?
In that case, I would make a config option. The default behavior would be the current behavior, the optional behavior would be to pass along data dict as is, and maybe support passing in the notification dict as a kwarg.
I could implement a flag that does that.
It was my understanding that the extra keyword argument was supposed to map roughly to each services' data hash, is that not the case? I'm just curious what the cases are that the notification body title other attributes would be passed in there instead of directly as kwargs.
@akoumjian I would assume for simplicity. Regardless, because it is supported, your change may break the functionality of other users.
Has anyone tried Cordova with the following?
gcm_devices.send_message(use_fcm_notifications=False, **gcm_data)
use_fcm_notifications turns off the Android OS level handling of the data and passes everything to the app. Is this still necessary?
We should reject this PR given its age and likely hood that it will break existing apps that rely on OS level handling of notifications.
@akoumjian @jamaalscarlett
