From d9b739f8031646a30dbe11a9ad1ae9c959d8afd2 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Tue, 12 Jul 2022 16:13:18 -0300 Subject: [PATCH] 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. --- .../discourse/components/ad-component.js | 18 +- .../discourse/components/ad-slot.js | 201 ++++++++++-------- .../discourse/helpers/slot-position.js | 15 ++ .../discourse-adplugin.js | 14 ++ 4 files changed, 148 insertions(+), 100 deletions(-) create mode 100644 assets/javascripts/discourse/helpers/slot-position.js create mode 100644 assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.js diff --git a/assets/javascripts/discourse/components/ad-component.js b/assets/javascripts/discourse/components/ad-component.js index 0663b95..87fef7e 100644 --- a/assets/javascripts/discourse/components/ad-component.js +++ b/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 this.get("postNumber") % n === 0; - } else { - return false; - } + return isNthPost(n, this.get("postNumber")); }, isNthTopicListItem(n) { - let indexNumber = this.get("indexNumber"); - indexNumber = indexNumber + 1; - if (n && n > 0 && indexNumber > 0) { - return indexNumber % n === 0; - } else { - return false; - } + return isNthTopicListItem(n, this.get("indexNumber")); }, }); diff --git a/assets/javascripts/discourse/components/ad-slot.js b/assets/javascripts/discourse/components/ad-slot.js index e2708c7..29ffb3d 100644 --- a/assets/javascripts/discourse/components/ad-slot.js +++ b/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 = []; - const houseAds = this.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" && - 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; + return slotContenders( + this.site, + this.siteSettings, + placement, + postNumber, + indexNumber + ); }, /** @@ -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)) - ); - }, }); diff --git a/assets/javascripts/discourse/helpers/slot-position.js b/assets/javascripts/discourse/helpers/slot-position.js new file mode 100644 index 0000000..a550533 --- /dev/null +++ b/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; + } +} diff --git a/assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.js b/assets/javascripts/discourse/templates/connectors/after-topic-list-item/discourse-adplugin.js new file mode 100644 index 0000000..a7fdfaf --- /dev/null +++ b/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 + ); + }, +};