-
Notifications
You must be signed in to change notification settings - Fork 148
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
15.0.0 Issues #423
15.0.0 Issues #423
Conversation
cordova-airship/www/Airship.js
Outdated
@@ -386,6 +382,11 @@ airship.push.onNotificationResponse = function (callback) { | |||
return registerListener("airship.event.notification_response", callback) | |||
} | |||
|
|||
airship.push.onNotificationReceived = function (callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is covered by onPushReceived
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not working as expected.
How to reproduce
a. Cold start
- Close application
- Send push notification via API
- Tap on notification
- Application Opened
- Callback "onPushReceived" does not receive any event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be onNotificationResponse. onPushReceived is very hit or miss on Cordova since there is no way to start cordova in the BG on Android. On iOS you can do it if you send the push with content-available:1
. I believe cordova context will start to handle that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it is necessary to change MIGRATION.md
because it's not implemented here https://github.com/urbanairship/urbanairship-cordova/blob/main/cordova-airship/www/Airship.js
https://github.com/urbanairship/urbanairship-cordova/blob/main/MIGRATION.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so this should be remove though before i can merge
airship.contact.getNamedUser = function (success, failure) { | ||
argscheck.checkArgs('fF', 'Airship.contact.getNamedUser', arguments) | ||
perform("contact#getNamedUser", null, success, failure) | ||
airship.contact.getNamedUserId = function (success, failure) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess its currently broken so maybe what you have is fine
I think what you currently have is great. Going to have another engineer on my team look over this as well. Thanks for the PR! |
Thanks! |
What do these changes do?
Bug fix
Airship.contact.getNamedUserId should return an empty string "" instead "OK"
Airship.channel.getChannelId should return an empty string "" instead "OK"
Why are these changes necessary?
Should fix a lot of errors
How did you verify these changes?
Verification Screenshots:
Anything else a reviewer should know?