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

fix: only throw error if response contains any failed data #2985

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

BiswaViraj
Copy link
Contributor

What change does this PR introduce?

Closes #2966

Throws error only when the notification fails, and should be able to send success response on notification being delivered successfully

Why was this change needed?

Notifications sent successfully were being marked as failed.

Other information (Screenshots)

@BiswaViraj BiswaViraj self-assigned this Mar 9, 2023
@linear
Copy link

linear bot commented Mar 9, 2023

NV-1817 🐛 Bug Report: APNS delivered notifications marked as failed (v 0.12.0)

📜 Description

We discovered something interesting while testing APNS integration for sending ios push notifications.

When we use a valid token, the notification is being received BUT we get an error (with an empty message) and the transaction is marked as failed.

When we use a wrong token then we get an error with the correct message and the transaction is also being marked as failed.

So, in both cases the transaction is marked as failed but in the first case we actually saw it coming in.

👟 Reproduction steps

  1. Setup APNS provider
  2. Create subscriber and notification template
  3. Send ios PUSH notification to the subscriber's device token

👍 Expected behavior

Message should be received and transaction should be marked as successful.

👎 Actual Behavior with Screenshots

Message is being received but transaction is marked as failed with an empty error.

{
  “stack”: “Error: \n    at APNSPushProvider.<anonymous> (/usr/src/app/providers/apns/build/main/lib/apns.provider.js:84:35)\n    at step (/usr/src/app/providers/apns/build/main/lib/apns.provider.js:44:23)\n    at Object.next (/usr/src/app/providers/apns/build/main/lib/apns.provider.js:25:53)\n    at fulfilled (/usr/src/app/providers/apns/build/main/lib/apns.provider.js:16:58)\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)“,
  “message”: “”
}

📃 Provide any additional context for the Bug.

No response

👀 Have you spent some time to check if this bug has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Contributing Guidelines?

Are you willing to submit PR?

None

Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟

@@ -44,7 +44,7 @@ export class APNSPushProvider implements IPushProvider {
});
const res = await this.provider.send(notification, options.target);

if (res.failed) {
if (res.failed.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BiswaViraj BiswaViraj added this pull request to the merge queue Mar 9, 2023
Merged via the queue into next with commit 56ddf76 Mar 9, 2023
@BiswaViraj BiswaViraj deleted the nv-1817-bug-report-apns-delivered-notifications branch March 9, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NV-1817] 🐛 Bug Report: APNS delivered notifications marked as failed (v 0.12.0)
2 participants