Fix a bug when user tries to delete an account while not having an email (#5486)

* Ask user to add email when deleting [WIP]

* Ask to provide an email before requesting account deletion

* Additional checks to make sure that user won't be deleted w/o an email

* Revert changes on confirm destroy view

app/controllers/users_controller.rb

@@ -65,9 +65,14 @@ class UsersController < ApplicationController
 
   def request_destroy
     set_tabs("account")
-    Users::RequestDestroy.call(@user)
-    flash[:settings_notice] = "You have requested account deletion. Please, check your email for further instructions."
-    redirect_to "/settings/#{@tab}"
+    if @user.email?
+      Users::RequestDestroy.call(@user)
+      flash[:settings_notice] = "You have requested account deletion. Please, check your email for further instructions."
+      redirect_to "/settings/#{@tab}"
+    else
+      flash[:settings_notice] = "Please, provide an email to delete your account"
+      redirect_to "/settings/account"
+    end
   end
 
   def confirm_destroy
@@ -79,10 +84,15 @@ class UsersController < ApplicationController
 
   def full_delete
     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
+    if @user.email?
+      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
+    else
+      flash[:settings_notice] = "Please, provide an email to delete your account"
+      redirect_to "/settings/account"
+    end
   end
 
   def remove_association

app/views/users/_account.html.erb

@@ -88,30 +88,33 @@
 <% end %>
 
 <h3>Delete Account</h3>
-<h4 style="font-weight:400;">
-  <%= form_tag user_request_destroy_path, method: :get, autocomplete: "off", id: "delete__account" do %>
-    Deleting your account will:
-    <ul class="delete__account">
-      <li>delete your profile, along with your Twitter and/or GitHub associations.
-        This does not include app permissions. You will have to remove them yourself on
-        <a href="https://twitter.com/settings/applications">Twitter profile settings</a> and
-        <a href="https://github.com/settings/applications">GitHub profile settings</a>.
-      </li>
-      <%# TODO: expand the delete messaging later %>
-      <li>delete any and all content you have, such as articles, comments, your reading list or chat messages.</li>
-      <li>allow your username to become available to anyone.</li>
-    </ul>
-    <button class="big-action danger-button small-button" id="delete__account__btn" type="submit">
-      DELETE ACCOUNT
-    </button>
-  <% end %>
-</h4>
+<% if current_user.email? %>
+  <h4 style="font-weight:400;">
+    <%= form_tag user_request_destroy_path, method: :get, autocomplete: "off", id: "delete__account" do %>
+      Deleting your account will:
+      <ul class="delete__account">
+        <li>delete your profile, along with your Twitter and/or GitHub associations.
+          This does not include app permissions. You will have to remove them yourself on
+          <a href="https://twitter.com/settings/applications">Twitter profile settings</a> and
+          <a href="https://github.com/settings/applications">GitHub profile settings</a>.
+        </li>
+        <%# TODO: expand the delete messaging later %>
+        <li>delete any and all content you have, such as articles, comments, your reading list or chat messages.</li>
+        <li>allow your username to become available to anyone.</li>
+      </ul>
+      <button class="big-action danger-button small-button" id="delete__account__btn" type="submit">
+        DELETE ACCOUNT
+      </button>
+    <% end %>
+  </h4>
+<% else %>
+  <p>Please, <%= link_to "provide an email", "/settings/profile" %> to fully delete your account.</p>
+<% end %>
 <br>
 If you would like to keep your content under the <%= link_to "@ghost", "/ghost" %> account, please:
 <h3>Request Account Deletion</h3>
 <h4 style="font-weight: 400;">
-  <br>
-  <a href="mailto:<%= SiteConfig.default_site_email %>?subject=Request Account Deletion&body=<%= @email_body %>">
+  <a href="mailto:<%= ApplicationConfig["DEFAULT_SITE_EMAIL"] %>?subject=Request Account Deletion&body=<%= @email_body %>">
     Click this link to request account deletion via email.
   </a>
   <br>

spec/requests/user/user_destroy_spec.rb

@@ -3,48 +3,96 @@ require "rails_helper"
 RSpec.describe "UserDestroy", type: :request do
   let(:user) { create(:user) }
 
-  describe "DELETE /users/full_delete" do
-    before do
+  describe "GET /settings/account" do
+    it "offers to delete account when user has an email" do
       sign_in user
+      get "/settings/account"
+      expect(response.body).to include("DELETE ACCOUNT").and include("Deleting your account will")
     end
 
-    it "schedules a user delete job" do
-      expect do
+    it "offers to set an email when user doesn't have an email" do
+      shallow_user = create(:user, email: nil)
+      sign_in shallow_user
+      get "/settings/account"
+      expect(response.body).not_to include("Deleting your account will")
+      expect(response.body).to include("provide an email")
+    end
+  end
+
+  describe "DELETE /users/full_delete" do
+    context "when user has an email" 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"
-      end.to have_enqueued_job(Users::SelfDeleteJob).with(user.id)
+        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
 
-    it "signs out" do
-      delete "/users/full_delete"
-      expect(controller.current_user).to eq nil
-    end
+    context "when user doesn't have an email" do
+      let!(:shallow_user) { create(:user, email: nil) }
 
-    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")
+      before do
+        sign_in shallow_user
+      end
+
+      it "redirects to account page" do
+        delete "/users/full_delete"
+        expect(response).to redirect_to("/settings/account")
+        expect(flash[:settings_notice]).to include("provide an email")
+      end
     end
   end
 
   describe "GET /users/request_destroy" do
-    before do
-      allow(Rails.cache).to receive(:write).and_call_original
-      allow(NotifyMailer).to receive(:account_deletion_requested_email).and_call_original
-      sign_in user
-      get "/users/request_destroy"
+    context "when user has an email" do
+      before do
+        allow(Rails.cache).to receive(:write).and_call_original
+        allow(NotifyMailer).to receive(:account_deletion_requested_email).and_call_original
+        sign_in user
+        get "/users/request_destroy"
+      end
+
+      it "sends an email" do
+        expect(NotifyMailer).to have_received(:account_deletion_requested_email).with(user, instance_of(String))
+      end
+
+      it "updates the destroy_token" do
+        user.reload
+        expect(Rails.cache).to have_received(:write).with("user-destroy-token-#{user.id}", any_args)
+      end
+
+      it "sets flash notice" do
+        expect(flash[:settings_notice]).to include("You have requested account deletion")
+      end
     end
 
-    it "sends an email" do
-      expect(NotifyMailer).to have_received(:account_deletion_requested_email).with(user, instance_of(String))
-    end
+    context "when user doesn't have an email" do
+      let!(:shallow_user) { create(:user, email: nil) }
 
-    it "updates the destroy_token" do
-      user.reload
-      expect(Rails.cache).to have_received(:write).with("user-destroy-token-#{user.id}", any_args)
-    end
+      before do
+        sign_in shallow_user
+      end
 
-    it "sets flash notice" do
-      expect(flash[:settings_notice]).to include("You have requested account deletion")
+      it "redirects to account page" do
+        get "/users/request_destroy"
+        expect(response).to redirect_to("/settings/account")
+        expect(flash[:settings_notice]).to include("provide an email")
+      end
     end
   end