FIX: Don't render the connector when we shouldn't display an ad in the topic list item. (#146)

We expose the ad-slot logic to determine which ads are potentially available for each slot and don't render the connector when there are none. Leaking the component logic is not ideal, but I don't see a better solution given the
current design.

assets/javascripts/discourse/components/ad-component.js

@@ -2,6 +2,10 @@ import Component from "@ember/component";
 import { inject as service } from "@ember/service";
 import { alias, or } from "@ember/object/computed";
 import discourseComputed from "discourse-common/utils/decorators";
+import {
+  isNthPost,
+  isNthTopicListItem,
+} from "discourse/plugins/discourse-adplugin/discourse/helpers/slot-position";
 
 export default Component.extend({
   router: service(),
@@ -93,20 +97,10 @@ export default Component.extend({
   },
 
   isNthPost(n) {
-    if (n && n > 0) {
+    return isNthPost(n, this.get("postNumber"));
-      return this.get("postNumber") % n === 0;
-    } else {
-      return false;
-    }
   },
 
   isNthTopicListItem(n) {
-    let indexNumber = this.get("indexNumber");
+    return isNthTopicListItem(n, this.get("indexNumber"));
-    indexNumber = indexNumber + 1;
-    if (n && n > 0 && indexNumber > 0) {
-      return indexNumber % n === 0;
-    } else {
-      return false;
-    }
   },
 });

assets/javascripts/discourse/components/ad-slot.js

@@ -2,6 +2,10 @@ import EmberObject from "@ember/object";
 import AdComponent from "discourse/plugins/discourse-adplugin/discourse/components/ad-component";
 import discourseComputed, { observes } from "discourse-common/utils/decorators";
 import { isBlank } from "@ember/utils";
+import {
+  isNthPost,
+  isNthTopicListItem,
+} from "discourse/plugins/discourse-adplugin/discourse/helpers/slot-position";
 
 const adConfig = EmberObject.create({
   "google-adsense": {
@@ -66,6 +70,108 @@ const displayCounts = {
   allAds: 0,
 };
 
+function _isNetworkAvailable(siteSettings, enabledNetworkSettingName) {
+  // False means there's no setting to enable or disable this ad network.
+  // Assume it's always enabled.
+  if (enabledNetworkSettingName === false) {
+    return true;
+  } else {
+    return (
+      enabledNetworkSettingName &&
+      !isBlank(siteSettings[enabledNetworkSettingName])
+    );
+  }
+}
+
+function _shouldPlaceAdInSlot(
+  siteSettings,
+  currentPostNumber,
+  positionToPlace
+) {
+  return (
+    !currentPostNumber ||
+    !positionToPlace ||
+    isNthPost(parseInt(siteSettings[positionToPlace], 10), currentPostNumber)
+  );
+}
+
+export function slotContenders(
+  site,
+  siteSettings,
+  placement,
+  postNumber,
+  indexNumber
+) {
+  let types = [];
+  const houseAds = site.get("house_creatives"),
+    placeUnderscored = placement.replace(/-/g, "_");
+
+  if (houseAds && houseAds.settings) {
+    const adsForSlot = houseAds.settings[placeUnderscored];
+
+    const adAvailable =
+      Object.keys(houseAds.creatives).length > 0 && !isBlank(adsForSlot);
+
+    // postNumber and indexNumber are both null for topic-list-top, topic-above-post-stream,
+    // and topic-above-suggested placements. Assume we want to place an ad outside the topic list.
+    const notPlacingBetweenTopics = !postNumber && !indexNumber;
+
+    const canBePlacedInBetweenTopics =
+      placeUnderscored === "topic_list_between" &&
+      isNthTopicListItem(
+        parseInt(houseAds.settings.after_nth_topic, 10),
+        indexNumber
+      );
+
+    if (
+      adAvailable &&
+      (notPlacingBetweenTopics ||
+        canBePlacedInBetweenTopics ||
+        isNthPost(parseInt(houseAds.settings.after_nth_post, 10), postNumber))
+    ) {
+      types.push("house-ad");
+    }
+  }
+
+  Object.keys(adConfig).forEach((adNetwork) => {
+    const config = adConfig[adNetwork];
+    let settingNames = null,
+      name;
+
+    if (
+      _isNetworkAvailable(siteSettings, config.enabledSetting) &&
+      _shouldPlaceAdInSlot(siteSettings, postNumber, config.nthPost)
+    ) {
+      if (site.mobileView) {
+        settingNames = config.mobile || config.desktop;
+      } else {
+        settingNames = config.desktop;
+      }
+
+      if (settingNames) {
+        name = settingNames[placement];
+      }
+
+      if (name === undefined) {
+        // follows naming convention: prefix_(mobile_)_{placement}_code
+        name = `${config.settingPrefix}_${
+          site.mobileView ? "mobile_" : ""
+        }${placeUnderscored}_code`;
+      }
+
+      if (
+        name !== false &&
+        siteSettings[name] !== false &&
+        !isBlank(siteSettings[name])
+      ) {
+        types.push(adNetwork);
+      }
+    }
+  });
+
+  return types;
+}
+
 export default AdComponent.extend({
   needsUpdate: false,
   tagName: "",
@@ -76,73 +182,13 @@ export default AdComponent.extend({
    */
   @discourseComputed("placement", "postNumber", "indexNumber")
   availableAdTypes(placement, postNumber, indexNumber) {
-    let types = [];
+    return slotContenders(
-    const houseAds = this.site.get("house_creatives"),
+      this.site,
-      placeUnderscored = placement.replace(/-/g, "_");
+      this.siteSettings,
-
+      placement,
-    if (houseAds && houseAds.settings) {
+      postNumber,
-      const adsForSlot = houseAds.settings[placeUnderscored];
+      indexNumber
-
+    );
-      const adAvailable =
-        Object.keys(houseAds.creatives).length > 0 && !isBlank(adsForSlot);
-
-      // postNumber and indexNumber are both null for topic-list-top, topic-above-post-stream,
-      // and topic-above-suggested placements. Assume we want to place an ad outside the topic list.
-      const notPlacingBetweenTopics = !postNumber && !indexNumber;
-
-      const canBePlacedInBetweenTopics =
-        placeUnderscored === "topic_list_between" &&
-        this.isNthTopicListItem(
-          parseInt(houseAds.settings.after_nth_topic, 10)
-        );
-
-      if (
-        adAvailable &&
-        (notPlacingBetweenTopics ||
-          this.isNthPost(parseInt(houseAds.settings.after_nth_post, 10)) ||
-          canBePlacedInBetweenTopics)
-      ) {
-        types.push("house-ad");
-      }
-    }
-
-    Object.keys(adConfig).forEach((adNetwork) => {
-      const config = adConfig[adNetwork];
-      let settingNames = null,
-        name;
-
-      if (
-        this._isNetworkAvailable(config.enabledSetting) &&
-        this._shouldPlaceAdInSlot(postNumber, config.nthPost)
-      ) {
-        if (this.site.mobileView) {
-          settingNames = config.mobile || config.desktop;
-        } else {
-          settingNames = config.desktop;
-        }
-
-        if (settingNames) {
-          name = settingNames[placement];
-        }
-
-        if (name === undefined) {
-          // follows naming convention: prefix_(mobile_)_{placement}_code
-          name = `${config.settingPrefix}_${
-            this.site.mobileView ? "mobile_" : ""
-          }${placeUnderscored}_code`;
-        }
-
-        if (
-          name !== false &&
-          this.siteSettings[name] !== false &&
-          !isBlank(this.siteSettings[name])
-        ) {
-          types.push(adNetwork);
-        }
-      }
-    });
-
-    return types;
   },
 
   /**
@@ -205,25 +251,4 @@ export default AdComponent.extend({
 
     return networkNames;
   },
-
-  _isNetworkAvailable(enabledNetworkSettingName) {
-    // False means there's no setting to enable or disable this ad network.
-    // Assume it's always enabled.
-    if (enabledNetworkSettingName === false) {
-      return true;
-    } else {
-      return (
-        enabledNetworkSettingName &&
-        !isBlank(this.siteSettings[enabledNetworkSettingName])
-      );
-    }
-  },
-
-  _shouldPlaceAdInSlot(currentPostNumber, positionToPlace) {
-    return (
-      !currentPostNumber ||
-      !positionToPlace ||
-      this.isNthPost(parseInt(this.siteSettings[positionToPlace], 10))
-    );
-  },
 });

assets/javascripts/discourse/helpers/slot-position.js

@@ -0,0 +1,15 @@
+export function isNthPost(every, currentPostNumber) {
+  if (every && every > 0) {
+    return currentPostNumber % every === 0;
+  } else {
+    return false;
+  }
+}
+
+export function isNthTopicListItem(every, currentIndexPosition) {
+  if (every && every > 0 && currentIndexPosition > 0) {
+    return (currentIndexPosition + 1) % every === 0;
+  } else {
+    return false;
+  }
+}

assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.js

@@ -0,0 +1,14 @@
+import { slotContenders } from "discourse/plugins/discourse-adplugin/discourse/components/ad-slot";
+
+export default {
+  shouldRender(args, component) {
+    return (
+      slotContenders(
+        component.site,
+        component.siteSettings,
+        "topic-list-between",
+        args.index
+      ).length === 0
+    );
+  },
+};