dcai / airnotifier

Push Notifications Server for Human Beings.
Other
593 stars 187 forks source link

fcm.process: get 'fcm' instead of 'payload' #212

Closed ghost closed 4 years ago

ghost commented 4 years ago

Hi, I noticed something weird and I hope it's not me misunderstanding:

In app.py:159 you call fcm.process with the arguments token, alert, extra, fcm.

However in fcm.py:91 you read the argument payload instead what I think should be fcm.

I changed it to be similar to apns.py process function.

EDIT: I just saw that you used the payload argument in push.py:116, so I also renamed it to fcm for consistency. Now the new calls are:

push.py fcmconn.process(token=self.token, alert=alert, extra=extra, fcm=fcm_payload)

app.py send_broadcast fcm.process(token=t, alert=alert, extra=extra, fcm=kwargs.get("fcm", {}))

and fcm.py process fcm_param = kwargs.get("fcm", {})

dcai commented 4 years ago

@lkstnr thanks for raising the PR.

I see you are renaming the named arguments in fcm.py, is this an coding issue you are concerned about or is it causing any bugs?

FYI apns.py in 2.x branch is deprecated which as been removed from latest code in favor of apples' new http/2 based apns2 protocol :) the new implemention uses payload param name: https://github.com/airnotifier/airnotifier/blob/master/pushservices/apns.py#L69

ghost commented 4 years ago

@dcai The naming is not causing any bugs, sorry if that was misleading.

My problem is, that the fcm param from this line is never being read in the fcm.process function

This applies to both branches.

And I think it should be consistent/similar to this line, so that it says payload= instead of fcm=

dcai commented 4 years ago

hi @lkstnr I agree with you, the naming is confusing, thanks for the suggestion.

However, I'm afraid to update 2.x branch to be honest, because it doesn't have unittest coverage, so I implemented the change in master branch with unit test coverage, you can find it here: https://github.com/airnotifier/airnotifier/commit/9cef643c58feacba0f76bb6c3ac9fc9eb94a037d

ghost commented 4 years ago

Thank you!

NHellFire commented 4 years ago

Looking at the code, this should fix the bug with not being able to supply extra data to FCM (like you could with v1 and GCM)