FEATURE: support placing ads between topic list for house ads (#143)

* init * more * Pass td and colspan to component * various fixes for house ads between n topics and add a test * Make adComponents condition easier to read Co-authored-by: Jordan Vidrine <jordan@jordanvidrine.com> Co-authored-by: Penar Musaraj <pmusaraj@gmail.com> Co-authored-by: romanrizzi <rizziromanalejandro@gmail.com>
2022-07-07 17:23:29 +05:30 · 2022-07-07 17:23:29 +05:30 · 5901a52dbc
parent c27f5717e4
commit 5901a52dbc
11 changed files with 130 additions and 22 deletions
--- a/app/models/house_ad_setting.rb
+++ b/app/models/house_ad_setting.rb
@ -6,7 +6,8 @@ module ::AdPlugin
      topic_list_top: '',
      topic_above_post_stream: '',
      topic_above_suggested: '',
-      post_bottom: ''
+      post_bottom: '',
+      topic_list_between: ''
    }

    def self.all
@ -28,6 +29,7 @@ module ::AdPlugin
      {
        settings: settings.merge(
          after_nth_post: SiteSetting.house_ads_after_nth_post,
+          after_nth_topic: SiteSetting.house_ads_after_nth_topic,
          house_ads_frequency: SiteSetting.house_ads_frequency
        ),
        creatives: ads.inject({}) { |h, ad| h[ad.name] = ad.html; h }
--- a/assets/javascripts/discourse/components/ad-component.js
+++ b/assets/javascripts/discourse/components/ad-component.js
@ -99,4 +99,14 @@ export default Component.extend({
      return false;
    }
  },
+
+  isNthTopicListItem(n) {
+    let indexNumber = this.get("indexNumber");
+    indexNumber = indexNumber + 1;
+    if (n && n > 0 && indexNumber > 0) {
+      return indexNumber % n === 0;
+    } else {
+      return false;
+    }
+  },
 });
--- a/assets/javascripts/discourse/components/ad-slot.js
+++ b/assets/javascripts/discourse/components/ad-slot.js
@ -68,13 +68,14 @@ const displayCounts = {

 export default AdComponent.extend({
  needsUpdate: false,
+  tagName: "",

  /**
   * For a given ad placement and optionally a post number if in between posts,
   * list all ad network names that are configured to show there.
   */
-  @discourseComputed("placement", "postNumber")
-  availableAdTypes(placement, postNumber) {
+  @discourseComputed("placement", "postNumber", "indexNumber")
+  availableAdTypes(placement, postNumber, indexNumber) {
    let types = [];
    const houseAds = this.site.get("house_creatives"),
      placeUnderscored = placement.replace(/-/g, "_");
@ -82,11 +83,24 @@ export default AdComponent.extend({
    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" &&
+        this.isNthTopicListItem(
+          parseInt(houseAds.settings.after_nth_topic, 10)
+        );
+
      if (
-        Object.keys(houseAds.creatives).length > 0 &&
-        !isBlank(adsForSlot) &&
-        (!postNumber ||
-          this.isNthPost(parseInt(houseAds.settings.after_nth_post, 10)))
+        adAvailable &&
+        (notPlacingBetweenTopics ||
+          this.isNthPost(parseInt(houseAds.settings.after_nth_post, 10)) ||
+          canBePlacedInBetweenTopics)
      ) {
        types.push("house-ad");
      }
@ -98,12 +112,8 @@ export default AdComponent.extend({
        name;

      if (
-        ((config.enabledSetting &&
-          !isBlank(this.siteSettings[config.enabledSetting])) ||
-          config.enabledSetting === false) &&
-        (!postNumber ||
-          !config.nthPost ||
-          this.isNthPost(parseInt(this.siteSettings[config.nthPost], 10)))
+        this._isNetworkAvailable(config.enabledSetting) &&
+        this._shouldPlaceAdInSlot(postNumber, config.nthPost)
      ) {
        if (this.site.mobileView) {
          settingNames = config.mobile || config.desktop;
@ -195,4 +205,25 @@ 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))
+    );
+  },
 });
--- a/assets/javascripts/discourse/components/house-ad.js
+++ b/assets/javascripts/discourse/components/house-ad.js
@ -7,26 +7,47 @@ const adIndex = {
  topic_above_post_stream: null,
  topic_above_suggested: null,
  post_bottom: null,
+  topic_list_between: null,
 };

 export default AdComponent.extend({
  classNames: ["house-creative"],
  classNameBindings: ["adUnitClass"],
+  attributeBindings: ["colspanAttribute:colspan"],
  adHtml: "",

+  @discourseComputed
+  colspanAttribute() {
+    return this.tagName === "td" ? "5" : null;
+  },
+
  @discourseComputed("placement", "showAd")
  adUnitClass(placement, showAd) {
    return showAd ? `house-${placement}` : "";
  },

-  @discourseComputed("showToGroups", "showAfterPost", "showOnCurrentPage")
-  showAd(showToGroups, showAfterPost, showOnCurrentPage) {
-    return showToGroups && showAfterPost && showOnCurrentPage;
+  @discourseComputed(
+    "showToGroups",
+    "showAfterPost",
+    "showAfterTopicListItem",
+    "showOnCurrentPage"
+  )
+  showAd(
+    showToGroups,
+    showAfterPost,
+    showAfterTopicListItem,
+    showOnCurrentPage
+  ) {
+    return (
+      showToGroups &&
+      (showAfterPost || showAfterTopicListItem) &&
+      showOnCurrentPage
+    );
  },

-  @discourseComputed("postNumber")
-  showAfterPost(postNumber) {
-    if (!postNumber) {
+  @discourseComputed("postNumber", "placement")
+  showAfterPost(postNumber, placement) {
+    if (!postNumber && placement !== "topic-list-between") {
      return true;
    }

@ -35,6 +56,17 @@ export default AdComponent.extend({
    );
  },

+  @discourseComputed("placement")
+  showAfterTopicListItem(placement) {
+    if (placement !== "topic-list-between") {
+      return true;
+    }
+
+    return this.isNthTopicListItem(
+      parseInt(this.site.get("house_creatives.settings.after_nth_topic"), 10)
+    );
+  },
+
  chooseAdHtml() {
    const houseAds = this.site.get("house_creatives"),
      placement = this.get("placement").replace(/-/g, "_"),
--- a/assets/javascripts/discourse/templates/admin/plugins-house-ads-index.hbs
+++ b/assets/javascripts/discourse/templates/admin/plugins-house-ads-index.hbs
@ -7,6 +7,7 @@
      {{house-ads-list-setting name="topic_above_post_stream" value=adSettings.topic_above_post_stream allAds=houseAds adSettings=adSettings}}
      {{house-ads-list-setting name="topic_above_suggested" value=adSettings.topic_above_suggested allAds=houseAds adSettings=adSettings}}
      {{house-ads-list-setting name="post_bottom" value=adSettings.post_bottom allAds=houseAds adSettings=adSettings}}
+      {{house-ads-list-setting name="topic_list_between" value=adSettings.topic_list_between allAds=houseAds adSettings=adSettings}}

      {{d-button label="admin.adplugin.house_ads.more_settings"
         icon="cog"
--- a/assets/javascripts/discourse/templates/components/ad-slot.hbs
+++ b/assets/javascripts/discourse/templates/components/ad-slot.hbs
@ -4,5 +4,8 @@
    refreshOnChange=refreshOnChange
    category=category
    listLoading=listLoading
-    postNumber=postNumber}}
+    postNumber=postNumber
+    indexNumber=indexNumber
+    tagName=childTagName
+  }}
 {{/each}}
--- a/assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.hbs
+++ b/assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.hbs
@ -0,0 +1,8 @@
+{{ad-slot
+  placement="topic-list-between"
+  refreshOnChange=listLoading
+  category=category.slug
+  listLoading=listLoading
+  indexNumber=index
+  childTagName="td"
+}}
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@ -37,3 +37,6 @@ en:
          post_bottom:
            title: "Between posts"
            description: "Ads to show in between posts, after every N posts."
+          topic_list_between:
+            title: "Between topics"
+            description: "Ads to show in between topics, after every N topics."
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@ -7,6 +7,7 @@ en:
    no_ads_for_groups: "Don't show ads to users in these groups."
    no_ads_for_categories: "Don't show ads on topic list and topic pages belonging to these categories."
    no_ads_for_tags: "Don't show ads on topic list and topic pages belonging to these tags."
+    house_ads_after_nth_topic: 'If "Between topics" house ads are defined, show an ad after every N topic, where N is this value.'
    house_ads_after_nth_post: 'If "Between posts" house ads are defined, show an ad after every N posts, where N is this value.'
    house_ads_frequency: "If other ad networks are configured to show in an ad slot, how often should house ads be shown, as a percentage."
    ads_txt: "Contents of your ads.txt file. More details available at <a href='https://support.google.com/adsense/answer/7532444?hl=en' target='_blank'>this Google AdSense help page</a>."
--- a/config/settings.yml
+++ b/config/settings.yml
@ -20,6 +20,11 @@ ad_plugin:
    client: true
    default: ""
    type: list
+  house_ads_after_nth_topic:
+    client: true
+    default: 20
+    min: 1
+    max: 10000
  house_ads_after_nth_post:
    client: true
    default: 20
--- a/test/javascripts/acceptance/house-ad-test.js
+++ b/test/javascripts/acceptance/house-ad-test.js
@ -10,23 +10,28 @@ acceptance("House Ads", function (needs) {
  needs.settings({
    no_ads_for_categories: "1",
    house_ads_after_nth_post: 6,
+    house_ads_after_nth_topic: 3,
  });
  needs.site({
    house_creatives: {
      settings: {
-        topic_list_top: "Topic List",
+        topic_list_top: "Topic List Top",
        topic_above_post_stream: "Above Post Stream",
        topic_above_suggested: "Above Suggested",
        post_bottom: "Post",
+        topic_list_between: "Between Topic List",
        after_nth_post: 6,
+        after_nth_topic: 6,
      },
      creatives: {
-        "Topic List": "<div class='h-topic-list'>TOPIC LIST</div>",
+        "Topic List Top": "<div class='h-topic-list'>TOPIC LIST TOP</div>",
        "Above Post Stream":
          "<div class='h-above-post-stream'>ABOVE POST STREAM</div>",
        "Above Suggested":
          "<div class='h-above-suggested'>ABOVE SUGGESTED</div>",
        Post: "<div class='h-post'>BELOW POST</div>",
+        "Between Topic List":
+          "<div class='h-between-topic-list'>BETWEEN TOPIC LIST</div>",
      },
    },
  });
@ -72,6 +77,13 @@ acceptance("House Ads", function (needs) {
      "it should render ad above topic list"
    );

+    await visit("/latest");
+    assert.equal(
+      find(".h-between-topic-list").length,
+      5,
+      "it should render 5 ads"
+    );
+
    await visit("/t/28830");
    assert.equal(
      find(".h-above-post-stream").length,