diff --git a/app/models/house_ad.rb b/app/models/house_ad.rb index cbf81b4c..1c9430a0 100644 --- a/app/models/house_ad.rb +++ b/app/models/house_ad.rb @@ -88,14 +88,16 @@ def self.all_for_logged_in_users(scope) self.all.select(&:visible_to_logged_in_users) else # otherwise, filter by group and visible categories - self.all.select do |ad| + 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 + ( + ad.category_ids.any? do |category_id| + Category.secured(scope).pluck(:id).include?(category_id) + end || true + ) end end end diff --git a/assets/javascripts/discourse/components/house-ad.js b/assets/javascripts/discourse/components/house-ad.js index 2a6d9ce9..7360aec5 100644 --- a/assets/javascripts/discourse/components/house-ad.js +++ b/assets/javascripts/discourse/components/house-ad.js @@ -78,8 +78,12 @@ export default AdComponent.extend({ } let ad = houseAds.creatives[adNames[adIndex[placement]]] || ""; adIndex[placement] = (adIndex[placement] + 1) % adNames.length; - // check if the ad includes the current category, otherwise don't show it - if (ad.category_ids?.includes(this.currentCategoryId)) { + // 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 { diff --git a/spec/requests/site_controller_spec.rb b/spec/requests/site_controller_spec.rb index 55938857..fced5d43 100644 --- a/spec/requests/site_controller_spec.rb +++ b/spec/requests/site_controller_spec.rb @@ -1,7 +1,12 @@ # frozen_string_literal: true RSpec.describe SiteController do + fab!(:group) + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:user) + fab!(:group_2) { Fabricate(:group) } + fab!(:user_with_group) { Fabricate(:user, group_ids: [group.id]) } let!(:anon_ad) do AdPlugin::HouseAd.create( @@ -25,6 +30,28 @@ ) 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 + let!(:everyone_ad) do AdPlugin::HouseAd.create( name: "everyone-ad", @@ -36,16 +63,44 @@ ) 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 @@ -54,6 +109,7 @@ 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 e6ab6d0b..0eacc9bc 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,79 @@ acceptance("House Ads", function (needs) { ); }); }); + +acceptance( + "House Ads | Category and Group Permissions | 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 | 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" + ); + }); + } +);