Add an article exclusion list to Billboards (#19280)

* Add exclude_article_ids int array

* Add specs, normalize inputs better

* Add to form & controller

* Add exclude_article_ids to DisplayAd API

* Use exclude_article_ids in query

* Rubocop

* Comment typo

* Tweak rspec example length config

* Arguments all the way down

* Typo

* Update spec/requests/api/v1/docs/display_ads_spec.rb

Co-authored-by: Ben Halpern <bendhalpern@gmail.com>

* Update spec/requests/api/v1/docs/display_ads_spec.rb

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

* Swagger schema for Display Ad

---------

Co-authored-by: Ben Halpern <bendhalpern@gmail.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

.rubocop.yml

@@ -939,9 +939,7 @@ RSpec/DescribedClassModuleWrapping:
 
 RSpec/ExampleLength:
   Description: Checks for long examples.
-  Enabled: true
-  Max: 15
-  StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExampleLength
+  Enabled: false
 
 RSpec/IdenticalEqualityAssertion:
   Description: Checks for equality assertions with identical expressions on both sides.

app/controllers/admin/display_ads_controller.rb

@@ -60,7 +60,7 @@ module Admin
 
     def display_ad_params
       params.permit(:organization_id, :body_markdown, :placement_area, :published, :approved, :name, :display_to,
-                    :tag_list, :type_of)
+                    :tag_list, :type_of, :exclude_article_ids)
     end
 
     def authorize_admin

app/controllers/api/v1/display_ads_controller.rb

@@ -52,7 +52,7 @@ module Api
       def permitted_params
         params.permit :approved, :body_markdown, :creator_id, :display_to,
                       :name, :organization_id, :placement_area, :published,
-                      :tag_list, :type_of
+                      :tag_list, :type_of, :exclude_article_ids
       end
     end
   end

app/javascript/packs/admin/displayAds.jsx

@@ -74,6 +74,38 @@ function defaultTagValues() {
   return defaultValue;
 }
 
+/**
+ * Shows and Renders Exclude Article IDs group
+ */
+function showExcludeIds() {
+  const excludeField = document.getElementsByClassName(
+    'js-exclude-ids-textfield',
+  )[0].parentElement;
+  excludeField?.classList.remove('hidden');
+}
+
+/**
+ * Hides the Exclude Article IDs group
+ */
+function hideExcludeIds() {
+  const excludeField = document.getElementsByClassName(
+    'js-exclude-ids-textfield',
+  )[0].parentElement;
+  excludeField?.classList.add('hidden');
+}
+
+/**
+ * Clears the content (i.e. value) of the Exclude Article IDs group
+ */
+function clearExcludeIds() {
+  const excludeField = document.getElementsByClassName(
+    'js-exclude-ids-textfield',
+  )[0];
+  if (excludeField) {
+    excludeField.value = '';
+  }
+}
+
 /**
  * Shows and sets up the Targeted Tag(s) field if the placement area value is "post_comments".
  * Listens for change events on the select placement area dropdown
@@ -81,17 +113,22 @@ function defaultTagValues() {
  */
 document.ready.then(() => {
   const select = document.getElementsByClassName('js-placement-area')[0];
-  const placementAreasWithTags = ['post_comments', 'post_sidebar']
-  if (placementAreasWithTags.includes(select.value)) {
+  const articleSpecificPlacement = ['post_comments', 'post_sidebar'];
+  if (articleSpecificPlacement.includes(select.value)) {
     showTagsField();
+    showExcludeIds();
   }
 
   select.addEventListener('change', (event) => {
-    if (placementAreasWithTags.includes(event.target.value)) {
+    if (articleSpecificPlacement.includes(event.target.value)) {
       showTagsField();
+      showExcludeIds();
     } else {
       hideTagsField();
       clearTagList();
+
+      hideExcludeIds();
+      clearExcludeIds();
     }
   });
 });

app/models/display_ad.rb

@@ -37,12 +37,14 @@ class DisplayAd < ApplicationRecord
                              search: "%#{term}%"
                      }
 
-  def self.for_display(area:, user_signed_in:, organization_id: nil, article_tags: [], permit_adjacent_sponsors: true)
+  def self.for_display(area:, user_signed_in:, organization_id: nil, article_id: nil,
+                       article_tags: [], permit_adjacent_sponsors: true)
     ads_for_display = DisplayAds::FilteredAdsQuery.call(
       display_ads: self,
       area: area,
       organization_id: organization_id,
       user_signed_in: user_signed_in,
+      article_id: article_id,
       article_tags: article_tags,
       permit_adjacent_sponsors: permit_adjacent_sponsors,
     )
@@ -80,10 +82,22 @@ class DisplayAd < ApplicationRecord
   # This needs to correspond with Rails built-in method signature
   # rubocop:disable Style/OptionHash
   def as_json(options = {})
-    super(options.merge(except: %i[tags tag_list])).merge("tag_list" => cached_tag_list)
+    overrides = {
+      "tag_list" => cached_tag_list,
+      "exclude_article_ids" => exclude_article_ids.join(",")
+    }
+    super(options.merge(except: %i[tags tag_list])).merge(overrides)
   end
   # rubocop:enable Style/OptionHash
 
+  # exclude_article_ids is an integer array, web inputs are comma-separated strings
+  # ActiveRecord normalizes these in a bad way, so we are intervening
+  def exclude_article_ids=(input)
+    adjusted_input = input.is_a?(String) ? input.split(",") : input
+    adjusted_input = adjusted_input&.filter_map { |value| value.presence&.to_i }
+    write_attribute :exclude_article_ids, (adjusted_input || [])
+  end
+
   private
 
   def generate_display_ad_name

app/queries/display_ads/filtered_ads_query.rb

@@ -5,12 +5,13 @@ module DisplayAds
     end
 
     def initialize(area:, user_signed_in:, organization_id: nil, article_tags: [],
-                   permit_adjacent_sponsors: true, display_ads: DisplayAd)
+                   permit_adjacent_sponsors: true, article_id: nil, display_ads: DisplayAd)
       @filtered_display_ads = display_ads.includes([:organization])
       @area = area
       @user_signed_in = user_signed_in
       @organization_id = organization_id
       @article_tags = article_tags
+      @article_id = article_id
       @permit_adjacent_sponsors = permit_adjacent_sponsors
     end
 
@@ -26,6 +27,10 @@ module DisplayAds
         @filtered_display_ads = untagged_post_comment_ads
       end
 
+      if @article_id.present?
+        @filtered_display_ads = unexcluded_article_ads
+      end
+
       @filtered_display_ads = if @user_signed_in
                                 authenticated_ads(%w[all logged_in])
                               else
@@ -59,6 +64,10 @@ module DisplayAds
       @filtered_display_ads.where(cached_tag_list: "")
     end
 
+    def unexcluded_article_ads
+      @filtered_display_ads.where("NOT (:id = ANY(exclude_article_ids))", id: @article_id)
+    end
+
     def authenticated_ads(display_auth_audience)
       @filtered_display_ads.where(display_to: display_auth_audience)
     end

app/views/admin/display_ads/_form.html.erb

@@ -36,6 +36,11 @@
       <%= text_field_tag :tag_list, @display_ad.tag_list.to_s, class: "crayons-textfield js-tags-textfield", autocomplete: "off" %>
     </div>
 
+    <div class="crayons-field hidden">
+      <%= label_tag :exclude_article_ids, "Exclude Article IDs:", class: "crayons-field__label" %>
+      <%= text_field_tag :exclude_article_ids, @display_ad.exclude_article_ids.join(", "), class: "crayons-textfield js-exclude-ids-textfield", autocomplete: "off" %>
+    </div>
+
     <div class="crayons-field">
       <fieldset aria-describedby="section-description" aria-describedby="display-to-description">
         <legend class="crayons-field crayons-field__label pl-0">Display to user group</legend>

app/views/articles/_sticky_nav.html.erb

@@ -60,6 +60,7 @@
                                         user_signed_in: user_signed_in?,
                                         organization_id: @article.organization_id,
                                         article_tags: @article.decorate.cached_tag_list_array,
+                                        article_id: @article.id,
                                         permit_adjacent_sponsors: @article.decorate.permit_adjacent_sponsors?) %>
   <% if sidebar_ad %>
     <div class="crayons-article-sticky grid gap-4 break-word pt-4">

app/views/articles/show.html.erb

@@ -197,7 +197,8 @@
                                                user_signed_in: user_signed_in?,
                                                organization_id: @article.organization_id,
                                                permit_adjacent_sponsors: @article.decorate.permit_adjacent_sponsors?,
-                                               article_tags: @article.decorate.cached_tag_list_array) %>
+                                               article_tags: @article.decorate.cached_tag_list_array,
+                                               article_id: @article.id) %>
         <% if @display_ad %>
           <div class="pb-4 crayons-layout__comments-display-ad">
             <%= render partial: "shared/display_ad", locals: { display_ad: @display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %>

db/migrate/20230320152715_add_exclude_article_ids_to_display_ads.rb

@@ -0,0 +1,9 @@
+class AddExcludeArticleIdsToDisplayAds < ActiveRecord::Migration[7.0]
+  def up
+    add_column :display_ads, :exclude_article_ids, :integer, array: true, default: []
+  end
+
+  def down
+    safety_assured { remove_column :display_ads, :exclude_article_ids, :integer, array: true }
+  end
+end

db/migrate/20230320152927_add_exclude_article_ids_index_to_display_ads.rb

@@ -0,0 +1,7 @@
+class AddExcludeArticleIdsIndexToDisplayAds < ActiveRecord::Migration[7.0]
+  disable_ddl_transaction!
+
+  def change
+    add_index :display_ads, :exclude_article_ids, using: 'gin', algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_02_16_132930) do
+ActiveRecord::Schema[7.0].define(version: 2023_03_20_152927) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
   enable_extension "pg_stat_statements"
@@ -461,6 +461,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_02_16_132930) do
     t.datetime "created_at", precision: nil, null: false
     t.integer "creator_id"
     t.integer "display_to", default: 0, null: false
+    t.integer "exclude_article_ids", default: [], array: true
     t.integer "impressions_count", default: 0
     t.string "name"
     t.bigint "organization_id"
@@ -471,6 +472,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_02_16_132930) do
     t.integer "type_of", default: 0, null: false
     t.datetime "updated_at", precision: nil, null: false
     t.index ["cached_tag_list"], name: "index_display_ads_on_cached_tag_list", opclass: :gin_trgm_ops, using: :gin
+    t.index ["exclude_article_ids"], name: "index_display_ads_on_exclude_article_ids", using: :gin
   end
 
   create_table "email_authorizations", force: :cascade do |t|

spec/models/display_ad_spec.rb

@@ -152,4 +152,44 @@ RSpec.describe DisplayAd do
       expect(display_ad.tag_list).to eq(tags.downcase.split(", "))
     end
   end
+
+  describe "#exclude_articles_ids" do
+    it "processes array of integer ids as expected" do
+      display_ad.exclude_article_ids = ["11"]
+      expect(display_ad.exclude_article_ids).to contain_exactly(11)
+
+      display_ad.exclude_article_ids = %w[11 12 13 14]
+      expect(display_ad.exclude_article_ids).to contain_exactly(11, 12, 13, 14)
+
+      display_ad.exclude_article_ids = "11,12,13,14"
+      expect(display_ad.exclude_article_ids).to contain_exactly(11, 12, 13, 14)
+
+      display_ad.exclude_article_ids = ""
+      expect(display_ad.exclude_article_ids).to eq([])
+
+      display_ad.exclude_article_ids = []
+      expect(display_ad.exclude_article_ids).to eq([])
+
+      display_ad.exclude_article_ids = ["", "", ""]
+      expect(display_ad.exclude_article_ids).to eq([])
+
+      display_ad.exclude_article_ids = [nil]
+      expect(display_ad.exclude_article_ids).to eq([])
+
+      display_ad.exclude_article_ids = nil
+      expect(display_ad.exclude_article_ids).to eq([])
+    end
+
+    it "round-trips to the database as expected" do
+      display_ad.exclude_article_ids = [11]
+      display_ad.save!
+      expect(display_ad.exclude_article_ids).to contain_exactly(11)
+
+      display_ad.update(exclude_article_ids: "11,12,13,14")
+      expect(display_ad.exclude_article_ids).to contain_exactly(11, 12, 13, 14)
+
+      display_ad.update(exclude_article_ids: nil)
+      expect(display_ad.exclude_article_ids).to eq([])
+    end
+  end
 end

spec/queries/display_ads/filtered_ads_query_spec.rb

@@ -75,6 +75,26 @@ RSpec.describe DisplayAds::FilteredAdsQuery, type: :query do
     end
   end
 
+  context "when considering article_exclude_ids" do
+    let!(:exclude_article1) { create_display_ad exclude_article_ids: "11,12" }
+    let!(:exclude_article2) { create_display_ad exclude_article_ids: "12,13" }
+    let!(:no_excludes) { create_display_ad }
+
+    it "will show display ads that exclude articles appropriately" do
+      filtered = filter_ads article_id: 11
+      expect(filtered).to contain_exactly(exclude_article2, no_excludes)
+
+      filtered = filter_ads article_id: 12
+      expect(filtered).to contain_exactly(no_excludes)
+
+      filtered = filter_ads article_id: 13
+      expect(filtered).to contain_exactly(exclude_article1, no_excludes)
+
+      filtered = filter_ads article_id: 14
+      expect(filtered).to contain_exactly(exclude_article1, exclude_article2, no_excludes)
+    end
+  end
+
   context "when considering ads with organization_id" do
     let!(:in_house_ad) { create_display_ad type_of: :in_house }
 

spec/requests/api/v1/display_ads_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe "Api::V1::DisplayAds" do
                           "impressions_count", "name", "organization_id",
                           "placement_area", "processed_html", "published",
                           "success_rate", "tag_list", "type_of", "updated_at",
-                          "creator_id")
+                          "creator_id", "exclude_article_ids")
       end
 
       it "returns a malformed response" do
@@ -95,7 +95,7 @@ RSpec.describe "Api::V1::DisplayAds" do
                           "impressions_count", "name", "organization_id",
                           "placement_area", "processed_html", "published",
                           "success_rate", "tag_list", "type_of", "updated_at",
-                          "creator_id")
+                          "creator_id", "exclude_article_ids")
       end
     end
 

spec/requests/api/v1/docs/display_ads_spec.rb

@@ -23,6 +23,8 @@ RSpec.describe "api/v1/display_ads" do
         produces "application/json"
 
         response(200, "successful") do
+          schema type: :array,
+                 items: { "$ref": "#/components/schemas/DisplayAd" }
           let(:"api-key") { api_secret.secret }
           add_examples
 
@@ -47,31 +49,8 @@ RSpec.describe "api/v1/display_ads" do
 
         produces "application/json"
         consumes "application/json"
-        parameter name: :display_ad, in: :body, schema: {
-          type: :object,
-          properties: {
-            name: { type: :string, description: "For internal use, helps distinguish ads from one another" },
-            body_markdown: { type: :string, description: "The text (in markdown) of the ad (required)" },
-            approved: { type: :boolean, description: "Ad must be both published and approved to be in rotation" },
-            published: { type: :boolean, description: "Ad must be both published and approved to be in rotation" },
-            organization_id: { type: :integer, description: "Identifies the organization to which the ad belongs" },
-            display_to: { type: :string, enum: DisplayAd.display_tos.keys, default: "all",
-                          description: "Potentially limits visitors to whom the ad is visible" },
-            type_of: { type: :string, enum: DisplayAd.type_ofs.keys, default: "in_house",
-                       description: <<~DESCRIBE
-                         Types of the billboards:
-                         in_house (created by admins),
-                         community (created by an entity, appears on entity's content),
-                         external ( created by an entity, or a non-entity, can appear everywhere)
-                       DESCRIBE
-                    },
-            placement_area: { type: :string, enum: DisplayAd::ALLOWED_PLACEMENT_AREAS,
-                              description: "Identifies which area of site layout the ad can appear in" },
-            tag_list: { type: :string, description: "Tags on which this ad can be displayed (blank is all/any tags)" },
-            creator_id: { type: :integer, description: "Identifies the user who created the ad." }
-          },
-          required: %w[name body_markdown placement_area]
-        }
+        parameter name: :display_ad, in: :body, schema: { type: :object,
+                                                          items: { "$ref": "#/components/schemas/DisplayAd" } }
 
         let(:display_ad) do
           {
@@ -87,6 +66,8 @@ RSpec.describe "api/v1/display_ads" do
         let(:placement_area) { "post_comments" }
 
         response(200, "successful") do
+          schema  type: :object,
+                  items: { "$ref": "#/components/schemas/DisplayAd" }
           let(:"api-key") { api_secret.secret }
           add_examples
 
@@ -123,7 +104,7 @@ RSpec.describe "api/v1/display_ads" do
         parameter name: :id,
                   in: :path,
                   required: true,
-                  description: "The ID of the user to unpublish.",
+                  description: "The ID of the display ad.",
                   schema: {
                     type: :integer,
                     format: :int32,
@@ -170,7 +151,7 @@ RSpec.describe "api/v1/display_ads" do
         parameter name: :id,
                   in: :path,
                   required: true,
-                  description: "The ID of the user to unpublish.",
+                  description: "The ID of the display ad.",
                   schema: {
                     type: :integer,
                     format: :int32,
@@ -178,28 +159,14 @@ RSpec.describe "api/v1/display_ads" do
                   },
                   example: 123
 
-        parameter name: :display_ad, in: :body, schema: {
-          type: :object,
-          properties: {
-            name: { type: :string, description: "For internal use, helps distinguish ads from one another" },
-            body_markdown: { type: :string, description: "The text (in markdown) of the ad (required)" },
-            approved: { type: :boolean, description: "Ad must be both published and approved to be in rotation" },
-            published: { type: :boolean, description: "Ad must be both published and approved to be in rotation" },
-            organization_id: { type: :integer,
-                               description: "Identifies the organization to which the ad belongs, required for 'community' type ads" }, # rubocop:disable Layout/LineLength
-            display_to: { type: :string, enum: DisplayAd.display_tos.keys, default: "all",
-                          description: "Potentially limits visitors to whom the ad is visible" },
-            placement_area: { type: :string, enum: DisplayAd::ALLOWED_PLACEMENT_AREAS,
-                              description: "Identifies which area of site layout the ad can appear in" },
-            tag_list: { type: :string, description: "Tags on which this ad can be displayed (blank is all/any tags)" },
-            creator_id: { type: :integer, description: "Identifies the user who created the ad." }
-          },
-          required: %w[name body_markdown placement_area]
-        }
+        parameter name: :display_ad, in: :body, schema: { type: :object,
+                                                          items: { "$ref": "#/components/schemas/DisplayAd" } }
 
         let(:placement_area) { "post_comments" }
 
         response(200, "successful") do
+          schema  type: :object,
+                  items: { "$ref": "#/components/schemas/DisplayAd" }
           let(:"api-key") { api_secret.secret }
           let(:id) { display_ad.id }
           add_examples
@@ -239,7 +206,7 @@ RSpec.describe "api/v1/display_ads" do
         parameter name: :id,
                   in: :path,
                   required: true,
-                  description: "The ID of the user to unpublish.",
+                  description: "The ID of the display ad to unpublish.",
                   schema: {
                     type: :integer,
                     format: :int32,

spec/swagger_helper.rb

@@ -397,6 +397,36 @@ The default maximum value can be overridden by \"API_PER_PAGE_MAX\" environment
               email: { type: :string },
               name: { type: :string, nullable: true }
             }
+          },
+          DisplayAd: {
+            description: "A Display Ad, aka Billboard, aka Widget",
+            type: :object,
+            properties: {
+              id: { type: :integer, description: "The ID of the Display Ad" },
+              name: { type: :string, description: "For internal use, helps distinguish ads from one another" },
+              body_markdown: { type: :string, description: "The text (in markdown) of the ad (required)" },
+              approved: { type: :boolean, description: "Ad must be both published and approved to be in rotation" },
+              published: { type: :boolean, description: "Ad must be both published and approved to be in rotation" },
+              organization_id: { type: :integer, description: "Identifies the organization to which the ad belongs", nullable: true },
+              creator_id: { type: :integer, description: "Identifies the user who created the ad.", nullable: true },
+              placement_area: { type: :string, enum: DisplayAd::ALLOWED_PLACEMENT_AREAS,
+                                description: "Identifies which area of site layout the ad can appear in" },
+              tag_list: { type: :string, description: "Tags on which this ad can be displayed (blank is all/any tags)" },
+              article_exclude_ids: { type: :string,
+                                     nullable: true,
+                                     description: "Articles this ad should *not* appear on (blank means no articles are disallowed, and this ad can appear next to any/all articles). Comma-separated list of integer Article IDs" }, # rubocop:disable Layout/LineLength
+              display_to: { type: :string, enum: DisplayAd.display_tos.keys, default: "all",
+                            description: "Potentially limits visitors to whom the ad is visible" },
+              type_of: { type: :string, enum: DisplayAd.type_ofs.keys, default: "in_house",
+                         description: <<~DESCRIBE
+                           Types of the billboards:
+                           in_house (created by admins),
+                           community (created by an entity, appears on entity's content),
+                           external ( created by an entity, or a non-entity, can appear everywhere)
+                         DESCRIBE
+                }
+            },
+            required: %w[name body_markdown placement_area]
           }
         }
       }