From dc03a7eeaffcee71ba9255a7511cf90db44fd43d Mon Sep 17 00:00:00 2001 From: Nan Jiang Date: Thu, 9 Jul 2020 15:10:21 +0000 Subject: [PATCH] Bug 1650104 - Enable reach event for Moments Page experiments r=andreio Differential Revision: https://phabricator.services.mozilla.com/D82770 --- browser/components/newtab/lib/ASRouter.jsm | 9 ++- .../components/newtab/lib/MomentsPageHub.jsm | 39 +++++++++- .../test/unit/asrouter/ASRouter.test.js | 30 +++++++- .../test/unit/lib/MomentsPageHub.test.js | 73 +++++++++++++++++-- 4 files changed, 134 insertions(+), 17 deletions(-) diff --git a/browser/components/newtab/lib/ASRouter.jsm b/browser/components/newtab/lib/ASRouter.jsm index a6ee800cd655..c19cfd0f189f 100644 --- a/browser/components/newtab/lib/ASRouter.jsm +++ b/browser/components/newtab/lib/ASRouter.jsm @@ -111,7 +111,7 @@ const USE_REMOTE_L10N_PREF = // Experiment groups that need to report the reach event in Messaging-Experiments. // If you're adding new groups to it, make sure they're also added in the // `messaging_experiments.reach.objects` defined in "toolkit/components/telemetry/Events.yaml" -const REACH_EVENT_GROUPS = ["cfr"]; +const REACH_EVENT_GROUPS = ["cfr", "moments-page"]; const REACH_EVENT_CATEGORY = "messaging_experiments"; const REACH_EVENT_METHOD = "reach"; @@ -383,7 +383,7 @@ const MessageLoaderUtils = { ) { experiments.push({ group, - forReachEvent: true, + forReachEvent: { sent: false }, experimentSlug: experimentData.slug, branchSlug: branch.slug, ...branch.value, @@ -2045,7 +2045,10 @@ class _ASRouter { const nonReachMessages = []; for (const message of messages) { if (message.forReachEvent) { - this._recordReachEvent(message); + if (!message.forReachEvent.sent) { + this._recordReachEvent(message); + message.forReachEvent.sent = true; + } } else { nonReachMessages.push(message); } diff --git a/browser/components/newtab/lib/MomentsPageHub.jsm b/browser/components/newtab/lib/MomentsPageHub.jsm index 9182fb55f051..6e97d0a80c4f 100644 --- a/browser/components/newtab/lib/MomentsPageHub.jsm +++ b/browser/components/newtab/lib/MomentsPageHub.jsm @@ -17,6 +17,13 @@ XPCOMUtils.defineLazyModuleGetters(this, { const SYSTEM_TICK_INTERVAL = 5 * 60 * 1000; const HOMEPAGE_OVERRIDE_PREF = "browser.startup.homepage_override.once"; +// For the "reach" event of Messaging Experiments +const REACH_EVENT_CATEGORY = "messaging_experiments"; +const REACH_EVENT_METHOD = "reach"; +// Note it's not "moments-page" as Telemetry Events only accepts understores +// for the event `object` +const REACH_EVENT_OBJECT = "moments_page"; + class _MomentsPageHub { constructor() { this.id = "moments-page-hub"; @@ -97,16 +104,42 @@ class _MomentsPageHub { } } + _recordReachEvent(message) { + const extra = { branches: message.branchSlug }; + Services.telemetry.recordEvent( + REACH_EVENT_CATEGORY, + REACH_EVENT_METHOD, + REACH_EVENT_OBJECT, + message.experimentSlug, + extra + ); + } + async messageRequest({ triggerId, template }) { const telemetryObject = { triggerId }; TelemetryStopwatch.start("MS_MESSAGE_REQUEST_TIME_MS", telemetryObject); - const message = await this._handleMessageRequest({ + const messages = await this._handleMessageRequest({ triggerId, template, + returnAll: true, }); TelemetryStopwatch.finish("MS_MESSAGE_REQUEST_TIME_MS", telemetryObject); - if (message) { - this.executeAction(message); + + // Record the "reach" event for all the messages with `forReachEvent`, + // only execute action for the first message without forReachEvent. + const nonReachMessages = []; + for (const message of messages) { + if (message.forReachEvent) { + if (!message.forReachEvent.sent) { + this._recordReachEvent(message); + message.forReachEvent.sent = true; + } + } else { + nonReachMessages.push(message); + } + } + if (nonReachMessages.length) { + this.executeAction(nonReachMessages[0]); } } diff --git a/browser/components/newtab/test/unit/asrouter/ASRouter.test.js b/browser/components/newtab/test/unit/asrouter/ASRouter.test.js index 607281f36fee..c85e4368946a 100644 --- a/browser/components/newtab/test/unit/asrouter/ASRouter.test.js +++ b/browser/components/newtab/test/unit/asrouter/ASRouter.test.js @@ -2333,7 +2333,7 @@ describe("ASRouter", () => { let messages = [ { id: "foo1", - forReachEvent: true, + forReachEvent: { sent: false }, experimentSlug: "exp01", branchSlug: "branch01", group: "cfr", @@ -2351,7 +2351,7 @@ describe("ASRouter", () => { }, { id: "foo3", - forReachEvent: true, + forReachEvent: { sent: false }, experimentSlug: "exp02", branchSlug: "branch02", group: "cfr", @@ -2371,6 +2371,30 @@ describe("ASRouter", () => { await Router.onMessage(msg); assert.calledTwice(Services.telemetry.recordEvent); }); + it("should not record the Reach event if it's already sent", async () => { + let messages = [ + { + id: "foo1", + forReachEvent: { sent: true }, + experimentSlug: "exp01", + branchSlug: "branch01", + group: "cfr", + template: "simple_template", + trigger: { id: "foo" }, + content: { title: "Foo1", body: "Foo123-1" }, + }, + ]; + sandbox.stub(Router, "handleMessageRequest").resolves(messages); + sandbox.spy(Services.telemetry, "recordEvent"); + + const msg = fakeAsyncMessage({ + type: "TRIGGER", + data: { trigger: { id: "foo" } }, + }); + + await Router.onMessage(msg); + assert.notCalled(Services.telemetry.recordEvent); + }); }); describe(".includeBundle", () => { @@ -4109,7 +4133,7 @@ describe("ASRouter", () => { assert.equal(result.messages[1].group, "cfr"); assert.equal(result.messages[1].experimentSlug, "exp01"); assert.equal(result.messages[1].branchSlug, "branch02"); - assert.ok(result.messages[1].forReachEvent); + assert.deepEqual(result.messages[1].forReachEvent, { sent: false }); }); it("should fetch json from url", async () => { let result = await MessageLoaderUtils.loadMessagesForProvider({ diff --git a/browser/components/newtab/test/unit/lib/MomentsPageHub.test.js b/browser/components/newtab/test/unit/lib/MomentsPageHub.test.js index 2b5dca8e099a..48829d68ca3f 100644 --- a/browser/components/newtab/test/unit/lib/MomentsPageHub.test.js +++ b/browser/components/newtab/test/unit/lib/MomentsPageHub.test.js @@ -20,10 +20,10 @@ describe("MomentsPageHub", () => { globals = new GlobalOverrider(); sandbox = sinon.createSandbox(); instance = new _MomentsPageHub(); - const [msg] = (await PanelTestProvider.getMessages()).filter( + const messages = (await PanelTestProvider.getMessages()).filter( ({ template }) => template === "update_action" ); - handleMessageRequestStub = sandbox.stub().resolves(msg); + handleMessageRequestStub = sandbox.stub().resolves(messages); addImpressionStub = sandbox.stub(); blockMessageByIdStub = sandbox.stub(); getStringPrefStub = sandbox.stub(); @@ -39,6 +39,9 @@ describe("MomentsPageHub", () => { getStringPref: getStringPrefStub, setStringPref: setStringPrefStub, }, + telemetry: { + recordEvent: () => {}, + }, }, }); }); @@ -101,12 +104,13 @@ describe("MomentsPageHub", () => { assert.calledWithExactly(handleMessageRequestStub, { triggerId: "trigger", template: "template", + returnAll: true, }); }); it("shouldn't do anything if no message is provided", async () => { // Reset the call from `instance.init` setStringPrefStub.reset(); - handleMessageRequestStub.resolves(null); + handleMessageRequestStub.resolves([]); await instance.messageRequest({ triggerId: "trigger" }); assert.notCalled(setStringPrefStub); @@ -136,6 +140,59 @@ describe("MomentsPageHub", () => { { triggerId: "trigger" } ); }); + it("should record Reach event for the Moments page experiment", async () => { + const momentsMessages = (await PanelTestProvider.getMessages()).filter( + ({ template }) => template === "update_action" + ); + const messages = [ + { + forReachEvent: { sent: false }, + experimentSlug: "foo", + branchSlug: "bar", + }, + ...momentsMessages, + ]; + handleMessageRequestStub.resolves(messages); + sandbox.spy(global.Services.telemetry, "recordEvent"); + sandbox.spy(instance, "executeAction"); + + await instance.messageRequest({ triggerId: "trigger" }); + + assert.calledOnce(global.Services.telemetry.recordEvent); + assert.calledOnce(instance.executeAction); + }); + it("should not record the Reach event if it's already sent", async () => { + const messages = [ + { + forReachEvent: { sent: true }, + experimentSlug: "foo", + branchSlug: "bar", + }, + ]; + handleMessageRequestStub.resolves(messages); + sandbox.spy(global.Services.telemetry, "recordEvent"); + + await instance.messageRequest({ triggerId: "trigger" }); + + assert.notCalled(global.Services.telemetry.recordEvent); + }); + it("should not trigger the action if it's only for the Reach event", async () => { + const messages = [ + { + forReachEvent: { sent: false }, + experimentSlug: "foo", + branchSlug: "bar", + }, + ]; + handleMessageRequestStub.resolves(messages); + sandbox.spy(global.Services.telemetry, "recordEvent"); + sandbox.spy(instance, "executeAction"); + + await instance.messageRequest({ triggerId: "trigger" }); + + assert.calledOnce(global.Services.telemetry.recordEvent); + assert.notCalled(instance.executeAction); + }); }); describe("executeAction", () => { beforeEach(async () => { @@ -147,7 +204,7 @@ describe("MomentsPageHub", () => { }); }); it("should set HOMEPAGE_OVERRIDE_PREF on `moments-wnp` action", async () => { - const msg = await handleMessageRequestStub(); + const [msg] = await handleMessageRequestStub(); sandbox.useFakeTimers(); instance.executeAction(msg); @@ -165,7 +222,7 @@ describe("MomentsPageHub", () => { ); }); it("should block after taking the action", async () => { - const msg = await handleMessageRequestStub(); + const [msg] = await handleMessageRequestStub(); instance.executeAction(msg); assert.calledOnce(blockMessageByIdStub); @@ -174,7 +231,7 @@ describe("MomentsPageHub", () => { it("should compute expire based on expireDelta", async () => { sandbox.spy(instance, "getExpirationDate"); - const msg = await handleMessageRequestStub(); + const [msg] = await handleMessageRequestStub(); instance.executeAction(msg); assert.calledOnce(instance.getExpirationDate); @@ -186,7 +243,7 @@ describe("MomentsPageHub", () => { it("should compute expire based on expireDelta", async () => { sandbox.spy(instance, "getExpirationDate"); - const msg = await handleMessageRequestStub(); + const [msg] = await handleMessageRequestStub(); const msgWithExpire = { ...msg, content: { @@ -212,7 +269,7 @@ describe("MomentsPageHub", () => { ); }); it("should send user telemetry", async () => { - const msg = await handleMessageRequestStub(); + const [msg] = await handleMessageRequestStub(); const sendUserEventTelemetrySpy = sandbox.spy( instance, "sendUserEventTelemetry"