From c2057a50560b4b7ddfb4cd7204f3ec15e7145729 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Fri, 7 Apr 2023 17:56:22 +0300 Subject: [PATCH] FEATURE: Add per-ad visibility settings for anons and logged-in users (#168) This commit adds 2 new settings to house ads to control whether an ad is shown to anonymous users and logged in users. Existing ads that were created before this feature will default to true for both settings; i.e., they will remain to be visible to both anonymous and logged-in users, but it will be possible to change the settings. Turning off both settings will effectively disable the ad completely. --- app/controllers/house_ads_controller.rb | 13 +++- app/models/house_ad.rb | 30 +++++++- app/models/house_ad_setting.rb | 19 ++++- .../admin-plugins-house-ads-show.js | 19 ++++- .../routes/admin-plugins-house-ads-show.js | 2 + .../admin/plugins-house-ads-show.hbs | 22 +++++- .../initializers/initialize-ad-plugin.js | 9 ++- assets/stylesheets/adplugin.scss | 3 + config/locales/client.en.yml | 2 + plugin.rb | 2 +- spec/models/house_ad_setting_spec.rb | 42 +++++++++- spec/models/house_ad_spec.rb | 50 ++++++++++-- spec/requests/house_ad_controller_spec.rb | 67 ++++++++++++++++ spec/requests/site_controller_spec.rb | 76 +++++++++++++++++++ spec/system/admin_house_ad_spec.rb | 37 +++++++++ 15 files changed, 375 insertions(+), 18 deletions(-) create mode 100644 spec/requests/house_ad_controller_spec.rb create mode 100644 spec/requests/site_controller_spec.rb create mode 100644 spec/system/admin_house_ad_spec.rb diff --git a/app/controllers/house_ads_controller.rb b/app/controllers/house_ads_controller.rb index ab4b76a..4c003c6 100644 --- a/app/controllers/house_ads_controller.rb +++ b/app/controllers/house_ads_controller.rb @@ -38,7 +38,18 @@ module ::AdPlugin private def house_ad_params - params.permit(:id, :name, :html) + @permitted ||= + begin + permitted = + params.permit(:id, :name, :html, :visible_to_anons, :visible_to_logged_in_users) + permitted[:visible_to_logged_in_users] = ActiveModel::Type::Boolean.new.cast( + permitted[:visible_to_logged_in_users], + ) + permitted[:visible_to_anons] = ActiveModel::Type::Boolean.new.cast( + permitted[:visible_to_anons], + ) + permitted + end end end end diff --git a/app/models/house_ad.rb b/app/models/house_ad.rb index 68ec19d..0e07db8 100644 --- a/app/models/house_ad.rb +++ b/app/models/house_ad.rb @@ -4,7 +4,7 @@ module ::AdPlugin class HouseAd include ActiveModel::Validations - attr_accessor :id, :name, :html + attr_accessor :id, :name, :html, :visible_to_logged_in_users, :visible_to_anons NAME_REGEX = /\A[[:alnum:]\s\.,'!@#$%&\*\-\+\=:]*\z/i @@ -20,6 +20,8 @@ module ::AdPlugin def initialize @name = "New Ad" @html = "
New Ad
" + @visible_to_logged_in_users = true + @visible_to_anons = true end def self.from_hash(h) @@ -27,6 +29,10 @@ 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_anons = h[:visible_to_anons] if h.key?(:visible_to_anons) ad end @@ -62,10 +68,19 @@ module ::AdPlugin .sort_by { |ad| ad.id } end + def self.all_for_anons + self.all.select(&:visible_to_anons) + end + + def self.all_for_logged_in_users + self.all.select(&:visible_to_logged_in_users) + end + def save if self.valid? self.id = self.class.alloc_id if self.id.to_i <= 0 AdPlugin.pstore_set("ad:#{id}", to_hash) + Site.clear_anon_cache! self.class.publish_if_ads_enabled true else @@ -76,15 +91,26 @@ 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_anons = attrs[:visible_to_anons] if attrs.key?(:visible_to_anons) self.save end def to_hash - { id: @id, name: @name, html: @html } + { + id: @id, + name: @name, + html: @html, + visible_to_logged_in_users: @visible_to_logged_in_users, + visible_to_anons: @visible_to_anons, + } end def destroy AdPlugin.pstore_delete("ad:#{id}") + Site.clear_anon_cache! self.class.publish_if_ads_enabled end diff --git a/app/models/house_ad_setting.rb b/app/models/house_ad_setting.rb index 3f9bef8..6c3d525 100644 --- a/app/models/house_ad_setting.rb +++ b/app/models/house_ad_setting.rb @@ -21,10 +21,17 @@ module ::AdPlugin settings end - def self.settings_and_ads + def self.settings_and_ads(for_anons: true) settings = AdPlugin::HouseAdSetting.all ad_names = settings.values.map { |v| v.split("|") }.flatten.uniq - ads = AdPlugin::HouseAd.all.select { |ad| ad_names.include?(ad.name) } + + if for_anons + ads = AdPlugin::HouseAd.all_for_anons + else + ads = AdPlugin::HouseAd.all_for_logged_in_users + end + ads = ads.select { |ad| ad_names.include?(ad.name) } + { settings: settings.merge( @@ -56,12 +63,18 @@ module ::AdPlugin else AdPlugin.pstore_set("ad-setting:#{setting_name}", new_value) end + Site.clear_anon_cache! publish_settings end def self.publish_settings - MessageBus.publish("/site/house-creatives", settings_and_ads) + MessageBus.publish("/site/house-creatives/anonymous", settings_and_ads(for_anons: true)) + MessageBus.publish( + "/site/house-creatives/logged-in", + settings_and_ads(for_anons: false), + group_ids: [Group::AUTO_GROUPS[:trust_level_0]], + ) end end end diff --git a/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js b/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js index 42bd26b..982a04f 100644 --- a/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js +++ b/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js @@ -14,7 +14,20 @@ export default Controller.extend(bufferedProperty("model"), { nameDirty: propertyNotEqual("buffered.name", "model.name"), htmlDirty: propertyNotEqual("buffered.html", "model.html"), - dirty: or("nameDirty", "htmlDirty"), + visibleToAnonsDirty: propertyNotEqual( + "buffered.visible_to_anons", + "model.visible_to_anons" + ), + visibleToLoggedInDirty: propertyNotEqual( + "buffered.visible_to_logged_in_users", + "model.visible_to_logged_in_users" + ), + dirty: or( + "nameDirty", + "htmlDirty", + "visibleToLoggedInDirty", + "visibleToAnonsDirty" + ), disableSave: not("dirty"), actions: { @@ -34,6 +47,10 @@ export default Controller.extend(bufferedProperty("model"), { } data.name = buffered.get("name"); data.html = buffered.get("html"); + data.visible_to_logged_in_users = buffered.get( + "visible_to_logged_in_users" + ); + data.visible_to_anons = buffered.get("visible_to_anons"); ajax( newRecord diff --git a/assets/javascripts/discourse/routes/admin-plugins-house-ads-show.js b/assets/javascripts/discourse/routes/admin-plugins-house-ads-show.js index 5dd45a6..a2dd4f5 100644 --- a/assets/javascripts/discourse/routes/admin-plugins-house-ads-show.js +++ b/assets/javascripts/discourse/routes/admin-plugins-house-ads-show.js @@ -8,6 +8,8 @@ export default DiscourseRoute.extend({ return EmberObject.create({ name: I18n.t("admin.adplugin.house_ads.new_name"), html: "", + visible_to_logged_in_users: true, + visible_to_anons: true, }); } else { return this.modelFor("adminPlugins.houseAds").findBy( diff --git a/assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs b/assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs index 7774d2d..478d710 100644 --- a/assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs +++ b/assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs @@ -4,10 +4,30 @@ {{ace-editor content=buffered.html mode="html"}}
+
+
+ + {{i18n "admin.adplugin.house_ads.show_to_logged_in_users"}} +
+ +
+ + {{i18n "admin.adplugin.house_ads.show_to_anons"}} +
+
+ {{d-button action=(action "save") disabled=disableSave - class="btn-primary" + class="btn-primary save-button" label="admin.adplugin.house_ads.save" }} diff --git a/assets/javascripts/initializers/initialize-ad-plugin.js b/assets/javascripts/initializers/initialize-ad-plugin.js index 672ed49..f0ac599 100644 --- a/assets/javascripts/initializers/initialize-ad-plugin.js +++ b/assets/javascripts/initializers/initialize-ad-plugin.js @@ -28,7 +28,14 @@ export default { return; } - messageBus.subscribe("/site/house-creatives", function (houseAdsSettings) { + const currentUser = container.lookup("service:current-user"); + let channel; + if (currentUser) { + channel = "/site/house-creatives/logged-in"; + } else { + channel = "/site/house-creatives/anonymous"; + } + messageBus.subscribe(channel, function (houseAdsSettings) { Site.currentProp("house_creatives", houseAdsSettings); }); }, diff --git a/assets/stylesheets/adplugin.scss b/assets/stylesheets/adplugin.scss index b3bd967..9198dd2 100644 --- a/assets/stylesheets/adplugin.scss +++ b/assets/stylesheets/adplugin.scss @@ -241,6 +241,9 @@ } .content-body { padding-left: 2%; + .visibility-settings { + margin-bottom: 1em; + } .controls { margin-bottom: 1em; } diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 690efa0..2cac931 100755 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -24,6 +24,8 @@ en: get_started: "Start by creating a new ad." filter_placeholder: "Select ads..." more_settings: "More Settings" + show_to_anons: "Show to anonymous users" + show_to_logged_in_users: "Show to logged in users" topic_list_top: title: "Topic list top ads" diff --git a/plugin.rb b/plugin.rb index 72f7034..8e1fe33 100755 --- a/plugin.rb +++ b/plugin.rb @@ -39,7 +39,7 @@ after_initialize do require_dependency "application_controller" add_to_serializer :site, :house_creatives do - AdPlugin::HouseAdSetting.settings_and_ads + AdPlugin::HouseAdSetting.settings_and_ads(for_anons: scope.anonymous?) 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 cea2bd4..211e170 100644 --- a/spec/models/house_ad_setting_spec.rb +++ b/spec/models/house_ad_setting_spec.rb @@ -5,7 +5,7 @@ require "rails_helper" describe AdPlugin::HouseAdSetting do let(:defaults) { AdPlugin::HouseAdSetting::DEFAULTS } - describe "#all" do + describe ".all" do subject { AdPlugin::HouseAdSetting.all } it "returns defaults when nothing has been set" do @@ -19,7 +19,7 @@ describe AdPlugin::HouseAdSetting do end end - describe "#update" do + describe ".update" do before do AdPlugin::HouseAd.create(name: "Banner", html: "

Banner

") AdPlugin::HouseAd.create(name: "Donate", html: "

Donate

") @@ -67,4 +67,42 @@ describe AdPlugin::HouseAdSetting do expect(AdPlugin::HouseAdSetting.all[:topic_list_top]).to eq("") end end + + describe ".publish_settings" do + let!(:anon_ad) do + AdPlugin::HouseAd.create( + name: "anon-ad", + html: "", + visible_to_anons: true, + visible_to_logged_in_users: false, + ) + end + + let!(:logged_in_ad) do + AdPlugin::HouseAd.create( + name: "logged-in-ad", + html: "", + visible_to_anons: false, + visible_to_logged_in_users: true, + ) + end + + before { AdPlugin::HouseAdSetting.update("topic_list_top", "logged-in-ad|anon-ad") } + + it "publishes different payloads to different channels for anons and logged in users" do + messages = MessageBus.track_publish { AdPlugin::HouseAdSetting.publish_settings } + expect(messages.size).to eq(2) + + anon_message = messages.find { |m| m.channel == "/site/house-creatives/anonymous" } + logged_in_message = messages.find { |m| m.channel == "/site/house-creatives/logged-in" } + + expect(anon_message.data[:creatives]).to eq("anon-ad" => "") + expect(anon_message.group_ids).to eq(nil) + expect(anon_message.user_ids).to eq(nil) + + expect(logged_in_message.data[:creatives]).to eq("logged-in-ad" => "") + expect(logged_in_message.group_ids).to eq([Group::AUTO_GROUPS[:trust_level_0]]) + expect(logged_in_message.user_ids).to eq(nil) + end + end end diff --git a/spec/models/house_ad_spec.rb b/spec/models/house_ad_spec.rb index d24e17b..b8834a2 100644 --- a/spec/models/house_ad_spec.rb +++ b/spec/models/house_ad_spec.rb @@ -11,7 +11,25 @@ describe AdPlugin::HouseAd do } end - describe "#find" do + def create_anon_ad + AdPlugin::HouseAd.create( + name: "anon-ad", + html: "
ANON
", + visible_to_logged_in_users: false, + visible_to_anons: true, + ) + end + + def create_logged_in_ad + AdPlugin::HouseAd.create( + name: "logged-in-ad", + html: "
LOGGED IN
", + visible_to_logged_in_users: true, + visible_to_anons: false, + ) + end + + describe ".find" do let!(:ad) { AdPlugin::HouseAd.create(valid_attrs) } it "returns nil if no match" do @@ -25,7 +43,7 @@ describe AdPlugin::HouseAd do end end - describe "#all" do + describe ".all" do it "returns empty array if no records" do expect(AdPlugin::HouseAd.all).to eq([]) end @@ -40,7 +58,27 @@ describe AdPlugin::HouseAd do end end - describe "save" do + describe ".all_for_anons" do + let!(:anon_ad) { create_anon_ad } + let!(:logged_in_ad) { create_logged_in_ad } + + it "doesn't include ads for logged in users" do + expect(AdPlugin::HouseAd.all_for_anons.map(&:id)).to contain_exactly(anon_ad.id) + end + end + + describe ".all_for_logged_in_users" do + let!(:anon_ad) { create_anon_ad } + let!(:logged_in_ad) { create_logged_in_ad } + + 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, + ) + end + end + + describe "#save" do it "assigns an id and attrs for new record" do ad = AdPlugin::HouseAd.from_hash(valid_attrs) expect(ad.save).to eq(true) @@ -112,7 +150,7 @@ describe AdPlugin::HouseAd do end end - describe "create" do + describe ".create" do it "can create new records" do ad = AdPlugin::HouseAd.create(valid_attrs) expect(ad).to be_a(AdPlugin::HouseAd) @@ -130,7 +168,7 @@ describe AdPlugin::HouseAd do end end - describe "destroy" do + describe "#destroy" do it "can delete a record" do ad = AdPlugin::HouseAd.create(valid_attrs) ad.destroy @@ -138,7 +176,7 @@ describe AdPlugin::HouseAd do end end - describe "update" do + describe "#update" do let(:ad) { AdPlugin::HouseAd.create(valid_attrs) } it "updates existing record" do diff --git a/spec/requests/house_ad_controller_spec.rb b/spec/requests/house_ad_controller_spec.rb new file mode 100644 index 0000000..2c9bf94 --- /dev/null +++ b/spec/requests/house_ad_controller_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe AdPlugin::HouseAdsController do + let(:admin) { Fabricate(:admin) } + + let!(:ad) do + AdPlugin::HouseAd.create( + name: "Banner", + html: "

Banner

", + visible_to_anons: true, + visible_to_logged_in_users: false, + ) + end + + describe "#update" do + context "when used by admins" do + before { sign_in(admin) } + + it "updates an existing ad" do + put "/admin/plugins/pluginad/house_creatives/#{ad.id}.json", + params: { + name: ad.name, + html: ad.html, + visible_to_anons: "false", + visible_to_logged_in_users: "true", + } + expect(response.status).to eq(200) + expect(response.parsed_body["house_ad"].symbolize_keys).to eq( + id: ad.id, + name: ad.name, + html: ad.html, + visible_to_anons: false, + visible_to_logged_in_users: true, + ) + + ad_copy = AdPlugin::HouseAd.find(ad.id) + expect(ad_copy.name).to eq(ad.name) + 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) + end + end + + context "when used by non-admins" do + before { sign_in(Fabricate(:user)) } + + it "can't update ads" do + put "/admin/plugins/pluginad/house_creatives/#{ad.id}.json", + params: { + name: "non sense goes here", + html: "blah ", + visible_to_anons: "false", + visible_to_logged_in_users: "true", + } + expect(response.status).to eq(404) + + ad_copy = AdPlugin::HouseAd.find(ad.id) + expect(ad_copy.name).to eq(ad.name) + expect(ad_copy.html).to eq(ad.html) + expect(ad_copy.visible_to_anons).to eq(true) + expect(ad_copy.visible_to_logged_in_users).to eq(false) + end + end + end +end diff --git a/spec/requests/site_controller_spec.rb b/spec/requests/site_controller_spec.rb new file mode 100644 index 0000000..17a2668 --- /dev/null +++ b/spec/requests/site_controller_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe SiteController do + fab!(:user) { Fabricate(:user) } + + let!(:anon_ad) do + AdPlugin::HouseAd.create( + name: "anon-ad", + html: "
ANON
", + visible_to_logged_in_users: false, + visible_to_anons: true, + ) + end + + let!(:logged_in_ad) do + AdPlugin::HouseAd.create( + name: "logged-in-ad", + html: "
LOGGED IN
", + visible_to_logged_in_users: true, + visible_to_anons: false, + ) + end + + let!(:everyone_ad) do + AdPlugin::HouseAd.create( + name: "everyone-ad", + html: "
EVERYONE
", + visible_to_logged_in_users: true, + visible_to_anons: true, + ) + end + + before { AdPlugin::HouseAdSetting.update("topic_list_top", "logged-in-ad|anon-ad|everyone-ad") } + + describe "#site" do + context "when logged in" do + before { sign_in(user) } + + it "only includes ads that are visible to logged in users" do + get "/site.json" + expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly( + "logged-in-ad", + "everyone-ad", + ) + end + end + + context "when anonymous" do + it "only includes ads that are visible to anonymous users" do + get "/site.json" + expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly( + "anon-ad", + "everyone-ad", + ) + end + + it "invalidates cache when an ad is updated" do + get "/site.json" + expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly( + "anon-ad", + "everyone-ad", + ) + + anon_ad.visible_to_anons = false + anon_ad.save + + get "/site.json" + expect(response.parsed_body["house_creatives"]["creatives"].keys).to contain_exactly( + "everyone-ad", + ) + end + end + end +end diff --git a/spec/system/admin_house_ad_spec.rb b/spec/system/admin_house_ad_spec.rb new file mode 100644 index 0000000..4491072 --- /dev/null +++ b/spec/system/admin_house_ad_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +describe "Admin House Ad", type: :system, js: true do + fab!(:admin) { Fabricate(:admin) } + let(:house_ad) do + AdPlugin::HouseAd.create( + name: "some-name", + html: "
somecode
", + visible_to_anons: true, + visible_to_logged_in_users: false, + ) + end + + before { sign_in(admin) } + + describe "when visiting the page for creating new ads" do + it "should have the visibility checkboxes on by default" do + visit("/admin/plugins/pluginad/house_creatives/new") + + expect(find("input.visible-to-anonymous-checkbox").checked?).to eq(true) + expect(find("input.visible-to-logged-in-checkbox").checked?).to eq(true) + end + end + + describe "when visiting the page of an existing ad" do + it "the controls reflect the correct state of the ad" do + visit("/admin/plugins/pluginad/house_creatives/#{house_ad.id}") + + expect(find("input.house-ad-name").value).to eq(house_ad.name) + expect(find("input.visible-to-anonymous-checkbox").checked?).to eq(true) + expect(find("input.visible-to-logged-in-checkbox").checked?).to eq(false) + # would be nice to assert for the HTML content in ace-editor, but there + # doesn't seem to be a way to check the content in ace-editor from the + # DOM + end + end +end