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

chore: Add a way to remove an invalid token if sendFCM returns 404 #32440

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

Gustrb
Copy link
Contributor

@Gustrb Gustrb commented May 15, 2024

Proposed changes (including videos or screenshots)

The old library (node-gcm) we used to send messages via the legacy push had some utilities to help us out implementing the specification described here on how to handle error codes, and we used to functions, _removeToken and _replaceToken to minimize the number of unnecessary errors we would propagate over.
Now, we had to implement the utility to send messages manually, in #32208 now, we implemented most of the error handling utilities in the function called fetchWithRetry, but there was a big one missing, the 404 case.
From the spec:

UNREGISTERED (HTTP error code = 404) App instance was unregistered from FCM. This usually means that the token used is no longer valid and a new one must be used.

This error can be caused by missing registration tokens, or unregistered tokens.
Missing Registration: If the message's target is a token value, check that the request contains a registration token.
Not registered: An existing registration token may cease to be valid in a number of scenarios, including:

  • If the client app unregisters with FCM.
  • If the client app is automatically unregistered, which can happen if the user uninstalls the application. For example, on iOS, if the APNs Feedback Service reported the APNs token as invalid.
  • If the registration token expires (for example, Google might decide to refresh registration tokens, or the APNs token has expired for iOS devices).
  • If the client app is updated but the new version is not configured to receive messages.

For all these cases, remove this registration token from the app server and stop using it to send messages.

OBS: not adding replaceToken because the new Firebase API does not return the new device token ever, so it is better to just remove it, and let it be recreated when a new push gets sent.

Issue(s)

CONN-205

Steps to test or reproduce

You can just follow: #32208

Further comments

Copy link
Contributor

dionisio-bot bot commented May 15, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 15, 2024

⚠️ No Changeset found

Latest commit: f940505

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@casalsgh casalsgh added this to the 6.9 milestone May 15, 2024
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.35%. Comparing base (99de6d2) to head (f940505).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32440      +/-   ##
===========================================
- Coverage    56.42%   56.35%   -0.07%     
===========================================
  Files         2447     2450       +3     
  Lines        53977    54019      +42     
  Branches     11125    11135      +10     
===========================================
- Hits         30457    30444      -13     
- Misses       20868    20922      +54     
- Partials      2652     2653       +1     
Flag Coverage Δ
e2e 56.16% <ø> (-0.06%) ⬇️
unit 71.90% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@Gustrb Gustrb marked this pull request as ready for review May 15, 2024 14:47
@casalsgh casalsgh modified the milestones: 6.9, 6.10 May 21, 2024
@casalsgh casalsgh added stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Jun 12, 2024
@kodiakhq kodiakhq bot merged commit bd9eb6b into develop Jun 12, 2024
47 of 50 checks passed
@kodiakhq kodiakhq bot deleted the chore/remove-fcm-tokens-on-404 branch June 12, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants