Skip to content

Add surfaces parameter to RemoteMessage and updated related classes. #7063

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

Merged
merged 3 commits into from
Nov 18, 2025
Merged

Add surfaces parameter to RemoteMessage and updated related classes. #7063

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df46ba9
Add surfaces parameter to RemoteMessage and updated related classes.
anikiki Nov 5, 2025
48ec67c
Update RemoteMessage instantiation in NewTabLegacyPageViewModelTest t…
anikiki Nov 5, 2025
8399799
Refactor surfaces handling in JsonRemoteMessageMapper and RemoteMessa…
anikiki Nov 18, 2025
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
20 changes: 10 additions & 10 deletions app/src/test/java/com/duckduckgo/app/browser/newtab/NewTabLegacyPageViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenViewModelIsInitializedThenViewStateShouldEmitInitialState() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))
whenever(mockDismissedCtaDao.exists(DAX_END)).thenReturn(false)

Expand All @@ -119,7 +119,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageAvailableAndOnboardingNotCompleteThenMessageNotShown() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))
whenever(mockDismissedCtaDao.exists(DAX_END)).thenReturn(false)

Expand All @@ -137,7 +137,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageAvailableAndOnboardingCompleteThenMessageShown() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))
whenever(mockDismissedCtaDao.exists(DAX_END)).thenReturn(true)

Expand All @@ -155,7 +155,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageShownThenFirePixelAndMarkAsShown() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))

testee.onStart(mockLifecycleOwner)
Expand All @@ -167,7 +167,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageCloseButtonClickedThenFirePixelAndDismiss() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))

testee.onStart(mockLifecycleOwner)
Expand All @@ -179,7 +179,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessagePrimaryButtonClickedThenFirePixelAndDismiss() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))

val action = Action.Dismiss
Expand All @@ -199,7 +199,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageSecondaryButtonClickedThenFirePixelAndDismiss() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))

val action = Action.Dismiss
Expand All @@ -219,7 +219,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageActionButtonClickedThenFirePixelAndDismiss() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))

val action = Action.Dismiss
Expand Down Expand Up @@ -252,7 +252,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun whenRemoteMessageAvailableAndLowPriorityMessageAvailableThenLowPriorityMessageIsNull() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
val lowPriorityMessage = LowPriorityMessage.DefaultBrowserMessage(
message = MessageCta.Message(
topIllustration = R.drawable.ic_device_mobile_default,
Expand Down Expand Up @@ -372,7 +372,7 @@ class NewTabLegacyPageViewModelTest {

@Test
fun `when onboarding complete and RMF available, then hide logo`() = runTest {
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList())
val remoteMessage = RemoteMessage("id1", Content.Small("", ""), emptyList(), emptyList(), emptyList())
whenever(mockRemoteMessageModel.getActiveMessages()).thenReturn(flowOf(remoteMessage))
whenever(mockDismissedCtaDao.exists(DAX_END)).thenReturn(true)

Expand Down
15 changes: 15 additions & 0 deletions ...g/remote-messaging-api/src/main/java/com/duckduckgo/remote/messaging/api/RemoteMessage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,23 @@ data class RemoteMessage(
val content: Content,
val matchingRules: List<Int>,
val exclusionRules: List<Int>,
val surfaces: List<Surface>,
Copy link
Contributor

@cmonfortep cmonfortep Nov 17, 2025

Choose a reason for hiding this comment

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

All good for now. This needs api proposal, I can review quick for visibility.

However, I'm interested on what's going to be final api interface as I can see different approaches. But will ask in Asana.

Copy link
Contributor Author

@anikiki anikiki Nov 18, 2025

Choose a reason for hiding this comment

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

Added the proposal 👍

)

enum class Surface(val jsonValue: String) {
MODAL("modal"),
NEW_TAB_PAGE("new_tab_page"),
;

companion object {
Copy link
Contributor

@cmonfortep cmonfortep Nov 17, 2025

Choose a reason for hiding this comment

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

I know there's a similar pattern on this same file, but I feel we don't need to follow it here. I'd propose to just declare the enum here, and keep it as pure data. The mapping logic + default value, make it part of the implementation logic inside the mapper.

Copy link
Contributor Author

@anikiki anikiki Nov 18, 2025

Choose a reason for hiding this comment

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

Thanks, makes sense. Updated 👍

fun fromList(jsonValues: List<String>?): List<Surface> {
return jsonValues?.mapNotNull { value ->
values().firstOrNull { it.jsonValue == value }
} ?: listOf(NEW_TAB_PAGE)
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The default fallback to NEW_TAB_PAGE when surfaces are null or empty could cause issues with messages intended for other surfaces. Consider returning an empty list for null/missing surfaces and handling the default at a higher level, or documenting this behavior clearly if it's intentional for backward compatibility.

Suggested change
} ?: listOf(NEW_TAB_PAGE)
} ?: emptyList()

Copilot uses AI. Check for mistakes.
}
}
}

sealed class Content(val messageType: MessageType) {
data class Small(val titleText: String, val descriptionText: String) : Content(SMALL)
data class Medium(val titleText: String, val descriptionText: String, val placeholder: Placeholder) : Content(MEDIUM)
Expand Down
2 changes: 2 additions & 0 deletions ...g-impl/src/main/java/com/duckduckgo/remote/messaging/impl/AppRemoteMessagingRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class AppRemoteMessagingRepository(
content = remoteMessage.content,
emptyList(),
emptyList(),
emptyList(),
)
return remoteMessage
}
Expand All @@ -83,6 +84,7 @@ class AppRemoteMessagingRepository(
content = message.content,
emptyList(),
emptyList(),
emptyList(),
)
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...mpl/src/main/java/com/duckduckgo/remote/messaging/impl/mappers/JsonRemoteMessageMapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import com.duckduckgo.remote.messaging.api.Content.Small
import com.duckduckgo.remote.messaging.api.JsonMessageAction
import com.duckduckgo.remote.messaging.api.MessageActionMapperPlugin
import com.duckduckgo.remote.messaging.api.RemoteMessage
import com.duckduckgo.remote.messaging.api.Surface
import com.duckduckgo.remote.messaging.impl.models.*
import com.duckduckgo.remote.messaging.impl.models.JsonMessageType.BIG_SINGLE_ACTION
import com.duckduckgo.remote.messaging.impl.models.JsonMessageType.BIG_TWO_ACTION
Expand Down Expand Up @@ -109,6 +110,7 @@ private fun JsonRemoteMessage.map(
content = this.content!!.mapToContent(this.content.messageType, actionMappers),
matchingRules = this.matchingRules.orEmpty(),
exclusionRules = this.exclusionRules.orEmpty(),
surfaces = Surface.fromList(this.surfaces),
)
remoteMessage.localizeMessage(this.translations, locale)
}.onFailure {
Expand Down
1 change: 1 addition & 0 deletions ...pl/src/main/java/com/duckduckgo/remote/messaging/impl/models/JsonRemoteMessagingConfig.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ data class JsonRemoteMessage(
val exclusionRules: List<Int>,
val matchingRules: List<Int>,
val translations: Map<String, JsonContentTranslations>,
val surfaces: List<String>?,
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The nullable surfaces field in JSON model allows null values but silently converts them to [NEW_TAB_PAGE]. This could hide missing data in the JSON configuration. Consider making this non-nullable with a default empty list, or documenting that null explicitly means NEW_TAB_PAGE for backward compatibility.

Suggested change
val surfaces: List<String>?,
val surfaces: List<String> = emptyList(),

Copilot uses AI. Check for mistakes.
)

data class JsonContent(
Expand Down
2 changes: 2 additions & 0 deletions ...saging-impl/src/test/java/com/duckduckgo/remote/messaging/fixtures/JsonRemoteMessageOM.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,14 @@ object JsonRemoteMessageOM {
exclusionRules: List<Int> = emptyList(),
matchingRules: List<Int> = emptyList(),
translations: Map<String, JsonContentTranslations> = emptyMap(),
surfaces: List<String>? = null,
) = JsonRemoteMessage(
id = id,
content = content,
exclusionRules = exclusionRules,
matchingRules = matchingRules,
translations = translations,
surfaces = surfaces,
)

fun aJsonRemoteMessagingConfig(
Expand Down
11 changes: 11 additions & 0 deletions ...-messaging-impl/src/test/java/com/duckduckgo/remote/messaging/fixtures/RemoteMessageOM.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import com.duckduckgo.remote.messaging.api.Content.Placeholder
import com.duckduckgo.remote.messaging.api.Content.Placeholder.ANNOUNCE
import com.duckduckgo.remote.messaging.api.Content.Placeholder.MAC_AND_WINDOWS
import com.duckduckgo.remote.messaging.api.RemoteMessage
import com.duckduckgo.remote.messaging.api.Surface

@Suppress("MemberVisibilityCanBePrivate")
object RemoteMessageOM {
Expand Down Expand Up @@ -98,12 +99,14 @@ object RemoteMessageOM {
content: Content = smallContent(),
exclusionRules: List<Int> = emptyList(),
matchingRules: List<Int> = emptyList(),
surfaces: List<Surface> = emptyList(),
): RemoteMessage {
return RemoteMessage(
id = id,
content = content,
exclusionRules = exclusionRules,
matchingRules = matchingRules,
surfaces = surfaces,
)
}

Expand All @@ -112,12 +115,14 @@ object RemoteMessageOM {
content: Content = mediumContent(),
exclusionRules: List<Int> = emptyList(),
matchingRules: List<Int> = emptyList(),
surfaces: List<Surface> = emptyList(),
): RemoteMessage {
return RemoteMessage(
id = id,
content = content,
exclusionRules = exclusionRules,
matchingRules = matchingRules,
surfaces = surfaces,
)
}

Expand All @@ -126,12 +131,14 @@ object RemoteMessageOM {
content: Content = bigSingleActionContent(),
exclusionRules: List<Int> = emptyList(),
matchingRules: List<Int> = emptyList(),
surfaces: List<Surface> = emptyList(),
): RemoteMessage {
return RemoteMessage(
id = id,
content = content,
exclusionRules = exclusionRules,
matchingRules = matchingRules,
surfaces = surfaces,
)
}

Expand All @@ -140,12 +147,14 @@ object RemoteMessageOM {
content: Content = bigTwoActionsContent(),
exclusionRules: List<Int> = emptyList(),
matchingRules: List<Int> = emptyList(),
surfaces: List<Surface> = emptyList(),
): RemoteMessage {
return RemoteMessage(
id = id,
content = content,
exclusionRules = exclusionRules,
matchingRules = matchingRules,
surfaces = surfaces,
)
}

Expand All @@ -154,12 +163,14 @@ object RemoteMessageOM {
content: Content = promoSingleActionContent(),
exclusionRules: List<Int> = emptyList(),
matchingRules: List<Int> = emptyList(),
surfaces: List<Surface> = emptyList(),
): RemoteMessage {
return RemoteMessage(
id = id,
content = content,
exclusionRules = exclusionRules,
matchingRules = matchingRules,
surfaces = surfaces,
)
}
}
11 changes: 11 additions & 0 deletions ...pl/src/test/java/com/duckduckgo/remote/messaging/impl/AppRemoteMessagingRepositoryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
)

Expand All @@ -102,6 +103,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
message,
)
Expand All @@ -120,6 +122,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
)

Expand All @@ -135,6 +138,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
message,
)
Expand All @@ -156,6 +160,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
)

Expand All @@ -174,6 +179,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
message,
)
Expand All @@ -197,6 +203,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
)

Expand All @@ -217,6 +224,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
message,
)
Expand All @@ -238,6 +246,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
)

Expand All @@ -256,6 +265,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
),
message,
)
Expand Down Expand Up @@ -426,6 +436,7 @@ class AppRemoteMessagingRepositoryTest {
),
matchingRules = emptyList(),
exclusionRules = emptyList(),
surfaces = emptyList(),
)
}
}
Loading
Loading