FEATURE: Add group and category restrictions to house ads (#205)

# Description This PR adds the ability to apply **group** and **category** restrictions to a **house ad**. # What is included - In order to get the group and category selectors to work within `admin/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js` I needed to modernize the file. - I dropped the `bufferedProperty` implementation in favor of a vanilla ember approach - I added `category_ids` and `group_ids` to our house ads model - I added tests for group / category restrictions - I added a preview button to display the house ad - `/site.json` would return a object called `house_creatives` and a list of key value pairs that matched the ad name with the html, like so: ```js { AD_KEY: ad.html } ``` I need access to the category ids on the client to conditionally render the house ads so the new format will be: ```js { AD_KEY: { html: ad.html, category_ids: ad.category_ids } } ``` # Screenshots <img width="658" alt="Screenshot 2024-04-08 at 2 39 22 PM" src="https://github.com/discourse/discourse-adplugin/assets/50783505/b44b386d-65a1-4a2a-a487-d735b13357dd"> # Preview Video https://github.com/discourse/discourse-adplugin/assets/50783505/6d0d8253-afef-4e15-b6fc-c6f696efd169
2024-04-09 11:54:11 -06:00 · 2024-04-09 11:54:11 -06:00 · 554f03f3da
parent c4227de1b5
commit 554f03f3da
17 changed files with 604 additions and 153 deletions
--- a/admin/assets/javascripts/discourse/components/house-ads-category-selector.gjs
+++ b/admin/assets/javascripts/discourse/components/house-ads-category-selector.gjs
@ -0,0 +1,7 @@
+import CategorySelector from "select-kit/components/category-selector";
+
+export default class HouseAdsCategorySelector extends CategorySelector {
+  get value() {
+    return this.selectedCategories.map((c) => c.id);
+  }
+}
--- a/admin/assets/javascripts/discourse/components/modal/preview.gjs
+++ b/admin/assets/javascripts/discourse/components/modal/preview.gjs
@ -0,0 +1,18 @@
+import { htmlSafe } from "@ember/template";
+import DModal from "discourse/components/d-modal";
+import i18n from "discourse-common/helpers/i18n";
+
+const Preview = <template>
+  <DModal
+    @closeModal={{@closeModal}}
+    @title={{i18n "admin.adplugin.house_ads.preview"}}
+  >
+    <:body>
+      <div class="house-ad-preview">
+        {{htmlSafe @model.html}}
+      </div>
+    </:body>
+  </DModal>
+</template>;
+
+export default Preview;
--- a/admin/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js
+++ b/admin/assets/javascripts/discourse/controllers/admin-plugins-house-ads-show.js
@ -1,115 +1,156 @@
+import { tracked } from "@glimmer/tracking";
 import Controller, { inject as controller } from "@ember/controller";
-import { not, or } from "@ember/object/computed";
-import { inject as service } from "@ember/service";
+import EmberObject, { action } from "@ember/object";
+import { service } from "@ember/service";
+import { TrackedObject } from "@ember-compat/tracked-built-ins";
+import { observes } from "@ember-decorators/object";
 import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
-import { propertyNotEqual } from "discourse/lib/computed";
-import { bufferedProperty } from "discourse/mixins/buffered-content";
+import Category from "discourse/models/category";
 import I18n from "I18n";
+import Preview from "../components/modal/preview";

-export default Controller.extend(bufferedProperty("model"), {
-  adminPluginsHouseAds: controller("adminPlugins.houseAds"),
-  router: service(),
+export default class adminPluginsHouseAdsShow extends Controller {
+  @service router;
+  @service modal;

-  saving: false,
-  savingStatus: "",
+  @controller("adminPlugins.houseAds") houseAdsController;

-  nameDirty: propertyNotEqual("buffered.name", "model.name"),
-  htmlDirty: propertyNotEqual("buffered.html", "model.html"),
-  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"),
+  @tracked selectedCategories = [];
+  @tracked selectedGroups = [];
+  @tracked saving = false;
+  @tracked savingStatus = "";
+  @tracked buffered;

-  actions: {
-    save() {
-      if (!this.get("saving")) {
-        this.setProperties({
-          saving: true,
-          savingStatus: I18n.t("saving"),
-        });
-
-        const data = {},
-          buffered = this.get("buffered"),
-          newRecord = !buffered.get("id");
-
-        if (!newRecord) {
-          data.id = buffered.get("id");
+  @observes("model")
+  modelChanged() {
+    this.buffered = new TrackedObject({ ...this.model });
+    this.selectedCategories = this.model.categories || [];
+    this.selectedGroups = this.model.group_ids || [];
  }
-        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(
+  get disabledSave() {
+    for (const key in this.buffered) {
+      // we don't want to compare the categories array
+      if (key !== "categories" && this.buffered[key] !== this.model[key]) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  @action
+  async save() {
+    if (!this.saving) {
+      this.saving = true;
+      this.savingStatus = I18n.t("saving");
+      const data = {};
+      const newRecord = !this.buffered.id;
+      if (!newRecord) {
+        data.id = this.buffered.id;
+      }
+      data.name = this.buffered.name;
+      data.html = this.buffered.html;
+      data.visible_to_logged_in_users =
+        this.buffered.visible_to_logged_in_users;
+      data.visible_to_anons = this.buffered.visible_to_anons;
+      data.category_ids = this.buffered.category_ids;
+      data.group_ids = this.buffered.group_ids;
+      try {
+        const ajaxData = await ajax(
          newRecord
            ? `/admin/plugins/pluginad/house_creatives`
-            : `/admin/plugins/pluginad/house_creatives/${buffered.get("id")}`,
+            : `/admin/plugins/pluginad/house_creatives/${this.buffered.id}`,
          {
            type: newRecord ? "POST" : "PUT",
            data,
          }
-        )
-          .then((ajaxData) => {
-            this.commitBuffer();
-            this.set("savingStatus", I18n.t("saved"));
+        );
+        this.savingStatus = I18n.t("saved");
+        const houseAds = this.houseAdsController.model;
        if (newRecord) {
-              const model = this.get("model");
-              model.set("id", ajaxData.house_ad.id);
-              const houseAds = this.get("adminPluginsHouseAds.model");
-              if (!houseAds.includes(model)) {
-                houseAds.pushObject(model);
+          this.buffered.id = ajaxData.house_ad.id;
+          if (!houseAds.includes(this.buffered)) {
+            houseAds.pushObject(EmberObject.create(this.buffered));
          }
          this.router.transitionTo(
            "adminPlugins.houseAds.show",
-                model.get("id")
+            this.buffered.id
          );
+        } else {
+          houseAds
+            .find((ad) => ad.id === this.buffered.id)
+            .setProperties(this.buffered);
+        }
+      } catch (error) {
+        popupAjaxError(error);
+      } finally {
+        this.set("model", this.buffered);
+        this.saving = false;
+        this.savingStatus = "";
+      }
    }
-          })
-          .catch(popupAjaxError)
-          .finally(() => {
-            this.setProperties({
-              saving: false,
-              savingStatus: "",
-            });
-          });
  }
-    },

+  @action
+  setCategoryIds(categoryArray) {
+    this.selectedCategories = categoryArray;
+    this.buffered.category_ids = categoryArray.map((c) => c.id);
+    this.setCategoriesForBuffered();
+  }
+
+  @action
+  setGroupIds(groupIds) {
+    this.selectedGroups = groupIds;
+    this.buffered.group_ids = groupIds.map((id) => id);
+  }
+
+  @action
  cancel() {
-      this.rollbackBuffer();
-    },
+    this.buffered = new TrackedObject({ ...this.model });
+    this.selectedCategories = this.model.categories || [];
+    this.selectedGroups = this.model.group_ids || [];
+    this.setCategoriesForBuffered();
+  }

-    destroy() {
-      const houseAds = this.get("adminPluginsHouseAds.model");
-      const model = this.get("model");
-
-      if (!model.get("id")) {
+  @action
+  async destroy() {
+    if (!this.buffered.id) {
      this.router.transitionTo("adminPlugins.houseAds.index");
      return;
    }
-
-      ajax(`/admin/plugins/pluginad/house_creatives/${model.get("id")}`, {
+    try {
+      await ajax(
+        `/admin/plugins/pluginad/house_creatives/${this.buffered.id}`,
+        {
          type: "DELETE",
-      })
-        .then(() => {
-          houseAds.removeObject(model);
+        }
+      );
+      this.houseAdsController.model.removeObject(
+        this.houseAdsController.model.findBy("id", this.buffered.id)
+      );
      this.router.transitionTo("adminPlugins.houseAds.index");
-        })
-        .catch(popupAjaxError);
-    },
+    } catch (error) {
+      popupAjaxError(error);
+    }
+  }
+
+  @action
+  openPreview() {
+    this.modal.show(Preview, {
+      model: {
+        html: this.buffered.html,
      },
    });
+  }
+
+  setCategoriesForBuffered() {
+    // we need to fetch the categories because the serializer is not being used
+    // to attach the category object to the house ads
+    this.buffered.categories = this.buffered.category_ids
+      ? this.buffered.category_ids.map((categoryId) =>
+          Category.findById(categoryId)
+        )
+      : [];
+  }
+}
--- a/admin/assets/javascripts/discourse/routes/admin-plugins-house-ads-show.js
+++ b/admin/assets/javascripts/discourse/routes/admin-plugins-house-ads-show.js
@ -1,20 +1,22 @@
-import EmberObject from "@ember/object";
+import { TrackedObject } from "@ember-compat/tracked-built-ins";
 import DiscourseRoute from "discourse/routes/discourse";
 import I18n from "I18n";

 export default DiscourseRoute.extend({
  model(params) {
    if (params.ad_id === "new") {
-      return EmberObject.create({
+      return new TrackedObject({
        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(
+      return new TrackedObject(
+        this.modelFor("adminPlugins.houseAds").findBy(
          "id",
          parseInt(params.ad_id, 10)
+        )
      );
    }
  },
--- a/admin/assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs
+++ b/admin/assets/javascripts/discourse/templates/admin/plugins-house-ads-show.hbs
@ -1,7 +1,7 @@
 <section class="edit-house-ad content-body">
-  <h1><TextField @value={{buffered.name}} class="house-ad-name" /></h1>
+  <h1><TextField @value={{this.buffered.name}} class="house-ad-name" /></h1>
  <div class="controls">
-    <AceEditor @content={{buffered.html}} @mode="html" />
+    <AceEditor @content={{this.buffered.html}} @mode="html" />
  </div>
  <div class="controls">
    <div class="visibility-settings">
@ -22,25 +22,51 @@
        />
        <span>{{i18n "admin.adplugin.house_ads.show_to_anons"}}</span>
      </div>
+
+      <HouseAdsCategorySelector
+        @categories={{this.site.categories}}
+        @selectedCategories={{this.selectedCategories}}
+        @onChange={{this.setCategoryIds}}
+        @options={{hash allowAny=true}}
+        class="house-ads-categories"
+      />
+      <div class="description">
+        {{i18n "admin.adplugin.house_ads.category_chooser_description"}}
+      </div>
+
+      <GroupChooser
+        @content={{this.site.groups}}
+        @onChange={{this.setGroupIds}}
+        @value={{this.selectedGroups}}
+        class="banner-groups"
+      />
+      <div class="description">
+        {{i18n "admin.adplugin.house_ads.group_chooser_description"}}
+      </div>
    </div>

    <DButton
-      @action={{action "save"}}
-      @disabled={{disableSave}}
+      @action={{this.save}}
+      @disabled={{this.disabledSave}}
      @label="admin.adplugin.house_ads.save"
      class="btn-primary save-button"
    />

-    {{#if saving}}
-      {{savingStatus}}
+    {{#if this.saving}}
+      {{this.savingStatus}}
    {{else}}
-      {{#if dirty}}
-        <a href {{action "cancel"}}>{{i18n "cancel"}}</a>
-      {{/if}}
+      {{#unless this.disabledSave}}
+        <DButton @action={{this.cancel}} @label="cancel" />
+      {{/unless}}
    {{/if}}

    <DButton
-      @action={{action "destroy"}}
+      @action={{this.openPreview}}
+      @label="admin.adplugin.house_ads.preview"
+    />
+
+    <DButton
+      @action={{this.destroy}}
      @label="admin.adplugin.house_ads.delete"
      class="btn-danger delete-button"
    />
--- a/app/controllers/house_ads_controller.rb
+++ b/app/controllers/house_ads_controller.rb
@ -5,11 +5,23 @@ module ::AdPlugin
    requires_plugin AdPlugin.plugin_name

    def index
-      render_json_dump(house_ads: HouseAd.all.map(&:to_hash), settings: HouseAdSetting.all)
+      render_json_dump(
+        house_ads:
+          HouseAd.all.map do |ad|
+            ad.to_hash.merge!(categories: Category.secured(@guardian).where(id: ad.category_ids))
+          end,
+        settings: HouseAdSetting.all,
+      )
    end

    def show
-      render_json_dump(house_ad: HouseAd.find(params[:id])&.to_hash)
+      house_ad_hash = HouseAd.find(params[:id])&.to_hash
+      if house_ad_hash
+        house_ad_hash.merge!(
+          categories: Category.secured(@guardian).where(id: house_ad_hash[:category_ids]),
+        )
+      end
+      render_json_dump(house_ad: house_ad_hash)
    end

    def create
@ -41,7 +53,15 @@ module ::AdPlugin
      @permitted ||=
        begin
          permitted =
-            params.permit(:id, :name, :html, :visible_to_anons, :visible_to_logged_in_users)
+            params.permit(
+              :id,
+              :name,
+              :html,
+              :visible_to_anons,
+              :visible_to_logged_in_users,
+              group_ids: [],
+              category_ids: [],
+            )
          permitted[:visible_to_logged_in_users] = ActiveModel::Type::Boolean.new.cast(
            permitted[:visible_to_logged_in_users],
          )
--- a/app/models/house_ad.rb
+++ b/app/models/house_ad.rb
@ -4,7 +4,13 @@ module ::AdPlugin
  class HouseAd
    include ActiveModel::Validations

-    attr_accessor :id, :name, :html, :visible_to_logged_in_users, :visible_to_anons
+    attr_accessor :id,
+                  :name,
+                  :html,
+                  :visible_to_logged_in_users,
+                  :visible_to_anons,
+                  :category_ids,
+                  :group_ids

    NAME_REGEX = /\A[[:alnum:]\s\.,'!@#$%&\*\-\+\=:]*\z/i

@ -22,6 +28,8 @@ module ::AdPlugin
      @html = "<div class='house-ad'>New Ad</div>"
      @visible_to_logged_in_users = true
      @visible_to_anons = true
+      @group_ids = []
+      @category_ids = []
    end

    def self.from_hash(h)
@ -29,10 +37,12 @@ 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_logged_in_users = h[:visible_to_logged_in_users] if h.key?(
+        :visible_to_logged_in_users,
+      )
      ad.visible_to_anons = h[:visible_to_anons] if h.key?(:visible_to_anons)
+      ad.group_ids = h[:group_ids].map(&:to_i) if h.key?(:group_ids)
+      ad.category_ids = h[:category_ids].map(&:to_i) if h.key?(:category_ids)
      ad
    end

@ -72,8 +82,24 @@ module ::AdPlugin
      self.all.select(&:visible_to_anons)
    end

-    def self.all_for_logged_in_users
+    def self.all_for_logged_in_users(scope)
+      if scope.nil?
+        # this is for our admin page, so we don't need to filter by group
        self.all.select(&:visible_to_logged_in_users)
+      else
+        # otherwise, filter by group and visible categories
+        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 || true
+            )
+        end
+      end
    end

    def save
@ -91,10 +117,13 @@ 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_logged_in_users = attrs[:visible_to_logged_in_users] if attrs.key?(
+        :visible_to_logged_in_users,
+      )
      self.visible_to_anons = attrs[:visible_to_anons] if attrs.key?(:visible_to_anons)
+      # ensure that group_ids and category_ids can be set to an empty array
+      self.group_ids = attrs.key?(:group_ids) ? attrs[:group_ids].map(&:to_i) : []
+      self.category_ids = attrs.key?(:category_ids) ? attrs[:category_ids].map(&:to_i) : []
      self.save
    end

@ -105,6 +134,8 @@ module ::AdPlugin
        html: @html,
        visible_to_logged_in_users: @visible_to_logged_in_users,
        visible_to_anons: @visible_to_anons,
+        group_ids: @group_ids,
+        category_ids: @category_ids,
      }
    end

--- a/app/models/house_ad_setting.rb
+++ b/app/models/house_ad_setting.rb
@ -21,14 +21,14 @@ module ::AdPlugin
      settings
    end

-    def self.settings_and_ads(for_anons: true)
+    def self.settings_and_ads(for_anons: true, scope: nil)
      settings = AdPlugin::HouseAdSetting.all
      ad_names = settings.values.map { |v| v.split("|") }.flatten.uniq

      if for_anons
        ads = AdPlugin::HouseAd.all_for_anons
      else
-        ads = AdPlugin::HouseAd.all_for_logged_in_users
+        ads = AdPlugin::HouseAd.all_for_logged_in_users(scope)
      end
      ads = ads.select { |ad| ad_names.include?(ad.name) }

@ -41,7 +41,7 @@ module ::AdPlugin
          ),
        creatives:
          ads.inject({}) do |h, ad|
-            h[ad.name] = ad.html
+            h[ad.name] = { html: ad.html, category_ids: ad.category_ids }
            h
          end,
      }
--- a/assets/javascripts/discourse/components/house-ad.js
+++ b/assets/javascripts/discourse/components/house-ad.js
@ -78,7 +78,14 @@ export default AdComponent.extend({
      }
      let ad = houseAds.creatives[adNames[adIndex[placement]]] || "";
      adIndex[placement] = (adIndex[placement] + 1) % adNames.length;
-      return ad;
+      // 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 {
      return "";
    }
--- a/assets/stylesheets/adplugin.scss
+++ b/assets/stylesheets/adplugin.scss
@ -214,6 +214,9 @@
 }

 .adplugin-mgmt {
+  .house-ad-name {
+    width: 100%;
+  }
  .house-ads-actions {
    .btn {
      margin-right: 8px;
@ -249,6 +252,16 @@
    padding-left: 2%;
    .visibility-settings {
      margin-bottom: 1em;
+
+      .description {
+        color: var(--primary-medium);
+        font-size: $font-down-1;
+      }
+
+      .category-selector,
+      .group-chooser {
+        margin-top: 1em;
+      }
    }
    .controls {
      margin-bottom: 1em;
@ -269,3 +282,7 @@
    }
  }
 }
+
+.house-ad-preview {
+  width: 100%;
+}
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@ -1,17 +1,17 @@
 en:
  js:
    adplugin:
-      advertisement_label: 'ADVERTISEMENT'
+      advertisement_label: "ADVERTISEMENT"
  admin_js:
    admin:
      site_settings:
        categories:
-          ad_plugin: 'Ad Plugin'
-          dfp_plugin: 'DFP/Ad Manager'
-          adsense_plugin: 'AdSense'
-          amazon_plugin: 'Amazon'
-          carbonads_plugin: 'Carbon Ads'
-          adbutler_plugin: 'AdButler'
+          ad_plugin: "Ad Plugin"
+          dfp_plugin: "DFP/Ad Manager"
+          adsense_plugin: "AdSense"
+          amazon_plugin: "Amazon"
+          carbonads_plugin: "Carbon Ads"
+          adbutler_plugin: "AdButler"
      adplugin:
        house_ads:
          title: "House Ads"
@ -26,6 +26,9 @@ en:
          more_settings: "More Settings"
          show_to_anons: "Show to anonymous users"
          show_to_logged_in_users: "Show to logged in users"
+          category_chooser_description: "Choose the categories where this ad should be displayed or leave empty to show the ad everywhere. The `no_ads_for_categories` site setting has priority over this setting."
+          group_chooser_description: "Choose the groups that can view this ad or leave empty to show the ad to all signed in users."
+          preview: "Preview"

          topic_list_top:
            title: "Topic list top ads"
--- a/plugin.rb
+++ b/plugin.rb
@ -42,7 +42,7 @@ after_initialize do
  reloadable_patch { Guardian.prepend ::AdPlugin::GuardianExtensions }

  add_to_serializer :site, :house_creatives do
-    AdPlugin::HouseAdSetting.settings_and_ads(for_anons: scope.anonymous?)
+    AdPlugin::HouseAdSetting.settings_and_ads(for_anons: scope.anonymous?, scope: scope)
  end

  add_to_serializer :topic_view, :tags_disable_ads do
--- a/spec/models/house_ad_setting_spec.rb
+++ b/spec/models/house_ad_setting_spec.rb
@ -73,6 +73,8 @@ describe AdPlugin::HouseAdSetting do
        html: "<whatever-anon>",
        visible_to_anons: true,
        visible_to_logged_in_users: false,
+        group_ids: [],
+        category_ids: [],
      )
    end

@ -82,6 +84,8 @@ describe AdPlugin::HouseAdSetting do
        html: "<whatever-logged-in>",
        visible_to_anons: false,
        visible_to_logged_in_users: true,
+        group_ids: [],
+        category_ids: [],
      )
    end

@ -94,11 +98,21 @@ describe AdPlugin::HouseAdSetting do
      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.data[:creatives]).to eq(
+        "anon-ad" => {
+          html: "<whatever-anon>",
+          category_ids: [],
+        },
+      )
      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.data[:creatives]).to eq(
+        "logged-in-ad" => {
+          html: "<whatever-logged-in>",
+          category_ids: [],
+        },
+      )
      expect(logged_in_message.group_ids).to eq([Group::AUTO_GROUPS[:trust_level_0]])
      expect(logged_in_message.user_ids).to eq(nil)
    end
--- a/spec/models/house_ad_spec.rb
+++ b/spec/models/house_ad_spec.rb
@ -15,6 +15,8 @@ describe AdPlugin::HouseAd do
      html: "<div>ANON</div>",
      visible_to_logged_in_users: false,
      visible_to_anons: true,
+      group_ids: [],
+      category_ids: [],
    )
  end

@ -24,6 +26,8 @@ describe AdPlugin::HouseAd do
      html: "<div>LOGGED IN</div>",
      visible_to_logged_in_users: true,
      visible_to_anons: false,
+      group_ids: [],
+      category_ids: [],
    )
  end

@ -68,11 +72,12 @@ describe AdPlugin::HouseAd do
  describe ".all_for_logged_in_users" do
    let!(:anon_ad) { create_anon_ad }
    let!(:logged_in_ad) { create_logged_in_ad }
+    let!(:user) { Fabricate(:user) }

    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,
-      )
+      expect(
+        AdPlugin::HouseAd.all_for_logged_in_users(Guardian.new(user)).map(&:id),
+      ).to contain_exactly(logged_in_ad.id)
    end
  end

--- a/spec/requests/house_ad_controller_spec.rb
+++ b/spec/requests/house_ad_controller_spec.rb
@ -2,6 +2,8 @@

 describe AdPlugin::HouseAdsController do
  let(:admin) { Fabricate(:admin) }
+  let(:category) { Fabricate(:category) }
+  let(:group) { Fabricate(:group) }

  let!(:ad) do
    AdPlugin::HouseAd.create(
@ -9,6 +11,8 @@ describe AdPlugin::HouseAdsController do
      html: "<p>Banner</p>",
      visible_to_anons: true,
      visible_to_logged_in_users: false,
+      category_ids: [],
+      group_ids: [],
    )
  end

@ -23,6 +27,8 @@ describe AdPlugin::HouseAdsController do
              html: ad.html,
              visible_to_anons: "false",
              visible_to_logged_in_users: "true",
+              category_ids: [category.id],
+              group_ids: [group.id],
            }
        expect(response.status).to eq(200)
        expect(response.parsed_body["house_ad"].symbolize_keys).to eq(
@ -31,6 +37,8 @@ describe AdPlugin::HouseAdsController do
          html: ad.html,
          visible_to_anons: false,
          visible_to_logged_in_users: true,
+          category_ids: [category.id],
+          group_ids: [group.id],
        )

        ad_copy = AdPlugin::HouseAd.find(ad.id)
@ -38,6 +46,8 @@ describe AdPlugin::HouseAdsController do
        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)
+        expect(ad_copy.category_ids).to eq([category.id])
+        expect(ad_copy.group_ids).to eq([group.id])
      end
    end

@ -51,6 +61,8 @@ describe AdPlugin::HouseAdsController do
              html: "blah <h4cked>",
              visible_to_anons: "false",
              visible_to_logged_in_users: "true",
+              group_ids: [group.id],
+              category_ids: [category.id],
            }
        expect(response.status).to eq(404)

@ -59,6 +71,8 @@ describe AdPlugin::HouseAdsController do
        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)
+        expect(ad_copy.category_ids).to eq([])
+        expect(ad_copy.group_ids).to eq([])
      end
    end
  end
--- 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(
@ -9,6 +14,8 @@ RSpec.describe SiteController do
      html: "<div>ANON</div>",
      visible_to_logged_in_users: false,
      visible_to_anons: true,
+      group_ids: [],
+      category_ids: [],
    )
  end

@ -18,6 +25,30 @@ RSpec.describe SiteController do
      html: "<div>LOGGED IN</div>",
      visible_to_logged_in_users: true,
      visible_to_anons: false,
+      group_ids: [],
+      category_ids: [],
+    )
+  end
+
+  let!(:logged_in_ad_with_category) do
+    AdPlugin::HouseAd.create(
+      name: "logged-in-ad-with-category",
+      html: "<div>LOGGED IN WITH CATEGORY</div>",
+      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: "<div>LOGGED IN WITH GROUP</div>",
+      visible_to_logged_in_users: true,
+      visible_to_anons: false,
+      group_ids: [group_2.id],
+      category_ids: [],
    )
  end

@ -27,19 +58,49 @@ RSpec.describe SiteController do
      html: "<div>EVERYONE</div>",
      visible_to_logged_in_users: true,
      visible_to_anons: true,
+      group_ids: [],
+      category_ids: [],
    )
  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: "<div>EVERYONE</div>",
+      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
@ -48,6 +109,7 @@ RSpec.describe SiteController do
    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",
--- 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": "<div class='h-topic-list'>TOPIC LIST TOP</div>",
-        "Above Post Stream":
-          "<div class='h-above-post-stream'>ABOVE POST STREAM</div>",
-        "Above Suggested":
-          "<div class='h-above-suggested'>ABOVE SUGGESTED</div>",
-        Post: "<div class='h-post'>BELOW POST</div>",
-        "Between Topic List":
-          "<div class='h-between-topic-list'>BETWEEN TOPIC LIST</div>",
+        "Topic List Top": {
+          html: "<div class='h-topic-list'>TOPIC LIST TOP</div>",
+          category_ids: [],
+        },
+        "Above Post Stream": {
+          html: "<div class='h-above-post-stream'>ABOVE POST STREAM</div>",
+          category_ids: [],
+        },
+        "Above Suggested": {
+          html: "<div class='h-above-suggested'>ABOVE SUGGESTED</div>",
+          category_ids: [],
+        },
+        Post: {
+          html: "<div class='h-post'>BELOW POST</div>",
+          category_ids: [],
+        },
+        "Between Topic List": {
+          html: "<div class='h-between-topic-list'>BETWEEN TOPIC LIST</div>",
+          category_ids: [],
+        },
      },
    },
  });
@ -114,3 +126,175 @@ acceptance("House Ads", function (needs) {
      );
  });
 });
+
+acceptance(
+  "House Ads | Category and Group Permissions | Authenticated | 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: "<div class='h-topic-list'>TOPIC LIST TOP</div>",
+            // 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 | Authenticated | 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: "<div class='h-topic-list'>TOPIC LIST TOP</div>",
+            // 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"
+        );
+    });
+  }
+);
+
+acceptance(
+  "House Ads | Category and Group Permissions | Anonymous | Hide Ad",
+  function (needs) {
+    needs.settings({
+      no_ads_for_categories: "",
+    });
+    needs.site({
+      house_creatives: {
+        settings: {
+          topic_list_top: "Topic List Top",
+        },
+        creatives: {
+          "Topic List Top": {
+            html: "<div class='h-topic-list'>TOPIC LIST TOP</div>",
+            // restrict ad to a different category than /c/bug/1
+            category_ids: [2],
+          },
+        },
+      },
+    });
+
+    test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+      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"
+        );
+    });
+  }
+);
+
+acceptance(
+  "House Ads | Category and Group Permissions | Anonymous | Hide Ad",
+  function (needs) {
+    needs.settings({
+      no_ads_for_categories: "",
+    });
+    needs.site({
+      house_creatives: {
+        settings: {
+          topic_list_top: "Topic List Top",
+        },
+        creatives: {
+          "Topic List Top": {
+            html: "<div class='h-topic-list'>TOPIC LIST TOP</div>",
+            // restrict ad to a different category than /c/bug/1
+            category_ids: [2],
+          },
+        },
+      },
+    });
+
+    test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+      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"
+        );
+    });
+  }
+);
+
+acceptance(
+  "House Ads | Category and Group Permissions | Anonymous | Show Ad",
+  function (needs) {
+    needs.settings({
+      no_ads_for_categories: "",
+    });
+    needs.site({
+      house_creatives: {
+        settings: {
+          topic_list_top: "Topic List Top",
+        },
+        creatives: {
+          "Topic List Top": {
+            html: "<div class='h-topic-list'>TOPIC LIST TOP</div>",
+            // match /c/bug/1
+            category_ids: [1],
+          },
+        },
+      },
+    });
+
+    test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+      await visit("/c/bug/1");
+      assert
+        .dom(".h-topic-list")
+        .exists(
+          "ad is displayed because the current category id is included in the ad category_ids"
+        );
+    });
+  }
+);