From 230e77e5cd795b5451204b14124dd1614b8e3706 Mon Sep 17 00:00:00 2001 From: Joshua Wehner Date: Thu, 16 Mar 2023 14:23:02 +0100 Subject: [PATCH] Update FilteredAdsQuery and user settings for opting-out of Billboards (#19208) * Users can only opt-out of external ads * Move random sampling out of FilteredAdsQuery * Refactor filtered_ads_query_spec * Add test for new permit_adjacent setting * Rubocop * Use permit_adjacent_sponsors on article sidebar and post-comments * Remove, unused * Update copy * type_matched -> type_of_ads * Remove non-applicable test --------- Co-authored-by: Ridhwana --- .../initializeDisplayAdVisibility.js | 2 +- app/models/display_ad.rb | 23 +- app/queries/display_ads/filtered_ads_query.rb | 50 ++--- app/views/articles/_sticky_nav.html.erb | 10 +- app/views/articles/show.html.erb | 8 +- app/views/shared/_display_ad.html.erb | 5 +- app/views/users/_customization.html.erb | 2 +- config/locales/views/settings/en.yml | 6 +- config/locales/views/settings/fr.yml | 6 +- .../display_ads/filtered_ads_query_spec.rb | 201 +++++++----------- 10 files changed, 145 insertions(+), 168 deletions(-) diff --git a/app/assets/javascripts/initializers/initializeDisplayAdVisibility.js b/app/assets/javascripts/initializers/initializeDisplayAdVisibility.js index 22bdc8a24..6462334a4 100644 --- a/app/assets/javascripts/initializers/initializeDisplayAdVisibility.js +++ b/app/assets/javascripts/initializers/initializeDisplayAdVisibility.js @@ -8,7 +8,7 @@ function initializeDisplayAdVisibility() { var user = userData(); displayAds.forEach((ad) => { - if (user && !user.display_sponsors) { + if (user && !user.display_sponsors && ad.dataset['typeOf'] == 'external') { ad.classList.add('hidden'); } else { ad.classList.remove('hidden'); diff --git a/app/models/display_ad.rb b/app/models/display_ad.rb index 46a068312..f1e59d140 100644 --- a/app/models/display_ad.rb +++ b/app/models/display_ad.rb @@ -37,14 +37,33 @@ class DisplayAd < ApplicationRecord search: "%#{term}%" } - def self.for_display(area:, user_signed_in:, organization_id: nil, article_tags: []) - DisplayAds::FilteredAdsQuery.call( + def self.for_display(area:, user_signed_in:, organization_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_tags: article_tags, + permit_adjacent_sponsors: permit_adjacent_sponsors, ) + + # Business Logic Context: + # We are always showing more of the good stuff — but we are also always testing the system to give any a chance to + # rise to the top. 1 out of every 8 times we show an ad (12.5%), it is totally random. This gives "not yet + # evaluated" stuff a chance to get some engagement and start showing up more. If it doesn't get engagement, it + # stays in this area. + + # Ads that get engagement have a higher "success rate", and among this category, we sample from the top 15 that + # meet that criteria. Within those 15 top "success rates" likely to be clicked, there is a weighting towards the + # top ranked outcome as well, and a steady decline over the next 15 — that's because it's not "Here are the top 15 + # pick one randomly", it is actually "Let's cut off the query at a random limit between 1 and 15 and sample from + # that". So basically the "limit" logic will result in 15 sets, and then we sample randomly from there. The + # "first ranked" ad will show up in all 15 sets, where as 15 will only show in 1 of the 15. + if rand(8) == 1 + ads_for_display.sample + else + ads_for_display.limit(rand(1..15)).sample + end end def human_readable_placement_area diff --git a/app/queries/display_ads/filtered_ads_query.rb b/app/queries/display_ads/filtered_ads_query.rb index 3022ded4a..937b71cca 100644 --- a/app/queries/display_ads/filtered_ads_query.rb +++ b/app/queries/display_ads/filtered_ads_query.rb @@ -4,12 +4,14 @@ module DisplayAds new(...).call end - def initialize(display_ads:, area:, user_signed_in:, organization_id: nil, article_tags: []) + def initialize(area:, user_signed_in:, display_ads: DisplayAd, organization_id: nil, article_tags: [], + permit_adjacent_sponsors: true) @filtered_display_ads = display_ads @area = area @user_signed_in = user_signed_in @organization_id = organization_id @article_tags = article_tags + @permit_adjacent_sponsors = permit_adjacent_sponsors end def call @@ -30,10 +32,9 @@ module DisplayAds authenticated_ads(%w[all logged_out]) end - @filtered_display_ads = community_or_in_house_ads + @filtered_display_ads = type_of_ads @filtered_display_ads = @filtered_display_ads.order(success_rate: :desc) - @filtered_display_ads = sample_ads end private @@ -59,33 +60,26 @@ module DisplayAds @filtered_display_ads.where(display_to: display_auth_audience) end - def community_or_in_house_ads - @filtered_display_ads.where( - "(type_of = :in_house) OR - (type_of = :community AND organization_id = :organization_id) OR - (type_of = :external AND organization_id != :organization_id)", - DisplayAd.type_ofs.merge({ organization_id: @organization_id }), - ) - end + def type_of_ads + # Always match in-house-type ads + in_house = "(type_of = :in_house)" - # Business Logic Context: - # We are always showing more of the good stuff — but we are also always testing the system to give any a chance to - # rise to the top. 1 out of every 8 times we show an ad (12.5%), it is totally random. This gives "not yet - # evaluated" stuff a chance to get some engagement and start showing up more. If it doesn't get engagement, it - # stays in this area. + # If this is an article that belongs to an organization, we might show community-type ads + community = if @organization_id + "(type_of = :community AND organization_id = :organization_id)" + end - # Ads that get engagement have a higher "success rate", and among this category, we sample from the top 15 that - # meet that criteria. Within those 15 top "success rates" likely to be clicked, there is a weighting towards the - # top ranked outcome as well, and a steady decline over the next 15 — that's because it's not "Here are the top 15 - # pick one randomly", it is actually "Let's cut off the query at a random limit between 1 and 15 and sample from - # that". So basically the "limit" logic will result in 15 sets, and then we sample randomly from there. The - # "first ranked" ad will show up in all 15 sets, where as 15 will only show in 1 of the 15. - def sample_ads - if rand(8) == 1 - @filtered_display_ads.sample - else - @filtered_display_ads.limit(rand(1..15)).sample - end + # If the article's author permits adjacent sponsors, we might show an external-type ad + # *if* the organization_id doesn't match the current article's organization id + external = if @permit_adjacent_sponsors && @organization_id + "(type_of = :external AND organization_id != :organization_id)" + elsif @permit_adjacent_sponsors + "(type_of = :external)" + end + + types_matching = [in_house, community, external].compact.join(" OR ") + @filtered_display_ads.where(types_matching, + DisplayAd.type_ofs.merge({ organization_id: @organization_id })) end end end diff --git a/app/views/articles/_sticky_nav.html.erb b/app/views/articles/_sticky_nav.html.erb index 893d6879c..2b212a6dd 100644 --- a/app/views/articles/_sticky_nav.html.erb +++ b/app/views/articles/_sticky_nav.html.erb @@ -56,10 +56,14 @@ <%# cache("article-sidebar-content-#{rand(5)}-#{@article.id}-#{user_signed_in?}-#{(@organization || @user).latest_article_updated_at}", expires_in: 15.minutes) do %> - <% sidebar_ad = DisplayAd.for_display(area: "post_sidebar", user_signed_in: user_signed_in?, organization_id: @article.organization_id, article_tags: @article.decorate.cached_tag_list_array) %> - <% if @article.permit_adjacent_sponsors? && sidebar_ad %> + <% sidebar_ad = DisplayAd.for_display(area: "post_sidebar", + user_signed_in: user_signed_in?, + organization_id: @article.organization_id, + article_tags: @article.decorate.cached_tag_list_array, + permit_adjacent_sponsors: @article.decorate.permit_adjacent_sponsors?) %> + <% if sidebar_ad %>
- <%= render partial: "shared/display_ad", locals: {display_ad: sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %> + <%= render partial: "shared/display_ad", locals: { display_ad: sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %>
<% end %> <%# end %> diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb index 6fedaddb9..34a547653 100644 --- a/app/views/articles/show.html.erb +++ b/app/views/articles/show.html.erb @@ -193,10 +193,14 @@ <% cache("article-bottom-content-#{rand(5)}-#{@article.id}-#{user_signed_in?}-#{(@organization || @user).latest_article_updated_at}", expires_in: 15.minutes) do %> - <% @display_ad = DisplayAd.for_display(area: "post_comments", user_signed_in: user_signed_in?, organization_id: @article.organization_id, article_tags: @article.decorate.cached_tag_list_array) %> + <% @display_ad = DisplayAd.for_display(area: "post_comments", + 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) %> <% if @display_ad %>
- <%= render partial: "shared/display_ad", locals: {display_ad: @display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %> + <%= render partial: "shared/display_ad", locals: { display_ad: @display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %>
<% end %> <% end %> diff --git a/app/views/shared/_display_ad.html.erb b/app/views/shared/_display_ad.html.erb index 9daa981e4..952fe6293 100644 --- a/app/views/shared/_display_ad.html.erb +++ b/app/views/shared/_display_ad.html.erb @@ -2,7 +2,8 @@ data-display-unit data-id="<%= display_ad.id %>" data-category-click="<%= DisplayAdEvent::CATEGORY_CLICK %>" data-category-impression="<%= DisplayAdEvent::CATEGORY_IMPRESSION %>" - data-context-type="<%= data_context_type %>"> + data-context-type="<%= data_context_type %>" + data-type-of="<%= display_ad.type_of %>">
<% if Page.exists?(slug: "billboards") && display_ad.organization_id? && display_ad.type_of == "community" %> profile @@ -10,7 +11,7 @@ <% else %>
<%= Settings::Community.community_name %>
<% end %> -