From 5901a52dbcbc4302c26a91ee05d57f3e1faea807 Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Thu, 7 Jul 2022 17:23:29 +0530 Subject: [PATCH] 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 Co-authored-by: Penar Musaraj Co-authored-by: romanrizzi --- app/models/house_ad_setting.rb | 4 +- .../discourse/components/ad-component.js | 10 ++++ .../discourse/components/ad-slot.js | 55 +++++++++++++++---- .../discourse/components/house-ad.js | 44 +++++++++++++-- .../admin/plugins-house-ads-index.hbs | 1 + .../templates/components/ad-slot.hbs | 5 +- .../discourse-adplugin.hbs | 8 +++ config/locales/client.en.yml | 3 + config/locales/server.en.yml | 1 + config/settings.yml | 5 ++ test/javascripts/acceptance/house-ad-test.js | 16 +++++- 11 files changed, 130 insertions(+), 22 deletions(-) create mode 100644 assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.hbs diff --git a/app/models/house_ad_setting.rb b/app/models/house_ad_setting.rb index 396f0be..3911028 100644 --- 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 } diff --git a/assets/javascripts/discourse/components/ad-component.js b/assets/javascripts/discourse/components/ad-component.js index e165560..0663b95 100644 --- 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; + } + }, }); diff --git a/assets/javascripts/discourse/components/ad-slot.js b/assets/javascripts/discourse/components/ad-slot.js index 1de7b9d..e2708c7 100644 --- 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)) + ); + }, }); diff --git a/assets/javascripts/discourse/components/house-ad.js b/assets/javascripts/discourse/components/house-ad.js index d22d4bc..f1fb452 100644 --- 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, "_"), diff --git a/assets/javascripts/discourse/templates/admin/plugins-house-ads-index.hbs b/assets/javascripts/discourse/templates/admin/plugins-house-ads-index.hbs index 5b72559..47eae3a 100644 --- 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" diff --git a/assets/javascripts/discourse/templates/components/ad-slot.hbs b/assets/javascripts/discourse/templates/components/ad-slot.hbs index ba4e35d..ef081bf 100644 --- 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}} diff --git 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 new file mode 100644 index 0000000..48af0fc --- /dev/null +++ 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" +}} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 4b5db05..690efa0 100755 --- 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." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 1396682..85a082d 100755 --- 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 this Google AdSense help page." diff --git a/config/settings.yml b/config/settings.yml index a2cdd93..7ba811c 100755 --- 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 diff --git a/test/javascripts/acceptance/house-ad-test.js b/test/javascripts/acceptance/house-ad-test.js index 890fe96..a844a91 100644 --- 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": "
TOPIC LIST
", + "Topic List Top": "
TOPIC LIST TOP
", "Above Post Stream": "
ABOVE POST STREAM
", "Above Suggested": "
ABOVE SUGGESTED
", Post: "
BELOW POST
", + "Between Topic List": + "
BETWEEN TOPIC LIST
", }, }, }); @@ -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,