From f7a93aabbb1c0af7d6ff4d1a8984b818ba300b80 Mon Sep 17 00:00:00 2001 From: Anna Buianova Date: Fri, 21 Oct 2022 14:57:20 +0300 Subject: [PATCH] Remove html variants tracking (#18594) * Removed code related to tracking html_variants * Remove code related to HtmlVariant#find_for_test * Remove spec for HtmlVariant#success_rate --- .../initializeBaseTracking.js.erb | 60 ------------------- .../admin/html_variants_controller.rb | 8 +-- .../html_variant_successes_controller.rb | 8 --- .../html_variant_trials_controller.rb | 8 --- app/controllers/pages_controller.rb | 1 - app/models/article.rb | 2 - app/models/html_variant.rb | 33 ---------- app/models/html_variant_success.rb | 4 -- app/models/html_variant_trial.rb | 4 -- app/views/articles/_actions.html.erb | 25 +++----- .../html_variants/remove_old_data_worker.rb | 15 ----- config/routes.rb | 2 - config/schedule.yml | 4 -- ...h_duplicate_user_id_title_body_markdown.rb | 10 +--- spec/factories/html_variant_successes.rb | 6 -- spec/models/article_spec.rb | 2 - spec/models/html_variant_spec.rb | 34 ----------- spec/models/html_variant_success_spec.rb | 6 -- spec/models/html_variant_trial_spec.rb | 6 -- spec/requests/html_variant_successes_spec.rb | 19 ------ spec/requests/html_variant_trials_spec.rb | 19 ------ .../remove_old_data_worker_spec.rb | 24 -------- 22 files changed, 11 insertions(+), 289 deletions(-) delete mode 100644 app/controllers/html_variant_successes_controller.rb delete mode 100644 app/controllers/html_variant_trials_controller.rb delete mode 100644 app/models/html_variant_success.rb delete mode 100644 app/models/html_variant_trial.rb delete mode 100644 app/workers/html_variants/remove_old_data_worker.rb delete mode 100644 spec/factories/html_variant_successes.rb delete mode 100644 spec/models/html_variant_success_spec.rb delete mode 100644 spec/models/html_variant_trial_spec.rb delete mode 100644 spec/requests/html_variant_successes_spec.rb delete mode 100644 spec/requests/html_variant_trials_spec.rb delete mode 100644 spec/workers/html_variants/remove_old_data_worker_spec.rb diff --git a/app/assets/javascripts/initializers/initializeBaseTracking.js.erb b/app/assets/javascripts/initializers/initializeBaseTracking.js.erb index cdf3a748e..ec445fc3d 100644 --- a/app/assets/javascripts/initializers/initializeBaseTracking.js.erb +++ b/app/assets/javascripts/initializers/initializeBaseTracking.js.erb @@ -130,39 +130,6 @@ function trackCustomImpressions() { var isBot = /bot|google|baidu|bing|msn|duckduckbot|teoma|slurp|yandex/i.test(navigator.userAgent) // is crawler var windowBigEnough = window.innerWidth > 1023 - // Sidebar HTML variant tracking - var stickyNav = document.getElementById('article-show-primary-sticky-nav'); - var sidebarHTMLVariant = document.getElementById('html-variant-article-show-sidebar'); - if (sidebarHTMLVariant && ArticleElement && tokenMeta && !isBot && windowBigEnough) { - var dataBody = { - html_variant_id: sidebarHTMLVariant.dataset.variantId, - article_id: ArticleElement.dataset.articleId, - }; - var csrfToken = tokenMeta.getAttribute('content'); - trackHTMLVariantTrial(dataBody, csrfToken) - var successLinks = stickyNav.querySelectorAll('a,button'); //track all links and button clicks within nav - for(var i = 0; i < successLinks.length; i++) - { - successLinks[i].addEventListener('click', function() { trackHtmlVariantSuccess(dataBody, csrfToken) }); - } - } - - // Below article HTML variant tracking - var belowArticleHTMLVariant = document.getElementById('html-variant-article-show-below-article'); - if (belowArticleHTMLVariant && ArticleElement && tokenMeta && !isBot && windowBigEnough) { - var dataBody = { - html_variant_id: belowArticleHTMLVariant.dataset.variantId, - article_id: ArticleElement.dataset.articleId, - }; - var csrfToken = tokenMeta.getAttribute('content'); - trackHTMLVariantTrial(dataBody, csrfToken) - var successLinks = belowArticleHTMLVariant.querySelectorAll('a,button'); //track all links and button clicks within nav - for(var i = 0; i < successLinks.length; i++) - { - successLinks[i].addEventListener('click', function() { trackHtmlVariantSuccess(dataBody, csrfToken) }); - } - } - // page view if (ArticleElement && tokenMeta && !isBot) { // See https://github.com/forem/forem/blob/main/app/controllers/page_views_controller.rb @@ -197,33 +164,6 @@ function trackCustomImpressions() { }, 1800) } -function trackHTMLVariantTrial(dataBody, csrfToken) { - var randomNumber = Math.floor(Math.random() * 10); // 1 in 10; Only track 1 in 10 impressions - if (randomNumber === 1) { - window.fetch('/html_variant_trials', { - method: 'POST', - headers: { - 'X-CSRF-Token': csrfToken, - 'Content-Type': 'application/json', - }, - body: JSON.stringify(dataBody), - credentials: 'same-origin', - }); - } -} - -function trackHtmlVariantSuccess(dataBody, csrfToken) { - window.fetch('/html_variant_successes', { - method: 'POST', - headers: { - 'X-CSRF-Token': csrfToken, - 'Content-Type': 'application/json', - }, - body: JSON.stringify(dataBody), - credentials: 'same-origin', - }) -} - function trackPageView(dataBody, csrfToken) { window.fetch('/page_views', { method: 'POST', diff --git a/app/controllers/admin/html_variants_controller.rb b/app/controllers/admin/html_variants_controller.rb index 6617cafda..578e6d57c 100644 --- a/app/controllers/admin/html_variants_controller.rb +++ b/app/controllers/admin/html_variants_controller.rb @@ -5,12 +5,10 @@ module Admin def index relation = if params[:state] == "mine" current_user.html_variants.order(created_at: :desc) - elsif params[:state] == "admin" - HtmlVariant.where(published: true, approved: false).order(created_at: :desc) - elsif params[:state].present? - HtmlVariant.where(published: true, approved: true, group: params[:state]).order(success_rate: :desc) + elsif params[:state].present? && params[:state] != "admin" + HtmlVariant.where(published: true, approved: true, group: params[:state]).order(created_at: :desc) else - HtmlVariant.where(published: true, approved: true).order(success_rate: :desc) + HtmlVariant.where(published: true, approved: true).order(created_at: :desc) end @html_variants = relation.includes(:user).page(params[:page]).per(30) diff --git a/app/controllers/html_variant_successes_controller.rb b/app/controllers/html_variant_successes_controller.rb deleted file mode 100644 index 88343cd96..000000000 --- a/app/controllers/html_variant_successes_controller.rb +++ /dev/null @@ -1,8 +0,0 @@ -class HtmlVariantSuccessesController < ApplicationMetalController - include ActionController::Head - - def create - HtmlVariantSuccess.create(html_variant_id: params[:html_variant_id], article_id: params[:article_id]) - head :ok - end -end diff --git a/app/controllers/html_variant_trials_controller.rb b/app/controllers/html_variant_trials_controller.rb deleted file mode 100644 index aeb1a1a4d..000000000 --- a/app/controllers/html_variant_trials_controller.rb +++ /dev/null @@ -1,8 +0,0 @@ -class HtmlVariantTrialsController < ApplicationMetalController - include ActionController::Head - - def create - HtmlVariantTrial.create!(html_variant_id: params[:html_variant_id], article_id: params[:article_id]) - head :ok - end -end diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index dd916a064..22ed7bfe7 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -23,7 +23,6 @@ class PagesController < ApplicationController end def badge - @html_variant = HtmlVariant.find_for_test([], "badge_landing_page") render layout: false set_surrogate_key_header "badge_page" end diff --git a/app/models/article.rb b/app/models/article.rb index 3e37365ff..0f791081e 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -104,8 +104,6 @@ class Article < ApplicationRecord has_many :context_notifications, as: :context, inverse_of: :context, dependent: :delete_all has_many :context_notifications_published, -> { where(context_notifications: { action: "Published" }) }, as: :context, inverse_of: :context, class_name: "ContextNotification" - has_many :html_variant_successes, dependent: :nullify - has_many :html_variant_trials, dependent: :nullify has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all has_many :page_views, dependent: :delete_all diff --git a/app/models/html_variant.rb b/app/models/html_variant.rb index 95b450915..4d7bd4282 100644 --- a/app/models/html_variant.rb +++ b/app/models/html_variant.rb @@ -5,15 +5,11 @@ class HtmlVariant < ApplicationRecord belongs_to :user, optional: true - has_many :html_variant_successes, dependent: :destroy - has_many :html_variant_trials, dependent: :destroy - before_validation :strip_whitespace validates :group, inclusion: { in: GROUP_NAMES } validates :html, presence: true validates :name, uniqueness: true - validates :success_rate, presence: true validate :no_edits @@ -21,35 +17,6 @@ class HtmlVariant < ApplicationRecord scope :relevant, -> { where(approved: true, published: true) } - def calculate_success_rate! - # x10 because we only capture every 10th - self.success_rate = html_variant_successes.size.to_f / (html_variant_trials.size * 10.0) - save! - end - - class << self - def find_for_test(tags = [], group = "article_show_below_article_cta") - tags_array = tags + ["", nil] - if rand(10) == 1 # 10% return completely random - find_random_for_test(tags_array, group) - else # 90% chance return one of the top posts - find_top_for_test(tags_array, group) - end - end - - private - - def find_top_for_test(tags_array, group) - where(group: group, approved: true, published: true, target_tag: tags_array) - .order(success_rate: :desc).limit(rand(1..20)).sample - end - - def find_random_for_test(tags_array, group) - where(group: group, approved: true, published: true, target_tag: tags_array) - .order(Arel.sql("RANDOM()")).first - end - end - private def no_edits diff --git a/app/models/html_variant_success.rb b/app/models/html_variant_success.rb deleted file mode 100644 index 93b55353c..000000000 --- a/app/models/html_variant_success.rb +++ /dev/null @@ -1,4 +0,0 @@ -class HtmlVariantSuccess < ApplicationRecord - belongs_to :html_variant - belongs_to :article, optional: true -end diff --git a/app/models/html_variant_trial.rb b/app/models/html_variant_trial.rb deleted file mode 100644 index 468a0530e..000000000 --- a/app/models/html_variant_trial.rb +++ /dev/null @@ -1,4 +0,0 @@ -class HtmlVariantTrial < ApplicationRecord - belongs_to :html_variant - belongs_to :article, optional: true -end diff --git a/app/views/articles/_actions.html.erb b/app/views/articles/_actions.html.erb index 52baac7de..e8b0cda9c 100644 --- a/app/views/articles/_actions.html.erb +++ b/app/views/articles/_actions.html.erb @@ -29,13 +29,13 @@ <% else %> <%= render partial: "articles/reaction_button", - locals: { - category: :unicorn, - description: t("views.reactions.unicorn.title"), - image_path: "unicorn.svg", - image_active_path: "unicorn-filled.svg", - aria_label: t("views.reactions.unicorn.aria_label") - } %> + locals: { + category: :unicorn, + description: t("views.reactions.unicorn.title"), + image_path: "unicorn.svg", + image_active_path: "unicorn-filled.svg", + aria_label: t("views.reactions.unicorn.aria_label") + } %> <% end %> <%= render partial: "articles/reaction_button", @@ -116,14 +116,3 @@ - -<% if !user_signed_in? && @article.body_markdown.size > 900 %> - <% cache("below-article-html-variant-#{rand(20)}", expires_in: 8.hours) do %> - <% @html_variant = HtmlVariant.find_for_test(@article.cached_tag_list_array, "article_show_below_article_cta") %> - <% if @html_variant %> -
- <%= @html_variant.html.html_safe %> -
- <% end %> - <% end %> -<% end %> diff --git a/app/workers/html_variants/remove_old_data_worker.rb b/app/workers/html_variants/remove_old_data_worker.rb deleted file mode 100644 index a7adcfb49..000000000 --- a/app/workers/html_variants/remove_old_data_worker.rb +++ /dev/null @@ -1,15 +0,0 @@ -module HtmlVariants - class RemoveOldDataWorker - include Sidekiq::Job - - sidekiq_options queue: :low_priority, retry: 15 - - def perform - HtmlVariantTrial.delete_by("created_at < ?", 2.weeks.ago) - HtmlVariantSuccess.delete_by("created_at < ?", 2.weeks.ago) - HtmlVariant.find_each do |html_variant| - html_variant.calculate_success_rate! if html_variant.html_variant_successes.any? - end - end - end -end diff --git a/config/routes.rb b/config/routes.rb index 32fdf3e2a..fa5742a01 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -122,8 +122,6 @@ Rails.application.routes.draw do resources :videos, only: %i[index create new] resources :video_states, only: [:create] resources :twilio_tokens, only: [:show] - resources :html_variant_trials, only: [:create] - resources :html_variant_successes, only: [:create] resources :tag_adjustments, only: %i[create destroy] resources :rating_votes, only: [:create] resources :page_views, only: %i[create update] diff --git a/config/schedule.yml b/config/schedule.yml index 7316e7b69..ddb3ec7ce 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -86,10 +86,6 @@ award_community_wellness_badges: - "" - award_community_wellness - "" -remove_old_html_variant_data: - description: "Deletes old HTML variants (runs hourly on the 10th minute after the hour)" - cron: "10 * * * *" - class: "HtmlVariants::RemoveOldDataWorker" resave_supported_tags: description: "Resaves supported tags to recalculate scores (runs daily at 00:25 UTC)" cron: "25 0 * * *" diff --git a/lib/data_update_scripts/20200910140109_cleanup_published_articles_with_duplicate_user_id_title_body_markdown.rb b/lib/data_update_scripts/20200910140109_cleanup_published_articles_with_duplicate_user_id_title_body_markdown.rb index 0a938f6db..65d5ebf5f 100644 --- a/lib/data_update_scripts/20200910140109_cleanup_published_articles_with_duplicate_user_id_title_body_markdown.rb +++ b/lib/data_update_scripts/20200910140109_cleanup_published_articles_with_duplicate_user_id_title_body_markdown.rb @@ -65,15 +65,7 @@ module DataUpdateScripts # NotificationSubscription, Notification and RatingVote rows will be removed # Poll is ignored because it's related to the liquid tag inside the article, also user's can't use polls # TagAdjustment is ignored as there's likely no reason for article to have an adjustment moved over - models_with_a_direct_relation = [ - HtmlVariantSuccess, - HtmlVariantSuccess, - HtmlVariantTrial, - PageView, - ] - models_with_a_direct_relation.each do |model_class| - model_class.where(article_id: articles_to_graft_ids).update_all(article_id: article_id) - end + PageView.where(article_id: articles_to_graft_ids).update_all(article_id: article_id) Comment .where(commentable_type: "Article", commentable_id: articles_to_graft_ids) diff --git a/spec/factories/html_variant_successes.rb b/spec/factories/html_variant_successes.rb deleted file mode 100644 index 258956db9..000000000 --- a/spec/factories/html_variant_successes.rb +++ /dev/null @@ -1,6 +0,0 @@ -FactoryBot.define do - factory :html_variant_success do - article - html_variant - end -end diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index c128285ae..d472dfe59 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -23,8 +23,6 @@ RSpec.describe Article, type: :model do it { is_expected.to have_many(:comments).dependent(:nullify) } it { is_expected.to have_many(:context_notifications).dependent(:delete_all) } it { is_expected.to have_many(:mentions).dependent(:delete_all) } - it { is_expected.to have_many(:html_variant_successes).dependent(:nullify) } - it { is_expected.to have_many(:html_variant_trials).dependent(:nullify) } it { is_expected.to have_many(:notification_subscriptions).dependent(:delete_all) } it { is_expected.to have_many(:notifications).dependent(:delete_all) } it { is_expected.to have_many(:page_views).dependent(:delete_all) } diff --git a/spec/models/html_variant_spec.rb b/spec/models/html_variant_spec.rb index fdd9cd87d..a26acae7a 100644 --- a/spec/models/html_variant_spec.rb +++ b/spec/models/html_variant_spec.rb @@ -9,46 +9,12 @@ RSpec.describe HtmlVariant, type: :model do it { is_expected.to belong_to(:user).optional } - it { is_expected.to have_many(:html_variant_trials).dependent(:destroy) } - it { is_expected.to have_many(:html_variant_successes).dependent(:destroy) } - it { is_expected.to validate_inclusion_of(:group).in_array(described_class::GROUP_NAMES) } it { is_expected.to validate_presence_of(:html) } - it { is_expected.to validate_presence_of(:success_rate) } it { is_expected.to validate_uniqueness_of(:name) } end end - it "calculates success rate" do - 4.times { HtmlVariantTrial.create!(html_variant_id: html_variant.id) } - HtmlVariantSuccess.create!(html_variant_id: html_variant.id) - - html_variant.calculate_success_rate! - expect(html_variant.success_rate).to eq(0.025) - end - - it "finds for test without tag" do - html_variant.save! - expect(described_class.find_for_test.id).to eq(html_variant.id) - end - - it "finds for test with tag given" do - html_variant.target_tag = "hello" - html_variant.save! - expect(described_class.find_for_test(["hello"]).id).to eq(html_variant.id) - end - - it "does not find if different tag targeted" do - html_variant.target_tag = "different_tag_yolo" - html_variant.save! - expect(described_class.find_for_test(["hello"])).to be_nil - end - - it "finds if no tag targeted and tag given" do - html_variant.update(target_tag: nil) - expect(described_class.find_for_test(["hello"]).id).to eq(html_variant.id) - end - it "prefixes an image with cloudinary", cloudinary: true do html = "
" html_variant.update(approved: false, html: html) diff --git a/spec/models/html_variant_success_spec.rb b/spec/models/html_variant_success_spec.rb deleted file mode 100644 index 0d84f1762..000000000 --- a/spec/models/html_variant_success_spec.rb +++ /dev/null @@ -1,6 +0,0 @@ -require "rails_helper" - -RSpec.describe HtmlVariantSuccess, type: :model do - it { is_expected.to belong_to(:html_variant) } - it { is_expected.to belong_to(:article).optional } -end diff --git a/spec/models/html_variant_trial_spec.rb b/spec/models/html_variant_trial_spec.rb deleted file mode 100644 index 9f3c824ca..000000000 --- a/spec/models/html_variant_trial_spec.rb +++ /dev/null @@ -1,6 +0,0 @@ -require "rails_helper" - -RSpec.describe HtmlVariantTrial, type: :model do - it { is_expected.to belong_to(:html_variant) } - it { is_expected.to belong_to(:article).optional } -end diff --git a/spec/requests/html_variant_successes_spec.rb b/spec/requests/html_variant_successes_spec.rb deleted file mode 100644 index 203c87427..000000000 --- a/spec/requests/html_variant_successes_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require "rails_helper" - -RSpec.describe "HtmlVariantSuccesses", type: :request do - let(:user) { create(:user) } - let(:article) { create(:article, user_id: user.id, approved: true) } - let(:html_variant) { create(:html_variant) } - - describe "POST /html_variant_successes" do - it "rejects non-permissioned user" do - sidekiq_perform_enqueued_jobs do - post "/html_variant_successes", params: { - article_id: article.id, - html_variant_id: html_variant.id - } - end - expect(HtmlVariantSuccess.all.size).to eq(1) - end - end -end diff --git a/spec/requests/html_variant_trials_spec.rb b/spec/requests/html_variant_trials_spec.rb deleted file mode 100644 index 0a2b4d9cc..000000000 --- a/spec/requests/html_variant_trials_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require "rails_helper" - -RSpec.describe "HtmlVariantTrials", type: :request do - let(:user) { create(:user) } - let(:article) { create(:article, user_id: user.id, approved: true) } - let(:html_variant) { create(:html_variant) } - - describe "POST /html_variant_trials" do - it "rejects non-permissioned user" do - sidekiq_perform_enqueued_jobs do - post "/html_variant_trials", params: { - article_id: article.id, - html_variant_id: html_variant.id - } - end - expect(HtmlVariantTrial.all.size).to eq(1) - end - end -end diff --git a/spec/workers/html_variants/remove_old_data_worker_spec.rb b/spec/workers/html_variants/remove_old_data_worker_spec.rb deleted file mode 100644 index 4b5a84d17..000000000 --- a/spec/workers/html_variants/remove_old_data_worker_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -require "rails_helper" - -RSpec.describe HtmlVariants::RemoveOldDataWorker, type: :worker do - let(:worker) { subject } - - include_examples "#enqueues_on_correct_queue", "low_priority" - - describe "#perform" do - it "removes old html variants" do - Timecop.freeze do - old_trial_id = create(:html_variant_trial, created_at: 1.month.ago).id - old_success_id = create(:html_variant_success, created_at: 1.month.ago).id - new_trial = create(:html_variant_trial, created_at: Time.current) - new_trial.html_variant.update(success_rate: 0) - - worker.perform - - expect(HtmlVariantTrial.find_by(id: old_trial_id)).to be_nil - expect(HtmlVariantSuccess.find_by(id: old_success_id)).to be_nil - expect(HtmlVariantTrial.find_by(id: new_trial.id)).not_to be_nil - end - end - end -end