diff --git a/app/helpers/display_ad_helper.rb b/app/helpers/display_ad_helper.rb index 56f888ed0..c5662b09e 100644 --- a/app/helpers/display_ad_helper.rb +++ b/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 diff --git a/app/models/audience_segment.rb b/app/models/audience_segment.rb index ae8be7ccf..5eec767a6 100644 --- a/app/models/audience_segment.rb +++ b/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 diff --git a/app/views/admin/billboards/_form.html.erb b/app/views/admin/billboards/_form.html.erb index 4dcb6ec25..aa2baf250 100644 --- a/app/views/admin/billboards/_form.html.erb +++ b/app/views/admin/billboards/_form.html.erb @@ -63,12 +63,19 @@ -
"> +
"> <%= 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 %>
diff --git a/cypress/e2e/seededFlows/adminFlows/billboards/deleteBillboards.spec.js b/cypress/e2e/seededFlows/adminFlows/billboards/deleteBillboards.spec.js index d3e8043b7..d6e4ab0b0 100644 --- a/cypress/e2e/seededFlows/adminFlows/billboards/deleteBillboards.spec.js +++ b/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'); }); }); }); diff --git a/cypress/e2e/seededFlows/adminFlows/billboards/createBillboards.spec.js b/cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js similarity index 55% rename from cypress/e2e/seededFlows/adminFlows/billboards/createBillboards.spec.js rename to cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js index c9429d8cd..25e906ecd 100644 --- a/cypress/e2e/seededFlows/adminFlows/billboards/createBillboards.spec.js +++ b/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'); + }); + }); }); }); }); diff --git a/db/seeds.rb b/db/seeds.rb index d4f4ae10d..ad67527c7 100644 --- a/db/seeds.rb +++ b/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 ############################################################################## diff --git a/spec/helpers/display_ad_helper_spec.rb b/spec/helpers/display_ad_helper_spec.rb index ff2957bef..958d47317 100644 --- a/spec/helpers/display_ad_helper_spec.rb +++ b/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 diff --git a/spec/support/seeds/seeds_e2e.rb b/spec/support/seeds/seeds_e2e.rb index 203750ed0..330b8633e 100644 --- a/spec/support/seeds/seeds_e2e.rb +++ b/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: "

This is a billboard with a manually managed audience

", + placement_area: "sidebar_left", + name: "Manual Audience Billboard", + published: true, + approved: true, + audience_segment: AudienceSegment.where(type_of: :manual).first, + ) end