From 8c58be75f5562b67486f725eba024bdc8e05f850 Mon Sep 17 00:00:00 2001 From: Anna Buianova Date: Mon, 11 Nov 2019 22:59:01 +0300 Subject: [PATCH] Self-deleting user account (#4480) [deploy] * Start with self deleting account * Moved deleting user content and activity out of moderator hierarchy * Added tests for the users delete services * Tests for Users::DeleteComments * User self-deletion (start) * Some tests for user self-delete * Specs for user self-deletion * Test flash settings on users delete * Added basic specs for the Users::DeleteJob * Send notification when a user was destroyed * Rename Users::DeleteJob to SelfDelete * Change texts about self-deletion * Fix users delete job spec * Rescue and log exceptions while self-deleting user * Added visible flash notices after deleting user * Remove unneeded css for flash notice * Fix link to a ghost account * Remove redundant css * Added github and twitter oauth apps links --- app/assets/stylesheets/shared.scss | 14 ++- app/controllers/users_controller.rb | 15 ++- app/jobs/users/self_delete_job.rb | 15 +++ app/policies/user_policy.rb | 4 + app/services/moderator/delete_user.rb | 8 +- .../moderator/manage_activity_and_roles.rb | 41 +----- app/services/users/delete.rb | 36 ++++++ app/services/users/delete_activity.rb | 17 +++ app/services/users/delete_articles.rb | 26 ++++ app/services/users/delete_comments.rb | 17 +++ app/views/layouts/application.html.erb | 5 + app/views/users/_account.html.erb | 117 +++++++++--------- config/routes.rb | 3 +- spec/jobs/users/self_delete_job_spec.rb | 45 +++++++ .../{ => user}/user_organization_spec.rb | 0 spec/requests/{ => user}/user_profile_spec.rb | 0 .../requests/{ => user}/user_settings_spec.rb | 27 ++++ spec/services/moderator/delete_user_spec.rb | 28 +++++ spec/services/users/delete_articles_spec.rb | 40 ++++++ spec/services/users/delete_comments_spec.rb | 25 ++++ spec/services/users/delete_spec.rb | 33 +++++ spec/system/user/user_self_destroy_spec.rb | 28 +++++ 22 files changed, 434 insertions(+), 110 deletions(-) create mode 100644 app/jobs/users/self_delete_job.rb create mode 100644 app/services/users/delete.rb create mode 100644 app/services/users/delete_activity.rb create mode 100644 app/services/users/delete_articles.rb create mode 100644 app/services/users/delete_comments.rb create mode 100644 spec/jobs/users/self_delete_job_spec.rb rename spec/requests/{ => user}/user_organization_spec.rb (100%) rename spec/requests/{ => user}/user_profile_spec.rb (100%) rename spec/requests/{ => user}/user_settings_spec.rb (94%) create mode 100644 spec/services/moderator/delete_user_spec.rb create mode 100644 spec/services/users/delete_articles_spec.rb create mode 100644 spec/services/users/delete_comments_spec.rb create mode 100644 spec/services/users/delete_spec.rb create mode 100644 spec/system/user/user_self_destroy_spec.rb diff --git a/app/assets/stylesheets/shared.scss b/app/assets/stylesheets/shared.scss index 3beb495f8..2edc6780b 100644 --- a/app/assets/stylesheets/shared.scss +++ b/app/assets/stylesheets/shared.scss @@ -200,4 +200,16 @@ 100% { width: 140%; } -} \ No newline at end of file +} + +.global-notice { + font-family: $helvetica; + background: $green; + color: black; + padding: 20px 0px 20px; + text-align: center; + position: relative; + top: 0px; + left: 0px; + right: 0px; +} diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fd15b5697..977c5f0c4 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -71,7 +71,7 @@ class UsersController < ApplicationController if @user.articles_count.zero? && @user.comments_count.zero? @user.destroy! NotifyMailer.account_deleted_email(@user).deliver - flash[:settings_notice] = "Your account has been deleted." + flash[:global_notice] = "Your account has been deleted." sign_out @user redirect_to root_path else @@ -80,6 +80,15 @@ class UsersController < ApplicationController end end + def full_delete + set_user + set_tabs("account") + Users::SelfDeleteJob.perform_later(@user.id) + sign_out @user + flash[:global_notice] = "Your account deletion is scheduled. You'll be notified when it's deleted." + redirect_to root_path + end + def remove_association set_user provider = params[:provider] @@ -249,10 +258,6 @@ class UsersController < ApplicationController %0A%0A You can keep any comments and discussion posts under the Ghost account. %0A - ---OR--- - %0A - Please delete all my personal information, including comments and discussion posts. - %0A %0A Regards, %0A diff --git a/app/jobs/users/self_delete_job.rb b/app/jobs/users/self_delete_job.rb new file mode 100644 index 000000000..b69096df8 --- /dev/null +++ b/app/jobs/users/self_delete_job.rb @@ -0,0 +1,15 @@ +module Users + class SelfDeleteJob < ApplicationJob + queue_as :users_self_delete + + def perform(user_id, service = Users::Delete) + user = User.find_by(id: user_id) + return unless user + + service.call(user) + NotifyMailer.account_deleted_email(user).deliver + rescue StandardError => e + Rails.logger.error("Error while deleting user: #{e}") + end + end +end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 542417016..83f306737 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -27,6 +27,10 @@ class UserPolicy < ApplicationPolicy current_user? end + def full_delete? + current_user? + end + def join_org? !user_is_banned? end diff --git a/app/services/moderator/delete_user.rb b/app/services/moderator/delete_user.rb index 8e63a2863..4fef3bffd 100644 --- a/app/services/moderator/delete_user.rb +++ b/app/services/moderator/delete_user.rb @@ -12,7 +12,7 @@ module Moderator if user_params[:ghostify] == "true" new(user: user, admin: admin, user_params: user_params).ghostify else - new(user: user, admin: admin, user_params: user_params).full_delete + Users::Delete.call(user) end end @@ -24,12 +24,6 @@ module Moderator CacheBuster.new.bust("/ghost") end - def full_delete - delete_comments - delete_articles - delete_non_content_activity_and_user - end - private def delete_non_content_activity_and_user diff --git a/app/services/moderator/manage_activity_and_roles.rb b/app/services/moderator/manage_activity_and_roles.rb index 894f81707..1a0b364f3 100644 --- a/app/services/moderator/manage_activity_and_roles.rb +++ b/app/services/moderator/manage_activity_and_roles.rb @@ -13,50 +13,15 @@ module Moderator end def delete_comments - return unless user.comments.any? - - cachebuster = CacheBuster.new - user.comments.find_each do |comment| - comment.reactions.delete_all - cachebuster.bust_comment(comment.commentable) - comment.delete - comment.remove_notifications - end - cachebuster.bust_user(user) + Users::DeleteComments.call(user) end def delete_articles - return unless user.articles.any? - - cachebuster = CacheBuster.new - virtual_articles = user.articles.map { |article| Article.new(article.attributes) } - user.articles.find_each do |article| - article.reactions.delete_all - article.comments.includes(:user).find_each do |comment| - comment.reactions.delete_all - cachebuster.bust_comment(comment.commentable) - cachebuster.bust_user(comment.user) - comment.delete - end - article.remove_algolia_index - article.delete - article.purge - end - virtual_articles.each do |article| - cachebuster.bust_article(article) - end + Users::DeleteArticles.call(user) end def delete_user_activity - user.notifications.delete_all - user.reactions.delete_all - user.follows.delete_all - Follow.where(followable_id: user.id, followable_type: "User").delete_all - user.messages.delete_all - user.chat_channel_memberships.delete_all - user.mentions.delete_all - user.badge_achievements.delete_all - user.github_repos.delete_all + Users::DeleteActivity.call(user) end def remove_privileges diff --git a/app/services/users/delete.rb b/app/services/users/delete.rb new file mode 100644 index 000000000..dd4a92c16 --- /dev/null +++ b/app/services/users/delete.rb @@ -0,0 +1,36 @@ +module Users + class Delete + def initialize(user) + @user = user + end + + def call + delete_comments + delete_articles + delete_user_activity + user.unsubscribe_from_newsletters + CacheBuster.new.bust("/#{user.username}") + user.delete + end + + def self.call(*args) + new(*args).call + end + + private + + attr_reader :user + + def delete_user_activity + DeleteActivity.call(user) + end + + def delete_comments + DeleteComments.call(user) + end + + def delete_articles + DeleteArticles.call(user) + end + end +end diff --git a/app/services/users/delete_activity.rb b/app/services/users/delete_activity.rb new file mode 100644 index 000000000..070fff596 --- /dev/null +++ b/app/services/users/delete_activity.rb @@ -0,0 +1,17 @@ +module Users + module DeleteActivity + module_function + + def call(user) + user.notifications.delete_all + user.reactions.delete_all + user.follows.delete_all + Follow.where(followable_id: user.id, followable_type: "User").delete_all + user.messages.delete_all + user.chat_channel_memberships.delete_all + user.mentions.delete_all + user.badge_achievements.delete_all + user.github_repos.delete_all + end + end +end diff --git a/app/services/users/delete_articles.rb b/app/services/users/delete_articles.rb new file mode 100644 index 000000000..0b6591248 --- /dev/null +++ b/app/services/users/delete_articles.rb @@ -0,0 +1,26 @@ +module Users + module DeleteArticles + module_function + + def call(user, cache_buster = CacheBuster.new) + return unless user.articles.any? + + virtual_articles = user.articles.map { |article| Article.new(article.attributes) } + user.articles.find_each do |article| + article.reactions.delete_all + article.comments.includes(:user).find_each do |comment| + comment.reactions.delete_all + cache_buster.bust_comment(comment.commentable) + cache_buster.bust_user(comment.user) + comment.delete + end + article.remove_algolia_index + article.delete + article.purge + end + virtual_articles.each do |article| + cache_buster.bust_article(article) + end + end + end +end diff --git a/app/services/users/delete_comments.rb b/app/services/users/delete_comments.rb new file mode 100644 index 000000000..662a943c4 --- /dev/null +++ b/app/services/users/delete_comments.rb @@ -0,0 +1,17 @@ +module Users + module DeleteComments + module_function + + def call(user, cache_buster = CacheBuster.new) + return unless user.comments.any? + + user.comments.find_each do |comment| + comment.reactions.delete_all + cache_buster.bust_comment(comment.commentable) + comment.delete + comment.remove_notifications + end + cache_buster.bust_user(user) + end + end +end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 5e1a490b0..51e7f5011 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -88,6 +88,11 @@ <% end %>
+ <% if flash[:global_notice] %> +
+ <%= flash[:global_notice] %> +
+ <% end %>
<%= yield %>
diff --git a/app/views/users/_account.html.erb b/app/views/users/_account.html.erb index 09b532e6c..5fa3d65dc 100644 --- a/app/views/users/_account.html.erb +++ b/app/views/users/_account.html.erb @@ -66,7 +66,7 @@ Note that this does not revoke our OAuth app access; you will have to do so in your - Twitter profile settings or your + Twitter profile settings or your GitHub profile settings.
@@ -87,71 +87,72 @@ <% end %> -<% if @user.articles_count.zero? && @user.comments_count.zero? %> -

Delete Account

-

- <%= form_tag "/users/destroy", method: :delete, autocomplete: "off" do %> - Deleting your account will: - -
- - -
-
- - -
- - <% end %> -

-
- -<% end %> + deleteAcccountVerificationInput.addEventListener('input', function () { + if (bothInputsVerified()) { + deleteAccountBtn.disabled = false; + } else { + deleteAccountBtn.disabled = true; + } + }) + +If you would like to keep your content under the <%= link_to "@ghost", "/ghost" %> account, please:

Request Account Deletion


?subject=Request Account Deletion&body=<%= @email_body %>"> Click this link to request account deletion via email. - This includes all articles, comments, reactions, etc. as well as any personal information you have. -
+
Be sure to change the email template to fit your needs.

diff --git a/config/routes.rb b/config/routes.rb index bc0d2d151..a3f43ca26 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -227,7 +227,8 @@ Rails.application.routes.draw do post "users/remove_org_admin" => "users#remove_org_admin" post "users/remove_from_org" => "users#remove_from_org" delete "users/remove_association", to: "users#remove_association" - delete "users/destroy", to: "users#destroy" + delete "users/destroy", to: "users#destroy", as: :user_destroy + delete "users/full_delete", to: "users#full_delete", as: :user_full_delete post "organizations/generate_new_secret" => "organizations#generate_new_secret" post "users/api_secrets" => "api_secrets#create", :as => :users_api_secrets delete "users/api_secrets/:id" => "api_secrets#destroy", :as => :users_api_secret diff --git a/spec/jobs/users/self_delete_job_spec.rb b/spec/jobs/users/self_delete_job_spec.rb new file mode 100644 index 000000000..bff321790 --- /dev/null +++ b/spec/jobs/users/self_delete_job_spec.rb @@ -0,0 +1,45 @@ +require "rails_helper" + +RSpec.describe Users::SelfDeleteJob, type: :job do + include_examples "#enqueues_job", "users_self_delete", 1 + + describe "#perform_now" do + let(:user) { create(:user) } + let(:delete) { double } + + before do + allow(delete).to receive(:call) + end + + context "when user is found" do + it "calls the service when a user is found" do + described_class.perform_now(user.id, delete) + expect(delete).to have_received(:call).with(user) + end + + it "sends the notification" do + expect do + described_class.perform_now(user.id, delete) + end.to change(ActionMailer::Base.deliveries, :count).by(1) + end + + it "sends the correct notification" do + allow(NotifyMailer).to receive(:account_deleted_email).and_call_original + described_class.perform_now(user.id, delete) + expect(NotifyMailer).to have_received(:account_deleted_email).with(user) + end + end + + context "when user is not found" do + it "doesn't fail" do + described_class.perform_now(-1, delete) + end + + it "doesn't send the notification" do + expect do + described_class.perform_now(-1, delete) + end.not_to change(ActionMailer::Base.deliveries, :count) + end + end + end +end diff --git a/spec/requests/user_organization_spec.rb b/spec/requests/user/user_organization_spec.rb similarity index 100% rename from spec/requests/user_organization_spec.rb rename to spec/requests/user/user_organization_spec.rb diff --git a/spec/requests/user_profile_spec.rb b/spec/requests/user/user_profile_spec.rb similarity index 100% rename from spec/requests/user_profile_spec.rb rename to spec/requests/user/user_profile_spec.rb diff --git a/spec/requests/user_settings_spec.rb b/spec/requests/user/user_settings_spec.rb similarity index 94% rename from spec/requests/user_settings_spec.rb rename to spec/requests/user/user_settings_spec.rb index 2f2cd6d30..256ecbd2b 100644 --- a/spec/requests/user_settings_spec.rb +++ b/spec/requests/user/user_settings_spec.rb @@ -270,6 +270,10 @@ RSpec.describe "UserSettings", type: :request do it "redirects successfully to the home page" do expect(response).to redirect_to "/" end + + it "sets flash settings" do + expect(flash[:global_notice]).to include("has been deleted") + end end context "when users are not allowed to destroy" do @@ -303,4 +307,27 @@ RSpec.describe "UserSettings", type: :request do end end end + + describe "DELETE /users/full_delete" do + before do + sign_in user + end + + it "schedules a user delete job" do + expect do + delete "/users/full_delete" + end.to have_enqueued_job(Users::SelfDeleteJob).with(user.id) + end + + it "signs out" do + delete "/users/full_delete" + expect(controller.current_user).to eq nil + end + + it "redirects to root" do + delete "/users/full_delete" + expect(response).to redirect_to "/" + expect(flash[:global_notice]).to include("Your account deletion is scheduled") + end + end end diff --git a/spec/services/moderator/delete_user_spec.rb b/spec/services/moderator/delete_user_spec.rb new file mode 100644 index 000000000..9153d60ee --- /dev/null +++ b/spec/services/moderator/delete_user_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +RSpec.describe Moderator::DeleteUser, type: :service do + let(:user) { create(:user) } + let(:admin) { create(:user, :super_admin) } + + describe "delete_user" do + it "deletes user" do + described_class.call_deletion(user: user, admin: admin, user_params: {}) + expect(User.find_by(id: user.id)).to be_nil + end + + it "deletes user's follows" do + create(:follow, follower: user) + create(:follow, followable: user) + + expect do + described_class.call_deletion(user: user, admin: admin, user_params: {}) + end.to change(Follow, :count).by(-2) + end + + it "deletes user's articles" do + article = create(:article, user: user) + described_class.call_deletion(user: user, admin: admin, user_params: {}) + expect(Article.find_by(id: article.id)).to be_nil + end + end +end diff --git a/spec/services/users/delete_articles_spec.rb b/spec/services/users/delete_articles_spec.rb new file mode 100644 index 000000000..bffb6276c --- /dev/null +++ b/spec/services/users/delete_articles_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +RSpec.describe Users::DeleteArticles, type: :service do + let(:user) { create(:user) } + let(:user2) { create(:user) } + let!(:article) { create(:article, user: user) } + let!(:article2) { create(:article, user: user) } + let!(:article3) { create(:article, user: user2) } + + it "deletes articles" do + described_class.call(user) + expect(Article.find_by(id: article.id)).to be_nil + expect(Article.find_by(id: article2.id)).to be_nil + expect(Article.find(article3.id)).to be_present + end + + context "with comments" do + let(:buster) { double } + + before do + allow(buster).to receive(:bust_comment) + allow(buster).to receive(:bust_article) + allow(buster).to receive(:bust_user) + + create_list(:comment, 2, commentable: article, user: user2) + end + + it "deletes articles' comments" do + described_class.call(user) + expect(Comment.where(commentable_id: article, commentable_type: "Article").any?).to be false + end + + it "busts cache" do + described_class.call(user, buster) + expect(buster).to have_received(:bust_comment).with(article).twice + expect(buster).to have_received(:bust_user).with(user2).at_least(:once) + expect(buster).to have_received(:bust_article).with(article) + end + end +end diff --git a/spec/services/users/delete_comments_spec.rb b/spec/services/users/delete_comments_spec.rb new file mode 100644 index 000000000..69a61ca87 --- /dev/null +++ b/spec/services/users/delete_comments_spec.rb @@ -0,0 +1,25 @@ +require "rails_helper" + +RSpec.describe Users::DeleteComments, type: :service do + let(:user) { create(:user) } + let(:article) { create(:article) } + let(:buster) { double } + + before do + create_list(:comment, 2, commentable: article, user: user) + + allow(buster).to receive(:bust_comment) + allow(buster).to receive(:bust_user) + end + + it "destroys user comments" do + described_class.call(user, buster) + expect(Comment.where(user_id: user.id).any?).to be false + end + + it "busts cache" do + described_class.call(user, buster) + expect(buster).to have_received(:bust_comment).with(article).at_least(:once) + expect(buster).to have_received(:bust_user).with(user) + end +end diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb new file mode 100644 index 000000000..925fda1c6 --- /dev/null +++ b/spec/services/users/delete_spec.rb @@ -0,0 +1,33 @@ +require "rails_helper" + +RSpec.describe Users::Delete, type: :service do + let(:user) { create(:user) } + + it "deletes user" do + described_class.call(user) + expect(User.find_by(id: user.id)).to be_nil + end + + it "busts user profile page" do + buster = double + allow(buster).to receive(:bust) + allow(CacheBuster).to receive(:new).and_return(buster) + described_class.new(user).call + expect(buster).to have_received(:bust).with("/#{user.username}") + end + + it "deletes user's follows" do + create(:follow, follower: user) + create(:follow, followable: user) + + expect do + described_class.call(user) + end.to change(Follow, :count).by(-2) + end + + it "deletes user's articles" do + article = create(:article, user: user) + described_class.call(user) + expect(Article.find_by(id: article.id)).to be_nil + end +end diff --git a/spec/system/user/user_self_destroy_spec.rb b/spec/system/user/user_self_destroy_spec.rb new file mode 100644 index 000000000..d623e94eb --- /dev/null +++ b/spec/system/user/user_self_destroy_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +RSpec.describe "User destroys their profile", type: :system, js: true do + let(:user) { create(:user, saw_onboarding: true) } + + before do + sign_in user + end + + it "destroys a user without content" do + visit "/settings/account" + fill_in "delete__account__username__field", with: user.username + fill_in "delete__account__verification__field", with: "delete my account" + click_button "DELETE ACCOUNT" + expect(User.find_by(id: user.id).present?).to be false + end + + it "destroys a user with content" do + create(:article, user: user) + user.update_attribute(:articles_count, 1) + visit "/settings/account" + fill_in "delete__account__username__field", with: user.username + fill_in "delete__account__verification__field", with: "delete my account" + expect do + click_button "DELETE ACCOUNT" + end.to have_enqueued_job(Users::SelfDeleteJob) + end +end