Push provider switch #2873

bmarty · 2024-05-17T16:22:53Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Add an advanced setting entry to switch between available push providers: Firebase, and available UnifiedPush distributors.
Also properly unregister existing pushers and update the codebase for everything to work as expected.

Please note that matrix.org user may have issue getting push from UnifiedPush since there is some rate limiting. This will be handled separately.

Motivation and context

Screenshots / GIFs

Tests

Install the application
Install the application ntfy from https://f-droid.org/fr/packages/io.heckel.ntfy/ for instance
Use the application and ensure that push is received
Go to settings / advanced settings and switch the push provider to ntfy.
The push should still be received (with the limitation on matrix.org)
You can also check on the notification troubleshoot screen that Push are received (even for matrix.org users)
It's also possible to check the registered pushers by using Element Android on the same account: go to setting / advanced settings / Notification targets to see the list of active pushers.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

ElementBot · 2024-05-17T16:24:11Z

	Warnings
⚠️	gradle/libs.versions.toml#L6 - A newer version of com.android.tools.build:gradle than 8.4.0 is available: 8.4.1
⚠️	gradle/libs.versions.toml#L12 - A newer version of androidx.core:core-ktx than 1.13.0 is available: 1.13.1
⚠️	gradle/libs.versions.toml#L18 - A newer version of androidx.datastore:datastore-preferences than 1.0.0 is available: 1.1.1
⚠️	gradle/libs.versions.toml#L21 - A newer version of androidx.lifecycle:lifecycle-runtime-ktx than 2.7.0 is available: 2.8.0
⚠️	gradle/libs.versions.toml#L22 - A newer version of androidx.activity:activity-compose than 1.8.2 is available: 1.9.0

	Messages
📖	gradle/libs.versions.toml#L136 - There are multiple dependencies junit:junit but with different version
📖	gradle/libs.versions.toml#L138 - There are multiple dependencies androidx.test.ext:junit but with different version
📖	gradle/libs.versions.toml#L207 - There are multiple dependencies junit:junit but with different version
📖	gradle/libs.versions.toml#L208 - There are multiple dependencies androidx.test.ext:junit but with different version

Generated by 🚫 dangerJS against 4ab0202

github-actions · 2024-05-17T16:31:16Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/ZpaH9f

ganfra

Some minor remarks, otherwise LGTM

ganfra · 2024-05-20T09:17:09Z

...in/kotlin/io/element/android/features/preferences/impl/advanced/AdvancedSettingsPresenter.kt

@@ -78,6 +147,9 @@ class AdvancedSettingsPresenter @Inject constructor(
            isSharePresenceEnabled = isSharePresenceEnabled,
            theme = theme,
            showChangeThemeDialog = showChangeThemeDialog,
+            pushDistributor = currentDistributorName,


rename currentPushDistributor to be clear?

ganfra · 2024-05-20T09:17:37Z

...in/kotlin/io/element/android/features/preferences/impl/advanced/AdvancedSettingsPresenter.kt

@@ -78,6 +147,9 @@ class AdvancedSettingsPresenter @Inject constructor(
            isSharePresenceEnabled = isSharePresenceEnabled,
            theme = theme,
            showChangeThemeDialog = showChangeThemeDialog,
+            pushDistributor = currentDistributorName,
+            pushDistributors = distributorNames.toImmutableList(),


availablePushDistributors ?

ganfra · 2024-05-20T09:20:40Z

...in/kotlin/io/element/android/features/preferences/impl/advanced/AdvancedSettingsPresenter.kt

+
+        // List of PushProvider -> Distributor
+        var distributors by remember { mutableStateOf<List<Pair<PushProvider, Distributor>>>(emptyList()) }
+        var distributorNames by remember { mutableStateOf<List<String>>(emptyList()) }


I'd use derivedStateOf instead of changing both field in the LaunchedEffect

Yeah, both calls are sync and initialized once so I don't see why we need to make these values mutable states, just calculating them inside the remember for distributors one and derivedStateOf for distributorNames should be simpler.

Good point, thanks. 97aa57b should be OK, just initialize data in remember (no need to use derivedStateOf). OK for you both?

frebib · 2024-05-20T11:07:09Z

Is there a specific reason to add the toggle to the "Advanced Settings" menu and not the "Notifications" menu? The latter would make more sense to me

bmarty · 2024-05-21T07:37:06Z

Is there a specific reason to add the toggle to the "Advanced Settings" menu and not the "Notifications" menu? The latter would make more sense to me

We discussed internally about it, and concluded that this feature is quite an advanced feature and we do not want to confuse users when they want to change their notification settings.

codecov · 2024-05-21T08:32:22Z

Codecov Report

Attention: Patch coverage is 29.26829% with 145 lines in your changes are missing coverage. Please review.

Project coverage is 74.24%. Comparing base (15e7a9d) to head (4ab0202).
Report is 5 commits behind head on develop.

Files	Patch %	Lines
.../unifiedpush/VectorUnifiedPushMessagingReceiver.kt	0.00%	20 Missing ⚠️
...ies/pushproviders/firebase/FirebasePushProvider.kt	0.00%	16 Missing ⚠️
...ment/android/libraries/push/impl/PushersManager.kt	0.00%	15 Missing ⚠️
...roviders/unifiedpush/RegisterUnifiedPushUseCase.kt	0.00%	14 Missing ⚠️
.../pushproviders/firebase/FirebaseNewTokenHandler.kt	0.00%	13 Missing ⚠️
.../android/libraries/push/impl/DefaultPushService.kt	0.00%	11 Missing ⚠️
...viders/unifiedpush/UnifiedPushNewGatewayHandler.kt	0.00%	10 Missing ⚠️
...viders/unifiedpush/UnregisterUnifiedPushUseCase.kt	0.00%	10 Missing ⚠️
.../preferences/impl/advanced/AdvancedSettingsView.kt	65.21%	0 Missing and 8 partials ⚠️
...edpush/registration/EndpointRegistrationHandler.kt	0.00%	7 Missing ⚠️
... and 9 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2873      +/-   ##
===========================================
- Coverage    74.34%   74.24%   -0.10%     
===========================================
  Files         1536     1538       +2     
  Lines        36731    36866     +135     
  Branches      7123     7145      +22     
===========================================
+ Hits         27306    27371      +65     
- Misses        5700     5763      +63     
- Partials      3725     3732       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarcloud · 2024-05-21T13:03:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

bmarty · 2024-05-27T08:07:03Z

@frebib FTR we have changed our mind and will move the setting to the Notifications settings screen. Tracked by #2912.

The switch will be displayed only if several providers are detected, to not confuse regular users about this.

bmarty requested a review from a team as a code owner May 17, 2024 16:22

bmarty requested review from ganfra and removed request for a team May 17, 2024 16:22

bmarty changed the title ~~Feature/bma/push provider switch~~ Push provider switch May 17, 2024

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 17, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 17, 2024

ganfra approved these changes May 20, 2024

View reviewed changes

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label May 21, 2024

bmarty enabled auto-merge May 21, 2024 10:27

bmarty added 12 commits May 21, 2024 14:45

Unset Http pusher: use SDK API.

56ca04a

Push distributor: ensure the current UnifiedPush distributor is stored

d2fbff0

UnifiedPush: correctly unregister the provider.

ad63f2b

Add distributor selection in advanced settings

d88ef87

Improve API, to avoid ignoring errors

a65c290

Improve code and display error.

7f9a30a

Ensure UnifiedPush endpoint is registered.

6870883

Update comment

2a20044

Wait for UnifiedPush pusher to be registered before returning Result.

d459c08

Upgrade com.github.UnifiedPush:android-connector from 2.1.1 to 2.4.0.

b63aa07

Fix bug, was not looking at the current PushProvider.

ff61eaa

Add missing methods.

495be55

bmarty added 7 commits May 21, 2024 14:45

Cleanup

82a2992

Localazy

92d6b72

Add test for push distributor change.

ada777e

Changelog

1e71ca3

Rename members for clarity

936579d

distributors and distributorNames do not need to be mutable.

4f03486

Add ui test on AdvancedSettingsView.

06d80ee

bmarty force-pushed the feature/bma/pushProviderSwitch branch from d73ae65 to 06d80ee Compare May 21, 2024 12:47

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 21, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 21, 2024

Update screenshots

4ab0202

ganfra added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 21, 2024

bmarty merged commit 71763e3 into develop May 21, 2024
23 of 25 checks passed

bmarty deleted the feature/bma/pushProviderSwitch branch May 21, 2024 14:52

This was referenced May 21, 2024

Allow configuring push notification provider #2340

Closed

Move Push provider settings to the notification settings #2912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push provider switch #2873

Push provider switch #2873

bmarty commented May 17, 2024 •

edited

ElementBot commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited

ganfra left a comment

ganfra May 20, 2024

bmarty May 21, 2024

ganfra May 20, 2024

bmarty May 21, 2024

ganfra May 20, 2024

jmartinesp May 20, 2024 •

edited

bmarty May 21, 2024

frebib commented May 20, 2024

bmarty commented May 21, 2024

codecov bot commented May 21, 2024 •

edited

sonarcloud bot commented May 21, 2024

bmarty commented May 27, 2024 •

edited

Push provider switch #2873

Push provider switch #2873

Conversation

bmarty commented May 17, 2024 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

ElementBot commented May 17, 2024 • edited

github-actions bot commented May 17, 2024 • edited

ganfra left a comment

Choose a reason for hiding this comment

ganfra May 20, 2024

Choose a reason for hiding this comment

bmarty May 21, 2024

Choose a reason for hiding this comment

ganfra May 20, 2024

Choose a reason for hiding this comment

bmarty May 21, 2024

Choose a reason for hiding this comment

ganfra May 20, 2024

Choose a reason for hiding this comment

jmartinesp May 20, 2024 • edited

Choose a reason for hiding this comment

bmarty May 21, 2024

Choose a reason for hiding this comment

frebib commented May 20, 2024

bmarty commented May 21, 2024

codecov bot commented May 21, 2024 • edited

Codecov Report

sonarcloud bot commented May 21, 2024

Quality Gate passed

bmarty commented May 27, 2024 • edited

bmarty commented May 17, 2024 •

edited

ElementBot commented May 17, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited

jmartinesp May 20, 2024 •

edited

codecov bot commented May 21, 2024 •

edited

bmarty commented May 27, 2024 •

edited