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

Push notifications via unifiedpush don't work #38

Closed
darhma opened this issue Dec 9, 2024 · 7 comments
Closed

Push notifications via unifiedpush don't work #38

darhma opened this issue Dec 9, 2024 · 7 comments
Labels
bug Something isn't working upstream This issue is also applicable upstream

Comments

@darhma
Copy link

darhma commented Dec 9, 2024

Describe the bug
As per title push notifications via unifiedpush (using conversations/cheogram as provider/distributor) do not work.

To Reproduce
Steps to reproduce the behavior:
Enable conversations/cheogram as push notification provider

Expected behavior
You should receive push notifications

Screenshots
Screenshot_20241209-122652
Screenshot_20241209-122728

Smartphone (please complete the following information):

  • Device: Pixel 7
  • OS: GrapheneOS (Android 15)

Additional context

  • App version: 0.7.4.sc16

  • Store: F-Droid (official repo)

  • Homeserver: tchncs.de

  • Related experimental/labs settings:
    Screenshot_20241209-124934

  • Logs have been submitted using the app's built-in bug report (/"rageshake") mechanism: no

Upstream relevance

  • Does this issue exist in Element X Android? I didn't try
  • If this issue exists in Element X Android, please also link the corresponding bug report (create a new one if it doesn't exist yet)

Add any other context about the problem here.
With the "classic" version of schildichat push notifications work, the problem occurs only with SchildiChat Next.
I use unifiedpush with mercurygram (a telegram client) without problems and testing them with the UP-Example app the notifications work.

@SpiritCroc
Copy link
Member

element-hq/element-x-android#3571

@SpiritCroc SpiritCroc added upstream This issue is also applicable upstream bug Something isn't working labels Dec 10, 2024
@p1gp1g
Copy link
Contributor

p1gp1g commented Dec 13, 2024

This is because of element-hq/element-x-android#3388 (as I mention here element-hq/element-x-android#3571 (comment)) and I don't really see why they always return the custom gateway

It would be nice to submit a patch, and ask why they did this. I can't do this patch atm.

I see one possible reason they would do that (broken) patch:

  • If the application tests the custom gateway every time it receives a new endpoint, it may be possible a request to check the custom gateway timeout and they fallback to the default gateway but it shouldn't

If that's the case, then I think we can do this:

  • If the check receives a 200: we use the custom, if the check returns a 404: we use the default one, else, we keep the old one if any (and fallback to the default)

@p1gp1g
Copy link
Contributor

p1gp1g commented Dec 13, 2024

BTW, MSC4174 will help to get rid of this gateway. If someone wants to implement it for android, that'd be nice :)

@SpiritCroc
Copy link
Member

I'd be happy to include a patch to fix this even if upstream takes longer to realize it's needed. Those commits that are to blame don't look like an easy git revert though so I didn't have the time to actually figure out how such patch should look in detail.

@p1gp1g
Copy link
Contributor

p1gp1g commented Dec 13, 2024

I don't think the whole PR should be reverted. We just need to change libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushGatewayResolver.kt to:

  • return the customGateway if the check returns 200 and discoveryResponse.unifiedpush.gateway == "matrix"
  • return the default gateway if the check returns 200 and there isn't discoveryResponse.unifiedpush.gateway == "matrix"
  • return the default gateway if the check returns 404
  • return the previous gateway if it fails
    • if there isn't a previous gateway, we can use one or the other, it doesn't really matter

@SpiritCroc
Copy link
Member

Thanks! Yeah I don't think we should revert the whole thing. Maybe I can implement your suggestion later this week

@p1gp1g
Copy link
Contributor

p1gp1g commented Dec 13, 2024

That'd be awesome, feel free to ping me (github or matrix) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working upstream This issue is also applicable upstream
Projects
None yet
Development

No branches or pull requests

3 participants