Troubleshoot notifications screen #2596

bmarty · 2024-03-22T16:41:19Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Add a troubleshoot notification screen with tests to help users understand why notification are not working.

Test coverage:

Remaining work (will be done in a separate PR):

Add a link to report an issue if at least one test is failing (this will also be added to the user story).
Improve logs: give more details about the failing tests, like outputting a report
Improve analytics: give details about the failing tests.
Add tests about the current push rules. In particular the app could show a list of muted rooms.

Motivation and context

Closes #2601

Screenshots / GIFs

Firebase	UnifiedPush	Quick fix

and recorded screenshot

Tests

Navigate to Setttings / Notification / Troubleshoot notifications
Run the tests

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

github-actions · 2024-03-22T16:52:04Z

📱 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/SmCSsH

codecov · 2024-03-25T09:36:08Z

Codecov Report

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

Project coverage is 73.37%. Comparing base (609f636) to head (5310456).
Report is 2 commits behind head on develop.

Files	Patch %	Lines
...troubleshoot/impl/TroubleshootNotificationsView.kt	71.83%	2 Missing and 18 partials ⚠️
...id/services/analytics/impl/DefaultScreenTracker.kt	0.00%	13 Missing ⚠️
...s/pushproviders/unifiedpush/UnifiedPushProvider.kt	0.00%	11 Missing ⚠️
...braries/push/impl/DefaultGetCurrentPushProvider.kt	0.00%	10 Missing ⚠️
...s/pushproviders/firebase/IsPlayServiceAvailable.kt	0.00%	9 Missing ⚠️
.../android/libraries/push/impl/DefaultPushService.kt	0.00%	7 Missing ⚠️
...ders/unifiedpush/UnifiedPushDistributorProvider.kt	0.00%	7 Missing ⚠️
...ment/android/libraries/push/impl/PushersManager.kt	0.00%	6 Missing ⚠️
...s/push/impl/notifications/NotificationDisplayer.kt	0.00%	6 Missing ⚠️
...ies/pushproviders/firebase/FirebasePushProvider.kt	0.00%	6 Missing ⚠️
... and 29 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2596      +/-   ##
===========================================
+ Coverage    73.23%   73.37%   +0.14%     
===========================================
  Files         1428     1459      +31     
  Lines        34585    35177     +592     
  Branches      6682     6752      +70     
===========================================
+ Hits         25328    25812     +484     
- Misses        5763     5851      +88     
- Partials      3494     3514      +20

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

jmartinesp

LGTM, just some suggestions. Thanks for the work on this!

jmartinesp · 2024-04-01T10:47:39Z

.../main/kotlin/io/element/android/libraries/core/notifications/NotificationTroubleshootTest.kt

+    suspend fun run(coroutineScope: CoroutineScope)
+    suspend fun reset()
+    suspend fun quickFix(coroutineScope: CoroutineScope) {
+        error("Quick fix not implemented, you need to override this method in your test")


Since this is an interface, wouldn't not providing a default implementation force any devs to implement this? It seems more effective than having it crash on runtime because someone forgot to override the default behaviour.

I agree, but I guess the developer who add a test with a quick fix will test it and so see the error.
And since quick fix support is optional I prefer to provide a default implementation.

jmartinesp · 2024-04-01T10:52:36Z

...impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultGetCurrentPushProvider.kt

+            .value
+            .navigationState
+            .currentSessionId()
+            ?.let { pushStoreFactory.create(it) }


Not part of this PR, but maybe this pushStoreFactory.create(it) should be instead pushStoreFactory.get(it) or maybe getOrCreate? I found it weird that we're creating an extra PushStore here until I realised the create method would actually get an existing one if it's there.

You're right: d592f3a.

jmartinesp · 2024-04-01T10:56:44Z

...push/api/src/main/kotlin/io/element/android/libraries/push/api/gateway/PushGatewayFailure.kt

-sealed class PushGatewayFailure : Throwable(cause = null) {
-    data object PusherRejected : PushGatewayFailure()
+sealed class PushGatewayFailure : Exception() {
+    class PusherRejected : PushGatewayFailure()


I guess this is needed to trigger UI changes?

I change this to fix this warning: Serializable object must implement 'readResolve' . Also I think it's better to extend Exception rather than Throwable.

Oh, I was actually talking about the data object -> class change.

Yes, I think the warning was about the data object. I did not dig further...

jmartinesp · 2024-04-01T11:00:24Z

...src/main/kotlin/io/element/android/libraries/push/impl/troubleshoot/DiagnosticPushHandler.kt

+import javax.inject.Inject
+
+@SingleIn(AppScope::class)
+class DiagnosticPushHandler @Inject constructor() {


Maybe move this and NotificationClickHandle to a sub-package?

I could create a subpackage per test, to ease the readability, but it's maybe over engineering since the package only have 6 classes.

I was thinking about separating the handlers from the tests, but I guess it's not really worth it.

jmartinesp · 2024-04-01T11:05:48Z

...impl/src/main/kotlin/io/element/android/libraries/push/impl/troubleshoot/NotificationTest.kt

+            notificationClickHandler.state.first()
+            Timber.d("Notification clicked!")
+        }
+        val s = withTimeoutOrNull(30.seconds) {


This seems a bit hard to understand, IMO. Maybe we could use just withTimeout inside a try/catch block or runCatching, the successful path leading to the Success state and the catch clause for TimeoutCancellationException emitting the Failure case?

Yes, good idea, thanks: 378a9c6.

…tString()

…by the way).

ElementBot · 2024-04-02T14:42:13Z

	Warnings
⚠️	features/messages/impl/src/main/res/values-be/translations.xml#L45 - For locale "be" (Belarusian) the following quantity should also be defined: `few` (e.g. "з 2 кніг за 2 дні")
⚠️	gradle/libs.versions.toml#L139 - A newer version of org.robolectric:robolectric than 4.11.1 is available: 4.12
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L7 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L12 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L21 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L29 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L42 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L50 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L65 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L7 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L12 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L21 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L29 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L42 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L50 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/push/impl/src/main/res/values-sk/translations.xml#L65 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/pushproviders/unifiedpush/src/main/res/values-cs/translations.xml#L5 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/pushproviders/unifiedpush/src/main/res/values-sk/translations.xml#L5 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L4 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L22 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L152 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-cs/translations.xml#L174 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L4 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L22 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L151 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")
⚠️	libraries/ui-strings/src/main/res/values-sk/translations.xml#L173 - For locale "sk" (Slovak) the following quantity should also be defined: `many` (e.g. "10.0 dňa")

Generated by 🚫 dangerJS against 5310456

…eate` for code clarity.

… of withTimeoutOrNull

sonarcloud · 2024-04-02T18:01:06Z

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 force-pushed the feature/bma/troubleshootNotification branch 5 times, most recently from e78e9d6 to 5b186ae Compare March 27, 2024 21:17

bmarty marked this pull request as ready for review March 28, 2024 08:02

bmarty requested a review from a team as a code owner March 28, 2024 08:02

bmarty requested review from jmartinesp and removed request for a team March 28, 2024 08:02

bmarty changed the title ~~Troubleshoot notifications screen WIP~~ Troubleshoot notifications screen Mar 28, 2024

jmartinesp approved these changes Apr 1, 2024

View reviewed changes

bmarty and others added 15 commits April 2, 2024 16:14

Create FakePushService.

6c9ea2b

Troubleshoot notifications screen

2bfe125

Rename Fake class with Mockk prefix for clarity

a839528

Avoid tryEmit(), use emit()

74048cc

Update analytics event lib.

2019448

Track NotificationTroubleshoot screen

ef48c2b

Track NotificationTroubleshoot

8da435b

use emit instead of tryEmit

a3cb7ab

FakeStringProvider: Ensure parameter are included in the result of ge…

5e27c54

…tString()

Fix test in VersionFormatterTest (there was a swap in the test names …

33526db

…by the way).

Localize Troubleshoot notification feature.

09b2cba

Add test on NotificationSettingsView

91de03e

Improve preview management for NotificationSettingsView

9f60a28

More tests on NotificationSettingsView.

7b16b10

Code quality

9c89a3b

bmarty force-pushed the feature/bma/troubleshootNotification branch from c9f68ef to 378a9c6 Compare April 2, 2024 14:40

bmarty added 8 commits April 2, 2024 18:12

towncrier

f7f25e1

Improve ScreenTracker.

5a0e766

Remove wrong comment

a89dde2

Improve openUrlInExternalApp API and move url to UnifiedPushConfig

565e5ce

Run ./tools/localazy/downloadStrings.sh --all

e18e5f1

Rename UserPushStoreFactory.create to `UserPushStoreFactory.getOrCr…

52c039d

…eate` for code clarity.

Code clarity: use withTimeout with combination of runCatching instead…

4f320fd

… of withTimeoutOrNull

Create dedicated module for notification troubleshoot.

cb435c5

bmarty force-pushed the feature/bma/troubleshootNotification branch from 3e20075 to cb435c5 Compare April 2, 2024 16:12

ElementBot and others added 4 commits April 2, 2024 16:25

Update screenshots

30d77e5

Add showkase processor

4032402

Update screenshots

2aa53f0

Fix test compilation.

5310456

bmarty merged commit f4076dc into develop Apr 2, 2024
16 checks passed

bmarty deleted the feature/bma/troubleshootNotification branch April 2, 2024 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Troubleshoot notifications screen #2596

Troubleshoot notifications screen #2596

bmarty commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited

codecov bot commented Mar 25, 2024 •

edited

jmartinesp left a comment

jmartinesp Apr 1, 2024

bmarty Apr 2, 2024

jmartinesp Apr 1, 2024

bmarty Apr 2, 2024 •

edited

jmartinesp Apr 1, 2024

bmarty Apr 2, 2024

jmartinesp Apr 2, 2024

bmarty Apr 2, 2024

jmartinesp Apr 1, 2024

bmarty Apr 2, 2024

jmartinesp Apr 2, 2024

jmartinesp Apr 1, 2024

bmarty Apr 2, 2024 •

edited

ElementBot commented Apr 2, 2024 •

edited

sonarcloud bot commented Apr 2, 2024

Troubleshoot notifications screen #2596

Troubleshoot notifications screen #2596

Conversation

bmarty commented Mar 22, 2024 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Mar 22, 2024 • edited

codecov bot commented Mar 25, 2024 • edited

Codecov Report

jmartinesp 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

bmarty Apr 2, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Apr 2, 2024 • edited

Choose a reason for hiding this comment

ElementBot commented Apr 2, 2024 • edited

sonarcloud bot commented Apr 2, 2024

Quality Gate passed

bmarty commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited

codecov bot commented Mar 25, 2024 •

edited

bmarty Apr 2, 2024 •

edited

bmarty Apr 2, 2024 •

edited

ElementBot commented Apr 2, 2024 •

edited