maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

[deploy] Add missing dependent clauses to User and cleanup relations (#10011)

Browse source 
* Add dependent: :destroy relation to user.notes

* Add cleanup script for orphaned notes

* Reorder relations and reorganize specs in User

* Update specs as notes are not needed after a user is deleted

* Tweets can just be nullified

* Add other cleanup scripts

* Tell the specs we keep tweets

* Fix comments

* Keep page_views and rating_votes when a user is deleted

* Fix specs
This commit is contained in:
rhymes 2020-08-28 20:19:24 +02:00 • committed by GitHub
parent 4786cbddfc
commit ba8b3dd443
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 208 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
.rubocop_todo.yml
View file

@ -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

3
app/models/note.rb
View file

@ -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

48
app/models/user.rb
View file

@ -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

3
app/services/users/delete_activity.rb
View file

@ -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

14
lib/data_update_scripts/20200826092816_remove_orphaned_notes_by_user.rb Normal file
View file

@ -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

15
lib/data_update_scripts/20200826133748_nullify_orphaned_tweets_by_user.rb Normal file
View file

@ -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

13
lib/data_update_scripts/20200826140317_remove_orphaned_articles_by_user.rb Normal file
View file

@ -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

13
lib/data_update_scripts/20200826140754_remove_orphaned_collections_by_user.rb Normal file
View file

@ -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

14
lib/data_update_scripts/20200826140924_remove_orphaned_credits_by_user.rb Normal file
View file

@ -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

13
lib/data_update_scripts/20200826141015_remove_orphaned_github_repos_by_user.rb Normal file
View file

@ -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

13
lib/data_update_scripts/20200826141105_remove_orphaned_mentions_by_user.rb Normal file
View file

@ -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

13
lib/data_update_scripts/20200826141145_remove_orphaned_organization_memberships_by_user.rb Normal file
View file

@ -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

15
lib/data_update_scripts/20200826141447_nullify_orphaned_page_views_by_user.rb Normal file
View file

@ -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

15
lib/data_update_scripts/20200826141550_nullify_orphaned_rating_votes_by_user.rb Normal file
View file

@ -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

13
lib/data_update_scripts/20200826141652_remove_orphaned_reactions_by_user.rb Normal file
View file

@ -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

5
spec/models/note_spec.rb
View file

@ -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

31
spec/models/user_spec.rb
View file

@ -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) }

10
spec/services/users/delete_spec.rb
View file

@ -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