Add special notifications for mentions #1846

jmartinesp · 2023-11-21T14:51:30Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Sets a special type of notification message for received messages mentioning the current user.

Motivation and context

Screenshots / GIFs

Before	After

Tests

Receive a message mentioning you in a room you have notifications enabled.

Tested devices

Physical
Emulator
OS version(s): 11

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/vector-im/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/vector-im/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 · 2023-11-21T14:53:05Z

	Warnings
⚠️	libraries/push/impl/src/main/res/values-cs/translations.xml#L23 - 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#L28 - 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#L33 - 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#L38 - 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#L43 - 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#L48 - For locale "cs" (Czech) the following quantity should also be defined: `many` (e.g. "10.0 dne")

Generated by 🚫 dangerJS against 849ea05

github-actions · 2023-11-21T14:58:37Z

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

codecov · 2023-11-21T15:09:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (12b3196) 66.48% compared to head (849ea05) 66.48%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1846   +/-   ##
========================================
  Coverage    66.48%   66.48%           
========================================
  Files         1308     1308           
  Lines        33055    33058    +3     
  Branches      7083     7084    +1     
========================================
+ Hits         21976    21979    +3     
  Misses        7724     7724           
  Partials      3355     3355

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

bmarty · 2023-11-23T21:29:05Z

.../main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt

@@ -83,6 +83,11 @@ class NotifiableEventResolver @Inject constructor(
        return when (val content = this.content) {
            is NotificationContent.MessageLike.RoomMessage -> {
                val messageBody = descriptionFromMessageContent(content, senderDisplayName ?: content.senderId.value)
+                val notificationBody = if (hasMention) {
+                    stringProvider.getString(R.string.notification_mentioned_you_body, messageBody)


The value of the string is:

<string name="notification_mentioned_you_body">"%1$s mentioned you. %2$s"</string>

so it seems like a parameter is missing here? The messageBody will be used for %1$s and it should be the sender. Can you double check please?

Maybe you want to use notification_mentioned_you_fallback_body instead?

It's not anymore: https://github.com/vector-im/element-x-android/pull/1846/files#diff-ceb20f078550a897a919802d4b7635a8299f6ac1ef679aefba72e4c294954f2dR11.

notification_mentioned_you_fallback_body was removed for this same reason. We decided to change what the message looked like since it was too verbose.

OK, it's fine now. I do not know what happened with all those string values, and it does not worth investigate more about it. Thanks!

…l-notifications-for-mentions

bmarty

Thanks!

bmarty · 2023-11-28T09:01:33Z

.../main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt

@@ -83,6 +83,11 @@ class NotifiableEventResolver @Inject constructor(
        return when (val content = this.content) {
            is NotificationContent.MessageLike.RoomMessage -> {
                val messageBody = descriptionFromMessageContent(content, senderDisplayName ?: content.senderId.value)
+                val notificationBody = if (hasMention) {
+                    stringProvider.getString(R.string.notification_mentioned_you_body, messageBody)


OK, it's fine now. I do not know what happened with all those string values, and it does not worth investigate more about it. Thanks!

bmarty · 2023-11-28T09:02:37Z

.../main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt

@@ -82,6 +82,11 @@ class NotifiableEventResolver @Inject constructor(
        return when (val content = this.content) {
            is NotificationContent.MessageLike.RoomMessage -> {
                val messageBody = descriptionFromMessageContent(content, senderDisplayName ?: content.senderId.value)
+                val notificationBody = if (hasMention) {
+                    stringProvider.getString(R.string.notification_mentioned_you_body, messageBody)


Maybe add a test in NotifiableEventResolverTest to cover this case?

sonarqubecloud · 2023-11-28T09:23:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

jmartinesp requested a review from a team as a code owner November 21, 2023 14:51

jmartinesp requested review from bmarty and removed request for a team November 21, 2023 14:51

jmartinesp marked this pull request as draft November 21, 2023 16:19

jmartinesp force-pushed the feature/jme/special-notifications-for-mentions branch from 013f875 to a9d4bdb Compare November 23, 2023 07:47

jmartinesp marked this pull request as ready for review November 23, 2023 07:49

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Nov 23, 2023

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

bmarty reviewed Nov 23, 2023

View reviewed changes

jmartinesp and others added 3 commits November 24, 2023 10:05

Add special notifications for mentions

eae77f9

Update screenshots

788080c

Add extra tests

36ca1b7

jmartinesp force-pushed the feature/jme/special-notifications-for-mentions branch from e0b9035 to 36ca1b7 Compare November 24, 2023 09:10

jmartinesp requested a review from bmarty November 27, 2023 06:58

Merge remote-tracking branch 'origin/develop' into feature/jme/specia…

bc47cb6

…l-notifications-for-mentions

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Nov 28, 2023

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

bmarty approved these changes Nov 28, 2023

View reviewed changes

Add test

849ea05

jmartinesp merged commit 4de256b into develop Nov 28, 2023
15 checks passed

jmartinesp deleted the feature/jme/special-notifications-for-mentions branch November 28, 2023 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special notifications for mentions #1846

Add special notifications for mentions #1846

jmartinesp commented Nov 21, 2023 •

edited

Loading

ElementBot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

bmarty Nov 23, 2023

bmarty Nov 23, 2023

jmartinesp Nov 24, 2023

bmarty Nov 28, 2023

bmarty left a comment

bmarty Nov 28, 2023

bmarty Nov 28, 2023

sonarqubecloud bot commented Nov 28, 2023

Add special notifications for mentions #1846

Add special notifications for mentions #1846

Conversation

jmartinesp commented Nov 21, 2023 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

ElementBot commented Nov 21, 2023 • edited Loading

github-actions bot commented Nov 21, 2023 • edited Loading

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

bmarty Nov 23, 2023

Choose a reason for hiding this comment

bmarty Nov 23, 2023

Choose a reason for hiding this comment

jmartinesp Nov 24, 2023

Choose a reason for hiding this comment

bmarty Nov 28, 2023

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty Nov 28, 2023

Choose a reason for hiding this comment

bmarty Nov 28, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 28, 2023

jmartinesp commented Nov 21, 2023 •

edited

Loading

ElementBot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading