Skip to content

API Compatibility #246

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

Closed

Conversation

charles-ramos
Copy link

@charles-ramos charles-ramos commented May 13, 2024

New Pull Request Checklist

Issue Description

Adding API Compatibility, so he user can now send both 'notification' and 'data' as objects for push notifications for Android.

Additionally, adding compatibility with nested objects, so, JSON is now accepted and the same behaviors from GCM are now replicable for FCM push notifications.

Approach

TODOs before merging

  • Add tests
  • Add entry to changelog
  • Add changes to documentation (guides, repository pages, in-code descriptions)

Copy link

parse-github-assistant bot commented May 13, 2024

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

@mman
Copy link
Contributor

mman commented May 13, 2024

@charles-ramos I honestly do not have any opinion on this PR. Is this PR bringing in some previous functionality that was working before and is now broken?

@jimnor0xF
Copy link
Contributor

You can already use notification currently so it's unclear to me what this adds.

@charles-ramos
Copy link
Author

Hey @mman and @jimnor0xF, the idea is to keep everything compatible with the current code (GCM) that uses "data" for sending push notifications for Android. Right now, in case I want to send push notifications to everyone (android and iOS), I must use both "data" and "notification". With the PR I raised, it will be possible to keep using "data" for both.

@mman
Copy link
Contributor

mman commented May 14, 2024

@charles-ramos Are you using Parse Android SDK to receive the notifications? If you do could you post a snippet of your broadcast receiver that handles receiving the push?

@jimnor0xF
Copy link
Contributor

jimnor0xF commented May 14, 2024

@charles-ramos
Are you sure you are using the Parse Android SDK for push notifications?

Back when we were using the Parse Android SDK we only used the data key. When we moved to using the native Firebase Cloud Messaging SDK on the client side, we started using the notification key. The notification key with that SDK is used for notifications that should pop up in the system tray. data is reserved for background (silent) notifications.

@charles-ramos
Copy link
Author

Hi @mman and @jimnor0xF, I'm testing with the Javascript SDK.

Parse.Push.send({
  channels: [ "Giants", "Mets" ],
  data: {
    alert: "The Giants won against the Mets 2-3."
  }
})
.then(function() {
  // Push was successful
}, function(error) {
  // Handle error
});

With this PR, you can both send "notification" and "data" will work. Without that, the "data" will not work for FCM.

Also, it will ensure that those who use the Parse Dashboard will be able to send push from there. It sends a notification as an "alert".

@mtrezza
Copy link
Member

mtrezza commented May 15, 2024

@charles-ramos Could you please resolve the conflicts?

@charles-ramos
Copy link
Author

With the latest release (version 6.2.0), everything covered by this PR is already working, so it's no longer required to proceed with it. Thank you everyone.

@mtrezza
Copy link
Member

mtrezza commented May 15, 2024

Thanks for investigating

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