Bug 1492454 - Normalise ASRouter frequency caps r=ursula
authork88hudson <k88hudson@gmail.com>
Fri, 21 Sep 2018 19:25:29 +0000
changeset 437740 46e2a6f480a0f3b7f9d25c806f17dc2fc3a68a00
parent 437739 1ae51750c3771499b605a8ad23c0b7a03f4855b8
child 437741 efcd26d12ba67440a1bb46cec29992f8b707136e
push id34695
push userapavel@mozilla.com
push dateSat, 22 Sep 2018 09:35:58 +0000
treeherdermozilla-central@5b433242973a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersursula
bugs1492454
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1492454 - Normalise ASRouter frequency caps r=ursula Differential Revision: https://phabricator.services.mozilla.com/D6527
browser/components/newtab/lib/ASRouter.jsm
browser/components/newtab/test/unit/asrouter/ASRouter.test.js
--- a/browser/components/newtab/lib/ASRouter.jsm
+++ b/browser/components/newtab/lib/ASRouter.jsm
@@ -292,16 +292,28 @@ class _ASRouter {
   // Update message providers and fetch new messages on pref change
   async onPrefChange() {
     this._updateMessageProviders();
     this.overrideOrEnableLegacyOnboarding();
     await this.loadMessagesFromAllProviders();
     this.dispatchToAS(ac.BroadcastToContent({type: at.AS_ROUTER_PREF_CHANGED, data: ASRouterPreferences.specialConditions}));
   }
 
+  // Replace all frequency time period aliases with their millisecond values
+  // This allows us to avoid accounting for special cases later on
+  normalizeItemFrequency({frequency}) {
+    if (frequency && frequency.custom) {
+      for (const setting of frequency.custom) {
+        if (setting.period === "daily") {
+          setting.period = ONE_DAY_IN_MS;
+        }
+      }
+    }
+  }
+
   // Fetch and decode the message provider pref JSON, and update the message providers
   _updateMessageProviders() {
     const providers = [
       // If we have added a `preview` provider, hold onto it
       ...this.state.providers.filter(p => p.id === "preview"),
       ...ASRouterPreferences.providers.filter(p => p.enabled),
     ].map(_provider => {
       // make a copy so we don't modify the source of the pref
@@ -311,16 +323,17 @@ class _ASRouter {
         // Get the messages from the local message provider
         const localProvider = this._localProviders[provider.localProvider];
         provider.messages = localProvider ? localProvider.getMessages() : [];
       }
       if (provider.type === "remote" && provider.url) {
         provider.url = provider.url.replace(/%STARTPAGE_VERSION%/g, STARTPAGE_VERSION);
         provider.url = Services.urlFormatter.formatURL(provider.url);
       }
+      this.normalizeItemFrequency(provider);
       // Reset provider update timestamp to force message refresh
       provider.lastUpdated = undefined;
       return provider;
     });
 
     const providerIDs = providers.map(p => p.id);
     this.setState(prevState => ({
       providers,
@@ -373,16 +386,20 @@ class _ASRouter {
         } else {
           // Skip updating this provider's messages if no update is required
           let messages = this.state.messages.filter(msg => msg.provider === provider.id);
           newState.providers.push(provider);
           newState.messages = [...newState.messages, ...messages];
         }
       }
 
+      for (const message of newState.messages) {
+        this.normalizeItemFrequency(message);
+      }
+
       // Some messages have triggers that require us to initalise trigger listeners
       const unseenListeners = new Set(ASRouterTriggerListeners.keys());
       for (const {trigger} of newState.messages) {
         if (trigger && ASRouterTriggerListeners.has(trigger.id)) {
           ASRouterTriggerListeners.get(trigger.id).init(this._triggerHandler, trigger.params);
           unseenListeners.delete(trigger.id);
         }
       }
@@ -533,19 +550,16 @@ class _ASRouter {
         impressions.length >= Math.min(item.frequency.lifetime, maxLifetimeCap)
       ) {
         return false;
       }
       if (item.frequency.custom) {
         const now = Date.now();
         for (const setting of item.frequency.custom) {
           let {period} = setting;
-          if (period === "daily") {
-            period = ONE_DAY_IN_MS;
-          }
           const impressionsInPeriod = impressions.filter(t => (now - t) < period);
           if (impressionsInPeriod.length >= setting.cap) {
             return false;
           }
         }
       }
     }
     return true;
@@ -655,26 +669,26 @@ class _ASRouter {
       this._storage.set(impressionsString, impressions);
     }
     return impressions;
   }
 
   /**
    * getLongestPeriod
    *
-   * @param {obj} message An ASRouter message
-   * @returns {int|null} if the message has custom frequency caps, the longest period found in the list of caps.
-                         if the message has no custom frequency caps, null
+   * @param {obj} item Either an ASRouter message or an ASRouter provider
+   * @returns {int|null} if the item has custom frequency caps, the longest period found in the list of caps.
+                         if the item has no custom frequency caps, null
    * @memberof _ASRouter
    */
-  getLongestPeriod(message) {
-    if (!message.frequency || !message.frequency.custom) {
+  getLongestPeriod(item) {
+    if (!item.frequency || !item.frequency.custom) {
       return null;
     }
-    return message.frequency.custom.sort((a, b) => b.period - a.period)[0].period;
+    return item.frequency.custom.sort((a, b) => b.period - a.period)[0].period;
   }
 
   /**
    * cleanupImpressions - this function cleans up obsolete impressions whenever
    * messages are refreshed or fetched. It will likely need to be more sophisticated in the future,
    * but the current behaviour for when both message impressions and provider impressions are
    * cleared is as follows (where `item` is either `message` or `provider`):
    *
--- a/browser/components/newtab/test/unit/asrouter/ASRouter.test.js
+++ b/browser/components/newtab/test/unit/asrouter/ASRouter.test.js
@@ -17,17 +17,17 @@ import {CFRPageActions} from "lib/CFRPag
 import {GlobalOverrider} from "test/unit/utils";
 import ProviderResponseSchema from "content-src/asrouter/schemas/provider-response.schema.json";
 
 const MESSAGE_PROVIDER_PREF_NAME = "browser.newtabpage.activity-stream.asrouter.messageProviders";
 const ONBOARDING_FINISHED_PREF = "browser.onboarding.notification.finished";
 const FAKE_PROVIDERS = [FAKE_LOCAL_PROVIDER, FAKE_REMOTE_PROVIDER, FAKE_REMOTE_SETTINGS_PROVIDER];
 const ALL_MESSAGE_IDS = [...FAKE_LOCAL_MESSAGES, ...FAKE_REMOTE_MESSAGES].map(message => message.id);
 const FAKE_BUNDLE = [FAKE_LOCAL_MESSAGES[1], FAKE_LOCAL_MESSAGES[2]];
-const ONE_DAY = 24 * 60 * 60 * 1000;
+const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000;
 
 // Creates a message object that looks like messages returned by
 // RemotePageManager listeners
 function fakeAsyncMessage(action) {
   return {data: action, target: new FakeRemotePageManager()};
 }
 // Create a message that looks like a user action
 function fakeExecuteUserAction(action) {
@@ -869,16 +869,33 @@ describe("ASRouter", () => {
     async function addProviderWithFrequency(id, frequency) {
       await Router.setState(state => {
         const newProvider = {id, frequency};
         const providers = [...state.providers, newProvider];
         return {providers};
       });
     }
 
+    describe("frequency normalisation", () => {
+      beforeEach(async () => {
+        const messages = [{frequency: {custom: [{period: "daily", cap: 10}]}}];
+        const provider = {id: "foo", frequency: {custom: [{period: "daily", cap: 100}]}, messages, enabled: true};
+        await createRouterAndInit([provider]);
+      });
+
+      it("period aliases in provider frequency caps should be normalised", () => {
+        const [provider] = Router.state.providers;
+        assert.equal(provider.frequency.custom[0].period, ONE_DAY_IN_MS);
+      });
+      it("period aliases in message frequency caps should be normalised", async () => {
+        const [message] = Router.state.messages;
+        assert.equal(message.frequency.custom[0].period, ONE_DAY_IN_MS);
+      });
+    });
+
     describe("#addImpression", () => {
       it("should add a message impression and update _storage with the current time if the message has frequency caps", async () => {
         clock.tick(42);
         const msg = fakeAsyncMessage({type: "IMPRESSION", data: {id: "foo", provider: FAKE_LOCAL_PROVIDER.id, frequency: {lifetime: 5}}});
         await Router.onMessage(msg);
         assert.isArray(Router.state.messageImpressions.foo);
         assert.deepEqual(Router.state.messageImpressions.foo, [42]);
         assert.calledWith(Router._storage.set, "messageImpressions", {foo: [42]});
@@ -957,17 +974,17 @@ describe("ASRouter", () => {
       describe("lifetime frequency caps", () => {
         it("should return true if .frequency is not defined on the item", () => {
           const item = {id: "foo"};
           const impressions = [0, 1];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isTrue(result);
         });
         it("should return true if there are no impressions", () => {
-          const item = {id: "foo", frequency: {lifetime: 10, custom: [{period: ONE_DAY, cap: 2}]}};
+          const item = {id: "foo", frequency: {lifetime: 10, custom: [{period: ONE_DAY_IN_MS, cap: 2}]}};
           const impressions = [];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isTrue(result);
         });
         it("should return true if the # of impressions is less than .frequency.lifetime of the item", () => {
           const item = {id: "foo", frequency: {lifetime: 3}};
           const impressions = [0, 1];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
@@ -984,75 +1001,69 @@ describe("ASRouter", () => {
           const impressions = [0, 1, 2, 3];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isFalse(result);
         });
       });
 
       describe("custom frequency caps", () => {
         it("should return true if impressions in the time period < the cap and total impressions < the lifetime cap", () => {
-          clock.tick(ONE_DAY + 10);
-          const item = {id: "foo", frequency: {custom: [{period: ONE_DAY, cap: 2}], lifetime: 3}};
-          const impressions = [0, ONE_DAY + 1];
+          clock.tick(ONE_DAY_IN_MS + 10);
+          const item = {id: "foo", frequency: {custom: [{period: ONE_DAY_IN_MS, cap: 2}], lifetime: 3}};
+          const impressions = [0, ONE_DAY_IN_MS + 1];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isTrue(result);
         });
         it("should return false if impressions in the time period > the cap and total impressions < the lifetime cap", () => {
           clock.tick(200);
           const item = {id: "msg1", frequency: {custom: [{period: 100, cap: 2}], lifetime: 3}};
           const impressions = [0, 160, 161];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isFalse(result);
         });
         it("should return false if impressions in one of the time periods > the cap and total impressions < the lifetime cap", () => {
-          clock.tick(ONE_DAY + 200);
+          clock.tick(ONE_DAY_IN_MS + 200);
           const itemTrue = {id: "msg2", frequency: {custom: [{period: 100, cap: 2}]}};
-          const itemFalse = {id: "msg1", frequency: {custom: [{period: 100, cap: 2}, {period: ONE_DAY, cap: 3}]}};
-          const impressions = [0, ONE_DAY + 160, ONE_DAY - 100, ONE_DAY - 200];
+          const itemFalse = {id: "msg1", frequency: {custom: [{period: 100, cap: 2}, {period: ONE_DAY_IN_MS, cap: 3}]}};
+          const impressions = [0, ONE_DAY_IN_MS + 160, ONE_DAY_IN_MS - 100, ONE_DAY_IN_MS - 200];
           assert.isTrue(Router._isBelowItemFrequencyCap(itemTrue, impressions));
           assert.isFalse(Router._isBelowItemFrequencyCap(itemFalse, impressions));
         });
         it("should return false if impressions in the time period < the cap and total impressions > the lifetime cap", () => {
-          clock.tick(ONE_DAY + 10);
-          const item = {id: "msg1", frequency: {custom: [{period: ONE_DAY, cap: 2}], lifetime: 3}};
-          const impressions = [0, 1, 2, 3, ONE_DAY + 1];
+          clock.tick(ONE_DAY_IN_MS + 10);
+          const item = {id: "msg1", frequency: {custom: [{period: ONE_DAY_IN_MS, cap: 2}], lifetime: 3}};
+          const impressions = [0, 1, 2, 3, ONE_DAY_IN_MS + 1];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isFalse(result);
         });
         it("should return true if daily impressions < the daily cap and there is no lifetime cap", () => {
-          clock.tick(ONE_DAY + 10);
-          const item = {id: "msg1", frequency: {custom: [{period: ONE_DAY, cap: 2}]}};
-          const impressions = [0, 1, 2, 3, ONE_DAY + 1];
+          clock.tick(ONE_DAY_IN_MS + 10);
+          const item = {id: "msg1", frequency: {custom: [{period: ONE_DAY_IN_MS, cap: 2}]}};
+          const impressions = [0, 1, 2, 3, ONE_DAY_IN_MS + 1];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isTrue(result);
         });
         it("should return false if daily impressions > the daily cap and there is no lifetime cap", () => {
-          clock.tick(ONE_DAY + 10);
-          const item = {id: "msg1", frequency: {custom: [{period: ONE_DAY, cap: 2}]}};
-          const impressions = [0, 1, 2, 3, ONE_DAY + 1, ONE_DAY + 2, ONE_DAY + 3];
+          clock.tick(ONE_DAY_IN_MS + 10);
+          const item = {id: "msg1", frequency: {custom: [{period: ONE_DAY_IN_MS, cap: 2}]}};
+          const impressions = [0, 1, 2, 3, ONE_DAY_IN_MS + 1, ONE_DAY_IN_MS + 2, ONE_DAY_IN_MS + 3];
           const result = Router._isBelowItemFrequencyCap(item, impressions);
           assert.isFalse(result);
         });
-        it("should allow the 'daily' alias for period", () => {
-          clock.tick(ONE_DAY + 10);
-          const item = {id: "msg1", frequency: {custom: [{period: "daily", cap: 2}]}};
-          assert.isFalse(Router._isBelowItemFrequencyCap(item, [0, 1, 2, 3, ONE_DAY + 1, ONE_DAY + 2, ONE_DAY + 3]));
-          assert.isTrue(Router._isBelowItemFrequencyCap(item, [0, 1, 2, 3, ONE_DAY + 1]));
-        });
       });
     });
 
     describe("#getLongestPeriod", () => {
       it("should return the period if there is only one definition", () => {
         const message = {id: "foo", frequency: {custom: [{period: 200, cap: 2}]}};
         assert.equal(Router.getLongestPeriod(message), 200);
       });
       it("should return the longest period if there are more than one definitions", () => {
-        const message = {id: "foo", frequency: {custom: [{period: 1000, cap: 3}, {period: ONE_DAY, cap: 5}, {period: 100, cap: 2}]}};
-        assert.equal(Router.getLongestPeriod(message), ONE_DAY);
+        const message = {id: "foo", frequency: {custom: [{period: 1000, cap: 3}, {period: ONE_DAY_IN_MS, cap: 5}, {period: 100, cap: 2}]}};
+        assert.equal(Router.getLongestPeriod(message), ONE_DAY_IN_MS);
       });
       it("should return null if there are is no .frequency", () => {
         const message = {id: "foo"};
         assert.isNull(Router.getLongestPeriod(message));
       });
       it("should return null if there are is no .frequency.custom", () => {
         const message = {id: "foo", frequency: {lifetime: 10}};
         assert.isNull(Router.getLongestPeriod(message));
@@ -1066,31 +1077,31 @@ describe("ASRouter", () => {
         // Impressions for "bar" should be removed since that id does not exist in messages
         const result = {foo: [0]};
 
         await createRouterAndInit([{id: "onboarding", type: "local", messages, enabled: true}]);
         assert.calledWith(Router._storage.set, "messageImpressions", result);
         assert.deepEqual(Router.state.messageImpressions, result);
       });
       it("should clear messageImpressions older than the period if no lifetime impression cap is included", async () => {
-        const CURRENT_TIME = ONE_DAY * 2;
+        const CURRENT_TIME = ONE_DAY_IN_MS * 2;
         clock.tick(CURRENT_TIME);
-        const messages = [{id: "foo", frequency: {custom: [{period: ONE_DAY, cap: 5}]}}];
+        const messages = [{id: "foo", frequency: {custom: [{period: ONE_DAY_IN_MS, cap: 5}]}}];
         messageImpressions = {foo: [0, 1, CURRENT_TIME - 10]};
         // Only 0 and 1 are more than 24 hours before CURRENT_TIME
         const result = {foo: [CURRENT_TIME - 10]};
 
         await createRouterAndInit([{id: "onboarding", type: "local", messages, enabled: true}]);
         assert.calledWith(Router._storage.set, "messageImpressions", result);
         assert.deepEqual(Router.state.messageImpressions, result);
       });
       it("should clear messageImpressions older than the longest period if no lifetime impression cap is included", async () => {
-        const CURRENT_TIME = ONE_DAY * 2;
+        const CURRENT_TIME = ONE_DAY_IN_MS * 2;
         clock.tick(CURRENT_TIME);
-        const messages = [{id: "foo", frequency: {custom: [{period: ONE_DAY, cap: 5}, {period: 100, cap: 2}]}}];
+        const messages = [{id: "foo", frequency: {custom: [{period: ONE_DAY_IN_MS, cap: 5}, {period: 100, cap: 2}]}}];
         messageImpressions = {foo: [0, 1, CURRENT_TIME - 10]};
         // Only 0 and 1 are more than 24 hours before CURRENT_TIME
         const result = {foo: [CURRENT_TIME - 10]};
 
         await createRouterAndInit([{id: "onboarding", type: "local", messages, enabled: true}]);
         assert.calledWith(Router._storage.set, "messageImpressions", result);
         assert.deepEqual(Router.state.messageImpressions, result);
       });