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

@@ -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

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 = "<div class='house-ad'>New Ad</div>"
+      @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
 

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

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

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(

assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs

@@ -4,10 +4,30 @@
     {{ace-editor content=buffered.html mode="html"}}
   </div>
   <div class="controls">
+    <div class="visibility-settings">
+      <div>
+        <Input
+          class="visible-to-logged-in-checkbox"
+          @type="checkbox"
+          @checked={{this.buffered.visible_to_logged_in_users}}
+        />
+        <span>{{i18n "admin.adplugin.house_ads.show_to_logged_in_users"}}</span>
+      </div>
+
+      <div>
+        <Input
+          class="visible-to-anonymous-checkbox"
+          @type="checkbox"
+          @checked={{this.buffered.visible_to_anons}}
+        />
+        <span>{{i18n "admin.adplugin.house_ads.show_to_anons"}}</span>
+      </div>
+    </div>
+
     {{d-button
       action=(action "save")
       disabled=disableSave
-      class="btn-primary"
+      class="btn-primary save-button"
       label="admin.adplugin.house_ads.save"
     }}
 

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);
     });
   },

assets/stylesheets/adplugin.scss

@@ -241,6 +241,9 @@
   }
   .content-body {
     padding-left: 2%;
+    .visibility-settings {
+      margin-bottom: 1em;
+    }
     .controls {
       margin-bottom: 1em;
     }

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"

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

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: "<p>Banner</p>")
       AdPlugin::HouseAd.create(name: "Donate", html: "<p>Donate</p>")
@@ -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: "<whatever-anon>",
+        visible_to_anons: true,
+        visible_to_logged_in_users: false,
+      )
+    end
+
+    let!(:logged_in_ad) do
+      AdPlugin::HouseAd.create(
+        name: "logged-in-ad",
+        html: "<whatever-logged-in>",
+        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" => "<whatever-anon>")
+      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" => "<whatever-logged-in>")
+      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

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: "<div>ANON</div>",
+      visible_to_logged_in_users: false,
+      visible_to_anons: true,
+    )
+  end
+
+  def create_logged_in_ad
+    AdPlugin::HouseAd.create(
+      name: "logged-in-ad",
+      html: "<div>LOGGED IN</div>",
+      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

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: "<p>Banner</p>",
+      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 <h4cked>",
+              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

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: "<div>ANON</div>",
+      visible_to_logged_in_users: false,
+      visible_to_anons: true,
+    )
+  end
+
+  let!(:logged_in_ad) do
+    AdPlugin::HouseAd.create(
+      name: "logged-in-ad",
+      html: "<div>LOGGED IN</div>",
+      visible_to_logged_in_users: true,
+      visible_to_anons: false,
+    )
+  end
+
+  let!(:everyone_ad) do
+    AdPlugin::HouseAd.create(
+      name: "everyone-ad",
+      html: "<div>EVERYONE</div>",
+      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

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: "<div>somecode</div>",
+      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