-
@@ -22,25 +22,51 @@
/>
{{i18n "admin.adplugin.house_ads.show_to_anons"}}
+
+
+ {{i18n "admin.adplugin.house_ads.category_chooser_description"}}
+
+
+
+ {{i18n "admin.adplugin.house_ads.group_chooser_description"}}
+
New Ad
"
@visible_to_logged_in_users = true
@visible_to_anons = true
+ @group_ids = []
+ @category_ids = []
end
def self.from_hash(h)
@@ -29,10 +37,12 @@ module ::AdPlugin
ad.name = h[:name]
ad.html = h[:html]
ad.id = h[:id].to_i if h[:id]
- if h.key?(:visible_to_logged_in_users)
- ad.visible_to_logged_in_users = h[:visible_to_logged_in_users]
- end
+ ad.visible_to_logged_in_users = h[:visible_to_logged_in_users] if h.key?(
+ :visible_to_logged_in_users,
+ )
ad.visible_to_anons = h[:visible_to_anons] if h.key?(:visible_to_anons)
+ ad.group_ids = h[:group_ids].map(&:to_i) if h.key?(:group_ids)
+ ad.category_ids = h[:category_ids].map(&:to_i) if h.key?(:category_ids)
ad
end
@@ -72,8 +82,24 @@ module ::AdPlugin
self.all.select(&:visible_to_anons)
end
- def self.all_for_logged_in_users
- self.all.select(&:visible_to_logged_in_users)
+ def self.all_for_logged_in_users(scope)
+ if scope.nil?
+ # this is for our admin page, so we don't need to filter by group
+ self.all.select(&:visible_to_logged_in_users)
+ else
+ # otherwise, filter by group and visible categories
+ self.all.select do |ad|
+ (
+ ad.group_ids.any? { |group_id| scope.user.groups.pluck(:id).include?(group_id) } ||
+ ad.group_ids.include?(Group::AUTO_GROUPS[:everyone]) || ad.group_ids.empty?
+ ) && ad.visible_to_logged_in_users &&
+ (
+ ad.category_ids.any? do |category_id|
+ Category.secured(scope).pluck(:id).include?(category_id)
+ end || true
+ )
+ end
+ end
end
def save
@@ -91,10 +117,13 @@ module ::AdPlugin
def update(attrs)
self.name = attrs[:name]
self.html = attrs[:html]
- if attrs.key?(:visible_to_logged_in_users)
- self.visible_to_logged_in_users = attrs[:visible_to_logged_in_users]
- end
+ self.visible_to_logged_in_users = attrs[:visible_to_logged_in_users] if attrs.key?(
+ :visible_to_logged_in_users,
+ )
self.visible_to_anons = attrs[:visible_to_anons] if attrs.key?(:visible_to_anons)
+ # ensure that group_ids and category_ids can be set to an empty array
+ self.group_ids = attrs.key?(:group_ids) ? attrs[:group_ids].map(&:to_i) : []
+ self.category_ids = attrs.key?(:category_ids) ? attrs[:category_ids].map(&:to_i) : []
self.save
end
@@ -105,6 +134,8 @@ module ::AdPlugin
html: @html,
visible_to_logged_in_users: @visible_to_logged_in_users,
visible_to_anons: @visible_to_anons,
+ group_ids: @group_ids,
+ category_ids: @category_ids,
}
end
diff --git a/app/models/house_ad_setting.rb b/app/models/house_ad_setting.rb
index 6c3d525..4129299 100644
--- a/app/models/house_ad_setting.rb
+++ b/app/models/house_ad_setting.rb
@@ -21,14 +21,14 @@ module ::AdPlugin
settings
end
- def self.settings_and_ads(for_anons: true)
+ def self.settings_and_ads(for_anons: true, scope: nil)
settings = AdPlugin::HouseAdSetting.all
ad_names = settings.values.map { |v| v.split("|") }.flatten.uniq
if for_anons
ads = AdPlugin::HouseAd.all_for_anons
else
- ads = AdPlugin::HouseAd.all_for_logged_in_users
+ ads = AdPlugin::HouseAd.all_for_logged_in_users(scope)
end
ads = ads.select { |ad| ad_names.include?(ad.name) }
@@ -41,7 +41,7 @@ module ::AdPlugin
),
creatives:
ads.inject({}) do |h, ad|
- h[ad.name] = ad.html
+ h[ad.name] = { html: ad.html, category_ids: ad.category_ids }
h
end,
}
diff --git a/assets/javascripts/discourse/components/house-ad.js b/assets/javascripts/discourse/components/house-ad.js
index a80d564..7360aec 100644
--- a/assets/javascripts/discourse/components/house-ad.js
+++ b/assets/javascripts/discourse/components/house-ad.js
@@ -78,7 +78,14 @@ export default AdComponent.extend({
}
let ad = houseAds.creatives[adNames[adIndex[placement]]] || "";
adIndex[placement] = (adIndex[placement] + 1) % adNames.length;
- return ad;
+ // check if the ad includes the current category, or if no category restrictions are set for the ad
+ // otherwise don't show it
+ if (
+ !ad.category_ids?.length ||
+ ad.category_ids.includes(this.currentCategoryId)
+ ) {
+ return ad.html;
+ }
} else {
return "";
}
diff --git a/assets/stylesheets/adplugin.scss b/assets/stylesheets/adplugin.scss
index b1a6afc..2999ac7 100644
--- a/assets/stylesheets/adplugin.scss
+++ b/assets/stylesheets/adplugin.scss
@@ -214,6 +214,9 @@
}
.adplugin-mgmt {
+ .house-ad-name {
+ width: 100%;
+ }
.house-ads-actions {
.btn {
margin-right: 8px;
@@ -249,6 +252,16 @@
padding-left: 2%;
.visibility-settings {
margin-bottom: 1em;
+
+ .description {
+ color: var(--primary-medium);
+ font-size: $font-down-1;
+ }
+
+ .category-selector,
+ .group-chooser {
+ margin-top: 1em;
+ }
}
.controls {
margin-bottom: 1em;
@@ -269,3 +282,7 @@
}
}
}
+
+.house-ad-preview {
+ width: 100%;
+}
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 2cac931..34c3121 100755
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -1,17 +1,17 @@
en:
js:
adplugin:
- advertisement_label: 'ADVERTISEMENT'
+ advertisement_label: "ADVERTISEMENT"
admin_js:
admin:
site_settings:
categories:
- ad_plugin: 'Ad Plugin'
- dfp_plugin: 'DFP/Ad Manager'
- adsense_plugin: 'AdSense'
- amazon_plugin: 'Amazon'
- carbonads_plugin: 'Carbon Ads'
- adbutler_plugin: 'AdButler'
+ ad_plugin: "Ad Plugin"
+ dfp_plugin: "DFP/Ad Manager"
+ adsense_plugin: "AdSense"
+ amazon_plugin: "Amazon"
+ carbonads_plugin: "Carbon Ads"
+ adbutler_plugin: "AdButler"
adplugin:
house_ads:
title: "House Ads"
@@ -26,6 +26,9 @@ en:
more_settings: "More Settings"
show_to_anons: "Show to anonymous users"
show_to_logged_in_users: "Show to logged in users"
+ category_chooser_description: "Choose the categories where this ad should be displayed or leave empty to show the ad everywhere. The `no_ads_for_categories` site setting has priority over this setting."
+ group_chooser_description: "Choose the groups that can view this ad or leave empty to show the ad to all signed in users."
+ preview: "Preview"
topic_list_top:
title: "Topic list top ads"
diff --git a/plugin.rb b/plugin.rb
index 4f2f41d..cb16c1d 100755
--- a/plugin.rb
+++ b/plugin.rb
@@ -42,7 +42,7 @@ after_initialize do
reloadable_patch { Guardian.prepend ::AdPlugin::GuardianExtensions }
add_to_serializer :site, :house_creatives do
- AdPlugin::HouseAdSetting.settings_and_ads(for_anons: scope.anonymous?)
+ AdPlugin::HouseAdSetting.settings_and_ads(for_anons: scope.anonymous?, scope: scope)
end
add_to_serializer :topic_view, :tags_disable_ads do
diff --git a/spec/models/house_ad_setting_spec.rb b/spec/models/house_ad_setting_spec.rb
index e7e4977..9043aaa 100644
--- a/spec/models/house_ad_setting_spec.rb
+++ b/spec/models/house_ad_setting_spec.rb
@@ -73,6 +73,8 @@ describe AdPlugin::HouseAdSetting do
html: "ANON
",
visible_to_logged_in_users: false,
visible_to_anons: true,
+ group_ids: [],
+ category_ids: [],
)
end
@@ -24,6 +26,8 @@ describe AdPlugin::HouseAd do
html: "LOGGED IN
",
visible_to_logged_in_users: true,
visible_to_anons: false,
+ group_ids: [],
+ category_ids: [],
)
end
@@ -68,11 +72,12 @@ describe AdPlugin::HouseAd do
describe ".all_for_logged_in_users" do
let!(:anon_ad) { create_anon_ad }
let!(:logged_in_ad) { create_logged_in_ad }
+ let!(:user) { Fabricate(:user) }
it "doesn't include ads for anonymous users" do
- expect(AdPlugin::HouseAd.all_for_logged_in_users.map(&:id)).to contain_exactly(
- logged_in_ad.id,
- )
+ expect(
+ AdPlugin::HouseAd.all_for_logged_in_users(Guardian.new(user)).map(&:id),
+ ).to contain_exactly(logged_in_ad.id)
end
end
diff --git a/spec/requests/house_ad_controller_spec.rb b/spec/requests/house_ad_controller_spec.rb
index 6ffa9f1..3069fe4 100644
--- a/spec/requests/house_ad_controller_spec.rb
+++ b/spec/requests/house_ad_controller_spec.rb
@@ -2,6 +2,8 @@
describe AdPlugin::HouseAdsController do
let(:admin) { Fabricate(:admin) }
+ let(:category) { Fabricate(:category) }
+ let(:group) { Fabricate(:group) }
let!(:ad) do
AdPlugin::HouseAd.create(
@@ -9,6 +11,8 @@ describe AdPlugin::HouseAdsController do
html: "Banner
", visible_to_anons: true, visible_to_logged_in_users: false, + category_ids: [], + group_ids: [], ) end @@ -23,6 +27,8 @@ describe AdPlugin::HouseAdsController do html: ad.html, visible_to_anons: "false", visible_to_logged_in_users: "true", + category_ids: [category.id], + group_ids: [group.id], } expect(response.status).to eq(200) expect(response.parsed_body["house_ad"].symbolize_keys).to eq( @@ -31,6 +37,8 @@ describe AdPlugin::HouseAdsController do html: ad.html, visible_to_anons: false, visible_to_logged_in_users: true, + category_ids: [category.id], + group_ids: [group.id], ) ad_copy = AdPlugin::HouseAd.find(ad.id) @@ -38,6 +46,8 @@ describe AdPlugin::HouseAdsController do expect(ad_copy.html).to eq(ad.html) expect(ad_copy.visible_to_anons).to eq(false) expect(ad_copy.visible_to_logged_in_users).to eq(true) + expect(ad_copy.category_ids).to eq([category.id]) + expect(ad_copy.group_ids).to eq([group.id]) end end @@ -51,6 +61,8 @@ describe AdPlugin::HouseAdsController do html: "blahANON
",
visible_to_logged_in_users: false,
visible_to_anons: true,
+ group_ids: [],
+ category_ids: [],
)
end
@@ -18,6 +25,30 @@ RSpec.describe SiteController do
html: "LOGGED IN
",
visible_to_logged_in_users: true,
visible_to_anons: false,
+ group_ids: [],
+ category_ids: [],
+ )
+ end
+
+ let!(:logged_in_ad_with_category) do
+ AdPlugin::HouseAd.create(
+ name: "logged-in-ad-with-category",
+ html: "LOGGED IN WITH CATEGORY
",
+ visible_to_logged_in_users: true,
+ visible_to_anons: false,
+ group_ids: [group.id],
+ category_ids: [private_category.id],
+ )
+ end
+
+ let!(:logged_in_ad_with_group_2) do
+ AdPlugin::HouseAd.create(
+ name: "logged-in-ad-with-group",
+ html: "LOGGED IN WITH GROUP
",
+ visible_to_logged_in_users: true,
+ visible_to_anons: false,
+ group_ids: [group_2.id],
+ category_ids: [],
)
end
@@ -27,19 +58,49 @@ RSpec.describe SiteController do
html: "EVERYONE
",
visible_to_logged_in_users: true,
visible_to_anons: true,
+ group_ids: [],
+ category_ids: [],
)
end
- before { AdPlugin::HouseAdSetting.update("topic_list_top", "logged-in-ad|anon-ad|everyone-ad") }
+ let!(:everyone_group_ad) do
+ AdPlugin::HouseAd.create(
+ name: "everyone-group-ad",
+ html: "EVERYONE
",
+ visible_to_logged_in_users: true,
+ visible_to_anons: false,
+ group_ids: [Group::AUTO_GROUPS[:everyone]],
+ category_ids: [],
+ )
+ end
+
+ before do
+ AdPlugin::HouseAdSetting.update(
+ "topic_list_top",
+ "logged-in-ad|anon-ad|everyone-ad|logged-in-ad-with-category|logged-in-ad-with-group|everyone-group-ad",
+ )
+ end
describe "#site" do
context "when logged in" do
- before { sign_in(user) }
-
it "only includes ads that are visible to logged in users" do
+ sign_in(user)
+ get "/site.json"
+ # excluded logged_in_ad_with_group_2 and logged_in_ad_with_category
+ expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly(
+ "logged-in-ad",
+ "everyone-group-ad",
+ "everyone-ad",
+ )
+ end
+
+ it "includes ads that are within the logged in user's category permissions" do
+ sign_in(user_with_group)
get "/site.json"
expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly(
"logged-in-ad",
+ "everyone-group-ad",
+ "logged-in-ad-with-category",
"everyone-ad",
)
end
@@ -48,6 +109,7 @@ RSpec.describe SiteController do
context "when anonymous" do
it "only includes ads that are visible to anonymous users" do
get "/site.json"
+ # excludes everyone_group_ad
expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly(
"anon-ad",
"everyone-ad",
diff --git a/test/javascripts/acceptance/house-ad-test.js b/test/javascripts/acceptance/house-ad-test.js
index e6ab6d0..2a35eb0 100644
--- a/test/javascripts/acceptance/house-ad-test.js
+++ b/test/javascripts/acceptance/house-ad-test.js
@@ -25,14 +25,26 @@ acceptance("House Ads", function (needs) {
after_nth_topic: 6,
},
creatives: {
- "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
",
+ "Topic List Top": {
+ html: "TOPIC LIST TOP
",
+ category_ids: [],
+ },
+ "Above Post Stream": {
+ html: "ABOVE POST STREAM
",
+ category_ids: [],
+ },
+ "Above Suggested": {
+ html: "ABOVE SUGGESTED
",
+ category_ids: [],
+ },
+ Post: {
+ html: "BELOW POST
",
+ category_ids: [],
+ },
+ "Between Topic List": {
+ html: "BETWEEN TOPIC LIST
",
+ category_ids: [],
+ },
},
},
});
@@ -114,3 +126,175 @@ acceptance("House Ads", function (needs) {
);
});
});
+
+acceptance(
+ "House Ads | Category and Group Permissions | Authenticated | Display Ad",
+ function (needs) {
+ needs.user();
+ needs.settings({
+ no_ads_for_categories: "",
+ });
+ needs.site({
+ house_creatives: {
+ settings: {
+ topic_list_top: "Topic List Top",
+ },
+ creatives: {
+ "Topic List Top": {
+ html: "TOPIC LIST TOP
",
+ // match /c/bug/1
+ category_ids: [1],
+ },
+ },
+ },
+ });
+
+ test("displays ad to users when current category id is included in ad category_ids", async (assert) => {
+ updateCurrentUser({
+ staff: false,
+ trust_level: 1,
+ show_to_groups: true,
+ });
+ await visit("/c/bug/1");
+ assert
+ .dom(".h-topic-list")
+ .exists(
+ "ad is displayed above the topic list because the current category id is included in the ad category_ids"
+ );
+ });
+ }
+);
+
+acceptance(
+ "House Ads | Category and Group Permissions | Authenticated | Hide Ad",
+ function (needs) {
+ needs.user();
+ needs.settings({
+ no_ads_for_categories: "",
+ });
+ needs.site({
+ house_creatives: {
+ settings: {
+ topic_list_top: "Topic List Top",
+ },
+ creatives: {
+ "Topic List Top": {
+ html: "TOPIC LIST TOP
",
+ // restrict ad to a different category than /c/bug/1
+ category_ids: [2],
+ },
+ },
+ },
+ });
+
+ test("hides ad to users when current category id is not included in ad category_ids", async (assert) => {
+ updateCurrentUser({
+ staff: false,
+ trust_level: 1,
+ show_to_groups: true,
+ });
+ await visit("/c/bug/1");
+ assert
+ .dom(".h-topic-list")
+ .doesNotExist(
+ "ad is not displayed because the current category id is included in the ad category_ids"
+ );
+ });
+ }
+);
+
+acceptance(
+ "House Ads | Category and Group Permissions | Anonymous | Hide Ad",
+ function (needs) {
+ needs.settings({
+ no_ads_for_categories: "",
+ });
+ needs.site({
+ house_creatives: {
+ settings: {
+ topic_list_top: "Topic List Top",
+ },
+ creatives: {
+ "Topic List Top": {
+ html: "TOPIC LIST TOP
",
+ // restrict ad to a different category than /c/bug/1
+ category_ids: [2],
+ },
+ },
+ },
+ });
+
+ test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+ await visit("/c/bug/1");
+ assert
+ .dom(".h-topic-list")
+ .doesNotExist(
+ "ad is not displayed because the current category id is included in the ad category_ids"
+ );
+ });
+ }
+);
+
+acceptance(
+ "House Ads | Category and Group Permissions | Anonymous | Hide Ad",
+ function (needs) {
+ needs.settings({
+ no_ads_for_categories: "",
+ });
+ needs.site({
+ house_creatives: {
+ settings: {
+ topic_list_top: "Topic List Top",
+ },
+ creatives: {
+ "Topic List Top": {
+ html: "TOPIC LIST TOP
",
+ // restrict ad to a different category than /c/bug/1
+ category_ids: [2],
+ },
+ },
+ },
+ });
+
+ test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+ await visit("/c/bug/1");
+ assert
+ .dom(".h-topic-list")
+ .doesNotExist(
+ "ad is not displayed because the current category id is included in the ad category_ids"
+ );
+ });
+ }
+);
+
+acceptance(
+ "House Ads | Category and Group Permissions | Anonymous | Show Ad",
+ function (needs) {
+ needs.settings({
+ no_ads_for_categories: "",
+ });
+ needs.site({
+ house_creatives: {
+ settings: {
+ topic_list_top: "Topic List Top",
+ },
+ creatives: {
+ "Topic List Top": {
+ html: "TOPIC LIST TOP
",
+ // match /c/bug/1
+ category_ids: [1],
+ },
+ },
+ },
+ });
+
+ test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+ await visit("/c/bug/1");
+ assert
+ .dom(".h-topic-list")
+ .exists(
+ "ad is displayed because the current category id is included in the ad category_ids"
+ );
+ });
+ }
+);