fix bug 🐛 push notifications not working for node #282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
loks0n
requested changes
May 7, 2024
| return res.json({ ok: true, messageId: response }); | ||
| } catch (e) { | ||
| error(e); | ||
| return res.json({ ok: false, error: 'Failed to send the message' }, 500); | ||
| return res.json({ ok: false, error: `failed due to ${e}` }, 500); |
There was a problem hiding this comment.
We prefer not to expose the error message to the user, can you revert this?
| }, | ||
| token: req.deviceToken, | ||
| // extra options payload here |
Comment on lines
4
to
10
| throwIfMissing(process.env, [ | ||
| 'FCM_PROJECT_ID', | ||
| 'FCM_PRIVATE_KEY', | ||
| 'FCM_CLIENT_EMAIL', | ||
| 'FCM_DATABASE_URL', | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Please move the throwIfMissing this to the top of the function
| try { | ||
| throwIfMissing(req.body, ['deviceToken', 'message']); | ||
| throwIfMissing(req.body.message, ['title', 'body']); | ||
| } catch (err) { | ||
| return res.json({ ok: false, error: err.message }, 400); | ||
| } | ||
|
|
||
| log(`Sending message to device: ${req.body.deviceToken}`); |
Comment on lines
13
to
20
| admin.initializeApp({ | ||
| credential: admin.credential.cert({ | ||
| projectId: process.env.FCM_PROJECT_ID, | ||
| clientEmail: process.env.FCM_CLIENT_EMAIL, | ||
| privateKey: process.env.FCM_PRIVATE_KEY, | ||
| }), | ||
| databaseURL: process.env.FCM_DATABASE_URL, | ||
| });} |
| databaseURL: process.env.FCM_DATABASE_URL, | ||
| }); | ||
| return await admin.messaging().send(payload); | ||
| console. log(`Message: ${JSON.stringify(payload)}`); |
|
Done with all the requested changes. Can you please review my code? @loks0n. |
loks0n
requested changes
May 22, 2024
Comment on lines
4
to
10
| throwIfMissing(process.env, [ | ||
| 'FCM_PROJECT_ID', | ||
| 'FCM_PRIVATE_KEY', | ||
| 'FCM_CLIENT_EMAIL', | ||
| 'FCM_DATABASE_URL', | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Please move the throwIfMissing this to the top of the function
|
moved |
loks0n
approved these changes
May 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR fix the issue where push notification does not work correctly for
push-notification-with-fcmtemplate for node.Test Plan
When trying to use push notification fcm cloud function from templates, I found that the function
is not working correctly.
Then, I came across this bug 🐛.
On previous code:
main.js - line : 26
token: req.deviceToken,this will be
token: req.body.deviceToken,for that reason the correct device token was not reaching to the
sendPushNotification().on utils.js
everytime we were calling sendPushNotification function we were initializing
admin.initializeApp ,
This gives the error
firebase-admin already initialized.To fix we need to initialize only once, so initializing outside before calling the sendPushNotification().
after modifying all this it is now working fine.
Feature added :
added a feature to pass additional
datapayload within the message with the notification.Have you read the Contributing Guidelines on issues?
Yes