Makes "Enable Notifications for this session" respond to enabled value in pusher #7281

ericdecanini · 2022-10-03T23:42:58Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Meets criteria:

the ‘notifications on this device’ toggle should check the pusher.enabled field and display as off when pusher.enabled is false
When there is a registered pusher with enabled: false - toggling on should set the enabled field to true on the existing pusher

(the other AC are already current behaviour)

Motivation and context

Epic: https://element-io.atlassian.net/browse/PSG-595
Closes https://element-io.atlassian.net/browse/PSG-785

Screenshots / GIFs

Tests

Disable pusher in device manager
Go to notification settings
See Enable Notifications for this session is unchecked
Check this setting
See pusher is now enabled

Tested devices

Physical
Emulator
OS version(s): Android 13

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-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/vector-im/element-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
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

…view

bmarty

Just a small remark

bmarty · 2022-10-04T07:43:13Z

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

@@ -117,6 +123,9 @@ class VectorSettingsNotificationPreferenceFragment :
                        }
                        findPreference<VectorPreference>(VectorPreferences.SETTINGS_NOTIFICATION_METHOD_KEY)
                                ?.summary = unifiedPushHelper.getCurrentDistributorName()
+                        lifecycleScope.launch {
+                            pushersManager.togglePusherForCurrentSession(true)


You need a try catch, it will crash if there is for instance no network.

Agree, ideally a runCatching may be added inside the PushersManager.togglePusherForCurrentSession() method. And failure may be prompted to the user.

mnaturel · 2022-10-04T12:00:39Z

vector/src/test/java/im/vector/app/test/fakes/FakePushersService.kt

        coVerify(ordering = Ordering.ALL) {
            getPushersLive() // verifies only getPushersLive and the following togglePusher was called
            togglePusher(pusher, enable)
        }
    }

+    fun verifyOnlyGetPushersAndTogglePusherCalled(pusher: Pusher, enable: Boolean) {
+        coVerify(ordering = Ordering.ALL) {
+            getPushers() // verifies only getPushersLive and the following togglePusher was called


Comment should be updated I guess by replacing getPushersLive() by getPushers(). Or comments in these methods could be removed.

mnaturel

Not tested but LGTM. Just added a minor comment.

…ifications # Conflicts: # vector/src/main/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewAction.kt # vector/src/main/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewFragment.kt # vector/src/main/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModel.kt # vector/src/main/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewState.kt # vector/src/main/res/layout/fragment_session_overview.xml # vector/src/test/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

…ic/notificaton-settings-enabled # Conflicts: # vector/src/test/java/im/vector/app/core/pushers/PushersManagerTest.kt

bmarty

SGTM, just a remark about the new string.

bmarty · 2022-10-07T12:23:16Z

library/ui-strings/src/main/res/values/strings.xml

@@ -931,6 +931,7 @@
    <string name="settings_call_notifications_preferences">Configure Call Notifications</string>
    <string name="settings_silent_notifications_preferences">Configure Silent Notifications</string>
    <string name="settings_system_preferences_summary">Choose LED color, vibration, sound…</string>
+    <string name="settings_error_toggle_pusher">Something went wrong. Please check your network connection and try again.</string>


Maybe use a more generic key for this string, which could be re-use in other circumstances.
Something like error_check_network (we already have error_no_network)

…ifications

…ic/notificaton-settings-enabled

mnaturel · 2022-10-10T08:03:00Z

...est/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt

@@ -206,110 +203,6 @@ class SessionOverviewViewModelTest {
                .finish()
    }

-    @Test


Is there a reason to having removed all tests on SignoutAction ?

I think they were lost during a merge conflict. Good catch

mnaturel

I think we need to cancel the removing of unit tests on SignoutAction in the `SessionOverviewViewModelTest.

…ifications

…ic/notificaton-settings-enabled

…icaton-settings-enabled # Conflicts: # vector/src/test/java/im/vector/app/features/settings/devices/v2/overview/SessionOverviewViewModelTest.kt # vector/src/test/java/im/vector/app/test/fakes/FakePushersService.kt

mnaturel

I have seen 3 points to fix:

on UI side, there is an issue with the constraints in the session overview screen, see the capture below.
on UI side, when the push notifications are disabled when arriving into the session overview screen, we see the toggle is moving from enabled to disabled. I wonder if we could do something to avoid that?
on Realm database side: since we updated the PusherEntity, there should be a database migration and it looks like we don't have it yet (could be done in another PR I guess).

ericdecanini · 2022-10-11T20:25:58Z

I have seen 3 points to fix:

on UI side, there is an issue with the constraints in the session overview screen, see the capture below.

on UI side, when the push notifications are disabled when arriving into the session overview screen, we see the toggle is moving from enabled to disabled. I wonder if we could do something to avoid that?

on Realm database side: since we updated the PusherEntity, there should be a database migration and it looks like we don't have it yet (could be done in another PR I guess).

All 3 points addressed

mnaturel

Thanks for the fixes! I think the small animation of the switchButton is not fixed since it happens now when the push notifications are enabled. We can see going from disabled state to enabled state. But honestly, I don't know how we could fix it.

mnaturel

And other thing, could we bind the whole push notification view to change the toggle instead of only the switch? It will ease the toggling for the user.

There is also another problem with navigation to session details, it may have been deleted during merges. I created a fix PR for that: #7340

mnaturel

LGTM, thanks for the fix on the switch.

sonarqubecloud · 2022-10-12T13:11:32Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

26.8% Coverage
0.0% Duplication

ericdecanini added 11 commits September 28, 2022 15:57

Adds push notifications switch

5ced831

Adds functionality to Push notification toggle

e1b78b9

Adds DefaultPushersServiceTest for togglePusher

233d07e

Adds DefaultTogglePusherTaskTest

da80c08

Adds SessionOverviewViewModelTest for toggling pusher

670cceb

Hides pusher toggle if there are no pushers of the device

71558ed

Adds changelog file

3cc22c5

Edits changelog file

66b8ebd

Fixes copyrights

ae2a02c

Unregisters checkedChangelistener in onDetachedFromWindow for switch …

47e5491

…view

Links notification settings toggle to pusher service

d91bbfd

ericdecanini changed the base branch from develop to feature/eric/push-toggle-notifications October 3, 2022 23:43

Adds changelog file

ac33ac1

ericdecanini marked this pull request as ready for review October 3, 2022 23:44

ericdecanini requested review from a team and mnaturel and removed request for a team October 3, 2022 23:45

bmarty reviewed Oct 4, 2022

View reviewed changes

mnaturel reviewed Oct 4, 2022

View reviewed changes

mnaturel approved these changes Oct 4, 2022

View reviewed changes

ericdecanini added 2 commits October 4, 2022 14:13

Adds error handling to VectorSettingsNotificationPreferenceFragment

757ee1a

Removes comment in FakePushersService

121415d

ericdecanini requested a review from bmarty October 4, 2022 18:14

ericdecanini added 3 commits October 5, 2022 18:00

Fixes post merge errors

c038540

Merge branch 'feature/eric/push-toggle-notifications' into feature/er…

2289fde

…ic/notificaton-settings-enabled # Conflicts: # vector/src/test/java/im/vector/app/core/pushers/PushersManagerTest.kt

bmarty approved these changes Oct 7, 2022

View reviewed changes

ericdecanini added 3 commits October 7, 2022 09:10

Merge branch 'feature/eric/msc3881' into feature/eric/push-toggle-not…

1639b71

…ifications

Merge branch 'feature/eric/push-toggle-notifications' into feature/er…

a1908b0

…ic/notificaton-settings-enabled

Fixes imports and improves string name

3398646

ericdecanini added 7 commits October 9, 2022 14:50

Fixes error

971fe8e

Merge branch 'feature/eric/push-toggle-notifications' into feature/er…

476aab3

…ic/notificaton-settings-enabled

Fixes error

e44bca3

Fixes error

1e3dbba

Merge branch 'feature/eric/push-toggle-notifications' into feature/er…

49c6240

…ic/notificaton-settings-enabled

Fixes error

9de170a

Merge branch 'feature/eric/push-toggle-notifications' into feature/er…

f93674d

…ic/notificaton-settings-enabled

mnaturel reviewed Oct 10, 2022

View reviewed changes

mnaturel suggested changes Oct 10, 2022

View reviewed changes

ericdecanini added 2 commits October 10, 2022 11:38

Merge branch 'feature/eric/msc3881' into feature/eric/push-toggle-not…

110600c

…ifications

Merge branch 'feature/eric/push-toggle-notifications' into feature/er…

2f2dc4e

…ic/notificaton-settings-enabled

Base automatically changed from feature/eric/push-toggle-notifications to develop October 10, 2022 23:21

ericdecanini added 2 commits October 10, 2022 22:10

Adds lost tests

b02b4a3

ericdecanini requested a review from mnaturel October 11, 2022 03:38

mnaturel suggested changes Oct 11, 2022

View reviewed changes

ericdecanini added 3 commits October 11, 2022 16:04

Adds PusherEntity migration

c6224b1

Fixes session overview layout overlap

d1562d3

Fixes switch being enabled by default

992d7d3

ericdecanini requested a review from mnaturel October 11, 2022 20:26

mnaturel approved these changes Oct 12, 2022

View reviewed changes

mnaturel suggested changes Oct 12, 2022

View reviewed changes

mnaturel added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Oct 12, 2022

Binds entire view to toggle switch

5a1eac6

ericdecanini requested a review from mnaturel October 12, 2022 12:25

mnaturel approved these changes Oct 12, 2022

View reviewed changes

ericdecanini merged commit 9857fa6 into develop Oct 12, 2022

ericdecanini deleted the feature/eric/notificaton-settings-enabled branch October 12, 2022 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes "Enable Notifications for this session" respond to enabled value in pusher #7281

Makes "Enable Notifications for this session" respond to enabled value in pusher #7281

ericdecanini commented Oct 3, 2022 •

edited

Loading

bmarty left a comment

bmarty Oct 4, 2022

mnaturel Oct 4, 2022

mnaturel Oct 4, 2022

mnaturel left a comment

bmarty left a comment

bmarty Oct 7, 2022

mnaturel Oct 10, 2022

ericdecanini Oct 11, 2022

mnaturel left a comment

mnaturel left a comment

ericdecanini commented Oct 11, 2022

mnaturel left a comment

mnaturel left a comment •

edited

Loading

mnaturel left a comment

sonarqubecloud bot commented Oct 12, 2022

Makes "Enable Notifications for this session" respond to enabled value in pusher #7281

Makes "Enable Notifications for this session" respond to enabled value in pusher #7281

Conversation

ericdecanini commented Oct 3, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

bmarty left a comment

Choose a reason for hiding this comment

bmarty Oct 4, 2022

Choose a reason for hiding this comment

mnaturel Oct 4, 2022

Choose a reason for hiding this comment

mnaturel Oct 4, 2022

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty Oct 7, 2022

Choose a reason for hiding this comment

mnaturel Oct 10, 2022

Choose a reason for hiding this comment

ericdecanini Oct 11, 2022

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

ericdecanini commented Oct 11, 2022

mnaturel left a comment

Choose a reason for hiding this comment

mnaturel left a comment • edited Loading

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 12, 2022

ericdecanini commented Oct 3, 2022 •

edited

Loading

mnaturel left a comment •

edited

Loading