diff --git a/app/controllers/billboard_events_controller.rb b/app/controllers/billboard_events_controller.rb index 228cb087d..909f5b9af 100644 --- a/app/controllers/billboard_events_controller.rb +++ b/app/controllers/billboard_events_controller.rb @@ -5,7 +5,7 @@ class BillboardEventsController < ApplicationMetalController def create # Only tracking for logged in users at the moment billboard_event_create_params = billboard_event_params.merge(user_id: session_current_user_id) - @billboard_event = DisplayAdEvent.create(billboard_event_create_params) + @billboard_event = BillboardEvent.create(billboard_event_create_params) update_billboards_data @@ -35,8 +35,8 @@ class BillboardEventsController < ApplicationMetalController def billboard_event_params event_params = params[:billboard_event] || params[:display_ad_event] # keeping while we may receive data in the "old" format from cached js - display_ad_id = event_params.delete(:display_ad_id) - event_params[:billboard_id] ||= display_ad_id + billboard_id = event_params.delete(:display_ad_id) + event_params[:billboard_id] ||= billboard_id event_params.slice(:context_type, :category, :billboard_id) end end diff --git a/app/models/display_ad_event.rb b/app/models/billboard_event.rb similarity index 91% rename from app/models/display_ad_event.rb rename to app/models/billboard_event.rb index 06d24891c..51c94a23d 100644 --- a/app/models/display_ad_event.rb +++ b/app/models/billboard_event.rb @@ -1,10 +1,12 @@ # @note When we destroy the related user, it's using dependent: # :delete for the relationship. That means no before/after # destroy callbacks will be called on this object. -class DisplayAdEvent < ApplicationRecord +class BillboardEvent < ApplicationRecord belongs_to :billboard, class_name: "DisplayAd", foreign_key: :display_ad_id, inverse_of: :billboard_events belongs_to :user, optional: true + self.table_name = "display_ad_events" + alias_attribute :billboard_id, :display_ad_id CATEGORY_IMPRESSION = "impression".freeze diff --git a/app/models/display_ad.rb b/app/models/display_ad.rb index 4a797e642..9b410774d 100644 --- a/app/models/display_ad.rb +++ b/app/models/display_ad.rb @@ -31,7 +31,7 @@ class DisplayAd < ApplicationRecord enum type_of: { in_house: 0, community: 1, external: 2 } belongs_to :organization, optional: true - has_many :billboard_events, class_name: "DisplayAdEvent", dependent: :destroy + has_many :billboard_events, dependent: :destroy validates :placement_area, presence: true, inclusion: { in: ALLOWED_PLACEMENT_AREAS } diff --git a/app/models/user.rb b/app/models/user.rb index 5ca5cc3e1..1000700e0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -70,7 +70,7 @@ class User < ApplicationRecord has_many :created_podcasts, class_name: "Podcast", foreign_key: :creator_id, inverse_of: :creator, dependent: :nullify has_many :credits, dependent: :destroy has_many :discussion_locks, dependent: :delete_all, inverse_of: :locking_user, foreign_key: :locking_user_id - has_many :billboard_events, class_name: "DisplayAdEvent", dependent: :nullify + has_many :billboard_events, dependent: :nullify has_many :email_authorizations, dependent: :delete_all has_many :email_messages, class_name: "Ahoy::Message", dependent: :destroy has_many :field_test_memberships, class_name: "FieldTest::Membership", as: :participant, dependent: :destroy diff --git a/app/services/billboard_event_rollup.rb b/app/services/billboard_event_rollup.rb index 49494011a..35f6d1d16 100644 --- a/app/services/billboard_event_rollup.rb +++ b/app/services/billboard_event_rollup.rb @@ -44,7 +44,7 @@ class BillboardEventRollup attr_reader :aggregator end - def self.rollup(date, relation: DisplayAdEvent) + def self.rollup(date, relation: BillboardEvent) new(relation: relation).rollup(date) end diff --git a/app/views/articles/index.html.erb b/app/views/articles/index.html.erb index 4919097dd..56932cead 100644 --- a/app/views/articles/index.html.erb +++ b/app/views/articles/index.html.erb @@ -27,7 +27,7 @@ <%= javascript_packs_with_chunks_tag "homePage", defer: true %> <% cache(release_adjusted_cache_key("main-stories-index-#{params}-#{user_signed_in?}"), expires_in: 90.seconds) do %> <% if @hero_display_ad && FeatureFlag.enabled?(:hero_billboard) %> - <%= render partial: "shared/display_ad", locals: { display_ad: @hero_display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %> + <%= render partial: "shared/display_ad", locals: { display_ad: @hero_display_ad, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %> <% end %>
" data-which="<%= @list_of %>" diff --git a/app/views/billboards/show.html.erb b/app/views/billboards/show.html.erb index e7d3bcdb4..8b3878676 100644 --- a/app/views/billboards/show.html.erb +++ b/app/views/billboards/show.html.erb @@ -1,9 +1,9 @@ <% if @display_ad %> <% if user_signed_in? %> - <%= 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: BillboardEvent::CONTEXT_TYPE_ARTICLE } %> <% else %> <% cache([params[:username], params[:slug], params[:placement_area]], expires_in: 15.minutes) do %> - <%= 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: BillboardEvent::CONTEXT_TYPE_ARTICLE } %> <% end %> <% end %> <% end %> diff --git a/app/views/shared/_display_ad.html.erb b/app/views/shared/_display_ad.html.erb index 3cde13b4b..e5bec2c63 100644 --- a/app/views/shared/_display_ad.html.erb +++ b/app/views/shared/_display_ad.html.erb @@ -1,8 +1,8 @@ <% if display_ad.placement_area.start_with?("feed_") %>
@@ -19,8 +19,8 @@ <% elsif display_ad.placement_area == "home_hero" %>
<%= render partial: "shared/display_ad_header", locals: { display_ad: display_ad } %> @@ -31,8 +31,8 @@ <% else %>
<%= render partial: "shared/display_ad_header", locals: { display_ad: display_ad } %> diff --git a/app/views/sidebars/_homepage_content.html.erb b/app/views/sidebars/_homepage_content.html.erb index 7fba92426..763a2a569 100644 --- a/app/views/sidebars/_homepage_content.html.erb +++ b/app/views/sidebars/_homepage_content.html.erb @@ -2,7 +2,7 @@ <% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}-#{user_signed_in?}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %> <% @sidebar_ad = DisplayAd.for_display(area: "sidebar_right", user_signed_in: user_signed_in?) %> <% if @sidebar_ad %> - <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %> + <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %> <% end %> <%= render "articles/sidebar_campaign" if Campaign.current.show_in_sidebar? %> diff --git a/app/views/stories/_main_stories_feed.html.erb b/app/views/stories/_main_stories_feed.html.erb index 3e35f0dd4..545cf33c8 100644 --- a/app/views/stories/_main_stories_feed.html.erb +++ b/app/views/stories/_main_stories_feed.html.erb @@ -10,7 +10,7 @@ <% if !user_signed_in? %> <% @sidebar_ad_first = DisplayAd.for_display(area: "feed_first", user_signed_in: user_signed_in?) %> <% if @sidebar_ad_first %> - <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_first, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %> + <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_first, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %> <% end %> <% end %> @@ -38,14 +38,14 @@ <% if !user_signed_in? && i == second_display_ad_position %> <% @sidebar_ad_second = DisplayAd.for_display(area: "feed_second", user_signed_in: user_signed_in?) %> <% if @sidebar_ad_second %> - <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_second, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %> + <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_second, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %> <% end %> <% end %> <% if !user_signed_in? && i == third_display_ad_position %> <% @sidebar_ad_third = DisplayAd.for_display(area: "feed_third", user_signed_in: user_signed_in?) %> <% if @sidebar_ad_third %> - <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_third, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %> + <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_third, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %> <% end %> <% end %> <% if story.id != @featured_story.id && story.id != @pinned_article&.id %> diff --git a/spec/factories/display_ad_events.rb b/spec/factories/billboard_events.rb similarity index 56% rename from spec/factories/display_ad_events.rb rename to spec/factories/billboard_events.rb index ccc393d71..d9279465d 100644 --- a/spec/factories/display_ad_events.rb +++ b/spec/factories/billboard_events.rb @@ -1,6 +1,6 @@ FactoryBot.define do - factory :display_ad_event do - category { DisplayAdEvent::CATEGORY_IMPRESSION } + factory :billboard_event do + category { BillboardEvent::CATEGORY_IMPRESSION } context_type { "home" } association :billboard, factory: :display_ad end diff --git a/spec/models/display_ad_event_spec.rb b/spec/models/billboard_event_spec.rb similarity index 87% rename from spec/models/display_ad_event_spec.rb rename to spec/models/billboard_event_spec.rb index 7dafb13c7..f390ce6ac 100644 --- a/spec/models/display_ad_event_spec.rb +++ b/spec/models/billboard_event_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe DisplayAdEvent do +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) } diff --git a/spec/requests/billboard_events_spec.rb b/spec/requests/billboard_events_spec.rb index a149ff8f0..f9377ef69 100644 --- a/spec/requests/billboard_events_spec.rb +++ b/spec/requests/billboard_events_spec.rb @@ -15,8 +15,8 @@ RSpec.describe "BillboardEvents" do post "/billboard_events", params: { billboard_event: { billboard_id: display_ad.id, - context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, - category: DisplayAdEvent::CATEGORY_CLICK + context_type: BillboardEvent::CONTEXT_TYPE_HOME, + category: BillboardEvent::CATEGORY_CLICK } } expect(display_ad.reload.clicks_count).to eq(1) @@ -26,8 +26,8 @@ RSpec.describe "BillboardEvents" do post "/billboard_events", params: { display_ad_event: { display_ad_id: display_ad.id, - context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, - category: DisplayAdEvent::CATEGORY_CLICK + context_type: BillboardEvent::CONTEXT_TYPE_HOME, + category: BillboardEvent::CATEGORY_CLICK } } expect(display_ad.reload.clicks_count).to eq(1) @@ -37,21 +37,21 @@ RSpec.describe "BillboardEvents" do post "/billboard_events", params: { billboard_event: { billboard_id: display_ad.id, - context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, - category: DisplayAdEvent::CATEGORY_IMPRESSION + context_type: BillboardEvent::CONTEXT_TYPE_HOME, + category: BillboardEvent::CATEGORY_IMPRESSION } } expect(display_ad.reload.impressions_count).to eq(1) end it "creates a display ad success rate" do - ad_event_params = { billboard_id: display_ad.id, context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } - impression_params = ad_event_params.merge(category: DisplayAdEvent::CATEGORY_IMPRESSION, user: user) - create_list(:display_ad_event, 4, impression_params) + ad_event_params = { billboard_id: display_ad.id, context_type: BillboardEvent::CONTEXT_TYPE_HOME } + impression_params = ad_event_params.merge(category: BillboardEvent::CATEGORY_IMPRESSION, user: user) + create_list(:billboard_event, 4, impression_params) post( "/billboard_events", - params: { billboard_event: ad_event_params.merge(category: DisplayAdEvent::CATEGORY_CLICK) }, + params: { billboard_event: ad_event_params.merge(category: BillboardEvent::CATEGORY_CLICK) }, ) expect(display_ad.reload.success_rate).to eq(0.25) @@ -61,19 +61,19 @@ RSpec.describe "BillboardEvents" do post "/billboard_events", params: { billboard_event: { billboard_id: display_ad.id, - context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, - category: DisplayAdEvent::CATEGORY_IMPRESSION + context_type: BillboardEvent::CONTEXT_TYPE_HOME, + category: BillboardEvent::CATEGORY_IMPRESSION } } - expect(DisplayAdEvent.last.user_id).to eq(user.id) + expect(BillboardEvent.last.user_id).to eq(user.id) end it "uses a ThrottledCall for data updates" do post "/billboard_events", params: { billboard_event: { billboard_id: display_ad.id, - context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, - category: DisplayAdEvent::CATEGORY_IMPRESSION + context_type: BillboardEvent::CONTEXT_TYPE_HOME, + category: BillboardEvent::CATEGORY_IMPRESSION } } diff --git a/spec/services/billboard_event_rollup_spec.rb b/spec/services/billboard_event_rollup_spec.rb index 5211fd120..bf4da6672 100644 --- a/spec/services/billboard_event_rollup_spec.rb +++ b/spec/services/billboard_event_rollup_spec.rb @@ -7,9 +7,9 @@ RSpec.describe BillboardEventRollup, type: :service do let(:user2) { create(:user) } def override_timestamps - DisplayAdEvent.record_timestamps = false + BillboardEvent.record_timestamps = false yield - DisplayAdEvent.record_timestamps = true + BillboardEvent.record_timestamps = true end def days_ago_as_range(num) @@ -18,24 +18,24 @@ RSpec.describe BillboardEventRollup, type: :service do it "fails if new attributes would be lost" do attributes_considered = described_class::ATTRIBUTES_PRESERVED + described_class::ATTRIBUTES_DESTROYED - expect(DisplayAdEvent.column_names.map(&:to_sym)).to match_array(attributes_considered) + expect(BillboardEvent.column_names.map(&:to_sym)).to match_array(attributes_considered) end context "when compacting many rows" do before do override_timestamps do - create(:display_ad_event, created_at: Date.current - 2, billboard: ad1, user_id: nil, updated_at: Date.current) - create(:display_ad_event, created_at: Date.current - 2, billboard: ad1, user_id: nil, updated_at: Date.current) - create(:display_ad_event, created_at: Date.current - 2, billboard: ad1, user_id: nil, updated_at: Date.current) + create(:billboard_event, created_at: Date.current - 2, billboard: ad1, user_id: nil, updated_at: Date.current) + create(:billboard_event, created_at: Date.current - 2, billboard: ad1, user_id: nil, updated_at: Date.current) + create(:billboard_event, created_at: Date.current - 2, billboard: ad1, user_id: nil, updated_at: Date.current) - create(:display_ad_event, created_at: Date.current - 2, billboard: ad1, user_id: user1.id, - updated_at: Date.current) - create(:display_ad_event, created_at: Date.current - 2, billboard: ad2, user_id: nil, updated_at: Date.current) + create(:billboard_event, created_at: Date.current - 2, billboard: ad1, user_id: user1.id, + updated_at: Date.current) + create(:billboard_event, created_at: Date.current - 2, billboard: ad2, user_id: nil, updated_at: Date.current) end end it "compacts one day's billboard_events" do - expect(DisplayAdEvent.where(created_at: days_ago_as_range(2)).count).to eq(5) + expect(BillboardEvent.where(created_at: days_ago_as_range(2)).count).to eq(5) described_class.rollup(Date.current - 2) @@ -44,7 +44,7 @@ RSpec.describe BillboardEventRollup, type: :service do [ad1.id, user1.id, 1], [ad2.id, nil, 1], ] - results_mapped = DisplayAdEvent.where(created_at: days_ago_as_range(2)).map do |event| + results_mapped = BillboardEvent.where(created_at: days_ago_as_range(2)).map do |event| [event.billboard_id, event.user_id, event.counts_for] end expect(results_mapped).to match_array(expectations) @@ -53,14 +53,14 @@ RSpec.describe BillboardEventRollup, type: :service do # separate category it "groups by category" do - create(:display_ad_event, category: "impression", billboard: ad1, user_id: nil) - create(:display_ad_event, category: "impression", billboard: ad1, user_id: nil) - create(:display_ad_event, category: "impression", billboard: ad1, user_id: nil) - create(:display_ad_event, category: "click", billboard: ad1, user_id: nil) - create(:display_ad_event, category: "click", billboard: ad1, user_id: nil) + create(:billboard_event, category: "impression", billboard: ad1, user_id: nil) + create(:billboard_event, category: "impression", billboard: ad1, user_id: nil) + create(:billboard_event, category: "impression", billboard: ad1, user_id: nil) + create(:billboard_event, category: "click", billboard: ad1, user_id: nil) + create(:billboard_event, category: "click", billboard: ad1, user_id: nil) described_class.rollup(Date.current) - results = DisplayAdEvent.where(created_at: Date.current.all_day) + results = BillboardEvent.where(created_at: Date.current.all_day) by_category = results.index_by { |r| r["category"] } expect(by_category["impression"]["counts_for"]).to eq(3) expect(by_category["click"]["counts_for"]).to eq(2) @@ -68,14 +68,14 @@ RSpec.describe BillboardEventRollup, type: :service do # separate billboard_id it "groups by billboard_id" do - create(:display_ad_event, billboard: ad1, user_id: nil) - create(:display_ad_event, billboard: ad1, user_id: nil) - create(:display_ad_event, billboard: ad1, user_id: nil) - create(:display_ad_event, billboard: ad2, user_id: nil) - create(:display_ad_event, billboard: ad2, user_id: nil) + create(:billboard_event, billboard: ad1, user_id: nil) + create(:billboard_event, billboard: ad1, user_id: nil) + create(:billboard_event, billboard: ad1, user_id: nil) + create(:billboard_event, billboard: ad2, user_id: nil) + create(:billboard_event, billboard: ad2, user_id: nil) described_class.rollup(Date.current) - results = DisplayAdEvent.where(created_at: Date.current.all_day) + results = BillboardEvent.where(created_at: Date.current.all_day) by_ad = results.index_by { |r| r["billboard_id"] } expect(by_ad[ad1.id]["counts_for"]).to eq(3) expect(by_ad[ad2.id]["counts_for"]).to eq(2) @@ -83,17 +83,17 @@ RSpec.describe BillboardEventRollup, type: :service do # separate user_id / null it "groups by user_id (including null / logged-out user)" do - create(:display_ad_event, billboard: ad1, user: user1) - create(:display_ad_event, billboard: ad1, user: user2) - create(:display_ad_event, billboard: ad1, user: user2) - create(:display_ad_event, billboard: ad1, user: nil) - create(:display_ad_event, billboard: ad1, user: nil) - create(:display_ad_event, billboard: ad1, user: nil) - create(:display_ad_event, billboard: ad1, user: nil) - create(:display_ad_event, billboard: ad1, user: nil) + create(:billboard_event, billboard: ad1, user: user1) + create(:billboard_event, billboard: ad1, user: user2) + create(:billboard_event, billboard: ad1, user: user2) + create(:billboard_event, billboard: ad1, user: nil) + create(:billboard_event, billboard: ad1, user: nil) + create(:billboard_event, billboard: ad1, user: nil) + create(:billboard_event, billboard: ad1, user: nil) + create(:billboard_event, billboard: ad1, user: nil) described_class.rollup(Date.current) - results = DisplayAdEvent.where(created_at: Date.current.all_day) + results = BillboardEvent.where(created_at: Date.current.all_day) by_user = results.index_by { |r| r["user_id"] } expect(by_user[user1.id]["counts_for"]).to eq(1) expect(by_user[user2.id]["counts_for"]).to eq(2) @@ -102,14 +102,14 @@ RSpec.describe BillboardEventRollup, type: :service do # sums counts_for > 1 it "counts previously crunched" do - create(:display_ad_event, billboard: ad1, counts_for: 10, user_id: nil) - create(:display_ad_event, billboard: ad1, counts_for: 15, user_id: nil) - create(:display_ad_event, billboard: ad1, user_id: nil) - create(:display_ad_event, billboard: ad1, user_id: nil) - create(:display_ad_event, billboard: ad1, user_id: nil) + create(:billboard_event, billboard: ad1, counts_for: 10, user_id: nil) + create(:billboard_event, billboard: ad1, counts_for: 15, user_id: nil) + create(:billboard_event, billboard: ad1, user_id: nil) + create(:billboard_event, billboard: ad1, user_id: nil) + create(:billboard_event, billboard: ad1, user_id: nil) described_class.rollup(Date.current) - results = DisplayAdEvent.where(created_at: Date.current.all_day) + results = BillboardEvent.where(created_at: Date.current.all_day) expect(results.count).to eq(1) expect(results.first.counts_for).to eq(28) end