From b56ae3acbbef302770af9c831318d477bc6f6715 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Wed, 5 Jan 2022 09:19:39 -0500 Subject: [PATCH] Extracting constants and tidying tests (#15946) Prior to this commit, we had a couple of references to magic strings regarding DisplayAdEvent objects. This commit seeks to consolidate that behavior. In a handful of cases we had javascript variables, but as these JS files are `erb` templates, we can inject the constant as well. This reduces repetition of knowledge. --- .../initializeBaseTracking.js.erb | 8 +++--- .../display_ad_events_controller.rb | 4 +-- app/models/display_ad_event.rb | 14 +++++++++-- spec/factories/display_ad_events.rb | 2 +- spec/models/display_ad_event_spec.rb | 21 ++-------------- spec/requests/display_ad_events_spec.rb | 25 +++++++++++-------- 6 files changed, 35 insertions(+), 39 deletions(-) diff --git a/app/assets/javascripts/initializers/initializeBaseTracking.js.erb b/app/assets/javascripts/initializers/initializeBaseTracking.js.erb index 9c8076b20..afe3c100b 100644 --- a/app/assets/javascripts/initializers/initializeBaseTracking.js.erb +++ b/app/assets/javascripts/initializers/initializeBaseTracking.js.erb @@ -206,8 +206,8 @@ function trackAdImpression(token, adBox) { var dataBody = { display_ad_event: { display_ad_id: adBox.dataset.id, - context_type: "home", - category: "impression", + context_type: "<%= DisplayAdEvent::CONTEXT_TYPE_HOME %>", + category: "<%= DisplayAdEvent::CATEGORY_IMPRESSION %>", } } window.fetch('/display_ad_events', { @@ -227,8 +227,8 @@ function trackAdClick(token, clickedElement) { var dataBody = { display_ad_event: { display_ad_id: adBox.dataset.id, - context_type: "home", - category: "click", + context_type: "<%= DisplayAdEvent::CONTEXT_TYPE_HOME %>", + category: "<%= DisplayAdEvent::CATEGORY_CLICK %>", } } window.fetch('/display_ad_events', { diff --git a/app/controllers/display_ad_events_controller.rb b/app/controllers/display_ad_events_controller.rb index b01370297..5a28ffa12 100644 --- a/app/controllers/display_ad_events_controller.rb +++ b/app/controllers/display_ad_events_controller.rb @@ -18,8 +18,8 @@ class DisplayAdEventsController < ApplicationMetalController ThrottledCall.perform(:display_ads_data_update, throttle_for: 15.minutes) do @display_ad = DisplayAd.find(display_ad_event_params[:display_ad_id]) - num_impressions = @display_ad.display_ad_events.where(category: "impression").size - num_clicks = @display_ad.display_ad_events.where(category: "click").size + num_impressions = @display_ad.display_ad_events.impressions.size + num_clicks = @display_ad.display_ad_events.clicks.size rate = num_clicks.to_f / num_impressions @display_ad.update_columns( diff --git a/app/models/display_ad_event.rb b/app/models/display_ad_event.rb index 370bf8d8e..5a457a2c1 100644 --- a/app/models/display_ad_event.rb +++ b/app/models/display_ad_event.rb @@ -5,6 +5,16 @@ class DisplayAdEvent < ApplicationRecord belongs_to :display_ad belongs_to :user - validates :category, inclusion: { in: %w[impression click] } - validates :context_type, inclusion: { in: %w[home] } + CATEGORY_IMPRESSION = "impression".freeze + CATEGORY_CLICK = "click".freeze + VALID_CATEGORIES = [CATEGORY_CLICK, CATEGORY_IMPRESSION].freeze + + CONTEXT_TYPE_HOME = "home".freeze + VALID_CONTEXT_TYPES = [CONTEXT_TYPE_HOME].freeze + + validates :category, inclusion: { in: VALID_CATEGORIES } + validates :context_type, inclusion: { in: VALID_CONTEXT_TYPES } + + scope :impressions, -> { where(category: CATEGORY_IMPRESSION) } + scope :clicks, -> { where(category: CATEGORY_CLICK) } end diff --git a/spec/factories/display_ad_events.rb b/spec/factories/display_ad_events.rb index 719a49add..a7399814c 100644 --- a/spec/factories/display_ad_events.rb +++ b/spec/factories/display_ad_events.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :display_ad_event do - category { "impression" } + category { DisplayAdEvent::CATEGORY_IMPRESSION } context_type { "home" } display_ad end diff --git a/spec/models/display_ad_event_spec.rb b/spec/models/display_ad_event_spec.rb index 37530ec95..5ab0b41ae 100644 --- a/spec/models/display_ad_event_spec.rb +++ b/spec/models/display_ad_event_spec.rb @@ -1,24 +1,7 @@ require "rails_helper" RSpec.describe DisplayAdEvent, type: :model do - let(:user) { build(:user) } - let(:organization) { build(:organization) } - let(:display_ad) { build(:display_ad, organization: organization) } + it { is_expected.to validate_inclusion_of(:category).in_array(described_class::VALID_CATEGORIES) } - describe "#category" do - it "is valid with a click category" do - event = build(:display_ad_event, category: "click", user: user, display_ad: display_ad) - expect(event).to be_valid - end - - it "is valid with an impression category" do - event = build(:display_ad_event, category: "impression", user: user, display_ad: display_ad) - expect(event).to be_valid - end - - it "is not valid with an unknown category" do - event = build(:display_ad_event, category: "wazoo", user: user, display_ad: display_ad) - expect(event).not_to be_valid - end - end + it { is_expected.to validate_inclusion_of(:context_type).in_array(described_class::VALID_CONTEXT_TYPES) } end diff --git a/spec/requests/display_ad_events_spec.rb b/spec/requests/display_ad_events_spec.rb index f6c51034b..c21b88836 100644 --- a/spec/requests/display_ad_events_spec.rb +++ b/spec/requests/display_ad_events_spec.rb @@ -15,8 +15,8 @@ RSpec.describe "DisplayAdEvents", type: :request do post "/display_ad_events", params: { display_ad_event: { display_ad_id: display_ad.id, - context_type: "home", - category: "click" + context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, + category: DisplayAdEvent::CATEGORY_CLICK } } expect(display_ad.reload.clicks_count).to eq(1) @@ -26,19 +26,22 @@ RSpec.describe "DisplayAdEvents", type: :request do post "/display_ad_events", params: { display_ad_event: { display_ad_id: display_ad.id, - context_type: "home", - category: "impression" + context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, + category: DisplayAdEvent::CATEGORY_IMPRESSION } } expect(display_ad.reload.impressions_count).to eq(1) end it "creates a display ad success rate" do - ad_event_params = { display_ad_id: display_ad.id, context_type: "home" } - impression_params = ad_event_params.merge(category: "impression", user: user) + ad_event_params = { display_ad_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) - post "/display_ad_events", params: { display_ad_event: ad_event_params.merge(category: "click") } + post( + "/display_ad_events", + params: { display_ad_event: ad_event_params.merge(category: DisplayAdEvent::CATEGORY_CLICK) }, + ) expect(display_ad.reload.success_rate).to eq(0.25) end @@ -47,8 +50,8 @@ RSpec.describe "DisplayAdEvents", type: :request do post "/display_ad_events", params: { display_ad_event: { display_ad_id: display_ad.id, - context_type: "home", - category: "impression" + context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, + category: DisplayAdEvent::CATEGORY_IMPRESSION } } expect(DisplayAdEvent.last.user_id).to eq(user.id) @@ -58,8 +61,8 @@ RSpec.describe "DisplayAdEvents", type: :request do post "/display_ad_events", params: { display_ad_event: { display_ad_id: display_ad.id, - context_type: "home", - category: "impression" + context_type: DisplayAdEvent::CONTEXT_TYPE_HOME, + category: DisplayAdEvent::CATEGORY_IMPRESSION } }