From 1a147a31765f7ee472205ad63fc0ae04475abb34 Mon Sep 17 00:00:00 2001 From: rhymes Date: Mon, 31 Aug 2020 15:39:57 +0200 Subject: [PATCH] [deploy] Add missing foreign keys to models related to users (#10018) * Add missing foreign keys to models related to users * Fix specs * Change FK for page_views and rating_votes to nullify * Improve rating vote validation and fix Users::Delete specs * Revert "Spec Fix:Nullify Rating Votes when Deleting Users (#10071)" This reverts commit 12dfe4ab3b0a9054a09cad83274558bc97be69a4. --- app/models/rating_vote.rb | 12 +++++++----- ...g_foreign_keys_to_models_related_to_users.rb | 17 +++++++++++++++++ db/schema.rb | 13 +++++++++++++ spec/factories/rating_votes.rb | 2 +- spec/models/notification_spec.rb | 8 ++++---- spec/models/rating_vote_spec.rb | 15 +++++++++++++-- spec/policies/organization_policy_spec.rb | 4 ++-- spec/services/users/delete_spec.rb | 11 +---------- 8 files changed, 58 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20200826132639_add_missing_foreign_keys_to_models_related_to_users.rb diff --git a/app/models/rating_vote.rb b/app/models/rating_vote.rb index fad020065..11ee932d1 100644 --- a/app/models/rating_vote.rb +++ b/app/models/rating_vote.rb @@ -1,11 +1,13 @@ class RatingVote < ApplicationRecord belongs_to :article - belongs_to :user + belongs_to :user, optional: true - validates :user_id, uniqueness: { scope: %i[article_id context] } - validates :group, inclusion: { in: %w[experience_level] } validates :context, inclusion: { in: %w[explicit readinglist_reaction comment] } + validates :group, inclusion: { in: %w[experience_level] } validates :rating, numericality: { greater_than: 0.0, less_than_or_equal_to: 10.0 } + validates :user_id, presence: true, on: :create + validates :user_id, uniqueness: { scope: %i[article_id context] } + validate :permissions after_create_commit :assign_article_rating @@ -13,12 +15,12 @@ class RatingVote < ApplicationRecord counter_culture :article counter_culture :user + private + def assign_article_rating RatingVotes::AssignRatingWorker.perform_async(article_id) end - private - def permissions return unless context == "explicit" && !user&.trusted && user_id != article&.user_id diff --git a/db/migrate/20200826132639_add_missing_foreign_keys_to_models_related_to_users.rb b/db/migrate/20200826132639_add_missing_foreign_keys_to_models_related_to_users.rb new file mode 100644 index 000000000..d912396a7 --- /dev/null +++ b/db/migrate/20200826132639_add_missing_foreign_keys_to_models_related_to_users.rb @@ -0,0 +1,17 @@ +class AddMissingForeignKeysToModelsRelatedToUsers < ActiveRecord::Migration[6.0] + def change + add_foreign_key :articles, :users, on_delete: :cascade, validate: false + add_foreign_key :collections, :users, on_delete: :cascade, validate: false + add_foreign_key :comments, :users, on_delete: :cascade, validate: false + add_foreign_key :credits, :users, on_delete: :cascade, validate: false + add_foreign_key :github_repos, :users, on_delete: :cascade, validate: false + add_foreign_key :html_variants, :users, on_delete: :cascade, validate: false + add_foreign_key :mentions, :users, on_delete: :cascade, validate: false + add_foreign_key :notifications, :users, on_delete: :cascade, validate: false + add_foreign_key :organization_memberships, :users, on_delete: :cascade, validate: false + add_foreign_key :page_views, :users, on_delete: :nullify, validate: false + add_foreign_key :rating_votes, :users, on_delete: :nullify, validate: false + add_foreign_key :reactions, :users, on_delete: :cascade, validate: false + add_foreign_key :tweets, :users, on_delete: :nullify, validate: false + end +end diff --git a/db/schema.rb b/db/schema.rb index e8a5a984b..60715dd03 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1369,6 +1369,7 @@ ActiveRecord::Schema.define(version: 2020_08_28_032013) do add_foreign_key "ahoy_visits", "users", on_delete: :cascade add_foreign_key "api_secrets", "users", on_delete: :cascade add_foreign_key "articles", "organizations", on_delete: :nullify + add_foreign_key "articles", "users", on_delete: :cascade add_foreign_key "audit_logs", "users" add_foreign_key "badge_achievements", "badges" add_foreign_key "badge_achievements", "users" @@ -1381,26 +1382,35 @@ ActiveRecord::Schema.define(version: 2020_08_28_032013) do add_foreign_key "classified_listings", "organizations", on_delete: :cascade add_foreign_key "classified_listings", "users", on_delete: :cascade add_foreign_key "collections", "organizations", on_delete: :nullify + add_foreign_key "collections", "users", on_delete: :cascade + add_foreign_key "comments", "users", on_delete: :cascade add_foreign_key "credits", "organizations", on_delete: :restrict + add_foreign_key "credits", "users", on_delete: :cascade add_foreign_key "display_ad_events", "display_ads", on_delete: :cascade add_foreign_key "display_ad_events", "users", on_delete: :cascade add_foreign_key "display_ads", "organizations", on_delete: :cascade add_foreign_key "email_authorizations", "users", on_delete: :cascade + add_foreign_key "github_repos", "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 "html_variants", "users", on_delete: :cascade add_foreign_key "identities", "users", on_delete: :cascade + add_foreign_key "mentions", "users", on_delete: :cascade add_foreign_key "messages", "chat_channels" add_foreign_key "messages", "users" add_foreign_key "notification_subscriptions", "users", on_delete: :cascade add_foreign_key "notifications", "organizations", on_delete: :cascade + add_foreign_key "notifications", "users", on_delete: :cascade add_foreign_key "oauth_access_grants", "oauth_applications", column: "application_id" add_foreign_key "oauth_access_grants", "users", column: "resource_owner_id" add_foreign_key "oauth_access_tokens", "oauth_applications", column: "application_id" add_foreign_key "oauth_access_tokens", "users", column: "resource_owner_id" add_foreign_key "organization_memberships", "organizations", on_delete: :cascade + add_foreign_key "organization_memberships", "users", on_delete: :cascade add_foreign_key "page_views", "articles", on_delete: :cascade + add_foreign_key "page_views", "users", on_delete: :nullify add_foreign_key "podcast_episodes", "podcasts", on_delete: :cascade add_foreign_key "podcasts", "users", column: "creator_id" add_foreign_key "poll_options", "polls", on_delete: :cascade @@ -1413,12 +1423,15 @@ ActiveRecord::Schema.define(version: 2020_08_28_032013) do add_foreign_key "profile_fields", "profile_field_groups" add_foreign_key "profiles", "users", on_delete: :cascade add_foreign_key "rating_votes", "articles", on_delete: :cascade + add_foreign_key "rating_votes", "users", on_delete: :nullify + add_foreign_key "reactions", "users", on_delete: :cascade add_foreign_key "response_templates", "users" add_foreign_key "sponsorships", "organizations" add_foreign_key "sponsorships", "users" add_foreign_key "tag_adjustments", "articles", on_delete: :cascade add_foreign_key "tag_adjustments", "tags", on_delete: :cascade add_foreign_key "tag_adjustments", "users", on_delete: :cascade + add_foreign_key "tweets", "users", on_delete: :nullify add_foreign_key "user_blocks", "users", column: "blocked_id" add_foreign_key "user_blocks", "users", column: "blocker_id" add_foreign_key "user_subscriptions", "users", column: "author_id" diff --git a/spec/factories/rating_votes.rb b/spec/factories/rating_votes.rb index ac4201020..e4f154898 100644 --- a/spec/factories/rating_votes.rb +++ b/spec/factories/rating_votes.rb @@ -4,7 +4,7 @@ FactoryBot.define do group { "experience_level" } rating { rand(1.0..8.0) } after(:build) do |vote| - vote.article ||= create(:article, user: vote.user) + vote.article ||= create(:article) end end end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 84948e50c..bc09e40ed 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -13,11 +13,11 @@ RSpec.describe Notification, type: :model do let(:comment) { create(:comment, user: user2, commentable: article) } let(:badge_achievement) { create(:badge_achievement) } - it do + it "validates a unique user_id according to the correct scope" do + notification = create(:notification, user: user2, notifiable: article, organization: nil) + scopes = %i[organization_id notifiable_id notifiable_type action] - # rubocop:disable RSpec/NamedSubject - expect(subject).to validate_uniqueness_of(:user_id).scoped_to(scopes) - # rubocop:enable RSpec/NamedSubject + expect(notification).to validate_uniqueness_of(:user_id).scoped_to(scopes) end describe "when trying to create duplicate notifications" do diff --git a/spec/models/rating_vote_spec.rb b/spec/models/rating_vote_spec.rb index dccbe1a4d..7eeb75164 100644 --- a/spec/models/rating_vote_spec.rb +++ b/spec/models/rating_vote_spec.rb @@ -4,10 +4,21 @@ RSpec.describe RatingVote, type: :model do let(:user) { create(:user, :trusted) } let(:user2) { create(:user, :trusted) } let(:article) { create(:article, user: user) } + let(:rating_vote) { create(:rating_vote, user: user2, article: article) } describe "validations" do - it { is_expected.to validate_numericality_of(:rating).is_greater_than(0.0).is_less_than_or_equal_to(10.0) } - it { is_expected.to validate_inclusion_of(:group).in_array(%w[experience_level]) } + describe "builtin validations" do + subject { rating_vote } + + it { is_expected.to belong_to(:article) } + it { is_expected.to belong_to(:user).optional } + + it { is_expected.to validate_inclusion_of(:context).in_array(%w[explicit readinglist_reaction comment]) } + it { is_expected.to validate_inclusion_of(:group).in_array(%w[experience_level]) } + it { is_expected.to validate_numericality_of(:rating).is_greater_than(0.0).is_less_than_or_equal_to(10.0) } + it { is_expected.to validate_presence_of(:user_id).on(:create) } + it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(%i[article_id context]) } + end end describe "uniqueness" do diff --git a/spec/policies/organization_policy_spec.rb b/spec/policies/organization_policy_spec.rb index 1dfbeb165..3b71aa81e 100644 --- a/spec/policies/organization_policy_spec.rb +++ b/spec/policies/organization_policy_spec.rb @@ -27,7 +27,7 @@ RSpec.describe OrganizationPolicy, type: :policy do context "when user is an org admin of an org" do subject(:organization_policy) { described_class.new(user, org) } - let(:user) { build_stubbed(:user) } + let(:user) { create(:user) } let(:org) { create(:organization) } before do @@ -42,7 +42,7 @@ RSpec.describe OrganizationPolicy, type: :policy do context "when user is an org admin of another org" do subject(:organization_policy) { described_class.new(user, new_org) } - let(:user) { build_stubbed(:user) } + let(:user) { create(:user) } let(:org) { create(:organization) } let(:new_org) { build_stubbed(:organization) } diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 5b5f32bb8..180b25216 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe Users::Delete, type: :service do before { omniauth_mock_github_payload } - let(:user) { create(:user, :with_identity, identities: ["github"]) } + let(:user) { create(:user, :trusted, :with_identity, identities: ["github"]) } it "deletes user" do described_class.call(user) @@ -115,10 +115,6 @@ RSpec.describe Users::Delete, type: :service do create_associations(direct_associations.select { |a| kept_association_names.include?(a.name) }) end - before do - allow(user).to receive(:trusted).and_return(true) - end - def create_associations(names) associations = [] @@ -150,11 +146,6 @@ RSpec.describe Users::Delete, type: :service do end it "keeps the kept associations" do - # Rating Vote must be for NOT the user's own article - other_article = create(:article) - rating_vote = kept_associations.detect { |a| a.is_a?(RatingVote) } - rating_vote.update(article_id: other_article.id) - expect(kept_associations).not_to be_empty user.reload described_class.call(user)