Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add special notifications for mentions #1846

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Add special notifications for mentions #1846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eae77f9
Add special notifications for mentions
jmartinesp Nov 23, 2023
788080c
Update screenshots
Nov 23, 2023
36ca1b7
Add extra tests
jmartinesp Nov 24, 2023
bc47cb6
Merge remote-tracking branch 'origin/develop' into feature/jme/specia…
jmartinesp Nov 28, 2023
849ea05
Add test
jmartinesp Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/1451.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Display different notifications for mentions.
7 changes: 6 additions & 1 deletion ...in/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you want to use notification_mentioned_you_fallback_body instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test in NotifiableEventResolverTest to cover this case?

} else {
messageBody
}
buildNotifiableMessageEvent(
sessionId = userId,
senderId = content.senderId,
Expand All @@ -90,7 +95,7 @@ class NotifiableEventResolver @Inject constructor(
noisy = isNoisy,
timestamp = this.timestamp,
senderName = senderDisplayName,
body = messageBody,
body = notificationBody,
imageUriString = this.contentUrl,
roomName = roomDisplayName,
roomIsDirect = isDirect,
Expand Down
1 change: 1 addition & 0 deletions libraries/push/impl/src/main/res/values-cs/translations.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<string name="notification_invitation_action_join">"Vstoupit"</string>
<string name="notification_invitation_action_reject">"Odmítnout"</string>
<string name="notification_invite_body">"Vás pozval(a) do chatu"</string>
<string name="notification_mentioned_you_body">"Zmínili vás: %1$s"</string>
<string name="notification_new_messages">"Nové zprávy"</string>
<string name="notification_reaction_body">"Reagoval(a) s %1$s"</string>
<string name="notification_room_action_mark_as_read">"Označit jako přečtené"</string>
Expand Down
71 changes: 70 additions & 1 deletion ...otlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolverTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import io.element.android.libraries.matrix.api.room.RoomMembershipState
import io.element.android.libraries.matrix.api.timeline.item.event.AudioMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.EmoteMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.FileMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.FormattedBody
import io.element.android.libraries.matrix.api.timeline.item.event.ImageMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.LocationMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.MessageFormat
import io.element.android.libraries.matrix.api.timeline.item.event.NoticeMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.TextMessageType
import io.element.android.libraries.matrix.api.timeline.item.event.VideoMessageType
Expand All @@ -51,6 +53,7 @@ import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import org.robolectric.RuntimeEnvironment
import org.robolectric.annotation.Config

@RunWith(RobolectricTestRunner::class)
class NotifiableEventResolverTest {
Expand Down Expand Up @@ -97,6 +100,71 @@ class NotifiableEventResolverTest {
assertThat(result).isEqualTo(expectedResult)
}

@Test
@Config(qualifiers = "en")
fun `resolve event message with mention`() = runTest {
val sut = createNotifiableEventResolver(
notificationResult = Result.success(
createNotificationData(
content = NotificationContent.MessageLike.RoomMessage(
senderId = A_USER_ID_2,
messageType = TextMessageType(body = "Hello world", formatted = null)
),
hasMention = true,
)
)
)
val result = sut.resolveEvent(A_SESSION_ID, A_ROOM_ID, AN_EVENT_ID)
val expectedResult = createNotifiableMessageEvent(body = "Mentioned you: Hello world")
assertThat(result).isEqualTo(expectedResult)
}

@Test
fun `resolve HTML formatted event message text takes plain text version`() = runTest {
val sut = createNotifiableEventResolver(
notificationResult = Result.success(
createNotificationData(
content = NotificationContent.MessageLike.RoomMessage(
senderId = A_USER_ID_2,
messageType = TextMessageType(
body = "Hello world!",
formatted = FormattedBody(
body = "<b>Hello world</b>",
format = MessageFormat.HTML,
)
)
)
)
)
)
val result = sut.resolveEvent(A_SESSION_ID, A_ROOM_ID, AN_EVENT_ID)
val expectedResult = createNotifiableMessageEvent(body = "Hello world")
assertThat(result).isEqualTo(expectedResult)
}

@Test
fun `resolve incorrectly formatted event message text uses fallback`() = runTest {
val sut = createNotifiableEventResolver(
notificationResult = Result.success(
createNotificationData(
content = NotificationContent.MessageLike.RoomMessage(
senderId = A_USER_ID_2,
messageType = TextMessageType(
body = "Hello world",
formatted = FormattedBody(
body = "???Hello world!???",
format = MessageFormat.UNKNOWN,
)
)
)
)
)
)
val result = sut.resolveEvent(A_SESSION_ID, A_ROOM_ID, AN_EVENT_ID)
val expectedResult = createNotifiableMessageEvent(body = "Hello world")
assertThat(result).isEqualTo(expectedResult)
}

@Test
fun `resolve event message audio`() = runTest {
val sut = createNotifiableEventResolver(
Expand Down Expand Up @@ -430,6 +498,7 @@ class NotifiableEventResolverTest {
private fun createNotificationData(
content: NotificationContent,
isDirect: Boolean = false,
hasMention: Boolean = false,
): NotificationData {
return NotificationData(
eventId = AN_EVENT_ID,
Expand All @@ -444,7 +513,7 @@ class NotifiableEventResolverTest {
timestamp = A_TIMESTAMP,
content = content,
contentUrl = null,
hasMention = false,
hasMention = hasMention,
)
}

Expand Down
3 changes: 3 additions & 0 deletions libraries/ui-strings/src/main/res/values/localazy.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@
<item quantity="other">"%d votes"</item>
</plurals>
<string name="preference_rageshake">"Rageshake to report bug"</string>
<string name="screen_edit_poll_delete_confirmation">"Are you sure you want to delete this poll?"</string>
<string name="screen_edit_poll_delete_confirmation_title">"Delete Poll"</string>
<string name="screen_edit_poll_title">"Edit poll"</string>
<string name="screen_media_picker_error_failed_selection">"Failed selecting media, please try again."</string>
<string name="screen_media_upload_preview_error_failed_processing">"Failed processing media to upload, please try again."</string>
<string name="screen_media_upload_preview_error_failed_sending">"Failed uploading media, please try again."</string>
Expand Down