Add email notification toggle #4052

ouchadam · 2021-09-21T15:09:10Z

#2243 Email notification toggles

Adds a Email notifications category to settings/notifications which allows for individually toggling email notifications for all associated emails
- When the account has no associated emails, the category contains a link to the Email and Phone numbers screen
- addEmailPusher functionality is added to the matrix-sdk PushersService which allows an email address to be registered or unregistered for notifications
Adds a REMOVE button to the settings/advanced settings/notification targets
Renames a few AddHttpPusher classes to AddPusher as they were able to be repurposed for emails

personal data has been manually overridden and replaced with [redacted]

BEFORE	NO EMAILS AVAILABLE	EMAIL PRESENT	LIGHT - MULTIPLE EMAILS

NAVIGATING EMPTY	REMOVING	ADDING	ERROR HANDLING

…reen

- email pushes make use of a new undocumented `brand` field, for now this field maps directly to the app display name so we can resuse it

- will display a _no emails_ when the matrix account has no emails set and tapping will navigate to the emails and phone numbers screen where the user can add an email - toggling the email notification with register and unregister push notification for the given email address

- Extracts out a transactional switch helper to handle reverting the switch back to its original state if an error occurs - Reuses existing toast message for unknown error - Does not include the isAdded to the async callback as the couroutine is tied to the fragment lifecycle scope

… can configure it

- always appending allows the same email to be used for other accounts see matrix-org/matrix-react-sdk#2727 https://github.com/matrix-org/matrix-react-sdk/pull/2727/files#diff-ec232520bf51337e5e6939b885d21f428ad6da3306c8e17a3ff660b2b341179dR165

…pushers - Updates the doc to reflect that to remove emails an appId of m.email is required

…self closing tag - the category is dynamically populated so we shouldn't be adding anything static here anyways!

…ch rather than when the listener is initially set

…ave a kind field

...m/vector/app/features/settings/notifications/VectorSettingsNotificationPreferenceFragment.kt

ouchadam · 2021-09-21T15:13:42Z

...droid/src/main/java/org/matrix/android/sdk/internal/session/pushers/DefaultPushersService.kt

-                append = append)
+                               withEventIdOnly: Boolean
+    ) = addPusher(
+            JsonPusher(


there's a lot of duplication here caused by the amount of parameters, the next PR coming up will address this by introducing a dedicated HttpPusher data class

ouchadam · 2021-09-21T15:15:08Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/pushers/PushersService.kt

     */
-    suspend fun removeHttpPusher(pushkey: String, appId: String)
+    suspend fun removePusher(pushkey: String, appId: String)


renamed because technically this function can remove email and http pushers

Ok, thanks, but maybe for clarity / symmetry keep removeHttpPusher and add new fun removeEmailPusher which takes only one parameter email?

I originally had that but it felt odd that PushGateway could still use the removeHttpPusher for both http and email pushers

the PushersManager has symmetrical register/unregister for http and email but that's a layer above the SDK

we could force the different flows based on appId

fun removePusher(pusher: Pusher) { when (pusher.appId) { Pusher.APP_ID_EMAIL -> session.removeEmailPusher(pusher.pushKey) else -> session.removeHttpPusher(pusher.pushKey, pusher.appId) } }

what do you think?

The PushersManager is on the app side, so I would say its Api is less "important" than the SDK API PushersService. I agree that removeHttpPusher is not KO to remove email pushers.

after speaking offline, I've opted for adding a 3rd method for removing Pushers 4482cbd this has the benefit of avoiding redundancy in checking the appId if we already have a pusher instance and we can have dedicated symmetrical http/email removal functions for convenience

ouchadam · 2021-09-21T15:17:41Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/pushers/PushersService.kt

+     * @param emailBranding     The branding placeholder to include in the email communications.
+     * @param appDisplayName    A human readable string that will allow the user to identify what application owns this pusher.
+     * @param deviceDisplayName A human readable string that will allow the user to identify what device owns this pusher.
+     * @return                  A work request uuid. Can be used to listen to the status


the next PR will remove the reliance on the WorkManager by switching to a suspend fun, allowing us to handle and simplify the registration errors

github-actions · 2021-09-21T15:31:02Z

Unit Test Results

  34 files ±0   34 suites ±0 19s ⏱️ ±0s
  73 tests ±0   73 ✔️ ±0 0 💤 ±0 0 ❌ ±0
180 runs ±0 180 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 045e4bb. ± Comparison against base commit 045e4bb.

♻️ This comment has been updated with latest results.

bmarty

Good work thanks.
A few comments and remarks

bmarty · 2021-09-23T09:59:55Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/pushers/PushersService.kt

     */
-    suspend fun removeHttpPusher(pushkey: String, appId: String)
+    suspend fun removePusher(pushkey: String, appId: String)


Ok, thanks, but maybe for clarity / symmetry keep removeHttpPusher and add new fun removeEmailPusher which takes only one parameter email?

...droid/src/main/java/org/matrix/android/sdk/internal/session/pushers/DefaultPushersService.kt

bmarty · 2021-09-23T10:05:50Z

...droid/src/main/java/org/matrix/android/sdk/internal/session/pushers/DefaultPushersService.kt


-        val params = AddHttpPusherWorker.Params(sessionId, pusher)
+    override fun addEmailPusher(email: String, lang: String, emailBranding: String, appDisplayName: String, deviceDisplayName: String) = addPusher(


One param per line is better when there are a lot of parameters like here

Also why there is no param append here?

append was removed as an option because it sounds like it has the side effect of causing other accounts which use the same email to lose email notifications https://github.com/matrix-org/matrix-react-sdk/pull/2727/files#diff-ec232520bf51337e5e6939b885d21f428ad6da3306c8e17a3ff660b2b341179dR165

happy to add back if you think it should be configurable by clients

It's sometimes difficult to decide between what we want the SDK to expose (as parameters for instance) and what we want the SDK to handle, so keep hidden from the client app. On one side the SDK should let the app do whatever it wants to, and on the other side we want the SDK to provide a high level service... So maybe add the parameter, with default value to true and with the same comment from https://github.com/matrix-org/matrix-react-sdk/pull/2727/files#diff-ec232520bf51337e5e6939b885d21f428ad6da3306c8e17a3ff660b2b341179dR165 in the doc of the fun addEmailPusher.
Thanks!

makes sense, will do 👍

...m/vector/app/features/settings/notifications/VectorSettingsNotificationPreferenceFragment.kt

vector/src/main/java/im/vector/app/features/settings/push/PushGatewaysViewModel.kt

vector/src/main/res/layout/item_pushgateway.xml

vector/src/main/res/values/strings.xml

… it's not directly tied to the class

- displays a dialog with a human readable version of the error

… is the case for email pushers)

…n returning from adding an email - makes use of both sync and async fetching so that the page can avoid jumping around on the initial load

vector/src/main/java/im/vector/app/core/extensions/ViewExtensions.kt

… typically we wouldn't want to overwrite other accounts but we can expose the option to clients if they want that behaviour

…ide other views

…hers - also adds a separate removePusher which supports removing any type of pusher

bmarty · 2021-09-24T18:40:46Z

vector/src/main/java/im/vector/app/core/pushers/PushersManager.kt

@@ -75,12 +75,12 @@ class PushersManager @Inject constructor(

    suspend fun unregisterEmailPusher(email: String) {
        val currentSession = activeSessionHolder.getSafeActiveSession() ?: return
-        currentSession.removePusher(email, appId = "m.email")
+        currentSession.removeEmailPusher(email)
    }

    suspend fun unregisterPusher(pushKey: String) {


Nit: rename to unregisterHttpPusher for parity?

bmarty

Thanks for the update, LGTM!

ouchadam added 12 commits September 20, 2021 16:35

adding ability to remove push targets from the notification target sc…

1591375

…reen

adding matrix SDK support for email adding and removing email pushes

ee3f287

- email pushes make use of a new undocumented `brand` field, for now this field maps directly to the app display name so we can resuse it

lifting the email branding to its own parameter so clients of the sdk…

516bb6e

… can configure it

making the removePusher more generic as it can handle email and http …

2c25efc

…pushers - Updates the doc to reflect that to remove emails an appId of m.email is required

cleaning up the notification email group xml definition to include a …

710f2f9

…self closing tag - the category is dynamically populated so we shouldn't be adding anything static here anyways!

capturing the original switch state at the point of clicking the swit…

03eb366

…ch rather than when the listener is initially set

removing trailing comma for lint

0617179

adding changelog entry for notifications

52d9600

removing redundant comments about pusher kind from apis which don't h…

8734d5d

…ave a kind field

ouchadam commented Sep 21, 2021

View reviewed changes

...m/vector/app/features/settings/notifications/VectorSettingsNotificationPreferenceFragment.kt Outdated Show resolved Hide resolved

ouchadam commented Sep 21, 2021

View reviewed changes

ouchadam mentioned this pull request Sep 22, 2021

Improved /settings/notifications push toggle error handling #4062

Closed

bmarty requested changes Sep 23, 2021

View reviewed changes

removing this. usages where there's no clashes

8316728

ouchadam force-pushed the feature/adm/email_notification_toggle branch from 5033ea5 to 8316728 Compare September 23, 2021 10:51

ouchadam added 8 commits September 23, 2021 12:02

documenting the getEmails function and lifting it out of the class as…

efec63e

… it's not directly tied to the class

catching remove pusher errors in the notification targets screen

4c4f2fc

- displays a dialog with a human readable version of the error

extracting constants for the pusher kinds and email appId

d31ad7e

hiding the notification url/format UI fields when they're null (which…

1865e3c

… is the case for email pushers)

adding translator note about the enable email placeholder

1d4c909

reusing existing remove string

43eff8b

formatting

0302d19

monitoring the ThreePids so that the notifications screen updates whe…

3a1cb1c

…n returning from adding an email - makes use of both sync and async fetching so that the page can avoid jumping around on the initial load

ouchadam force-pushed the feature/adm/email_notification_toggle branch from 8cfdc43 to 3a1cb1c Compare September 23, 2021 13:46

bmarty reviewed Sep 24, 2021

View reviewed changes

vector/src/main/java/im/vector/app/core/extensions/ViewExtensions.kt Outdated Show resolved Hide resolved

ouchadam added 3 commits September 24, 2021 15:39

making the add email pusher append parameter configurable by clients,…

95b4f99

… typically we wouldn't want to overwrite other accounts but we can expose the option to clients if they want that behaviour

reusing existing setTextOrHide by adding an additional parameter to h…

7fd794b

…ide other views

using dedicated pusher removal methods for the different types of pus…

4482cbd

…hers - also adds a separate removePusher which supports removing any type of pusher

bmarty reviewed Sep 24, 2021

View reviewed changes

bmarty approved these changes Sep 24, 2021

View reviewed changes

bmarty merged commit 045e4bb into develop Sep 24, 2021

bmarty deleted the feature/adm/email_notification_toggle branch September 24, 2021 18:42

ouchadam mentioned this pull request Oct 12, 2021

Improved /settings/notifications push toggle error handling #4228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add email notification toggle #4052

Add email notification toggle #4052

ouchadam commented Sep 21, 2021 •

edited

Loading

ouchadam Sep 21, 2021

ouchadam Sep 21, 2021

bmarty Sep 23, 2021

ouchadam Sep 23, 2021

bmarty Sep 24, 2021

ouchadam Sep 24, 2021

ouchadam Sep 21, 2021

github-actions bot commented Sep 21, 2021 •

edited

Loading

bmarty left a comment

bmarty Sep 23, 2021

bmarty Sep 23, 2021

bmarty Sep 23, 2021

ouchadam Sep 23, 2021 •

edited

Loading

bmarty Sep 24, 2021

ouchadam Sep 24, 2021

bmarty Sep 24, 2021

bmarty left a comment


		val params = AddHttpPusherWorker.Params(sessionId, pusher)
		override fun addEmailPusher(email: String, lang: String, emailBranding: String, appDisplayName: String, deviceDisplayName: String) = addPusher(

Add email notification toggle #4052

Add email notification toggle #4052

Conversation

ouchadam commented Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2021 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ouchadam commented Sep 21, 2021 •

edited

Loading

github-actions bot commented Sep 21, 2021 •

edited

Loading

ouchadam Sep 23, 2021 •

edited

Loading