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 Spontaneous Multiplication of Push Notifications #1740

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

PeterNerlich
Copy link
Contributor

Short description

Sometimes, the Firebase API seems to not provide a message_id for submitted push notifications, while it is still received on subscribed devices. As it still sent HTTP status code 200, we assumed message_id was there and tried logging it, causing an exception. This lead to resend attempts and thus multiplied push notifications.

Proposed changes

  • Log API full response if no message_id is received on submitting a push notification, to gather further information
  • Deliberately don't raise an exception since PNs seem to be sent correctly, (but avoid previous exception from failing to log non-existent dict field)

Side effects

  • Scenarios where a push notification might fail with an error in the API response, but HTTP code 200, will not be retried. Instances of this will only be visible from the logs.

Resolved issues

Fixes: #1630


Pull Request Review Guidelines

…tification

don't raise an exception since PNs seem to be sent correctly,
but avoid exception from failing to log non-existent dict field
@PeterNerlich PeterNerlich requested a review from a team as a code owner October 6, 2022 15:13
@codeclimate
Copy link

codeclimate bot commented Oct 6, 2022

Code Climate has analyzed commit b393ba7 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 0.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 74.3%.

View more on Code Climate.

Copy link
Member

@ulliholtgrave ulliholtgrave left a comment

Choose a reason for hiding this comment

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

Looks good. I wasn't able to reproduce this issue locally, but your explanation makes sense and I would be up for bringing this on production 👍

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Looks good! 👍
Thanks a lot for investigating the problem and for the detailed explanation 🙏

@PeterNerlich PeterNerlich merged commit 5c3265e into develop Oct 6, 2022
@PeterNerlich PeterNerlich deleted the bugfix/push-notification-multiplication branch October 6, 2022 20:06
@PeterNerlich
Copy link
Contributor Author

Further detail that might be helpful: If I didn't get it wrong while quickly googling this, this section of the API doc outlines some error conditions while still sending HTTP status code 200.

timobrembeck added a commit that referenced this pull request Oct 17, 2022
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.

Push Notification sent but status not updated due to missing firebase response ID
3 participants