Avoid delete himself

* Extract in UserDestroy the user destroy system with callback * Avoid delete himself * little refactor on rspec to be more rspec compiliant

Avoid delete himself
* Extract in UserDestroy the user destroy system with callback * Avoid delete himself * little refactor on rspec to be more rspec compiliant
Cyril Mougel
1 parent 2be92c1e
Showing 7 changed files with 76 additions and 20 deletions Show diff stats
app/controllers/users_controller.rb
app/interactors/user_destroy.rb
app/models/user.rb
config/locales/en.yml
spec/controllers/users_controller_spec.rb
spec/fabricators/app_fabricator.rb
spec/interactors/user_destroy_spec.rb
@@ -40,9 +40,12 @@ class UsersController &lt; ApplicationController
   end
  
   def destroy
-    user.destroy
-
-    flash[:success] = "That's sad. #{user.name} is no longer part of your team."
+    if user == current_user
+      flash[:error] = I18n.t('controllers.users.flash.destroy.error')
+    else
+      UserDestroy.new(user).destroy
+      flash[:success] = "That's sad. #{user.name} is no longer part of your team."
+    end
     redirect_to users_path
   end
  
@@ -0,0 +1,11 @@
+class UserDestroy
+  def initialize(user)
+    @user = user
+  end
+
+  def destroy
+    @user.destroy
+    @user.watchers.each(&:destroy)
+  end
+
+end
@@ -13,7 +13,6 @@ class User
   field :per_page, :type => Fixnum, :default => PER_PAGE
   field :time_zone, :default => "UTC"
  
-  after_destroy :destroy_watchers
   before_save :ensure_authentication_token
  
   validates_presence_of :name
@@ -57,10 +56,5 @@ class User
     self[:github_login] = login
   end
  
-  protected
-
-    def destroy_watchers
-      watchers.each(&:destroy)
-    end
 end
  
@@ -14,3 +14,11 @@ en:
   n_errs_have:
     one:   "%{count} err has"
     other: "%{count} errs have"
+
+
+  controllers:
+    users:
+      flash:
+        destroy:
+          success: "That's sad. %{name} is no longer part of your team."
+          error: "You can't delete yourself"
@@ -212,19 +212,30 @@ describe UsersController do
  
     context "DELETE /users/:id" do
  
-      it "destroys the user" do
-        delete :destroy, :id => user.id
-        User.where(:id => user.id).first.should be_nil
-      end
+      context "with a destroy success" do
+        let(:user_destroy) { mock(:destroy => true) }
+
+        before {
+          UserDestroy.should_receive(:new).with(user).and_return(user_destroy)
+          delete :destroy, :id => user.id
+        }
  
-      it "redirects to the users index page" do
-        delete :destroy, :id => user.id
-        response.should redirect_to(users_path)
+        it 'should destroy user' do
+          expect(request.flash[:success]).to eq I18n.t('controllers.users.flash.destroy.success', :name => user.name)
+          response.should redirect_to(users_path)
+        end
       end
  
-      it "sets a message to display" do
-        delete :destroy, :id => user.id
-        request.flash[:success].should include('no longer part of your team')
+      context "with trying destroy himself" do
+        before {
+          UserDestroy.should_not_receive(:new)
+          delete :destroy, :id => admin.id
+        }
+
+        it 'should not destroy user' do
+          response.should redirect_to(users_path)
+          expect(request.flash[:error]).to eq I18n.t('controllers.users.flash.destroy.error')
+        end
       end
     end
   end
@@ -4,7 +4,9 @@ Fabricator(:app) do
 end
  
 Fabricator(:app_with_watcher, :from => :app) do
-  watchers(:count => 1) { |parent, i| Fabricate.build(:watcher, :app => parent) }
+  watchers(:count => 1) { |parent, i|
+    Fabricate.build(:watcher, :app => parent)
+  }
 end
  
 Fabricator(:watcher) do
@@ -0,0 +1,27 @@
+require 'spec_helper'
+
+describe UserDestroy do
+  let(:app) { Fabricate(
+    :app,
+    :watchers => [
+      Fabricate.build(:user_watcher, :user => user)
+    ])
+  }
+
+  describe "#destroy" do
+    let!(:user) { Fabricate(:user) }
+    it 'should delete user' do
+      expect {
+        UserDestroy.new(user).destroy
+      }.to change(User, :count)
+    end
+
+    it 'should delete watcher' do
+      expect {
+        UserDestroy.new(user).destroy
+      }.to change{
+        app.reload.watchers.where(:user_id => user.id).count
+      }.from(1).to(0)
+    end
+  end
+end
...	...	@@ -40,9 +40,12 @@ class UsersController < ApplicationController
40	40	end
41	41
42	42	def destroy
43		- user.destroy
44		-
45		- flash[:success] = "That's sad. #{user.name} is no longer part of your team."
	43	+ if user == current_user
	44	+ flash[:error] = I18n.t('controllers.users.flash.destroy.error')
	45	+ else
	46	+ UserDestroy.new(user).destroy
	47	+ flash[:success] = "That's sad. #{user.name} is no longer part of your team."
	48	+ end
46	49	redirect_to users_path
47	50	end
48	51
...	...
...	...	@@ -0,0 +1,11 @@
	1	+class UserDestroy
	2	+ def initialize(user)
	3	+ @user = user
	4	+ end
	5	+
	6	+ def destroy
	7	+ @user.destroy
	8	+ @user.watchers.each(&:destroy)
	9	+ end
	10	+
	11	+end
...	...
...	...	@@ -13,7 +13,6 @@ class User
13	13	field :per_page, :type => Fixnum, :default => PER_PAGE
14	14	field :time_zone, :default => "UTC"
15	15
16		- after_destroy :destroy_watchers
17	16	before_save :ensure_authentication_token
18	17
19	18	validates_presence_of :name
...	...	@@ -57,10 +56,5 @@ class User
57	56	self[:github_login] = login
58	57	end
59	58
60		- protected
61		-
62		- def destroy_watchers
63		- watchers.each(&:destroy)
64		- end
65	59	end
66	60
...	...
...	...	@@ -14,3 +14,11 @@ en:
14	14	n_errs_have:
15	15	one: "%{count} err has"
16	16	other: "%{count} errs have"
	17	+
	18	+
	19	+ controllers:
	20	+ users:
	21	+ flash:
	22	+ destroy:
	23	+ success: "That's sad. %{name} is no longer part of your team."
	24	+ error: "You can't delete yourself"
...	...
...	...	@@ -212,19 +212,30 @@ describe UsersController do
212	212
213	213	context "DELETE /users/:id" do
214	214
215		- it "destroys the user" do
216		- delete :destroy, :id => user.id
217		- User.where(:id => user.id).first.should be_nil
218		- end
	215	+ context "with a destroy success" do
	216	+ let(:user_destroy) { mock(:destroy => true) }
	217	+
	218	+ before {
	219	+ UserDestroy.should_receive(:new).with(user).and_return(user_destroy)
	220	+ delete :destroy, :id => user.id
	221	+ }
219	222
220		- it "redirects to the users index page" do
221		- delete :destroy, :id => user.id
222		- response.should redirect_to(users_path)
	223	+ it 'should destroy user' do
	224	+ expect(request.flash[:success]).to eq I18n.t('controllers.users.flash.destroy.success', :name => user.name)
	225	+ response.should redirect_to(users_path)
	226	+ end
223	227	end
224	228
225		- it "sets a message to display" do
226		- delete :destroy, :id => user.id
227		- request.flash[:success].should include('no longer part of your team')
	229	+ context "with trying destroy himself" do
	230	+ before {
	231	+ UserDestroy.should_not_receive(:new)
	232	+ delete :destroy, :id => admin.id
	233	+ }
	234	+
	235	+ it 'should not destroy user' do
	236	+ response.should redirect_to(users_path)
	237	+ expect(request.flash[:error]).to eq I18n.t('controllers.users.flash.destroy.error')
	238	+ end
228	239	end
229	240	end
230	241	end
...	...
...	...	@@ -4,7 +4,9 @@ Fabricator(:app) do
4	4	end
5	5
6	6	Fabricator(:app_with_watcher, :from => :app) do
7		- watchers(:count => 1) { \|parent, i\| Fabricate.build(:watcher, :app => parent) }
	7	+ watchers(:count => 1) { \|parent, i\|
	8	+ Fabricate.build(:watcher, :app => parent)
	9	+ }
8	10	end
9	11
10	12	Fabricator(:watcher) do
...	...
...	...	@@ -0,0 +1,27 @@
	1	+require 'spec_helper'
	2	+
	3	+describe UserDestroy do
	4	+ let(:app) { Fabricate(
	5	+ :app,
	6	+ :watchers => [
	7	+ Fabricate.build(:user_watcher, :user => user)
	8	+ ])
	9	+ }
	10	+
	11	+ describe "#destroy" do
	12	+ let!(:user) { Fabricate(:user) }
	13	+ it 'should delete user' do
	14	+ expect {
	15	+ UserDestroy.new(user).destroy
	16	+ }.to change(User, :count)
	17	+ end
	18	+
	19	+ it 'should delete watcher' do
	20	+ expect {
	21	+ UserDestroy.new(user).destroy
	22	+ }.to change{
	23	+ app.reload.watchers.where(:user_id => user.id).count
	24	+ }.from(1).to(0)
	25	+ end
	26	+ end
	27	+end
...	...