From a86f4c83f2a7296ca9a004c247bed15817c66ead Mon Sep 17 00:00:00 2001 From: rhymes Date: Tue, 18 Aug 2020 17:33:38 +0200 Subject: [PATCH] [deploy] Add relations and foreign keys between HtmlVariant related models (#9846) * Add inverse has_many relation betwen User and Ahoy::[Event|Visit ] * Add FKs and proper relations to HtmlVariant models --- .rubocop_todo.yml | 2 -- app/models/article.rb | 2 ++ app/models/html_variant.rb | 12 +++++++----- app/models/html_variant_success.rb | 3 ++- app/models/html_variant_trial.rb | 3 ++- app/models/user.rb | 2 ++ ...ing_foreign_keys_to_html_variants_models.rb | 9 +++++++++ ...ing_foreign_keys_to_html_variants_models.rb | 8 ++++++++ db/schema.rb | 6 +++++- spec/models/article_spec.rb | 2 ++ spec/models/html_variant_spec.rb | 18 +++++++++++++++--- spec/models/user_spec.rb | 2 ++ 12 files changed, 56 insertions(+), 13 deletions(-) create mode 100644 db/migrate/20200818101637_add_missing_foreign_keys_to_html_variants_models.rb create mode 100644 db/migrate/20200818101700_validate_add_missing_foreign_keys_to_html_variants_models.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index a632939f7..5ba9f6108 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -25,8 +25,6 @@ Rails/HasManyOrHasOneDependent: Exclude: - 'app/models/article.rb' - 'app/models/concerns/user_subscription_sourceable.rb' - - 'app/models/display_ad.rb' - - 'app/models/html_variant.rb' - 'app/models/listing_category.rb' - 'app/models/organization.rb' - 'app/models/podcast.rb' diff --git a/app/models/article.rb b/app/models/article.rb index c59e9a4ea..25e18e375 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -41,6 +41,8 @@ class Article < ApplicationRecord class_name: "Comment" has_many :profile_pins, as: :pinnable, inverse_of: :pinnable has_many :buffer_updates, dependent: :destroy + has_many :html_variant_successes, dependent: :nullify + has_many :html_variant_trials, dependent: :nullify has_many :notifications, as: :notifiable, inverse_of: :notifiable, dependent: :delete_all has_many :notification_subscriptions, as: :notifiable, inverse_of: :notifiable, dependent: :destroy has_many :rating_votes diff --git a/app/models/html_variant.rb b/app/models/html_variant.rb index 09817e03a..fa9f8ca98 100644 --- a/app/models/html_variant.rb +++ b/app/models/html_variant.rb @@ -1,15 +1,17 @@ class HtmlVariant < ApplicationRecord GROUP_NAMES = %w[article_show_below_article_cta badge_landing_page campaign].freeze + belongs_to :user, optional: true + + has_many :html_variant_successes, dependent: :destroy + has_many :html_variant_trials, dependent: :destroy + + validates :group, inclusion: { in: GROUP_NAMES } validates :html, presence: true validates :name, uniqueness: true - validates :group, inclusion: { in: GROUP_NAMES } validates :success_rate, presence: true - validate :no_edits - belongs_to :user, optional: true - has_many :html_variant_trials - has_many :html_variant_successes + validate :no_edits before_save :prefix_all_images diff --git a/app/models/html_variant_success.rb b/app/models/html_variant_success.rb index ef2d2d4aa..c074107a2 100644 --- a/app/models/html_variant_success.rb +++ b/app/models/html_variant_success.rb @@ -1,5 +1,6 @@ class HtmlVariantSuccess < ApplicationRecord - validates :html_variant_id, presence: true belongs_to :html_variant belongs_to :article, optional: true + + validates :html_variant_id, presence: true end diff --git a/app/models/html_variant_trial.rb b/app/models/html_variant_trial.rb index 3502bfcf5..9872d142b 100644 --- a/app/models/html_variant_trial.rb +++ b/app/models/html_variant_trial.rb @@ -1,5 +1,6 @@ class HtmlVariantTrial < ApplicationRecord - validates :html_variant_id, presence: true belongs_to :html_variant belongs_to :article, optional: true + + validates :html_variant_id, presence: true end diff --git a/app/models/user.rb b/app/models/user.rb index a9ff87985..0b389ed3a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -62,6 +62,8 @@ class User < ApplicationRecord foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all has_many :affected_feedback_messages, class_name: "FeedbackMessage", inverse_of: :affected, foreign_key: :affected_id, dependent: :nullify + has_many :ahoy_events, class_name: "Ahoy::Event", dependent: :destroy + has_many :ahoy_visits, class_name: "Ahoy::Visit", dependent: :destroy has_many :api_secrets, dependent: :destroy has_many :articles, dependent: :destroy has_many :audit_logs, dependent: :nullify diff --git a/db/migrate/20200818101637_add_missing_foreign_keys_to_html_variants_models.rb b/db/migrate/20200818101637_add_missing_foreign_keys_to_html_variants_models.rb new file mode 100644 index 000000000..a208cc951 --- /dev/null +++ b/db/migrate/20200818101637_add_missing_foreign_keys_to_html_variants_models.rb @@ -0,0 +1,9 @@ +class AddMissingForeignKeysToHtmlVariantsModels < ActiveRecord::Migration[6.0] + def change + add_foreign_key :html_variant_successes, :html_variants, column: :html_variant_id, on_delete: :cascade, validate: false + add_foreign_key :html_variant_successes, :articles, column: :article_id, on_delete: :nullify, validate: false + + add_foreign_key :html_variant_trials, :html_variants, column: :html_variant_id, on_delete: :cascade, validate: false + add_foreign_key :html_variant_trials, :articles, column: :article_id, on_delete: :nullify, validate: false + end +end diff --git a/db/migrate/20200818101700_validate_add_missing_foreign_keys_to_html_variants_models.rb b/db/migrate/20200818101700_validate_add_missing_foreign_keys_to_html_variants_models.rb new file mode 100644 index 000000000..dd78562d7 --- /dev/null +++ b/db/migrate/20200818101700_validate_add_missing_foreign_keys_to_html_variants_models.rb @@ -0,0 +1,8 @@ +class ValidateAddMissingForeignKeysToHtmlVariantsModels < ActiveRecord::Migration[6.0] + def change + validate_foreign_key :html_variant_successes, :html_variants + validate_foreign_key :html_variant_successes, :articles + validate_foreign_key :html_variant_trials, :html_variants + validate_foreign_key :html_variant_trials, :articles + end +end diff --git a/db/schema.rb b/db/schema.rb index 9d936a41f..9391faddf 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_08_14_142648) do +ActiveRecord::Schema.define(version: 2020_08_18_101700) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -1352,6 +1352,10 @@ ActiveRecord::Schema.define(version: 2020_08_14_142648) do add_foreign_key "display_ad_events", "display_ads", on_delete: :cascade add_foreign_key "display_ad_events", "users", on_delete: :cascade add_foreign_key "email_authorizations", "users", on_delete: :cascade + add_foreign_key "html_variant_successes", "articles", on_delete: :nullify + add_foreign_key "html_variant_successes", "html_variants", on_delete: :cascade + add_foreign_key "html_variant_trials", "articles", on_delete: :nullify + add_foreign_key "html_variant_trials", "html_variants", on_delete: :cascade add_foreign_key "identities", "users", on_delete: :cascade add_foreign_key "messages", "chat_channels" add_foreign_key "messages", "users" diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index 1d53efefd..0768e5b70 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -24,6 +24,8 @@ RSpec.describe Article, type: :model do it { is_expected.to belong_to(:organization).optional } it { is_expected.to belong_to(:collection).optional } it { is_expected.to have_many(:comments) } + 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(:reactions).dependent(:destroy) } it { is_expected.to have_many(:notifications).dependent(:delete_all) } it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) } diff --git a/spec/models/html_variant_spec.rb b/spec/models/html_variant_spec.rb index 38e5602c7..0a4f605be 100644 --- a/spec/models/html_variant_spec.rb +++ b/spec/models/html_variant_spec.rb @@ -3,9 +3,21 @@ require "rails_helper" RSpec.describe HtmlVariant, type: :model do let(:html_variant) { create(:html_variant, approved: true, published: true) } - it { is_expected.to validate_uniqueness_of(:name) } - it { is_expected.to validate_presence_of(:html) } - it { is_expected.to belong_to(:user).optional } + describe "validations" do + describe "builtin validations" do + subject { html_variant } + + 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) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 95a6a0aaa..6603ff925 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -34,6 +34,8 @@ RSpec.describe User, type: :model do describe "builtin validations" do subject { user } + it { is_expected.to have_many(:ahoy_events).dependent(:destroy) } + it { is_expected.to have_many(:ahoy_visits).dependent(:destroy) } it { is_expected.to have_many(:api_secrets).dependent(:destroy) } it { is_expected.to have_many(:articles).dependent(:destroy) } it { is_expected.to have_many(:audit_logs).dependent(:nullify) }