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