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 push for subsequent subscriptions #4697

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Fix push for subsequent subscriptions #4697

merged 3 commits into from
Dec 2, 2021

Conversation

tarikeshaq
Copy link
Contributor

fixes #4691

Uses the client cached uaid when the server doesn't return one. If we didn't have a uaid, we error out, because we cannot return the subscription back to our consumer.

From the logs, it seems like we are hitting this only on subsequent subscriptions (i.e not the first one where we got the uaid)

I'll need to cut a release once this merges

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

@tarikeshaq tarikeshaq requested a review from mhammond December 2, 2021 02:16
@tarikeshaq tarikeshaq changed the title Fix web push Fix push for subsequent subscriptions Dec 2, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #4697 (21cc0eb) into main (5981760) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4697   +/-   ##
=======================================
  Coverage   80.53%   80.53%           
=======================================
  Files          48       48           
  Lines        5220     5220           
=======================================
  Hits         4204     4204           
  Misses       1016     1016           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5981760...21cc0eb. Read the comment docs.

@tarikeshaq
Copy link
Contributor Author

tarikeshaq commented Dec 2, 2021

Tested this locally and verified that it works (and that it was broken!!!)
How to repro if anyone is interested:

  • On fenix, sign in to Firefox accounts, this will create the first subscription
  • then, navigate to https://webpushdemo.azurewebsites.net/ (easy way to test web-push, the push notification will come in 5 seconds) and click on "initiate push"
  • quickly minimize the app,
  • 5 seconds later, nothing happens if you're on the latest nightly

With this patch on top of 82.2.0 (note that the latest main has breaking changes for fenix, so I'll create a dot-release on 82.2 that includes only this patch)
it works and 5 seconds later the push notification shows up, even if the app is minimized

@tarikeshaq tarikeshaq merged commit 531a088 into main Dec 2, 2021
@tarikeshaq tarikeshaq deleted the fix-web-push branch December 2, 2021 04:58
tarikeshaq pushed a commit that referenced this pull request Dec 2, 2021
* Fixes uaid not found in subscription response

* Adds test for using cached uaid
tarikeshaq pushed a commit that referenced this pull request Dec 2, 2021
* Fixes uaid not found in subscription response

* Adds test for using cached uaid
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.

Perma-failure with Push Communication Error: "response has no uaid"
3 participants