From 958136723935826579ea38a7522d5dab5f419cdd Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Thu, 8 Feb 2024 19:42:40 -0700 Subject: [PATCH] FIX: Add exclude groups for each ad platforms (#197) With the new group system for displaying ads we no longer can check if a user belongs to a trust level group lower than specified. The other problem is that ALL users including staff and higher trust levels all belong to trust level 0. So without this fix if we say that an ad should be visible to trust level 0 users then it will be shown to all users. This fix adds a new default setting for each ad platform for excluding trust level 3, 4, and staff users from being shown ads. - Make display_groups hidden (they will be removed in a later commit) - Switch to using only exclude_groups instead of display groups and exclude groups - rename showToDisplayGroups to showXAds for each provider --- .../discourse/components/adbutler-ad.js | 8 +- .../components/amazon-product-links.js | 4 +- .../discourse/components/carbonads-ad.js | 18 +-- .../discourse/components/google-adsense.js | 8 +- .../discourse/components/google-dfp-ad.js | 8 +- config/locales/server.en.yml | 5 + config/settings.yml | 35 ++++++ ...95100_migrate_adsense_to_exclude_groups.rb | 33 ++++++ ...195101_migrate_amazon_to_exclude_groups.rb | 33 ++++++ ...208195102_migrate_dfp_to_exclude_groups.rb | 33 ++++++ ...195103_migrate_carbon_to_exclude_groups.rb | 33 ++++++ ...5104_migrate_adbutler_to_exclude_groups.rb | 35 ++++++ lib/adplugin/guardian_extensions.rb | 10 +- .../current_user_serializer_spec.rb | 108 ++++++++++++++++++ 14 files changed, 338 insertions(+), 33 deletions(-) create mode 100644 db/migrate/20240208195100_migrate_adsense_to_exclude_groups.rb create mode 100644 db/migrate/20240208195101_migrate_amazon_to_exclude_groups.rb create mode 100644 db/migrate/20240208195102_migrate_dfp_to_exclude_groups.rb create mode 100644 db/migrate/20240208195103_migrate_carbon_to_exclude_groups.rb create mode 100644 db/migrate/20240208195104_migrate_adbutler_to_exclude_groups.rb create mode 100644 spec/serializer/current_user_serializer_spec.rb diff --git a/assets/javascripts/discourse/components/adbutler-ad.js b/assets/javascripts/discourse/components/adbutler-ad.js index 3fba41b..6f8a6e2 100644 --- a/assets/javascripts/discourse/components/adbutler-ad.js +++ b/assets/javascripts/discourse/components/adbutler-ad.js @@ -112,7 +112,7 @@ export default AdComponent.extend({ }, @discourseComputed - showToDisplayGroups() { + showAdbutlerAds() { if (!this.currentUser) { return true; } @@ -122,21 +122,21 @@ export default AdComponent.extend({ @discourseComputed( "publisherId", - "showToDisplayGroups", + "showAdbutlerAds", "showToGroups", "showAfterPost", "showOnCurrentPage" ) showAd( publisherId, - showToDisplayGroups, + showAdbutlerAds, showToGroups, showAfterPost, showOnCurrentPage ) { return ( publisherId && - showToDisplayGroups && + showAdbutlerAds && showToGroups && showAfterPost && showOnCurrentPage diff --git a/assets/javascripts/discourse/components/amazon-product-links.js b/assets/javascripts/discourse/components/amazon-product-links.js index 4b805ed..7133b98 100644 --- a/assets/javascripts/discourse/components/amazon-product-links.js +++ b/assets/javascripts/discourse/components/amazon-product-links.js @@ -7,7 +7,7 @@ export default AdComponent.extend({ classNames: ["amazon-product-links"], showAd: and( - "showToDisplayGroups", + "showAmazonAds", "showToGroups", "showAfterPost", "showOnCurrentPage" @@ -174,7 +174,7 @@ export default AdComponent.extend({ }, @discourseComputed - showToDisplayGroups() { + showAmazonAds() { if (!this.currentUser) { return true; } diff --git a/assets/javascripts/discourse/components/carbonads-ad.js b/assets/javascripts/discourse/components/carbonads-ad.js index 04955e6..d323661 100644 --- a/assets/javascripts/discourse/components/carbonads-ad.js +++ b/assets/javascripts/discourse/components/carbonads-ad.js @@ -20,7 +20,7 @@ export default AdComponent.extend({ }, @discourseComputed - showToDisplayGroups() { + showCarbonAds() { if (!this.currentUser) { return true; } @@ -31,23 +31,13 @@ export default AdComponent.extend({ @discourseComputed( "placement", "serve_id", - "showToDisplayGroups", + "showCarbonAds", "showToGroups", "showOnCurrentPage" ) - showAd( - placement, - serveId, - showToDisplayGroups, - showToGroups, - showOnCurrentPage - ) { + showAd(placement, serveId, showCarbonAds, showToGroups, showOnCurrentPage) { return ( - placement && - serveId && - showToDisplayGroups && - showToGroups && - showOnCurrentPage + placement && serveId && showCarbonAds && showToGroups && showOnCurrentPage ); }, }); diff --git a/assets/javascripts/discourse/components/google-adsense.js b/assets/javascripts/discourse/components/google-adsense.js index 743b160..4850b34 100644 --- a/assets/javascripts/discourse/components/google-adsense.js +++ b/assets/javascripts/discourse/components/google-adsense.js @@ -207,7 +207,7 @@ export default AdComponent.extend({ }, @discourseComputed - showToDisplayGroups() { + showAdsenseAds() { if (!this.currentUser) { return true; } @@ -217,21 +217,21 @@ export default AdComponent.extend({ @discourseComputed( "publisher_id", - "showToDisplayGroups", + "showAdsenseAds", "showToGroups", "showAfterPost", "showOnCurrentPage" ) showAd( publisherId, - showToDisplayGroups, + showAdsenseAds, showToGroups, showAfterPost, showOnCurrentPage ) { return ( publisherId && - showToDisplayGroups && + showAdsenseAds && showToGroups && showAfterPost && showOnCurrentPage diff --git a/assets/javascripts/discourse/components/google-dfp-ad.js b/assets/javascripts/discourse/components/google-dfp-ad.js index 7989e67..d2347be 100755 --- a/assets/javascripts/discourse/components/google-dfp-ad.js +++ b/assets/javascripts/discourse/components/google-dfp-ad.js @@ -299,7 +299,7 @@ export default AdComponent.extend({ @discourseComputed( "publisherId", - "showToDisplayGroups", + "showDfpAds", "showToGroups", "showAfterPost", "showOnCurrentPage", @@ -307,7 +307,7 @@ export default AdComponent.extend({ ) showAd( publisherId, - showToDisplayGroups, + showDfpAds, showToGroups, showAfterPost, showOnCurrentPage, @@ -315,7 +315,7 @@ export default AdComponent.extend({ ) { return ( publisherId && - showToDisplayGroups && + showDfpAds && showToGroups && showAfterPost && showOnCurrentPage && @@ -324,7 +324,7 @@ export default AdComponent.extend({ }, @discourseComputed - showToDisplayGroups() { + showDfpAds() { if (!this.currentUser) { return true; } diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 85a082d..09e7012 100755 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -15,6 +15,7 @@ en: dfp_publisher_id: "Input your Google Ad Manager (formerly called DFP) network code, which is found in your network settings. NOTE: You will need to update the 'content security policy script src' setting. See this topic for the latest instructions." dfp_publisher_id_mobile: "(Optional) If you want to use a different Ad Manager publisher id for the mobile version of the site, enter it here. Leave blank to use dfp_publisher_id on mobile." dfp_through_trust_level: "Show your ads to users based on trust levels. Users with trust level higher than this value will not see ads." + dfp_exclude_groups: "Ads will not be shown to members of these groups" dfp_topic_list_top_code: "Enter the unique Code of the ad unit to display above topic lists. This is the short code (max 100 chars) given to the ad unit when it was created, not the JavaScript code." dfp_topic_list_top_ad_sizes: "Choose your ad size for the ad unit above topic lists." @@ -47,6 +48,7 @@ en: adsense_publisher_code: "Your publisher ID. Enter only the number, excluding 'pub-'. NOTE: You will need to update the 'content security policy script src' setting. See this topic for the latest instructions." adsense_through_trust_level: "Show your ads to users based on trust levels. Users with trust level higher than this value will not see ads." + adsense_exclude_groups: "Ads will not be shown to members of these groups" adsense_topic_list_top_code: "Enter code of the ad unit to display at topic list top location. This is the number assigned to the ad unit, not the JavaScript code." adsense_mobile_topic_list_top_code: "Enter code of the ad unit to display mobile ads at topic list top location. This is the number assigned to the ad unit, not the JavaScript code." adsense_topic_list_top_ad_sizes: "Choose your ad size" @@ -66,6 +68,7 @@ en: adsense_nth_post_code: "Show an ad after every N posts, where N is this value." amazon_through_trust_level: "Show your ads to users based on trust levels. Users with trust level higher than this value will not see ads." + amazon_exclude_groups: "Ads will not be shown to members of these groups" amazon_topic_list_top_src_code: "Enter src code to display at topic list top location" amazon_topic_list_top_ad_width_code: "Input your ad width" amazon_topic_list_top_ad_height_code: "Input your ad height" @@ -95,6 +98,7 @@ en: carbonads_serve_id: "Your Carbon Ads Serve ID" carbonads_placement: "Your Carbon Ads Placement" carbonads_through_trust_level: "Show your ads to users based on trust levels. Users with trust level higher than this value will not see ads." + carbonads_exclude_groups: "Ads will not be shown to members of these groups" carbonads_topic_list_top_enabled: "Show an ad above the topic list" carbonads_above_post_stream_enabled: "Show an ad above the post stream" @@ -109,4 +113,5 @@ en: adbutler_topic_above_suggested_zone_id: "Zone ID for topic above suggested location" adbutler_nth_post: "Show an ad after every N posts, where N is this value" adbutler_through_trust_level: "Show your ads to users based on trust levels. Users with trust level higher than this value will not see ads" + adbutler_exclude_groups: "Ads will not be shown to members of these groups" adbutler_adserver_hostname: "The hostname that AdButler is serving your ads from" diff --git a/config/settings.yml b/config/settings.yml index b754f40..148c5af 100755 --- a/config/settings.yml +++ b/config/settings.yml @@ -56,6 +56,13 @@ adsense_plugin: allow_any: false refresh: true validator: "AtLeastOneGroupValidator" + hidden: true + adsense_exclude_groups: + default: "3|13|14" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" adsense_topic_list_top_code: client: true default: "" @@ -177,6 +184,13 @@ dfp_plugin: allow_any: false refresh: true validator: "AtLeastOneGroupValidator" + hidden: true + dfp_exclude_groups: + default: "3|13|14" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" dfp_topic_list_top_code: client: true default: "" @@ -334,6 +348,13 @@ amazon_plugin: allow_any: false refresh: true validator: "AtLeastOneGroupValidator" + hidden: true + amazon_exclude_groups: + default: "3|13|14" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" amazon_topic_list_top_src_code: client: true default: "" @@ -430,6 +451,13 @@ carbonads_plugin: allow_any: false refresh: true validator: "AtLeastOneGroupValidator" + hidden: true + carbonads_exclude_groups: + default: "3|13|14" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" carbonads_topic_list_top_enabled: client: true default: false @@ -453,6 +481,13 @@ adbutler_plugin: allow_any: false refresh: true validator: "AtLeastOneGroupValidator" + hidden: true + adbutler_exclude_groups: + default: "3|13|14" + type: group_list + allow_any: false + refresh: true + validator: "AtLeastOneGroupValidator" adbutler_topic_list_top_zone_id: client: true default: "" diff --git a/db/migrate/20240208195100_migrate_adsense_to_exclude_groups.rb b/db/migrate/20240208195100_migrate_adsense_to_exclude_groups.rb new file mode 100644 index 0000000..3458037 --- /dev/null +++ b/db/migrate/20240208195100_migrate_adsense_to_exclude_groups.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class MigrateAdsenseToExcludeGroups < ActiveRecord::Migration[7.0] + def up + adsense_display_groups_raw = + DB.query_single("SELECT value FROM site_settings WHERE name = 'adsense_display_groups'").first + + if adsense_display_groups_raw.present? + adsense_exclude_groups = + case adsense_display_groups_raw + when "10" + "3|11|12|13|14" + when "10|11" + "3|12|13|14" + when "10|11|12" + "3|13|14" + when "10|11|12|13" + "3|14" + when "10|11|12|14" + "3" + end + + DB.exec(<<~SQL, setting: adsense_exclude_groups) + INSERT INTO site_settings(name, value, data_type, created_at, updated_at) + VALUES('adsense_exclude_groups', :setting, '20', NOW(), NOW()) + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20240208195101_migrate_amazon_to_exclude_groups.rb b/db/migrate/20240208195101_migrate_amazon_to_exclude_groups.rb new file mode 100644 index 0000000..b4f7a4b --- /dev/null +++ b/db/migrate/20240208195101_migrate_amazon_to_exclude_groups.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class MigrateAmazonToExcludeGroups < ActiveRecord::Migration[7.0] + def up + amazon_display_groups_raw = + DB.query_single("SELECT value FROM site_settings WHERE name = 'amazon_display_groups'").first + + if amazon_display_groups_raw.present? + amazon_exclude_groups = + case amazon_display_groups_raw + when "10" + "3|11|12|13|14" + when "10|11" + "3|12|13|14" + when "10|11|12" + "3|13|14" + when "10|11|12|13" + "3|14" + when "10|11|12|14" + "3" + end + + DB.exec(<<~SQL, setting: amazon_exclude_groups) + INSERT INTO site_settings(name, value, data_type, created_at, updated_at) + VALUES('amazon_exclude_groups', :setting, '20', NOW(), NOW()) + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20240208195102_migrate_dfp_to_exclude_groups.rb b/db/migrate/20240208195102_migrate_dfp_to_exclude_groups.rb new file mode 100644 index 0000000..a8280cb --- /dev/null +++ b/db/migrate/20240208195102_migrate_dfp_to_exclude_groups.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class MigrateDfpToExcludeGroups < ActiveRecord::Migration[7.0] + def up + dfp_display_groups_raw = + DB.query_single("SELECT value FROM site_settings WHERE name = 'dfp_display_groups'").first + + if dfp_display_groups_raw.present? + dfp_exclude_groups = + case dfp_display_groups_raw + when "10" + "3|11|12|13|14" + when "10|11" + "3|12|13|14" + when "10|11|12" + "3|13|14" + when "10|11|12|13" + "3|14" + when "10|11|12|14" + "3" + end + + DB.exec(<<~SQL, setting: dfp_exclude_groups) + INSERT INTO site_settings(name, value, data_type, created_at, updated_at) + VALUES('dfp_exclude_groups', :setting, '20', NOW(), NOW()) + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20240208195103_migrate_carbon_to_exclude_groups.rb b/db/migrate/20240208195103_migrate_carbon_to_exclude_groups.rb new file mode 100644 index 0000000..27a7cc7 --- /dev/null +++ b/db/migrate/20240208195103_migrate_carbon_to_exclude_groups.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class MigrateCarbonToExcludeGroups < ActiveRecord::Migration[7.0] + def up + carbon_display_groups_raw = + DB.query_single("SELECT value FROM site_settings WHERE name = 'carbon_display_groups'").first + + if carbon_display_groups_raw.present? + carbon_exclude_groups = + case carbon_display_groups_raw + when "10" + "3|11|12|13|14" + when "10|11" + "3|12|13|14" + when "10|11|12" + "3|13|14" + when "10|11|12|13" + "3|14" + when "10|11|12|14" + "3" + end + + DB.exec(<<~SQL, setting: carbon_exclude_groups) + INSERT INTO site_settings(name, value, data_type, created_at, updated_at) + VALUES('carbon_exclude_groups', :setting, '20', NOW(), NOW()) + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20240208195104_migrate_adbutler_to_exclude_groups.rb b/db/migrate/20240208195104_migrate_adbutler_to_exclude_groups.rb new file mode 100644 index 0000000..77e39a4 --- /dev/null +++ b/db/migrate/20240208195104_migrate_adbutler_to_exclude_groups.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class MigrateAdbutlerToExcludeGroups < ActiveRecord::Migration[7.0] + def up + adbutler_display_groups_raw = + DB.query_single( + "SELECT value FROM site_settings WHERE name = 'adbutler_display_groups'", + ).first + + if adbutler_display_groups_raw.present? + adbutler_exclude_groups = + case adbutler_display_groups_raw + when "10" + "3|11|12|13|14" + when "10|11" + "3|12|13|14" + when "10|11|12" + "3|13|14" + when "10|11|12|13" + "3|14" + when "10|11|12|14" + "3" + end + + DB.exec(<<~SQL, setting: adbutler_exclude_groups) + INSERT INTO site_settings(name, value, data_type, created_at, updated_at) + VALUES('adbutler_exclude_groups', :setting, '20', NOW(), NOW()) + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/adplugin/guardian_extensions.rb b/lib/adplugin/guardian_extensions.rb index 440a5eb..db786ad 100644 --- a/lib/adplugin/guardian_extensions.rb +++ b/lib/adplugin/guardian_extensions.rb @@ -3,23 +3,23 @@ module ::AdPlugin module GuardianExtensions def show_dfp_ads? - self.user.in_any_groups?(SiteSetting.dfp_display_groups_map) + !self.user.in_any_groups?(SiteSetting.dfp_exclude_groups_map) end def show_adsense_ads? - self.user.in_any_groups?(SiteSetting.adsense_display_groups_map) + !self.user.in_any_groups?(SiteSetting.adsense_exclude_groups_map) end def show_carbon_ads? - self.user.in_any_groups?(SiteSetting.carbonads_display_groups_map) + !self.user.in_any_groups?(SiteSetting.carbonads_exclude_groups_map) end def show_amazon_ads? - self.user.in_any_groups?(SiteSetting.amazon_display_groups_map) + !self.user.in_any_groups?(SiteSetting.amazon_exclude_groups_map) end def show_adbutler_ads? - self.user.in_any_groups?(SiteSetting.adbutler_display_groups_map) + !self.user.in_any_groups?(SiteSetting.adbutler_exclude_groups_map) end end end diff --git a/spec/serializer/current_user_serializer_spec.rb b/spec/serializer/current_user_serializer_spec.rb new file mode 100644 index 0000000..9439653 --- /dev/null +++ b/spec/serializer/current_user_serializer_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +RSpec.describe CurrentUserSerializer do + fab!(:tl0_user) { Fabricate(:user, trust_level: 0, refresh_auto_groups: true) } + fab!(:tl2_user) { Fabricate(:user, trust_level: 2, refresh_auto_groups: true) } + fab!(:tl3_user) { Fabricate(:user, trust_level: 3, refresh_auto_groups: true) } + fab!(:admin) { Fabricate(:admin) } + + let(:tl0_serializer) { described_class.new(tl0_user, scope: Guardian.new(tl0_user), root: false) } + + let(:tl2_serializer) { described_class.new(tl2_user, scope: Guardian.new(tl2_user), root: false) } + + let(:tl3_serializer) { described_class.new(tl3_user, scope: Guardian.new(tl3_user), root: false) } + + let(:admin_serializer) { described_class.new(admin, scope: Guardian.new(admin), root: false) } + + before { SiteSetting.discourse_adplugin_enabled = true } + + describe "#adsense" do + it "is displayed for TL0 by default" do + expect(tl0_serializer.show_adsense_ads).to eq(true) + end + + it "is displayed for TL2 by default" do + expect(tl2_serializer.show_adsense_ads).to eq(true) + end + + it "is off for TL3 by default" do + expect(tl3_serializer.show_adsense_ads).to eq(false) + end + + it "is off for admin by default" do + expect(admin_serializer.show_adsense_ads).to eq(false) + end + end + + describe "#amazon" do + it "is displayed for TL0 by default" do + expect(tl0_serializer.show_amazon_ads).to eq(true) + end + + it "is displayed for TL2 by default" do + expect(tl2_serializer.show_amazon_ads).to eq(true) + end + + it "is off for TL3 by default" do + expect(tl3_serializer.show_amazon_ads).to eq(false) + end + + it "is off for admin by default" do + expect(admin_serializer.show_amazon_ads).to eq(false) + end + end + + describe "#dfp" do + it "is displayed for TL0 by default" do + expect(tl0_serializer.show_dfp_ads).to eq(true) + end + + it "is displayed for TL2 by default" do + expect(tl2_serializer.show_dfp_ads).to eq(true) + end + + it "is off for TL3 by default" do + expect(tl3_serializer.show_dfp_ads).to eq(false) + end + + it "is off for admin by default" do + expect(admin_serializer.show_dfp_ads).to eq(false) + end + end + + describe "#carbon" do + it "is displayed for TL0 by default" do + expect(tl0_serializer.show_carbon_ads).to eq(true) + end + + it "is displayed for TL2 by default" do + expect(tl2_serializer.show_carbon_ads).to eq(true) + end + + it "is off for TL3 by default" do + expect(tl3_serializer.show_carbon_ads).to eq(false) + end + + it "is off for admin by default" do + expect(admin_serializer.show_carbon_ads).to eq(false) + end + end + + describe "#adbutler" do + it "is displayed for TL0 by default" do + expect(tl0_serializer.show_adbutler_ads).to eq(true) + end + + it "is displayed for TL2 by default" do + expect(tl2_serializer.show_adbutler_ads).to eq(true) + end + + it "is off for TL3 by default" do + expect(tl3_serializer.show_adbutler_ads).to eq(false) + end + + it "is off for admin by default" do + expect(admin_serializer.show_adbutler_ads).to eq(false) + end + end +end