Exclude manual segments from dropdown in billboard admin form (#19677)

* exclude manual segments from audience segments dropdown

* specs 🎉

* fix deleteDisplayAds specs

* add seed for display ad with manually managed segment

* moar specs

app/helpers/display_ad_helper.rb

@@ -3,8 +3,17 @@ module DisplayAdHelper
     DisplayAd::ALLOWED_PLACEMENT_AREAS_HUMAN_READABLE.zip(DisplayAd::ALLOWED_PLACEMENT_AREAS)
   end
 
-  def audience_segments_for_display_ads
-    AudienceSegment.human_readable_segments
+  def automatic_audience_segments_options_array
+    AudienceSegment.not_manual.pluck(:id, :type_of)
+      .map { |(id, type)| [AudienceSegment.human_readable_description_for(type), id] }
+  end
+
+  def single_audience_segment_option(billboard)
+    segment = billboard.audience_segment
+    # This should never happen
+    raise ArgumentError, "Billboard must have a target audience segment to build option for" if segment.blank?
+
+    [[AudienceSegment.human_readable_description_for(segment.type_of), segment.id]]
   end
 
   # Determines whether the area provided as a parameter is a targeted tag placement on the feed

app/models/audience_segment.rb

@@ -52,10 +52,8 @@ class AudienceSegment < ApplicationRecord
     QUERIES[segment_type.to_sym]
   end
 
-  def self.human_readable_segments
-    AudienceSegment.pluck(:id, :type_of)
-      .sort_by { |(id, str)| [(str == "manual" ? 1 : 0), id] } # move manual to end of list
-      .map { |(id, type)| [I18n.t("models.#{model_name.i18n_key}.type_ofs.#{type}"), id] }
+  def self.human_readable_description_for(type)
+    I18n.t("models.#{model_name.i18n_key}.type_ofs.#{type}")
   end
 
   def self.including_user_counts

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

@@ -63,12 +63,19 @@
       </fieldset>
     </div>
 
-    <div class="crayons-field <%= "hidden" if @display_ad.audience_segment_id.blank? %>">
+    <div class="crayons-field <%= "hidden" if @display_ad.audience_segment.blank? %>">
       <%= label_tag :audience_segment_id, "Users who:", class: "crayons-field__label" %>
-      <%= select_tag :audience_segment_id,
-                     options_for_select(audience_segments_for_display_ads, selected: @display_ad.audience_segment_id),
-                     include_blank: "All users",
-                     class: "crayons-select js-audience-segment" %>
+      <% if @display_ad.audience_segment&.manual? %>
+        <%= select_tag :audience_segment_id,
+                       options_for_select(single_audience_segment_option(@display_ad), selected: @display_ad.audience_segment_id),
+                       disabled: true,
+                       class: "crayons-select js-audience-segment" %>
+      <% else %>
+        <%= select_tag :audience_segment_id,
+                       options_for_select(automatic_audience_segments_options_array, selected: @display_ad.audience_segment_id),
+                       include_blank: "All users",
+                       class: "crayons-select js-audience-segment" %>
+      <% end %>
     </div>
 
     <div class="crayons-field">

cypress/e2e/seededFlows/adminFlows/billboards/deleteBillboards.spec.js

@@ -7,7 +7,11 @@ describe('Delete Display Ads', () => {
       cy.loginAndVisit(user, '/admin/customization/billboards');
 
       cy.findByRole('table').within(() => {
-        cy.findByRole('button', { name: 'Destroy' }).click({ force: true });
+        cy.contains('Tests Display Ad')
+          .closest('tr')
+          .findByRole('button', { name: 'Destroy' })
+          .as('deleteButton')
+          .click({ force: true });
       });
     });
   });
@@ -29,9 +33,7 @@ describe('Delete Display Ads', () => {
         cy.findByRole('button', { name: 'Close' }).click();
       });
 
-      cy.findByRole('table').within(() => {
-        cy.findByRole('button', { name: 'Destroy' }).should('be.visible');
-      });
+      cy.get('@deleteButton').should('be.visible');
     });
 
     it('should remove display ad if confirmation text matches', () => {
@@ -50,9 +52,7 @@ describe('Delete Display Ads', () => {
           );
         });
 
-        cy.findByRole('table').within(() => {
-          cy.findByRole('button', { name: 'Destroy' }).should('not.exist');
-        });
+        cy.get('@deleteButton').should('not.exist');
       });
     });
 
@@ -78,9 +78,7 @@ describe('Delete Display Ads', () => {
             .should('be.visible');
         });
 
-        cy.findByRole('table').within(() => {
-          cy.findByRole('button', { name: 'Destroy' }).should('be.visible');
-        });
+        cy.get('@deleteButton').should('be.visible');
       });
     });
   });

cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js

@@ -1,5 +1,5 @@
-describe('Create Display Ads', () => {
-  context('when creating a new display ad', () => {
+describe('Billboards Form', () => {
+  context('when creating a new billboard', () => {
     beforeEach(() => {
       cy.testSetup();
       cy.fixture('users/adminUser.json').as('user');
@@ -57,6 +57,50 @@ describe('Create Display Ads', () => {
         cy.findByRole('radio', { name: 'Only logged in users' }).click();
         cy.findByLabelText('Users who:').should('be.visible');
       });
+
+      it('should not include manual segments in the audience segment field', () => {
+        cy.findByRole('radio', { name: 'Only logged in users' }).click();
+        cy.findByLabelText('Users who:')
+          .as('audienceSegments')
+          .should('be.visible');
+
+        cy.get('@audienceSegments').select('Are trusted').should('exist');
+        cy.get('@audienceSegments').select('Have not posted yet').should('exist');
+        cy.get('@audienceSegments')
+          .select('Have not set an experience level')
+          .should('exist');
+
+        cy.get('@audienceSegments')
+          .contains('Managed elsewhere')
+          .should('not.exist');
+      });
+
+      context('when editing a display ad with a manually managed audience', () => {
+        beforeEach(() => {
+          cy.visit('/admin/customization/billboards');
+          cy.findByRole('link', { name: 'Manual Audience Billboard' }).click({
+            force: true,
+          });
+        });
+
+        it('shows the audience segment field but disabled', () => {
+          cy.findByLabelText('Users who:')
+            .as('audienceSegments')
+            .should('be.disabled');
+
+          cy.get('@audienceSegments')
+            .find(':selected')
+            .should('have.text', 'Managed elsewhere');
+
+          cy.get('@audienceSegments').contains('Are trusted').should('not.exist');
+          cy.get('@audienceSegments')
+            .contains('Have not posted yet')
+            .should('not.exist');
+          cy.get('@audienceSegments')
+            .contains('Have not set an experience level')
+            .should('not.exist');
+        });
+      });
     });
   });
 });

db/seeds.rb

@@ -565,9 +565,19 @@ seeder.create_if_none(DisplayAd) do
       body_markdown: Faker::Lorem.sentence,
       published: true,
       approved: true,
-      placement_area: placement_area
+      placement_area: placement_area,
     )
   end
+
+  segment = AudienceSegment.create!(type_of: :manual)
+  DisplayAd.create!(
+    name: "#{Faker::Lorem.word} (Manually Managed Audience)",
+    body_markdown: Faker::Lorem.sentence,
+    published: true,
+    approved: true,
+    placement_area: DisplayAd::ALLOWED_PLACEMENT_AREAS.sample,
+    audience_segment: segment,
+  )
 end
 
 ##############################################################################

spec/helpers/display_ad_helper_spec.rb

@@ -7,4 +7,48 @@ describe DisplayAdHelper do
                                                                         "sidebar_left_2"]
     end
   end
+
+  describe ".automatic_audience_segments_options_array" do
+    subject(:options) { helper.automatic_audience_segments_options_array }
+
+    let!(:no_posts) { create(:audience_segment, type_of: "no_posts_yet") }
+    let!(:manual_1) { create(:audience_segment, type_of: "manual") }
+    let!(:trusted) { create(:audience_segment, type_of: "trusted") }
+    let!(:manual_2) { create(:audience_segment, type_of: "manual") }
+    let!(:posted) { create(:audience_segment, type_of: "posted") }
+
+    it "returns proper human values for only automatic segments" do
+      expect(options).to include(
+        ["Have not posted yet", no_posts.id],
+        ["Are trusted", trusted.id],
+        ["Have at least one post", posted.id],
+      )
+
+      expect(options).not_to include(
+        ["Managed elsewhere", manual_1.id],
+        ["Managed elsewhere", manual_2.id],
+      )
+    end
+  end
+
+  describe ".single_audience_segment_option" do
+    subject(:options) { helper.single_audience_segment_option(billboard) }
+
+    let(:target_segment) { create(:audience_segment, type_of: "manual") }
+    let!(:different_segment) { create(:audience_segment, type_of: "manual") }
+    let(:billboard) { build(:display_ad, name: "Manual Test", audience_segment: target_segment) }
+
+    it "returns a single option with the billboard's audience segment" do
+      expect(options).to include(["Managed elsewhere", target_segment.id])
+      expect(options).not_to include(["Managed elsewhere", different_segment.id])
+    end
+
+    context "when the billboard doesn't have an audience segment" do
+      let(:billboard) { build(:display_ad, name: "No Audience Segment") }
+
+      it "raises ArgumentError" do
+        expect { options }.to raise_error(ArgumentError, /must have a target audience segment/)
+      end
+    end
+  end
 end

spec/support/seeds/seeds_e2e.rb

@@ -1067,6 +1067,14 @@ end
 
 ##############################################################################
 
+seeder.create_if_none(AudienceSegment) do
+  AudienceSegment.type_ofs.each_key do |type|
+    AudienceSegment.create!(type_of: type)
+  end
+end
+
+##############################################################################
+
 seeder.create_if_none(DisplayAd) do
   org_id = Organization.find_by(slug: "bachmanity").id
   DisplayAd.create!(
@@ -1077,4 +1085,14 @@ seeder.create_if_none(DisplayAd) do
     published: true,
     approved: true,
   )
+
+  DisplayAd.create!(
+    organization_id: org_id,
+    body_markdown: "<h1>This is a billboard with a manually managed audience</h1>",
+    placement_area: "sidebar_left",
+    name: "Manual Audience Billboard",
+    published: true,
+    approved: true,
+    audience_segment: AudienceSegment.where(type_of: :manual).first,
+  )
 end