maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Refactor the home feed + add user tag filtering on in-feed billboards (#19600)

Browse source 
* feat: fix of there is no article

* feat: set the feed order in a data structure rather than in a view

* feat: update yarn.lock

* feat: push the items to the array

* feat: update feed to only show billboards if we have the correct length of items in the feed

* fix: organizedFeedItems.legth

* feat: rename 'featured' to 'image'

* feat: rename 'featured' to 'image'

* feat: rename 'featured' to 'image'

* feat: add some utilities for the feed that we can use in the FeedTest

* feat/WIP: the setup for the feed tests and a first working test

* test: imageItem

* fix: the podcasts can be passed through as an array of objects in the feedItems so that they can be grouped in one card

* feat: remove podcastEpisode state and the attribute in the object

* fix: check that the items exist before trying to slice them

* feat: setup the userdata and the podcast data in the test

* whoops - commit the podcast episodes

* feat: write soem more tests for the feed and including the podcasts

* feat: add some more tests

* feat: add more billboards tests to chcek the order of stuff

* feat: set the timeframe not empty

* feat: update the logic for organizaed feed by inserting the last on first

* doc: jsdoc for functions

* refactor: break the code up into smaller functions

* refactor: make the code more readable and easier to follow

* refactor: pull function out into a utlity

* feat: add specs to utility

* feat: chcek if pinned post

* test the latest timeframe correctly

* chore: update var name

* move the podcast items out of the object clause

* feat: update text

* feat: add a pack file that duplicates initializeDisplayAdVisibility

* feat: create callbacks that will help us to determine when the feed has been rendered so that we can observe the dsplay ads accordingly

* chore: rename to billboards instead of display ad

* feat: abstract out a function that will work for any tagged resource and also limit the article tags within a scope of the article_id

* feat: update the spec for the previous changes

* feat: write tests for the user_tags

* feat: add user_tags to for_display adn the query

* feat: add user_tags to the endpoint where we query the tags to send it through to for_display

* feat: pass through the suer tags to the async request

* feat: update the user_tags fiter query + tests

* feat: update the admin view to show targeted tags on the new feed option billboards too

* feat: add tests for targeted tags field on admin

* refactor: consolidate all of the options into one

* feat: move comment close to attribute

* feat: use a helper method

* feat: include helper on ads query

* feat: move the query from the frontend to live on the backend

* feat: first pass at some error handling whilst maintaining the order of the items

* test: for feed error

* feat: abstract out some code

* update the feed items for errors

* chore: comment

* feat: update the variable names

* feat: update honeybadger message

* feat: update the name of the variable

* refactor: do not conflate the duty of the untagged_ads

* refactor: rename the variables + add note for clarity

* fix: update var
This commit is contained in:
Ridhwana 2023-07-06 16:47:08 +02:00 • committed by GitHub
parent 5410f6f92c
commit ccacf0bc3a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 159 additions and 95 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
app/controllers/display_ads_controller.rb
View file

@ -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

10
app/helpers/display_ad_helper.rb
View file

@ -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

26
app/javascript/packs/admin/displayAds.jsx
View file

@ -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();
}

59
app/javascript/packs/homePageFeed.jsx
View file

@ -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 (
<Article
{...commonProps}
key={item.id}
article={pinnedItem}
pinned={true}
feedStyle={feedStyle}
isBookmarked={bookmarkedFeedItems.has(pinnedItem?.id)}
saveable={pinnedItem.user_id != currentUserId}
/>
);
}
if (item.id === imageItem?.id) {
return (
<Article
{...commonProps}
key={item.id}
article={imageItem}
isFeatured
feedStyle={feedStyle}
isBookmarked={bookmarkedFeedItems.has(imageItem.id)}
saveable={imageItem.user_id != currentUserId}
/>
);
}
if (item.class_name === 'Article') {
return (
<Article
{...commonProps}
key={item.id}
article={item}
feedStyle={feedStyle}
isBookmarked={bookmarkedFeedItems.has(item.id)}
saveable={item.user_id != currentUserId}
/>
);
}
return (
<Article
{...commonProps}
key={item.id}
article={item}
pinned={item.id === pinnedItem?.id}
isFeatured={item.id === imageItem?.id}
feedStyle={feedStyle}
isBookmarked={bookmarkedFeedItems.has(item.id)}
saveable={item.user_id != currentUserId}
// For "saveable" props, "!=" is used instead of "!==" to compare user_id
// and currentUserId because currentUserId is a String while user_id is an Integer
/>
);
}
});
}
@ -187,7 +157,6 @@ export const renderFeed = async (timeFrame, afterRender) => {
bookmarkedFeedItems,
bookmarkClick,
currentUserId,
timeFrame,
)}
</div>
);

6
app/models/display_ad.rb
View file

@ -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

38
app/queries/display_ads/filtered_ads_query.rb
View file

@ -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

63
cypress/e2e/seededFlows/adminFlows/displayAds/createDisplayAds.spec.js
View file

@ -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');
});
});
});
});

34
spec/queries/display_ads/filtered_ads_query_spec.rb
View file

@ -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)