-
Notifications
You must be signed in to change notification settings - Fork 338
Fix push gateway with some push provider (Sunup/autopush) #5741
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
Conversation
Fix Push notifications with Mozilla's autopush that returns 406
|
Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:
|
jmartinesp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this fixed my issues with using Sunup! @bmarty since you're the most knowledgeable around unified push, would you like to take a look too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nitpick: since this isn't a constant, it should start with lower case. We can change this later though.
|
It seems like there are some unit tests to fix: https://github.com/element-hq/element-x-android/actions/runs/19391981774/job/55563077771?pr=5741 |
|
Oh yeah, I forgot to patch it |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5741 +/- ##
===========================================
- Coverage 79.71% 79.71% -0.01%
===========================================
Files 2422 2422
Lines 65059 65066 +7
Branches 8303 8304 +1
===========================================
+ Hits 51861 51865 +4
- Misses 10225 10229 +4
+ Partials 2973 2972 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bmarty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Content
This PR adds a few HTTP codes that imply the push server doesn't have any matrix gateway: it returns NoMatrixGateway instead of Error.
Fixes: #5723
Motivation and context
Autopush (Sunup push server) returns a 406 to the matrix gateway discovery request (GET to /_matrix/push/v1/notify), which is currently considered as a network error and therefore fallback to the stored push gateway if it exists or to the custom gateway *.
* It should fallback to the default gateway - but I'm opening another PR for this, in case there is a need I am not aware of
Tests
Tested devices
Checklist