diff --git a/app/controllers/display_ads_controller.rb b/app/controllers/display_ads_controller.rb
index c685ec129..6e8ac3a57 100644
--- a/app/controllers/display_ads_controller.rb
+++ b/app/controllers/display_ads_controller.rb
@@ -1,21 +1,23 @@
class DisplayAdsController < ApplicationController
before_action :set_cache_control_headers, only: %i[show], unless: -> { current_user }
+ include DisplayAdHelper
CACHE_EXPIRY_FOR_DISPLAY_ADS = 15.minutes.to_i.freeze
def show
skip_authorization
set_cache_control_headers(CACHE_EXPIRY_FOR_DISPLAY_ADS) unless session_current_user_id
- if params[:placement_area]
+ if placement_area
if params[:username].present? && params[:slug].present?
@article = Article.find_by(slug: params[:slug])
end
@display_ad = DisplayAd.for_display(
- area: params[:placement_area],
+ area: placement_area,
user_signed_in: user_signed_in?,
user_id: current_user&.id,
article: @article ? ArticleDecorator.new(@article) : nil,
+ user_tags: user_tags,
)
if @display_ad && !session_current_user_id
@@ -25,4 +27,16 @@ class DisplayAdsController < ApplicationController
render layout: false
end
+
+ private
+
+ def placement_area
+ params[:placement_area]
+ end
+
+ def user_tags
+ return unless feed_targeted_tag_placement?(placement_area)
+
+ current_user.cached_followed_tag_names
+ end
end
diff --git a/app/helpers/display_ad_helper.rb b/app/helpers/display_ad_helper.rb
index fdd118181..56f888ed0 100644
--- a/app/helpers/display_ad_helper.rb
+++ b/app/helpers/display_ad_helper.rb
@@ -6,4 +6,14 @@ module DisplayAdHelper
def audience_segments_for_display_ads
AudienceSegment.human_readable_segments
end
+
+ # Determines whether the area provided as a parameter is a targeted tag placement on the feed
+ #
+ # @return [Boolean] true or false on whether the area is a targeted tag placement on the feed.
+ #
+ # @note An area of "sidebar_left_2" will return false as it is not part of DisplayAd::HOME_FEED_PLACEMENTS
+ # whilst an area of "feed_first" will return false.
+ def feed_targeted_tag_placement?(area)
+ DisplayAd::HOME_FEED_PLACEMENTS.include?(area)
+ end
end
diff --git a/app/javascript/packs/admin/displayAds.jsx b/app/javascript/packs/admin/displayAds.jsx
index b2548ce64..e8f283d67 100644
--- a/app/javascript/packs/admin/displayAds.jsx
+++ b/app/javascript/packs/admin/displayAds.jsx
@@ -141,19 +141,35 @@ function clearExcludeIds() {
document.ready.then(() => {
const select = document.getElementsByClassName('js-placement-area')[0];
const articleSpecificPlacement = ['post_comments', 'post_sidebar'];
- if (articleSpecificPlacement.includes(select.value)) {
+ const targetedTagPlacements = [
+ 'post_comments',
+ 'post_sidebar',
+ 'feed_first',
+ 'feed_second',
+ 'feed_third',
+ ];
+
+ if (targetedTagPlacements.includes(select.value)) {
showTagsField();
+ }
+
+ select.addEventListener('change', (event) => {
+ if (targetedTagPlacements.includes(event.target.value)) {
+ showTagsField();
+ } else {
+ hideTagsField();
+ clearTagList();
+ }
+ });
+
+ if (articleSpecificPlacement.includes(select.value)) {
showExcludeIds();
}
select.addEventListener('change', (event) => {
if (articleSpecificPlacement.includes(event.target.value)) {
- showTagsField();
showExcludeIds();
} else {
- hideTagsField();
- clearTagList();
-
hideExcludeIds();
clearExcludeIds();
}
diff --git a/app/javascript/packs/homePageFeed.jsx b/app/javascript/packs/homePageFeed.jsx
index b7d5634b1..6b44fc331 100644
--- a/app/javascript/packs/homePageFeed.jsx
+++ b/app/javascript/packs/homePageFeed.jsx
@@ -50,7 +50,6 @@ function feedConstruct(
bookmarkedFeedItems,
bookmarkClick,
currentUserId,
- timeFrame,
) {
const commonProps = {
bookmarkClick,
@@ -82,49 +81,20 @@ function feedConstruct(
}
if (typeof item === 'object') {
- // For "saveable" props, "!=" is used instead of "!==" to compare user_id
- // and currentUserId because currentUserId is a String while user_id is an Integer
-
- if (item.id === pinnedItem?.id && timeFrame === '') {
- return (
-
- );
- }
-
- if (item.id === imageItem?.id) {
- return (
-
- );
- }
-
- if (item.class_name === 'Article') {
- return (
-
- );
- }
+ return (
+
+ );
}
});
}
@@ -187,7 +157,6 @@ export const renderFeed = async (timeFrame, afterRender) => {
bookmarkedFeedItems,
bookmarkClick,
currentUserId,
- timeFrame,
)}
);
diff --git a/app/models/display_ad.rb b/app/models/display_ad.rb
index f2c396664..a126890f6 100644
--- a/app/models/display_ad.rb
+++ b/app/models/display_ad.rb
@@ -18,6 +18,8 @@ class DisplayAd < ApplicationRecord
"Sidebar Right (Individual Post)",
"Below the comment section"].freeze
+ HOME_FEED_PLACEMENTS = %w[feed_first feed_second feed_third].freeze
+
MAX_TAG_LIST_SIZE = 10
POST_WIDTH = 775
SIDEBAR_WIDTH = 350
@@ -57,8 +59,9 @@ class DisplayAd < ApplicationRecord
scope :seldom_seen, ->(area) { where("impressions_count < ?", low_impression_count(area)).or(where(priority: true)) }
- def self.for_display(area:, user_signed_in:, user_id: nil, article: nil)
+ def self.for_display(area:, user_signed_in:, user_id: nil, article: nil, user_tags: nil)
permit_adjacent = article ? article.permit_adjacent_sponsors? : true
+
ads_for_display = DisplayAds::FilteredAdsQuery.call(
display_ads: self,
area: area,
@@ -68,6 +71,7 @@ class DisplayAd < ApplicationRecord
organization_id: article&.organization_id,
permit_adjacent_sponsors: permit_adjacent,
user_id: user_id,
+ user_tags: user_tags,
)
case rand(99) # output integer from 0-99
diff --git a/app/queries/display_ads/filtered_ads_query.rb b/app/queries/display_ads/filtered_ads_query.rb
index 0b16d63f8..9d6159ede 100644
--- a/app/queries/display_ads/filtered_ads_query.rb
+++ b/app/queries/display_ads/filtered_ads_query.rb
@@ -1,5 +1,6 @@
module DisplayAds
class FilteredAdsQuery
+ include DisplayAdHelper
def self.call(...)
new(...).call
end
@@ -9,7 +10,7 @@ module DisplayAds
# @param display_ads [DisplayAd] can be a filtered scope or Arel relationship
def initialize(area:, user_signed_in:, organization_id: nil, article_tags: [],
permit_adjacent_sponsors: true, article_id: nil, display_ads: DisplayAd,
- user_id: nil)
+ user_id: nil, user_tags: nil)
@filtered_display_ads = display_ads.includes([:organization])
@area = area
@user_signed_in = user_signed_in
@@ -18,24 +19,36 @@ module DisplayAds
@article_tags = article_tags
@article_id = article_id
@permit_adjacent_sponsors = permit_adjacent_sponsors
+ @user_tags = user_tags
end
def call
@filtered_display_ads = approved_and_published_ads
@filtered_display_ads = placement_area_ads
- if @article_tags.any?
- @filtered_display_ads = tagged_post_comment_ads
- end
-
- if @article_tags.blank?
- @filtered_display_ads = untagged_post_comment_ads
- end
-
if @article_id.present?
+ if @article_tags.any?
+ @filtered_display_ads = tagged_ads(@article_tags).or(untagged_ads)
+ end
+
+ if @article_tags.blank?
+ @filtered_display_ads = untagged_ads
+ end
+
@filtered_display_ads = unexcluded_article_ads
end
+ if @user_tags.present? && @user_tags.any?
+ @filtered_display_ads = tagged_ads(@user_tags).or(untagged_ads)
+ end
+
+ # We apply the condition feed_targeted_tag_placement? because we only want to filter by
+ # untagged ads on the home feed area placements. We do not want to have any side effects happen
+ # on the article page or anywhere else.
+ if @user_tags.blank? && feed_targeted_tag_placement?(@area)
+ @filtered_display_ads = untagged_ads
+ end
+
@filtered_display_ads = user_targeting_ads
@filtered_display_ads = if @user_signed_in
@@ -62,12 +75,11 @@ module DisplayAds
@filtered_display_ads.where(placement_area: @area)
end
- def tagged_post_comment_ads
- display_ads_with_targeted_article_tags = @filtered_display_ads.cached_tagged_with_any(@article_tags)
- untagged_post_comment_ads.or(display_ads_with_targeted_article_tags)
+ def tagged_ads(tag_type)
+ @filtered_display_ads.cached_tagged_with_any(tag_type)
end
- def untagged_post_comment_ads
+ def untagged_ads
@filtered_display_ads.where(cached_tag_list: "")
end
diff --git a/cypress/e2e/seededFlows/adminFlows/displayAds/createDisplayAds.spec.js b/cypress/e2e/seededFlows/adminFlows/displayAds/createDisplayAds.spec.js
index e03d5fcaf..c0f4f569b 100644
--- a/cypress/e2e/seededFlows/adminFlows/displayAds/createDisplayAds.spec.js
+++ b/cypress/e2e/seededFlows/adminFlows/displayAds/createDisplayAds.spec.js
@@ -12,40 +12,51 @@ describe('Create Display Ads', () => {
});
});
- it('should not show the tags field if the placement is not one of the post page areas', () => {
- cy.findByRole('combobox', { name: 'Placement Area:' }).select(
+ describe('Targeted Tags field', () => {
+ [
'Sidebar Right (Home)',
- );
- cy.findByRole('input', { name: 'Targeted Tag(s)' }).should('not.exist');
- });
+ 'Sidebar Left (First Position)',
+ 'Sidebar Left (Second Position)',
+ 'Home Hero',
+ ].forEach((area) => {
+ it(`should not show the tags field if the placement is ${area}`, () => {
+ cy.findByRole('combobox', { name: 'Placement Area:' }).select(area);
+ cy.findByRole('input', { name: 'Targeted Tag(s)' }).should(
+ 'not.exist',
+ );
+ });
+ });
- it('should show the tags field if the placement is "Below the comment section"', () => {
- cy.findByRole('combobox', { name: 'Placement Area:' }).select(
+ [
'Below the comment section',
- );
- cy.findByLabelText('Targeted Tag(s)').should('exist');
- });
-
- it('should show the tags field if the placement is "Sidebar Right (Individual Post)"', () => {
- cy.findByRole('combobox', { name: 'Placement Area:' }).select(
'Sidebar Right (Individual Post)',
- );
- cy.findByLabelText('Targeted Tag(s)').should('exist');
+ 'Sidebar Right (Individual Post)',
+ 'Home Feed First',
+ 'Home Feed Second',
+ 'Home Feed Third',
+ ].forEach((area) => {
+ it(`should show the tags field if the placement is ${area}`, () => {
+ cy.findByRole('combobox', { name: 'Placement Area:' }).select(area);
+ cy.findByLabelText('Targeted Tag(s)').should('exist');
+ });
+ });
});
- it('should not show the audience segment field if the display to is logged-out users', () => {
- cy.findByRole('radio', { name: 'Only logged out users' }).click();
- cy.findByLabelText('Users who:').should('not.be.visible');
- });
+ describe('Audience Segment field', () => {
+ it('should not show the audience segment field if the display to is logged-out users', () => {
+ cy.findByRole('radio', { name: 'Only logged out users' }).click();
+ cy.findByLabelText('Users who:').should('not.be.visible');
+ });
- it('should not show the audience segment field if the display to is all users', () => {
- cy.findByRole('radio', { name: 'All users' }).click();
- cy.findByLabelText('Users who:').should('not.be.visible');
- });
+ it('should not show the audience segment field if the display to is all users', () => {
+ cy.findByRole('radio', { name: 'All users' }).click();
+ cy.findByLabelText('Users who:').should('not.be.visible');
+ });
- it('should show the audience segment field if the display to is logged-in users', () => {
- cy.findByRole('radio', { name: 'Only logged in users' }).click();
- cy.findByLabelText('Users who:').should('be.visible');
+ it('should show the audience segment field if the display to is logged-in users', () => {
+ cy.findByRole('radio', { name: 'Only logged in users' }).click();
+ cy.findByLabelText('Users who:').should('be.visible');
+ });
});
});
});
diff --git a/spec/queries/display_ads/filtered_ads_query_spec.rb b/spec/queries/display_ads/filtered_ads_query_spec.rb
index d5ae520ba..86272d2db 100644
--- a/spec/queries/display_ads/filtered_ads_query_spec.rb
+++ b/spec/queries/display_ads/filtered_ads_query_spec.rb
@@ -38,13 +38,13 @@ RSpec.describe DisplayAds::FilteredAdsQuery, type: :query do
let!(:mismatched) { create_display_ad cached_tag_list: "career" }
it "will show no-tag display ads if the article tags do not contain matching tags" do
- filtered = filter_ads(article_tags: %w[javascript])
+ filtered = filter_ads(article_id: 11, article_tags: %w[javascript])
expect(filtered).not_to include(mismatched)
expect(filtered).to include(no_tags)
end
it "will show display ads with no tags set if there are no article tags" do
- filtered = filter_ads(article_tags: [])
+ filtered = filter_ads(article_id: 11, article_tags: [])
expect(filtered).not_to include(mismatched)
expect(filtered).to include(no_tags)
end
@@ -53,7 +53,35 @@ RSpec.describe DisplayAds::FilteredAdsQuery, type: :query do
let!(:matching) { create_display_ad cached_tag_list: "linux, git, go" }
it "will show the display ads that contain tags that match any of the article tags" do
- filtered = filter_ads article_tags: %w[linux productivity]
+ filtered = filter_ads article_id: 11, article_tags: %w[linux productivity]
+ expect(filtered).not_to include(mismatched)
+ expect(filtered).to include(matching)
+ expect(filtered).to include(no_tags)
+ end
+ end
+ end
+
+ context "when considering user_tags" do
+ let!(:no_tags) { create_display_ad placement_area: "feed_first", cached_tag_list: "" }
+ let!(:mismatched) { create_display_ad placement_area: "feed_first", cached_tag_list: "career" }
+
+ it "will show no-tag display ads if the user tags do not contain matching tags" do
+ filtered = filter_ads(area: "feed_first", user_tags: %w[javascript])
+ expect(filtered).not_to include(mismatched)
+ expect(filtered).to include(no_tags)
+ end
+
+ it "will show display ads with no tags set if there are no user tags" do
+ filtered = filter_ads(area: "feed_first", user_tags: [])
+ expect(filtered).not_to include(mismatched)
+ expect(filtered).to include(no_tags)
+ end
+
+ context "when available ads have matching tags" do
+ let!(:matching) { create_display_ad placement_area: "feed_first", cached_tag_list: "linux, git, go" }
+
+ it "will show the display ads that contain tags that match any of the user tags" do
+ filtered = filter_ads area: "feed_first", user_tags: %w[linux productivity]
expect(filtered).not_to include(mismatched)
expect(filtered).to include(matching)
expect(filtered).to include(no_tags)