EXP 2991: Add surface to messaging fml #28423

jhugman · 2023-01-05T21:21:55Z

This adds a surface parameter to the getMessages() method in NimbusMessagingStorage.

It also takes the opportunity to refactor the messaging feature in the nimbus.fml.yaml into a separate messaging.fml.yaml.

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-debug task.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Used by GitHub Actions.

t-p-white · 2023-01-06T12:29:24Z

messaging.fml.yaml

+          notification-config:
+            polling-interval: 180 # 3 minutes
+
+objects:


out of interest - the original file had objects: in a types: tag. What was that doing and is it no longer required?

The types tag turned out to be unnecessary cruft, and we quietly deprecated it.

Gotcha, thanks 👍

Amejia481 · 2023-01-09T14:50:19Z

app/src/main/java/org/mozilla/fenix/components/Components.kt

+                MessagingMiddleware(
+                    surface = MessageSurfaceId.HOMESCREEN,
+                    messagingStorage = analytics.messagingStorage,
+                ),


I'm a bit confused about how we are going to handle multiple surface, if we are just passing one to the middleware?

Are we just going to have one surface per app startup?

I'm a bit confused about how we are going to handle multiple surface

I'm not sure where the MessagingMiddleware fits in: does it look after one surface, or should it be looking after all surfaces? If all surfaces, how does it fit in with a WorkManager job?

My thought here was that the Notification Worker would use a messaging storage directly.

Should the MessagingSurfaceId travel in the Action/Facts rather than the middleware directly?

I'm not sure where the MessagingMiddleware fits in: does it look after one surface, or should it be looking after all surfaces? If all surfaces, how does it fit in with a WorkManager job?

At the moment, it contains all the logic on handling when a message should be displayed or interacted with, ideally as all the logic practically should be the same for handling messages (Just the surface is different) we could have the middleware to take of all these actions for all surfaces and from other surfaces like the Notification Worker we could dispatch actions, indicating we would like to Evaluate for a specific surface the same could be done in the home screen, just by adding the surface to the Evaluate action.

It will be good to define if we will be able support sending multiple messages in multiple surfaces at the same time per app startup.

My thought here was that the Notification Worker would use a messaging storage directly.

We could use the storage directly but we may need to duplicate some of the logic that already live on the MessagingMiddleware.

Should the MessagingSurfaceId travel in the Action/Facts rather than the middleware directly?

Yeah, having the surface as part of the actions could be ideal to keep the common logic in the middleware.

From where are we going to get the MessageSurfaceId? As if we get it directly from the message, we won't need to change much as the message could tell us where it should be shown, without a lot of changes.

From where are we going to get the MessageSurfaceId? As if we get it directly from the message,

Yes, this is correct.

We could use the storage directly but we may need to duplicate some of the logic that already live on the MessagingMiddleware.

Yes, that was my thought too. A judicious refactor of MessagingMiddlewarewould be required, but definitely do-able.

It will be good to define if we will be able support sending multiple messages in multiple surfaces at the same time per app startup.

If I understand your question correctly: it's intended that multiple messages be sent to any surface; however, only one message per surface is displayed at any one time.

If there are two messages eligible for the same surface, the highest priority message is displayed. If it is clicked or dismissed by the user, or it becomes ineligible, the next highest priority message is displayed.

Perfect, thanks for the clarification!
I think If we add the MessageSurfaceId as part of the message we will need to update fewer points, only the ones that are specific about where the message should be shown :) .

Amejia481 · 2023-01-10T14:34:42Z

app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt

@@ -147,7 +148,7 @@ sealed class AppAction : Action {
        /**
         * Updates [MessagingState.messageToShow] with the given [message].
         */
-        object ConsumeMessageToShow : MessagingAction()
+        data class ConsumeMessageToShow(val surface: MessageSurfaceId) : MessagingAction()


Amejia481 · 2023-01-10T14:35:17Z

app/src/main/java/org/mozilla/fenix/gleanplumb/MessagingFeature.kt


 /**
 * A message observer that updates the provided.
 */
 class MessagingFeature(val appStore: AppStore) : LifecycleAwareFeature {

    override fun start() {
-        appStore.dispatch(MessagingAction.Evaluate)
+        appStore.dispatch(MessagingAction.Evaluate(MessageSurfaceId.HOMESCREEN))


Amejia481 · 2023-01-10T14:41:03Z

app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingReducerTest.kt

            ),
        )

+        val m = createMessage("message-1")


nit: maybe message1 😅

Amejia481

In general this looks good 👏🏽 !
Let's address the failing test. Please let's not land this PR until we cut the release branch for 110 very likely it will happen today.

Amejia481 · 2023-01-11T18:28:44Z

Let's wait until we cut the new release Monday the 16th to have full cycle to bake on nightly.

jhugman · 2023-01-17T11:04:51Z

Removed waiting label following merge.

Amejia481

@jhugman awesome work 🏅 , lets land this 🛩️ !

mergify · 2023-01-17T11:46:37Z

This pull request has conflicts when rebasing. Could you fix it @jhugman? 🙏

* Move messaging fml to a separate file * Add surface property to message data * Get messages for just a single surface * Add surface to messaging middleware * ktlint * Add tests for filtering by surface * Add homescreen to default-browser message * Move surface param to MessageActions instead of MessagingMiddleware * Added computed property for surface to message * ktlint * Address reviewer comment * Fixup tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

…28423) * Move messaging fml to a separate file * Add surface property to message data * Get messages for just a single surface * Add surface to messaging middleware * ktlint * Add tests for filtering by surface * Add homescreen to default-browser message * Move surface param to MessageActions instead of MessagingMiddleware * Added computed property for surface to message * ktlint * Address reviewer comment * Fixup tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

jhugman requested review from a team as code owners January 5, 2023 21:21

jhugman marked this pull request as draft January 5, 2023 21:22

t-p-white self-assigned this Jan 6, 2023

t-p-white reviewed Jan 6, 2023

View reviewed changes

jhugman force-pushed the jhugman/exp-2991-add-surface-to-messaging-fml branch 2 times, most recently from 6149e95 to 50a37d6 Compare January 6, 2023 16:38

jhugman marked this pull request as ready for review January 6, 2023 16:39

jhugman requested a review from t-p-white January 6, 2023 19:58

t-p-white approved these changes Jan 9, 2023

View reviewed changes

Amejia481 reviewed Jan 9, 2023

View reviewed changes

jhugman force-pushed the jhugman/exp-2991-add-surface-to-messaging-fml branch 2 times, most recently from 6074e23 to a5c4f82 Compare January 10, 2023 14:03

Amejia481 reviewed Jan 10, 2023

View reviewed changes

Amejia481 added the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Jan 10, 2023

t-p-white mentioned this pull request Jan 10, 2023

Bug 1809444: Add Worker to generate Notifications using Nimbus messaging #28480

Closed

4 tasks

jhugman force-pushed the jhugman/exp-2991-add-surface-to-messaging-fml branch 2 times, most recently from 81ded56 to e1cc1cd Compare January 11, 2023 18:15

jhugman mentioned this pull request Jan 16, 2023

EXP-2994 Refactor of MessageMiddleware so as to be usable from MessageNotificationWorker. #28554

Closed

4 tasks

jhugman force-pushed the jhugman/exp-2991-add-surface-to-messaging-fml branch from e1cc1cd to 5b64739 Compare January 17, 2023 11:03

jhugman added pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] and removed 🙅 waiting Issues that are blocked or has dependencies that are not ready labels Jan 17, 2023

Amejia481 approved these changes Jan 17, 2023

View reviewed changes

jhugman force-pushed the jhugman/exp-2991-add-surface-to-messaging-fml branch from 5b64739 to 038eb98 Compare January 17, 2023 12:08

jhugman added 12 commits January 17, 2023 13:43

Move messaging fml to a separate file

e1a31f6

Add surface property to message data

ff738e2

Get messages for just a single surface

4c06f00

Add surface to messaging middleware

6f309ca

ktlint

bf52471

Add tests for filtering by surface

8ab49bb

Add homescreen to default-browser message

b0a3aa0

Move surface param to MessageActions instead of MessagingMiddleware

24c98f1

Added computed property for surface to message

df2bbc8

ktlint

e7a968c

Address reviewer comment

57342b0

Fixup tests

eb33711

jhugman force-pushed the jhugman/exp-2991-add-surface-to-messaging-fml branch from 038eb98 to eb33711 Compare January 17, 2023 13:44

jhugman and others added 2 commits January 17, 2023 14:16

Merge branch 'main' into jhugman/exp-2991-add-surface-to-messaging-fml

a7e2140

Merge branch 'main' into jhugman/exp-2991-add-surface-to-messaging-fml

9f701db

mergify bot merged commit c332f1b into main Jan 17, 2023

bors bot deleted the jhugman/exp-2991-add-surface-to-messaging-fml branch January 17, 2023 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EXP 2991: Add surface to messaging fml #28423

EXP 2991: Add surface to messaging fml #28423

jhugman commented Jan 5, 2023 •

edited

t-p-white Jan 6, 2023 •

edited

jhugman Jan 6, 2023

t-p-white Jan 6, 2023

Amejia481 Jan 9, 2023

Amejia481 Jan 9, 2023

jhugman Jan 9, 2023

Amejia481 Jan 9, 2023

Amejia481 Jan 9, 2023

jhugman Jan 9, 2023

Amejia481 Jan 9, 2023

Amejia481 Jan 10, 2023

Amejia481 Jan 10, 2023

Amejia481 Jan 10, 2023

Amejia481 left a comment

Amejia481 commented Jan 11, 2023

jhugman commented Jan 17, 2023

Amejia481 left a comment

mergify bot commented Jan 17, 2023

EXP 2991: Add surface to messaging fml #28423

EXP 2991: Add surface to messaging fml #28423

Conversation

jhugman commented Jan 5, 2023 • edited

Pull Request checklist

QA

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

t-p-white Jan 6, 2023 • 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

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

Amejia481 left a comment

Choose a reason for hiding this comment

Amejia481 commented Jan 11, 2023

jhugman commented Jan 17, 2023

Amejia481 left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 17, 2023

jhugman commented Jan 5, 2023 •

edited

t-p-white Jan 6, 2023 •

edited