Fix Bug 1470170 - Implement priority system for AS Router messages

content-src/asrouter/asrouter-content.jsx

@@ -187,7 +187,13 @@ export class ASRouterUISurface extends React.PureComponent {
   componentWillMount() {
     const endpoint = ASRouterUtils.getEndpoint();
     ASRouterUtils.addListener(this.onMessageFromParent);
-    ASRouterUtils.sendMessage({type: "CONNECT_UI_REQUEST", data: {endpoint}});
+
+    // If we are loading about:welcome we want to trigger the onboarding messages
+    if (this.props.document.location.href === "about:welcome") {
+      ASRouterUtils.sendMessage({type: "TRIGGER", data: {trigger: "firstRun"}});
+    } else {
+      ASRouterUtils.sendMessage({type: "CONNECT_UI_REQUEST", data: {endpoint}});
+    }
   }
 
   componentWillUnmount() {

content-src/asrouter/schemas/provider-response.schema.json

@@ -27,6 +27,10 @@
           "targeting": {
             "type": "string",
             "description": "a JEXL expression representing targeting information"
+          },
+          "trigger": {
+            "type": "string",
+            "description": "A string representing what the trigger to show this message is."
           }
         },
         "required": ["id", "template", "content"]

lib/ASRouter.jsm

@@ -232,7 +232,22 @@ class _ASRouter {
     this.messageChannel.sendAsyncMessage(OUTGOING_MESSAGE_NAME, {type: "ADMIN_SET_STATE", data: state});
   }
 
-  async _getBundledMessages(originalMessage, target, force = false) {
+  async _findMessage(msgs, target, data = {}) {
+    let message;
+    let {trigger} = data;
+    if (trigger) {
+      // Find a message that matches the targeting context as well as the trigger context
+      message = await ASRouterTargeting.findMatchingMessageWithTrigger(msgs, target, trigger);
+    }
+    if (!message) {
+      // If there was no messages with this trigger, try finding a regular targeted message
+      message = await ASRouterTargeting.findMatchingMessage(msgs, target);
+    }
+
+    return message;
+  }
+
+  async _getBundledMessages(originalMessage, target, data, force = false) {
     let result = [{content: originalMessage.content, id: originalMessage.id}];
 
     // First, find all messages of same template. These are potential matching targeting candidates
@@ -251,7 +266,7 @@ class _ASRouter {
     } else {
       while (bundledMessagesOfSameTemplate.length) {
         // Find a message that matches the targeting context - or break if there are no matching messages
-        const message = await ASRouterTargeting.findMatchingMessage(bundledMessagesOfSameTemplate, target);
+        const message = await this._findMessage(bundledMessagesOfSameTemplate, target, data);
         if (!message) {
           /* istanbul ignore next */ // Code coverage in mochitests
           break;
@@ -279,11 +294,11 @@ class _ASRouter {
     return state.messages.filter(item => !state.blockList.includes(item.id));
   }
 
-  async _sendMessageToTarget(message, target, force = false) {
+  async _sendMessageToTarget(message, target, data, force = false) {
     let bundledMessages;
     // If this message needs to be bundled with other messages of the same template, find them and bundle them together
     if (message && message.bundled) {
-      bundledMessages = await this._getBundledMessages(message, target, force);
+      bundledMessages = await this._getBundledMessages(message, target, data, force);
     }
     if (message && !message.bundled) {
       // If we only need to send 1 message, send the message
@@ -296,7 +311,8 @@ class _ASRouter {
     }
   }
 
-  async sendNextMessage(target) {
+  async sendNextMessage(target, action = {}) {
+    let {data} = action;
     const msgs = this._getUnblockedMessages();
     let message = null;
     const previewMsgs = this.state.messages.filter(item => item.provider === "preview");
@@ -304,18 +320,18 @@ class _ASRouter {
     if (previewMsgs.length) {
       [message] = previewMsgs;
     } else {
-      message = await ASRouterTargeting.findMatchingMessage(msgs, target);
+      message = await this._findMessage(msgs, target, data);
     }
     await this.setState({lastMessageId: message ? message.id : null});
 
-    await this._sendMessageToTarget(message, target);
+    await this._sendMessageToTarget(message, target, data);
   }
 
-  async setMessageById(id, target, force = true) {
+  async setMessageById(id, target, force = true, action = {}) {
     await this.setState({lastMessageId: id});
     const newMessage = this.getMessageById(id);
 
-    await this._sendMessageToTarget(newMessage, target, force);
+    await this._sendMessageToTarget(newMessage, target, force, action.data);
   }
 
   async blockById(idOrIds) {
@@ -368,6 +384,7 @@ class _ASRouter {
     switch (action.type) {
       case "CONNECT_UI_REQUEST":
       case "GET_NEXT_MESSAGE":
+      case "TRIGGER":
         // Wait for our initial message loading to be done before responding to any UI requests
         await this.waitForInitialized;
         if (action.data && action.data.endpoint) {
@@ -375,7 +392,7 @@ class _ASRouter {
         }
         // Check if any updates are needed first
         await this.loadMessagesFromAllProviders();
-        await this.sendNextMessage(target);
+        await this.sendNextMessage(target, action);
         break;
       case ra.OPEN_PRIVATE_BROWSER_WINDOW:
         // Forcefully open about:privatebrowsing
@@ -414,7 +431,7 @@ class _ASRouter {
         });
         break;
       case "OVERRIDE_MESSAGE":
-        await this.setMessageById(action.data.id, target);
+        await this.setMessageById(action.data.id, target, true, action);
         break;
       case "ADMIN_CONNECT_STATE":
         if (action.data && action.data.endpoint) {

lib/ASRouterTargeting.jsm

@@ -33,15 +33,11 @@ const TargetingGetters = {
   }
 };
 
-function EnvironmentTargeting(target) {
-  return {isFirstRun: target.url === "about:welcome"};
-}
-
 this.ASRouterTargeting = {
   Environment: TargetingGetters,
 
   isMatch(filterExpression, target, context = this.Environment) {
-    return FilterExpressions.eval(filterExpression, {...context, ...EnvironmentTargeting(target)});
+    return FilterExpressions.eval(filterExpression, context);
   },
 
   /**
@@ -58,7 +54,20 @@ this.ASRouterTargeting = {
     let candidate;
     while (!match && arrayOfItems.length) {
       candidate = removeRandomItemFromArray(arrayOfItems);
-      if (candidate && (!candidate.targeting || await this.isMatch(candidate.targeting, target, context))) {
+      if (candidate && !candidate.trigger && (!candidate.targeting || await this.isMatch(candidate.targeting, target, context))) {
+        match = candidate;
+      }
+    }
+    return match;
+  },
+
+  async findMatchingMessageWithTrigger(messages, target, trigger, context) {
+    const arrayOfItems = [...messages];
+    let match;
+    let candidate;
+    while (!match && arrayOfItems.length) {
+      candidate = removeRandomItemFromArray(arrayOfItems);
+      if (candidate && candidate.trigger === trigger && (!candidate.targeting || await this.isMatch(candidate.targeting, target, context))) {
         match = candidate;
       }
     }

lib/OnboardingMessageProvider.jsm

@@ -15,7 +15,7 @@ const ONBOARDING_MESSAGES = [
       button_label: "Try It Now",
       button_action: "OPEN_PRIVATE_BROWSER_WINDOW"
     },
-    targeting: "isFirstRun"
+    trigger: "firstRun"
   },
   {
     id: "ONBOARDING_2",
@@ -29,7 +29,7 @@ const ONBOARDING_MESSAGES = [
       button_action: "OPEN_URL",
       button_action_params: "https://screenshots.firefox.com/#tour"
     },
-    targeting: "isFirstRun"
+    trigger: "firstRun"
   },
   {
     id: "ONBOARDING_3",
@@ -43,7 +43,8 @@ const ONBOARDING_MESSAGES = [
       button_action: "OPEN_ABOUT_PAGE",
       button_action_params: "addons"
     },
-    targeting: "isFirstRun && isInExperimentCohort == 1"
+    targeting: "isInExperimentCohort == 1",
+    trigger: "firstRun"
   },
   {
     id: "ONBOARDING_4",
@@ -57,7 +58,8 @@ const ONBOARDING_MESSAGES = [
       button_action: "OPEN_URL",
       button_action_params: "https://addons.mozilla.org/en-US/firefox/addon/ghostery/"
     },
-    targeting: "isFirstRun && isInExperimentCohort == 2"
+    targeting: "isInExperimentCohort == 2",
+    trigger: "firstRun"
   }
 ];
 

test/unit/asrouter/ASRouter.test.js

@@ -145,7 +145,7 @@ describe("ASRouter", () => {
       await Router.setState(() => ({blockList: ALL_MESSAGE_IDS.slice(1)}));
       const targetStub = {sendAsyncMessage: sandbox.stub()};
 
-      await Router.sendNextMessage(targetStub, null);
+      await Router.sendNextMessage(targetStub);
 
       assert.calledOnce(targetStub.sendAsyncMessage);
       assert.equal(Router.state.lastMessageId, ALL_MESSAGE_IDS[0]);
@@ -154,7 +154,7 @@ describe("ASRouter", () => {
       await Router.setState(() => ({blockList: ALL_MESSAGE_IDS}));
       const targetStub = {sendAsyncMessage: sandbox.stub()};
 
-      await Router.sendNextMessage(targetStub, null);
+      await Router.sendNextMessage(targetStub);
 
       assert.calledOnce(targetStub.sendAsyncMessage);
       assert.equal(Router.state.lastMessageId, null);
@@ -331,7 +331,7 @@ describe("ASRouter", () => {
       await Router.onMessage(msg);
 
       assert.calledOnce(Router.sendNextMessage);
-      assert.calledWithExactly(Router.sendNextMessage, sinon.match.instanceOf(FakeRemotePageManager));
+      assert.calledWithExactly(Router.sendNextMessage, sinon.match.instanceOf(FakeRemotePageManager), {type: "CONNECT_UI_REQUEST"});
     });
     it("should call sendNextMessage on GET_NEXT_MESSAGE", async () => {
       sandbox.stub(Router, "sendNextMessage").resolves();
@@ -340,7 +340,7 @@ describe("ASRouter", () => {
       await Router.onMessage(msg);
 
       assert.calledOnce(Router.sendNextMessage);
-      assert.calledWithExactly(Router.sendNextMessage, sinon.match.instanceOf(FakeRemotePageManager));
+      assert.calledWithExactly(Router.sendNextMessage, sinon.match.instanceOf(FakeRemotePageManager), {type: "GET_NEXT_MESSAGE"});
     });
     it("should return the preview message if that's available", async () => {
       const expectedObj = {provider: "preview"};
@@ -398,6 +398,39 @@ describe("ASRouter", () => {
     });
   });
 
+  describe("#onMessage: TRIGGER", () => {
+    it("should pass the trigger to ASRouterTargeting on TRIGGER message", async () => {
+      sandbox.stub(Router, "_findMessage").resolves();
+      const msg = fakeAsyncMessage({type: "TRIGGER", data: {trigger: "firstRun"}});
+      await Router.onMessage(msg);
+
+      assert.calledOnce(Router._findMessage);
+      assert.deepEqual(Router._findMessage.firstCall.args[2], {trigger: "firstRun"});
+    });
+    it("consider the trigger when picking a message", async () => {
+      let messages = [
+        {id: "foo1", template: "simple_template", bundled: 1, trigger: "foo", content: {title: "Foo1", body: "Foo123-1"}}
+      ];
+
+      const {target, data} = fakeAsyncMessage({type: "TRIGGER", data: {trigger: "foo"}});
+      let message = await Router._findMessage(messages, target, data.data);
+      assert.equal(message, messages[0]);
+    });
+    it("should pick a message with the right targeting and trigger", async () => {
+      let messages = [
+        {id: "foo1", template: "simple_template", bundled: 2, trigger: "foo", content: {title: "Foo1", body: "Foo123-1"}},
+        {id: "foo2", template: "simple_template", bundled: 2, trigger: "bar", content: {title: "Foo2", body: "Foo123-2"}},
+        {id: "foo3", template: "simple_template", bundled: 2, trigger: "foo", content: {title: "Foo3", body: "Foo123-3"}}
+      ];
+      await Router.setState({messages});
+      const {target, data} = fakeAsyncMessage({type: "TRIGGER", data: {trigger: "foo"}});
+      let {bundle} = await Router._getBundledMessages(messages[0], target, data.data);
+      assert.equal(bundle.length, 2);
+      // it should have picked foo1 and foo3 only
+      assert.isTrue(bundle.every(elem => elem.id === "foo1" || elem.id === "foo3"));
+    });
+  });
+
   describe("#onMessage: OVERRIDE_MESSAGE", () => {
     it("should broadcast a SET_MESSAGE message to all clients with a particular id", async () => {
       const [testMessage] = Router.state.messages;

test/unit/asrouter/asrouter-content.test.jsx

@@ -43,6 +43,7 @@ describe("ASRouterUISurface", () => {
   beforeEach(() => {
     sandbox = sinon.sandbox.create();
     fakeDocument = {
+      location: {href: ""},
       _listeners: new Set(),
       _visibilityState: "hidden",
       get visibilityState() {