Add surfaces parameter to RemoteMessage and updated related classes. #7063
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new surfaces field to the RemoteMessage data structure, allowing messages to specify which UI surfaces they should appear on (e.g., modal or new tab page). The PR introduces a Surface enum with a default fallback to NEW_TAB_PAGE when surfaces are not specified.
- Adds
Surfaceenum withMODALandNEW_TAB_PAGEvalues - Updates
RemoteMessagedata class to include asurfacesfield - Updates all test fixtures and test cases to accommodate the new field
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RemoteMessage.kt | Adds Surface enum and surfaces field to RemoteMessage data class with a fromList factory method that defaults to NEW_TAB_PAGE |
| JsonRemoteMessagingConfig.kt | Adds nullable surfaces field to JSON model |
| JsonRemoteMessageMapper.kt | Maps JSON surfaces list to Surface enum list using Surface.fromList |
| AppRemoteMessagingRepository.kt | Provides empty list for surfaces when creating remote messages |
| RemoteMessageOM.kt | Updates test fixture builders to accept surfaces parameter |
| JsonRemoteMessageOM.kt | Updates JSON test fixture builder to accept surfaces parameter |
| RemoteMessageViewModelTest.kt | Updates test cases to include empty surfaces list in RemoteMessage construction |
| RemoteMessagingModelTests.kt | Updates test cases to include empty surfaces list in RemoteMessage construction |
| RemoteMessagingConfigJsonMapperTest.kt | Updates test assertions to include NEW_TAB_PAGE in expected surfaces |
| JsonRemoteMessageMapperTest.kt | Updates test expectations to include NEW_TAB_PAGE in expected surfaces |
| AppRemoteMessagingRepositoryTest.kt | Updates test cases to include empty surfaces list in RemoteMessage construction |
| fun fromList(jsonValues: List<String>?): List<Surface> { | ||
| return jsonValues?.mapNotNull { value -> | ||
| values().firstOrNull { it.jsonValue == value } | ||
| } ?: listOf(NEW_TAB_PAGE) |
There was a problem hiding this comment.
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.
| } ?: listOf(NEW_TAB_PAGE) | |
| } ?: emptyList() |
| val exclusionRules: List<Int>, | ||
| val matchingRules: List<Int>, | ||
| val translations: Map<String, JsonContentTranslations>, | ||
| val surfaces: List<String>?, |
There was a problem hiding this comment.
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.
| val surfaces: List<String>?, | |
| val surfaces: List<String> = emptyList(), |
…o include new surfaces field.
| NEW_TAB_PAGE("new_tab_page"), | ||
| ; | ||
|
|
||
| companion object { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, makes sense. Updated 👍
| val content: Content, | ||
| val matchingRules: List<Int>, | ||
| val exclusionRules: List<Int>, | ||
| val surfaces: List<Surface>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added the proposal 👍
anikiki
deleted the
feature/ana/android_whats_new_promo_message_add_a_new_surfaces_field
branch
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1200581511062568/task/1211783737656573?focus=true
Description
Added support in the RemoteMessage model for displaying remote messages on different surfaces.
This PR introduces a new
Surfaceenum with two values:MODALandNEW_TAB_PAGEand a newsurfacesfield in the RemoteMessage.If no surfaces are specified in the JSON, messages will default to appearing on the
NEW_TAB_PAGEsurface.Steps to test this PR
NO UI changes