diff --git a/app/assets/javascripts/initializers/initializeBillboardVisibility.js b/app/assets/javascripts/initializers/initializeBillboardVisibility.js index 8dd180b0f..e5983b8f1 100644 --- a/app/assets/javascripts/initializers/initializeBillboardVisibility.js +++ b/app/assets/javascripts/initializers/initializeBillboardVisibility.js @@ -52,7 +52,10 @@ function trackAdImpression(adBox) { adBox.dataset.impressionRecorded = true; } -function trackAdClick(adBox) { +function trackAdClick(adBox, event) { + if (event && !event.target.closest('a')) { + return; + } var isBot = /bot|google|baidu|bing|msn|duckduckbot|teoma|slurp|yandex/i.test( navigator.userAgent, ); // is crawler @@ -111,7 +114,7 @@ function observeBillboards() { observer.observe(ad); ad.removeEventListener('click', trackAdClick, false); ad.addEventListener('click', function (e) { - trackAdClick(ad); + trackAdClick(ad, e); }); }); } diff --git a/app/controllers/billboard_events_controller.rb b/app/controllers/billboard_events_controller.rb index d8b147b2c..b6c3ded9c 100644 --- a/app/controllers/billboard_events_controller.rb +++ b/app/controllers/billboard_events_controller.rb @@ -1,5 +1,6 @@ class BillboardEventsController < ApplicationMetalController include ActionController::Head + SIGNUP_SUCCESS_MODIFIER = 10 # One signup is worth 10 clicks # No policy needed. All views are for all users def create @@ -22,7 +23,8 @@ class BillboardEventsController < ApplicationMetalController num_impressions = @billboard.billboard_events.impressions.sum(:counts_for) num_clicks = @billboard.billboard_events.clicks.sum(:counts_for) - rate = num_clicks.to_f / num_impressions + signup_success = @billboard.billboard_events.signups.sum(:counts_for) * SIGNUP_SUCCESS_MODIFIER + rate = (num_clicks + signup_success).to_f / num_impressions @billboard.update_columns( success_rate: rate, diff --git a/app/javascript/onboarding/Onboarding.jsx b/app/javascript/onboarding/Onboarding.jsx index 363fbbf90..5ade54918 100644 --- a/app/javascript/onboarding/Onboarding.jsx +++ b/app/javascript/onboarding/Onboarding.jsx @@ -10,6 +10,8 @@ export class Onboarding extends Component { constructor(props) { super(props); + this.recordBillboardConversion(); + const url = new URL(window.location); const previousLocation = url.searchParams.get('referrer'); @@ -59,6 +61,30 @@ export class Onboarding extends Component { } } + recordBillboardConversion() { + if (!localStorage || !localStorage.getItem('last_interacted_billboard')) { + return; + } + if (localStorage.getItem('last_interacted_billboard')) { + const tokenMeta = document.querySelector("meta[name='csrf-token']"); + const csrfToken = tokenMeta && tokenMeta.getAttribute('content'); + const dataBody = JSON.parse( + localStorage.getItem('last_interacted_billboard'), + ); + dataBody['billboard_event']['category'] = 'signup'; + window.fetch('/billboard_events', { + method: 'POST', + headers: { + 'X-CSRF-Token': csrfToken, + 'Content-Type': 'application/json', + }, + body: JSON.stringify(dataBody), + credentials: 'same-origin', + }); + localStorage.removeItem('last_billboard_click_id'); + } + } + // TODO: Update main element id to enable skip link. See issue #1153. render() { const { currentSlide } = this.state; diff --git a/app/javascript/onboarding/__tests__/Onboarding.test.jsx b/app/javascript/onboarding/__tests__/Onboarding.test.jsx index 042b0bc98..f90e8bc8c 100644 --- a/app/javascript/onboarding/__tests__/Onboarding.test.jsx +++ b/app/javascript/onboarding/__tests__/Onboarding.test.jsx @@ -37,6 +37,29 @@ describe('', () => { document.body.setAttribute('data-user', getUserData()); const csrfToken = 'this-is-a-csrf-token'; global.getCsrfToken = async () => csrfToken; + + // Mock localStorage + const localStorageMock = (function () { + let store = {}; + return { + getItem(key) { + return store[key] || null; + }, + setItem(key, value) { + store[key] = value.toString(); + }, + removeItem(key) { + delete store[key]; + }, + clear() { + store = {}; + }, + }; + })(); + + Object.defineProperty(window, 'localStorage', { + value: localStorageMock, + }); }); it('should have no a11y violations', async () => { @@ -45,6 +68,39 @@ describe('', () => { expect(results).toHaveNoViolations(); }); + it('should record billboard conversion correctly', async () => { + const fakeBillboardData = { + billboard_event: { someData: 'test', category: 'signup' }, + }; + window.localStorage.setItem( + 'last_interacted_billboard', + JSON.stringify(fakeBillboardData), + ); + + fetch.mockResponseOnce(() => Promise.resolve(JSON.stringify({}))); // Mock for the billboard event + fetch.mockResponseOnce(() => Promise.resolve(JSON.stringify({}))); // Mock for any subsequent fetch calls, if necessary + + renderOnboarding(); + + await waitFor(() => { + expect(fetch).toHaveBeenCalledWith( + '/billboard_events', + expect.objectContaining({ + method: 'POST', + headers: expect.objectContaining({ + 'Content-Type': 'application/json', + 'X-CSRF-Token': expect.any(String), + }), + body: JSON.stringify(fakeBillboardData), + }), + ); + }); + + // Cleanup + window.localStorage.clear(); + fetch.resetMocks(); + }); + it('should render the ProfileForm first', () => { const { queryByTestId } = renderOnboarding(); diff --git a/app/javascript/packs/billboardAfterRenderActions.js b/app/javascript/packs/billboardAfterRenderActions.js index de505ddc9..8fe3b08f3 100644 --- a/app/javascript/packs/billboardAfterRenderActions.js +++ b/app/javascript/packs/billboardAfterRenderActions.js @@ -43,16 +43,18 @@ export function executeBBScripts(el) { } export function implementSpecialBehavior(element) { - if (element.querySelector('.js-billboard') && element.querySelector('.js-billboard').dataset.special === 'delayed') { - element.classList.add('hidden'); + if ( + element.querySelector('.js-billboard') && + element.querySelector('.js-billboard').dataset.special === 'delayed' + ) { + element.classList.add('hidden'); const observerOptions = { root: null, rootMargin: '-150px', - threshold: 0.2 + threshold: 0.2, }; - + const observer = new IntersectionObserver(showDelayed, observerOptions); - const target = document.getElementById('new_comment'); observer.observe(target); } @@ -87,7 +89,7 @@ export function observeBillboards() { } function showDelayed(entries) { - entries.forEach(entry => { + entries.forEach((entry) => { if (entry.isIntersecting) { // The target element has come into the viewport // Place the behavior you want to trigger here @@ -161,6 +163,10 @@ function trackAdClick(adBox, event) { }, }; + if (localStorage) { + localStorage.setItem('last_interacted_billboard', JSON.stringify(dataBody)); + } + window.fetch('/billboard_events', { method: 'POST', headers: { diff --git a/app/models/billboard_event.rb b/app/models/billboard_event.rb index 234713ce6..286930c5d 100644 --- a/app/models/billboard_event.rb +++ b/app/models/billboard_event.rb @@ -7,13 +7,17 @@ class BillboardEvent < ApplicationRecord # We also have an article_id param, but not belongs_to because it is not indexed and not designed to be # consistently referenced within the application. + validate :unique_on_user_if_signup_conversion, on: :create + validate :only_recent_registrations, on: :create + self.table_name = "display_ad_events" alias_attribute :billboard_id, :display_ad_id CATEGORY_IMPRESSION = "impression".freeze CATEGORY_CLICK = "click".freeze - VALID_CATEGORIES = [CATEGORY_CLICK, CATEGORY_IMPRESSION].freeze + CATEGORY_SIGNUP = "signup".freeze + VALID_CATEGORIES = [CATEGORY_CLICK, CATEGORY_IMPRESSION, CATEGORY_SIGNUP].freeze CONTEXT_TYPE_HOME = "home".freeze CONTEXT_TYPE_ARTICLE = "article".freeze @@ -24,4 +28,19 @@ class BillboardEvent < ApplicationRecord scope :impressions, -> { where(category: CATEGORY_IMPRESSION) } scope :clicks, -> { where(category: CATEGORY_CLICK) } + scope :signups, -> { where(category: CATEGORY_SIGNUP) } + + def unique_on_user_if_signup_conversion + return unless category == CATEGORY_SIGNUP && user_id.present? + return unless self.class.exists?(user_id: user_id, category: CATEGORY_SIGNUP) + + errors.add(:user_id, "has already converted a signup") + end + + def only_recent_registrations + return unless category == CATEGORY_SIGNUP && user_id.present? + return unless user.registered_at < 1.day.ago + + errors.add(:user_id, "is not a recent registration") + end end diff --git a/spec/models/billboard_event_spec.rb b/spec/models/billboard_event_spec.rb index f390ce6ac..a19eb6ecb 100644 --- a/spec/models/billboard_event_spec.rb +++ b/spec/models/billboard_event_spec.rb @@ -4,4 +4,37 @@ RSpec.describe BillboardEvent do it { is_expected.to validate_inclusion_of(:category).in_array(described_class::VALID_CATEGORIES) } it { is_expected.to validate_inclusion_of(:context_type).in_array(described_class::VALID_CONTEXT_TYPES) } + + describe "#unique_on_user_if_signup_conversion" do + let(:user) { create(:user) } + let(:billboard_event) { build(:billboard_event, category: "signup", user: user) } + + it "adds an error if user has already converted a signup" do + create(:billboard_event, category: "signup", user: user) + billboard_event.valid? + expect(billboard_event.errors[:user_id]).to include("has already converted a signup") + end + + it "does not add an error if not a signup" do + billboard_event.category = "click" + billboard_event.valid? + expect(billboard_event.errors[:user_id]).to be_empty + end + end + + describe "#only_recent_registrations" do + let(:user) { create(:user, registered_at: 2.days.ago) } + let(:billboard_event) { build(:billboard_event, category: "signup", user: user) } + + it "adds an error if user is not a recent registration" do + billboard_event.valid? + expect(billboard_event.errors[:user_id]).to include("is not a recent registration") + end + + it "does not add an error if user is a recent registration" do + user.update(registered_at: 23.hours.ago) + billboard_event.valid? + expect(billboard_event.errors[:user_id]).to be_empty + end + end end diff --git a/spec/requests/billboard_events_spec.rb b/spec/requests/billboard_events_spec.rb index 3affbc0c4..dced57cdf 100644 --- a/spec/requests/billboard_events_spec.rb +++ b/spec/requests/billboard_events_spec.rb @@ -57,6 +57,22 @@ RSpec.describe "BillboardEvents" do expect(billboard.reload.success_rate).to eq(0.25) end + it "accounts for signups in the success rate" do + ad_event_params = { billboard_id: billboard.id, context_type: BillboardEvent::CONTEXT_TYPE_HOME } + impression_params = ad_event_params.merge(category: BillboardEvent::CATEGORY_IMPRESSION, user: user) + click_params = ad_event_params.merge(category: BillboardEvent::CATEGORY_CLICK, user: user) + create_list(:billboard_event, 3, click_params) + create_list(:billboard_event, 4, impression_params) + + post( + "/billboard_events", + params: { billboard_event: ad_event_params.merge(category: BillboardEvent::CATEGORY_SIGNUP) }, + ) + + # 13 / 4 = 3.25 because 3 clicks + (1 signup * 10) / 4 impressions + expect(billboard.reload.success_rate).to eq(3.25) + end + it "assigns event to current user" do post "/billboard_events", params: { billboard_event: {