Skip to content
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

Feature/fcmv1 #311

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Feature/fcmv1 #311

wants to merge 3 commits into from

Conversation

rextor92
Copy link

@rextor92 rextor92 commented Mar 22, 2024

Description

I have added the newly created SendFcmV1NativeNotificationAsync methods to the INotificationHubClient so that we can continue mocking a client in projects after updating.

Also added some FCMv1 tests with the new payload structure for notifications.

Related PRs or issues

Fixes #307
Relates to #110

Misc

  • Upon running the tests without Notification Hub settings in appconfig.json (in RecordingMode.Playback), the newly added tests will fail as there are no files in MockData. I have ran the tests with my notification hub (RecordingMode.Recording) and can add the necessary files, but I am unsure whether you'd want to do it yourself against the sdk-sample-namespace ANH?
  • We are not just adding FCMv1 support here, GCM/FCM will be disabled on July 1st, 2024. I would like to add deprecated tags on the relevant methods - as in the Java implementation of the change. I am thinking of something like
    [Obsolete("SendFcmNativeNotificationAsync will be deprecated on July 1st, 2024. Please use SendFcmV1NativeNotificationAsync instead.")]
  • We'll also have to bump the package version to 4.2.1

Things to consider before you submit the PR:

  • Are tests passing locally?
  • Are the files formatted correctly?
  • Did you add unit tests?
  • Did you test your change with either the sample apps that are included in the repository or with a blank app that uses your change?

Created a set of tests around creating FCMv1 registrations and
sending FCMv1 notifications. Also updated some common tests
to use FCMv1 instead of FCM to future proof them a bit
@rextor92
Copy link
Author

@microsoft-github-policy-service agree

@rextor92 rextor92 marked this pull request as draft March 22, 2024 23:44
@developer9969
Copy link

hi - would it possible to know when this will be merged?

@rextor92 rextor92 marked this pull request as ready for review May 28, 2024 09:49
@rextor92
Copy link
Author

It's been sitting with some open questions for a while. I can see @jessHuh is typically assigned to review some recent PRs.

@developer9969
Copy link

ok - is it a case of lack of time for @jessHuh to have a look at this and move it forward?

@developer9969
Copy link

Greatly appreciated if this could be reviewed

@developer9969
Copy link

developer9969 commented Jun 23, 2024

this is just a joke now - Common what is the problem not even the bliming courtesy to say something

@lomagdal2
Copy link
Member

@rextor92 looks good.

No need for you to upload your test files. Go ahead and add the deprecation tags. Yes, package will be bumped to 4.2.1.

After you push up your changes I will sign off on this PR.

@lomagdal2 lomagdal2 self-requested a review July 1, 2024 23:28
@lomagdal2 lomagdal2 self-assigned this Jul 1, 2024
@gabsamples6
Copy link

gabsamples6 commented Aug 15, 2024

Hi - why does it take sooooo long to merge this one and push a new nuget??? It would help to know @lomagdal2

@TedMobile
Copy link

Any chance of this pull request being merged? Many thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants