diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 0bcbbfba9..b17b02073 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -18,13 +18,6 @@ Performance/OpenStruct: - 'spec/models/github_repo_spec.rb' - 'spec/requests/github_repos_spec.rb' -# Offense count: 13 -# Configuration parameters: Include. -# Include: app/models/**/*.rb -Rails/HasManyOrHasOneDependent: - Exclude: - - 'app/models/user.rb' - # Offense count: 4 # Configuration parameters: Include. # Include: app/models/**/*.rb diff --git a/app/models/note.rb b/app/models/note.rb index 8247eab61..42b1f49a6 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -1,5 +1,6 @@ class Note < ApplicationRecord - belongs_to :noteable, polymorphic: true, touch: true belongs_to :author, class_name: "User", optional: true + belongs_to :noteable, polymorphic: true, touch: true + validates :content, :reason, presence: true end diff --git a/app/models/user.rb b/app/models/user.rb index 418aa6572..4e5682804 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -105,16 +105,11 @@ class User < ApplicationRecord acts_as_follower has_one :profile, dependent: :destroy - has_many :source_authored_user_subscriptions, class_name: "UserSubscription", - foreign_key: :author_id, inverse_of: :author, dependent: :destroy - has_many :subscribers, through: :source_authored_user_subscriptions, dependent: :destroy - has_many :subscribed_to_user_subscriptions, class_name: "UserSubscription", - foreign_key: :subscriber_id, inverse_of: :subscriber, dependent: :destroy - has_many :access_grants, class_name: "Doorkeeper::AccessGrant", - foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all - has_many :access_tokens, class_name: "Doorkeeper::AccessToken", - foreign_key: :resource_owner_id, inverse_of: :resource_owner, dependent: :delete_all + has_many :access_grants, class_name: "Doorkeeper::AccessGrant", foreign_key: :resource_owner_id, + inverse_of: :resource_owner, dependent: :delete_all + has_many :access_tokens, class_name: "Doorkeeper::AccessToken", 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 @@ -123,18 +118,16 @@ class User < ApplicationRecord has_many :articles, dependent: :destroy has_many :audit_logs, dependent: :nullify has_many :authored_notes, inverse_of: :author, class_name: "Note", foreign_key: :author_id, dependent: :delete_all - has_many :backup_data, foreign_key: "instance_user_id", - inverse_of: :instance_user, class_name: "BackupData", dependent: :delete_all + has_many :backup_data, foreign_key: "instance_user_id", inverse_of: :instance_user, + class_name: "BackupData", dependent: :delete_all has_many :badge_achievements, dependent: :destroy has_many :badges, through: :badge_achievements - has_many :blocked_blocks, class_name: "UserBlock", - foreign_key: :blocked_id, inverse_of: :blocked, dependent: :delete_all - has_many :blocker_blocks, class_name: "UserBlock", - foreign_key: :blocker_id, inverse_of: :blocker, dependent: :delete_all + has_many :blocked_blocks, class_name: "UserBlock", foreign_key: :blocked_id, + inverse_of: :blocked, dependent: :delete_all + has_many :blocker_blocks, class_name: "UserBlock", foreign_key: :blocker_id, + inverse_of: :blocker, dependent: :delete_all has_many :chat_channel_memberships, dependent: :destroy has_many :chat_channels, through: :chat_channel_memberships - has_many :listings, dependent: :destroy - has_many :endorsements, dependent: :destroy, class_name: "ListingEndorsement" has_many :collections, dependent: :destroy has_many :comments, dependent: :destroy has_many :created_podcasts, class_name: "Podcast", foreign_key: :creator_id, inverse_of: :creator, dependent: :nullify @@ -142,30 +135,43 @@ class User < ApplicationRecord has_many :display_ad_events, dependent: :destroy has_many :email_authorizations, dependent: :delete_all has_many :email_messages, class_name: "Ahoy::Message", dependent: :destroy + has_many :endorsements, dependent: :destroy, class_name: "ListingEndorsement" has_many :field_test_memberships, class_name: "FieldTest::Membership", as: :participant, dependent: :destroy has_many :github_repos, dependent: :destroy has_many :html_variants, dependent: :destroy has_many :identities, dependent: :destroy has_many :identities_enabled, -> { enabled }, class_name: "Identity", inverse_of: false + has_many :listings, dependent: :destroy has_many :mentions, dependent: :destroy has_many :messages, dependent: :destroy - has_many :notes, as: :noteable, inverse_of: :noteable + has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy has_many :notification_subscriptions, dependent: :destroy has_many :notifications, dependent: :destroy has_many :offender_feedback_messages, class_name: "FeedbackMessage", inverse_of: :offender, foreign_key: :offender_id, dependent: :nullify has_many :organization_memberships, dependent: :destroy has_many :organizations, through: :organization_memberships - has_many :page_views, dependent: :destroy + + # we keep page views as they belong to the article, not to the user who viewed it + has_many :page_views, dependent: :nullify + has_many :poll_skips, dependent: :destroy has_many :poll_votes, dependent: :destroy has_many :profile_pins, as: :profile, inverse_of: :profile, dependent: :delete_all - has_many :rating_votes, dependent: :destroy + + # we keep rating votes as they belong to the article, not to the user who viewed it + has_many :rating_votes, dependent: :nullify + has_many :reactions, dependent: :destroy has_many :reporter_feedback_messages, class_name: "FeedbackMessage", inverse_of: :reporter, foreign_key: :reporter_id, dependent: :nullify has_many :response_templates, inverse_of: :user, dependent: :destroy - has_many :tweets, dependent: :destroy + has_many :source_authored_user_subscriptions, class_name: "UserSubscription", + foreign_key: :author_id, inverse_of: :author, dependent: :destroy + has_many :subscribed_to_user_subscriptions, class_name: "UserSubscription", + foreign_key: :subscriber_id, inverse_of: :subscriber, dependent: :destroy + has_many :subscribers, through: :source_authored_user_subscriptions, dependent: :destroy + has_many :tweets, dependent: :nullify has_many :webhook_endpoints, class_name: "Webhook::Endpoint", inverse_of: :user, dependent: :delete_all mount_uploader :profile_image, ProfileImageUploader diff --git a/app/services/users/delete_activity.rb b/app/services/users/delete_activity.rb index 69b9c3d85..cf261a56e 100644 --- a/app/services/users/delete_activity.rb +++ b/app/services/users/delete_activity.rb @@ -18,10 +18,8 @@ module Users user.display_ad_events.delete_all user.email_messages.delete_all user.html_variants.delete_all - user.page_views.delete_all user.poll_skips.delete_all user.poll_votes.delete_all - user.rating_votes.delete_all user.response_templates.delete_all user.listings.destroy_all delete_feedback_messages(user) @@ -35,7 +33,6 @@ module Users end def delete_social_media(user) - user.tweets.delete_all user.github_repos.delete_all end diff --git a/lib/data_update_scripts/20200826092816_remove_orphaned_notes_by_user.rb b/lib/data_update_scripts/20200826092816_remove_orphaned_notes_by_user.rb new file mode 100644 index 000000000..75c00fd9a --- /dev/null +++ b/lib/data_update_scripts/20200826092816_remove_orphaned_notes_by_user.rb @@ -0,0 +1,14 @@ +module DataUpdateScripts + class RemoveOrphanedNotesByUser + def run + # Delete all Notes about users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM notes + WHERE noteable_type = 'User' + AND noteable_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826133748_nullify_orphaned_tweets_by_user.rb b/lib/data_update_scripts/20200826133748_nullify_orphaned_tweets_by_user.rb new file mode 100644 index 000000000..9a456353c --- /dev/null +++ b/lib/data_update_scripts/20200826133748_nullify_orphaned_tweets_by_user.rb @@ -0,0 +1,15 @@ +module DataUpdateScripts + class NullifyOrphanedTweetsByUser + def run + # Nullify user_id for all Tweets linked to a non existing User + ActiveRecord::Base.connection.execute( + <<~SQL, + UPDATE tweets + SET user_id = NULL + WHERE user_id IS NOT NULL + AND user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826140317_remove_orphaned_articles_by_user.rb b/lib/data_update_scripts/20200826140317_remove_orphaned_articles_by_user.rb new file mode 100644 index 000000000..f136b6420 --- /dev/null +++ b/lib/data_update_scripts/20200826140317_remove_orphaned_articles_by_user.rb @@ -0,0 +1,13 @@ +module DataUpdateScripts + class RemoveOrphanedArticlesByUser + def run + # Delete all Articles belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM articles + WHERE user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826140754_remove_orphaned_collections_by_user.rb b/lib/data_update_scripts/20200826140754_remove_orphaned_collections_by_user.rb new file mode 100644 index 000000000..2c56d4215 --- /dev/null +++ b/lib/data_update_scripts/20200826140754_remove_orphaned_collections_by_user.rb @@ -0,0 +1,13 @@ +module DataUpdateScripts + class RemoveOrphanedCollectionsByUser + def run + # Delete all Collections belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM collections + WHERE user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826140924_remove_orphaned_credits_by_user.rb b/lib/data_update_scripts/20200826140924_remove_orphaned_credits_by_user.rb new file mode 100644 index 000000000..b8090fdff --- /dev/null +++ b/lib/data_update_scripts/20200826140924_remove_orphaned_credits_by_user.rb @@ -0,0 +1,14 @@ +module DataUpdateScripts + class RemoveOrphanedCreditsByUser + def run + # Delete all Credits belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM credits + WHERE user_id IS NOT NULL + AND user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826141015_remove_orphaned_github_repos_by_user.rb b/lib/data_update_scripts/20200826141015_remove_orphaned_github_repos_by_user.rb new file mode 100644 index 000000000..66b899500 --- /dev/null +++ b/lib/data_update_scripts/20200826141015_remove_orphaned_github_repos_by_user.rb @@ -0,0 +1,13 @@ +module DataUpdateScripts + class RemoveOrphanedGithubReposByUser + def run + # Delete all Collections belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM github_repos + WHERE user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826141105_remove_orphaned_mentions_by_user.rb b/lib/data_update_scripts/20200826141105_remove_orphaned_mentions_by_user.rb new file mode 100644 index 000000000..0b2cde85e --- /dev/null +++ b/lib/data_update_scripts/20200826141105_remove_orphaned_mentions_by_user.rb @@ -0,0 +1,13 @@ +module DataUpdateScripts + class RemoveOrphanedMentionsByUser + def run + # Delete all Mentions belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM mentions + WHERE user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826141145_remove_orphaned_organization_memberships_by_user.rb b/lib/data_update_scripts/20200826141145_remove_orphaned_organization_memberships_by_user.rb new file mode 100644 index 000000000..a4848d3fd --- /dev/null +++ b/lib/data_update_scripts/20200826141145_remove_orphaned_organization_memberships_by_user.rb @@ -0,0 +1,13 @@ +module DataUpdateScripts + class RemoveOrphanedOrganizationMembershipsByUser + def run + # Delete all OrganizationMemberships belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM organization_memberships + WHERE user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826141447_nullify_orphaned_page_views_by_user.rb b/lib/data_update_scripts/20200826141447_nullify_orphaned_page_views_by_user.rb new file mode 100644 index 000000000..2479c4869 --- /dev/null +++ b/lib/data_update_scripts/20200826141447_nullify_orphaned_page_views_by_user.rb @@ -0,0 +1,15 @@ +module DataUpdateScripts + class NullifyOrphanedPageViewsByUser + def run + # Nullify all PageViews belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + UPDATE page_views + SET user_id = NULL + WHERE user_id IS NOT NULL + AND user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826141550_nullify_orphaned_rating_votes_by_user.rb b/lib/data_update_scripts/20200826141550_nullify_orphaned_rating_votes_by_user.rb new file mode 100644 index 000000000..6294c3c58 --- /dev/null +++ b/lib/data_update_scripts/20200826141550_nullify_orphaned_rating_votes_by_user.rb @@ -0,0 +1,15 @@ +module DataUpdateScripts + class NullifyOrphanedRatingVotesByUser + def run + # Nullify all RatingVotes belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + UPDATE rating_votes + SET user_id = NULL + WHERE user_id IS NOT NULL + AND user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/lib/data_update_scripts/20200826141652_remove_orphaned_reactions_by_user.rb b/lib/data_update_scripts/20200826141652_remove_orphaned_reactions_by_user.rb new file mode 100644 index 000000000..ea7435865 --- /dev/null +++ b/lib/data_update_scripts/20200826141652_remove_orphaned_reactions_by_user.rb @@ -0,0 +1,13 @@ +module DataUpdateScripts + class RemoveOrphanedReactionsByUser + def run + # Delete all Reactions belonging to Users that don't exist anymore + ActiveRecord::Base.connection.execute( + <<~SQL, + DELETE FROM reactions + WHERE user_id NOT IN (SELECT id FROM users); + SQL + ) + end + end +end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index ef1de1111..fd3922cfd 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -1,8 +1,9 @@ require "rails_helper" RSpec.describe Note, type: :model do - it { is_expected.to validate_presence_of(:reason) } - it { is_expected.to validate_presence_of(:content) } it { is_expected.to belong_to(:noteable) } it { is_expected.to belong_to(:author).class_name("User").optional } + + it { is_expected.to validate_presence_of(:content) } + it { is_expected.to validate_presence_of(:reason) } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index de91767b7..b69de85f2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -34,8 +34,12 @@ 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_one(:profile).dependent(:destroy) } + + it { is_expected.to have_many(:access_grants).class_name("Doorkeeper::AccessGrant").dependent(:delete_all) } + it { is_expected.to have_many(:access_tokens).class_name("Doorkeeper::AccessToken").dependent(:delete_all) } + it { is_expected.to have_many(:ahoy_events).class_name("Ahoy::Event").dependent(:destroy) } + it { is_expected.to have_many(:ahoy_visits).class_name("Ahoy::Visit").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) } @@ -43,33 +47,38 @@ RSpec.describe User, type: :model do it { is_expected.to have_many(:badges).through(:badge_achievements) } it { is_expected.to have_many(:chat_channel_memberships).dependent(:destroy) } it { is_expected.to have_many(:chat_channels).through(:chat_channel_memberships) } - it { is_expected.to have_many(:listings).dependent(:destroy) } it { is_expected.to have_many(:collections).dependent(:destroy) } it { is_expected.to have_many(:comments).dependent(:destroy) } it { is_expected.to have_many(:credits).dependent(:destroy) } it { is_expected.to have_many(:display_ad_events).dependent(:destroy) } it { is_expected.to have_many(:email_authorizations).dependent(:delete_all) } it { is_expected.to have_many(:email_messages).class_name("Ahoy::Message").dependent(:destroy) } + it { is_expected.to have_many(:endorsements).dependent(:destroy) } it { is_expected.to have_many(:field_test_memberships).class_name("FieldTest::Membership").dependent(:destroy) } it { is_expected.to have_many(:github_repos).dependent(:destroy) } it { is_expected.to have_many(:html_variants).dependent(:destroy) } it { is_expected.to have_many(:identities).dependent(:destroy) } + it { is_expected.to have_many(:identities_enabled) } + it { is_expected.to have_many(:listings).dependent(:destroy) } it { is_expected.to have_many(:mentions).dependent(:destroy) } it { is_expected.to have_many(:messages).dependent(:destroy) } - it { is_expected.to have_many(:notes) } + it { is_expected.to have_many(:notes).dependent(:destroy) } it { is_expected.to have_many(:notification_subscriptions).dependent(:destroy) } it { is_expected.to have_many(:notifications).dependent(:destroy) } it { is_expected.to have_many(:organization_memberships).dependent(:destroy) } it { is_expected.to have_many(:organizations).through(:organization_memberships) } - it { is_expected.to have_many(:page_views).dependent(:destroy) } + it { is_expected.to have_many(:page_views).dependent(:nullify) } it { is_expected.to have_many(:poll_skips).dependent(:destroy) } it { is_expected.to have_many(:poll_votes).dependent(:destroy) } it { is_expected.to have_many(:profile_pins).dependent(:delete_all) } - it { is_expected.to have_many(:rating_votes).dependent(:destroy) } + it { is_expected.to have_many(:rating_votes).dependent(:nullify) } it { is_expected.to have_many(:reactions).dependent(:destroy) } it { is_expected.to have_many(:response_templates).dependent(:destroy) } - it { is_expected.to have_many(:tweets).dependent(:destroy) } - it { is_expected.to have_many(:endorsements).dependent(:destroy) } + it { is_expected.to have_many(:source_authored_user_subscriptions).dependent(:destroy) } + it { is_expected.to have_many(:subscribed_to_user_subscriptions).dependent(:destroy) } + it { is_expected.to have_many(:subscribers).dependent(:destroy) } + it { is_expected.to have_many(:tweets).dependent(:nullify) } + it { is_expected.to have_many(:webhook_endpoints).class_name("Webhook::Endpoint").dependent(:delete_all) } # rubocop:disable RSpec/NamedSubject it do @@ -141,12 +150,6 @@ RSpec.describe User, type: :model do .with_foreign_key(:reporter_id) .dependent(:nullify) end - - it do - expect(subject).to have_many(:webhook_endpoints) - .class_name("Webhook::Endpoint") - .dependent(:delete_all) - end # rubocop:enable RSpec/NamedSubject it { is_expected.not_to allow_value("#xyz").for(:bg_color_hex) } diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 319e62a5a..28c69e9c3 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -93,8 +93,14 @@ RSpec.describe Users::Delete, type: :service do describe "deleting associations" do let(:kept_association_names) do %i[ - affected_feedback_messages audit_logs created_podcasts notes - offender_feedback_messages reporter_feedback_messages + affected_feedback_messages + audit_logs + created_podcasts + offender_feedback_messages + page_views + rating_votes + reporter_feedback_messages + tweets ] end let(:direct_associations) do