Skip to content

Commit

Permalink
Add browser/device context to billboards (forem#20953)
Browse files Browse the repository at this point in the history
* Add browser context to billboards

* Run migrations

* Add key

* Add key

* Modify spec

* Update cypress

* Update cypress
  • Loading branch information
benhalpern authored May 23, 2024
1 parent 426668e commit fe1ffa8
Show file tree
Hide file tree
Showing 12 changed files with 97 additions and 46 deletions.
2 changes: 1 addition & 1 deletion app/controllers/admin/billboards_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def destroy
def billboard_params
params.permit(:organization_id, :body_markdown, :placement_area, :target_geolocations,
:published, :approved, :name, :display_to, :tag_list, :type_of,
:exclude_article_ids, :audience_segment_id, :priority,
:exclude_article_ids, :audience_segment_id, :priority, :browser_context,
:render_mode, :template, :custom_display_label, :requires_cookies)
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/billboards_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def require_admin
end

def permitted_params
params.permit :approved, :body_markdown, :creator_id, :display_to, :page_id,
params.permit :approved, :body_markdown, :creator_id, :display_to, :page_id, :browser_context,
:name, :organization_id, :placement_area, :published, :dismissal_sku,
:tag_list, :type_of, :exclude_article_ids, :weight, :requires_cookies,
:audience_segment_type, :audience_segment_id, :priority, :special_behavior,
Expand Down
1 change: 1 addition & 0 deletions app/controllers/billboards_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ def show
user_tags: user_tags,
cookies_allowed: cookies_allowed?,
location: client_geolocation,
user_agent: request.user_agent,
)

if @billboard && !session_current_user_id
Expand Down
36 changes: 26 additions & 10 deletions app/javascript/packs/admin/billboards.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function saveTags(selectionString) {
/**
* Shows and Renders a Tags preact component for the Targeted Tag(s) field
*/
function showTagsField() {
function showPrecisionFields() {
const billboardsTargetedTags = document.getElementById(
'billboard-targeted-tags',
);
Expand All @@ -35,17 +35,33 @@ function showTagsField() {
billboardsTargetedTags,
);
}

const billboardPrecisionElements = document.querySelectorAll(
'.billboard-requires-precision-targeting',
);

billboardPrecisionElements.forEach((element) => {
element?.classList.remove('hidden');
});
}

/**
* Hides the Targeted Tag(s) field
*/
function hideTagsField() {
function hidePrecisionFields() {
const billboardsTargetedTags = document.getElementById(
'billboard-targeted-tags',
);

billboardsTargetedTags?.classList.add('hidden');

const billboardPrecisionElements = document.querySelectorAll(
'.billboard-requires-precision-targeting',
);

billboardPrecisionElements.forEach((element) => {
element?.classList.add('hidden');
});
}

/**
Expand Down Expand Up @@ -140,32 +156,32 @@ function clearExcludeIds() {
*/
document.ready.then(() => {
const select = document.getElementsByClassName('js-placement-area')[0];
const articleSpecificPlacement = ['post_comments', 'post_sidebar', 'post_fixed_bottom'];
const articleSpecificPlacement = [
'post_comments',
'post_sidebar',
'post_fixed_bottom',
];
const targetedTagPlacements = [
'post_fixed_bottom',
'post_body_bottom',
'post_comments',
'post_comments_mid',
'post_sidebar',
'sidebar_right',
'sidebar_right_second',
'sidebar_right_third',
'feed_first',
'feed_second',
'feed_third',
'digest_first',
'digest_second',
];

if (targetedTagPlacements.includes(select.value)) {
showTagsField();
showPrecisionFields();
}

select.addEventListener('change', (event) => {
if (targetedTagPlacements.includes(event.target.value)) {
showTagsField();
showPrecisionFields();
} else {
hideTagsField();
hidePrecisionFields();
clearTagList();
}
});
Expand Down
4 changes: 3 additions & 1 deletion app/models/billboard.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class Billboard < ApplicationRecord
enum render_mode: { forem_markdown: 0, raw: 1 }
enum template: { authorship_box: 0, plain: 1 }
enum :special_behavior, { nothing: 0, delayed: 1 }
enum :browser_context, { all_browsers: 0, desktop: 1, mobile_web: 2, mobile_in_app: 3 }

belongs_to :organization, optional: true
has_many :billboard_events, foreign_key: :display_ad_id, inverse_of: :billboard, dependent: :destroy
Expand Down Expand Up @@ -90,7 +91,7 @@ class Billboard < ApplicationRecord
self.table_name = "display_ads"

def self.for_display(area:, user_signed_in:, user_id: nil, article: nil, user_tags: nil,
location: nil, cookies_allowed: false, page_id: nil)
location: nil, cookies_allowed: false, page_id: nil, user_agent: nil)
permit_adjacent = article ? article.permit_adjacent_sponsors? : true

billboards_for_display = Billboards::FilteredAdsQuery.call(
Expand All @@ -106,6 +107,7 @@ def self.for_display(area:, user_signed_in:, user_id: nil, article: nil, user_ta
user_tags: user_tags,
location: location,
cookies_allowed: cookies_allowed,
user_agent: user_agent,
)

case rand(99) # output integer from 0-99
Expand Down
17 changes: 16 additions & 1 deletion app/queries/billboards/filtered_ads_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def self.call(...)
# @param location [Geolocation|String] the visitor's geographic location
def initialize(area:, user_signed_in:, organization_id: nil, article_tags: [], page_id: nil,
permit_adjacent_sponsors: true, article_id: nil, billboards: Billboard,
user_id: nil, user_tags: nil, location: nil, cookies_allowed: false)
user_id: nil, user_tags: nil, location: nil, cookies_allowed: false, user_agent: nil)
@filtered_billboards = billboards.includes([:organization])
@area = area
@user_signed_in = user_signed_in
Expand All @@ -22,13 +22,15 @@ def initialize(area:, user_signed_in:, organization_id: nil, article_tags: [], p
@article_id = article_id
@permit_adjacent_sponsors = permit_adjacent_sponsors
@user_tags = user_tags
@user_agent = user_agent
@location = Geolocation.from_iso3166(location)
@cookies_allowed = cookies_allowed
end

def call
@filtered_billboards = approved_and_published_ads
@filtered_billboards = placement_area_ads
@filtered_billboards = browser_context_ads if @user_agent.present?
@filtered_billboards = page_ads if @page_id.present?
@filtered_billboards = cookies_allowed_ads unless @cookies_allowed

Expand Down Expand Up @@ -85,6 +87,19 @@ def placement_area_ads
@filtered_billboards.where(placement_area: @area)
end

def browser_context_ads
case @user_agent
when /DEV-Native-ios|DEV-Native-android|ForemWebView/
@filtered_billboards.where(browser_context: %i[all_browsers mobile_in_app])
when /Mobile|iPhone|Android/
@filtered_billboards.where(browser_context: %i[all_browsers mobile_web])
when /Windows|Macintosh|Mac OS X|Linux/
@filtered_billboards.where(browser_context: %i[all_browsers desktop])
else
@filtered_billboards
end
end

def page_ads
@filtered_billboards.where(page_id: @page_id)
end
Expand Down
7 changes: 6 additions & 1 deletion app/views/admin/billboards/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,17 @@
<%= select_tag :placement_area, options_for_select(billboards_placement_area_options_array, selected: @billboard.placement_area), include_blank: "Select...", class: "crayons-select js-placement-area" %>
</div>

<div class="crayons-field billboard-requires-precision-targeting hidden">
<%= label_tag :browser_context, "Browser context:", class: "crayons-field__label" %>
<%= select_tag :browser_context, options_for_select([["All browsers", "all_browsers"], ["Desktop", "desktop"], ["Mobile web", "mobile_web"], ["Mobile in-app", "mobile_in_app"]], selected: @billboard.browser_context), class: "crayons-select js-placement-area" %>
</div>

<div class="crayons-field">
<div id="billboard-targeted-tags"></div>
</div>

<% if FeatureFlag.enabled?(Geolocation::FEATURE_FLAG) %>
<div class="crayons-field">
<div class="crayons-field billboard-requires-precision-targeting hidden">
<%= label_tag :target_geolocations, "Target Geolocations:", class: "crayons-field__label" %>
<%= text_field_tag :target_geolocations, @billboard.target_geolocations.map(&:to_iso3166).join(", "), class: "crayons-textfield", placeholder: "US-NY, CA-ON", autocomplete: "off" %>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,34 +132,6 @@ describe('Billboards Form', () => {
.should('exist');
cy.get('input[placeholder="US-NY, CA-ON"]').should('exist');
});

it('should not return iso3166 errors if given valid geolocation code inputs', () => {
cy.findByRole('textbox', { name: 'Target Geolocations:' }).type(
'CA-ON, US-OH, US-MI',
);
cy.findByRole('button', { name: 'Save Billboard' }).click();
cy.get('#flash-0').should(($flashMessage) => {
expect($flashMessage).to.not.contain(
'is not an enabled target ISO 3166-2 code',
);
});
});

it('should generate errors if some or all of the input is invalid', () => {
cy.findByRole('textbox', { name: 'Target Geolocations:' }).type(
'US-NY, MX-CMX',
);
cy.findByRole('button', { name: 'Save Billboard' }).click();
cy.get('#flash-0').should(($flashMessage) => {
// We currently support only the US and CA
expect($flashMessage).to.contain(
'MX-CMX is not an enabled target ISO 3166-2 code',
);
expect($flashMessage).to.not.contain(
'US-NY is not an enabled target ISO 3166-2 code',
);
});
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddBrowserContextToBillboards < ActiveRecord::Migration[7.0]
def change
add_column :display_ads, :browser_context, :integer, default: 0, null: false
end
end
1 change: 1 addition & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@
t.boolean "approved", default: false
t.integer "audience_segment_id"
t.text "body_markdown"
t.integer "browser_context", default: 0, null: false
t.string "cached_tag_list"
t.integer "clicks_count", default: 0
t.datetime "created_at", precision: nil, null: false
Expand Down
34 changes: 34 additions & 0 deletions spec/queries/billboards/filtered_ads_query_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -351,4 +351,38 @@ def filter_billboards(**options)
end
end
end

context "when considering browser context" do
let!(:all_browsers_ad) { create_billboard browser_context: :all_browsers }
let!(:mobile_in_app_ad) { create_billboard browser_context: :mobile_in_app }
let!(:mobile_web_ad) { create_billboard browser_context: :mobile_web }
let!(:desktop_ad) { create_billboard browser_context: :desktop }

it "filters ads based on user_agent string for mobile in-app context" do
filtered = filter_billboards(user_agent: "DEV-Native-ios")
expect(filtered).to include(all_browsers_ad, mobile_in_app_ad)
expect(filtered).not_to include(mobile_web_ad, desktop_ad)

filtered = filter_billboards(user_agent: "DEV-Native-android")
expect(filtered).to include(all_browsers_ad, mobile_in_app_ad)
expect(filtered).not_to include(mobile_web_ad, desktop_ad)
end

it "filters ads based on user_agent string for mobile web context" do
filtered = filter_billboards(user_agent: "Mobile Safari")
expect(filtered).to include(all_browsers_ad, mobile_web_ad)
expect(filtered).not_to include(mobile_in_app_ad, desktop_ad)
end

it "filters ads based on user_agent string for desktop context" do
filtered = filter_billboards(user_agent: "Windows NT 10.0; Win64; x64")
expect(filtered).to include(all_browsers_ad, desktop_ad)
expect(filtered).not_to include(mobile_in_app_ad, mobile_web_ad)
end

it "includes all ads for unknown user_agent contexts" do
filtered = filter_billboards(user_agent: "SomeUnknownBrowser/1.0")
expect(filtered).to include(all_browsers_ad, mobile_in_app_ad, mobile_web_ad, desktop_ad)
end
end
end
6 changes: 3 additions & 3 deletions spec/requests/api/v1/billboards_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
"impressions_count", "name", "organization_id",
"placement_area", "processed_html", "published",
"success_rate", "tag_list", "type_of", "updated_at",
"creator_id", "exclude_article_ids", "dismissal_sku",
"creator_id", "exclude_article_ids", "dismissal_sku", "browser_context",
"audience_segment_type", "audience_segment_id", "page_id",
"custom_display_label", "template", "render_mode", "preferred_article_ids",
"priority", "weight", "target_geolocations", "requires_cookies", "special_behavior")
Expand All @@ -71,7 +71,7 @@
"impressions_count", "name", "organization_id",
"placement_area", "processed_html", "published",
"success_rate", "tag_list", "type_of", "updated_at",
"creator_id", "exclude_article_ids", "dismissal_sku",
"creator_id", "exclude_article_ids", "dismissal_sku", "browser_context",
"audience_segment_type", "audience_segment_id", "page_id",
"custom_display_label", "template", "render_mode", "preferred_article_ids",
"priority", "weight", "target_geolocations", "requires_cookies", "special_behavior")
Expand Down Expand Up @@ -136,7 +136,7 @@
"clicks_count", "created_at", "display_to", "id",
"impressions_count", "name", "organization_id",
"placement_area", "processed_html", "published", "dismissal_sku",
"success_rate", "tag_list", "type_of", "updated_at",
"success_rate", "tag_list", "type_of", "updated_at", "browser_context",
"creator_id", "exclude_article_ids", "requires_cookies", "page_id",
"audience_segment_type", "audience_segment_id", "special_behavior",
"custom_display_label", "template", "render_mode", "preferred_article_ids",
Expand Down

0 comments on commit fe1ffa8

Please sign in to comment.