Ruby Ch9 Tutorial Exercise # 9 - Do Not Allow Administrators to Delete themselves

Question

I am new to Ruby and Rails, so I go through the Michael Hartl Rails Tutorial. I am stuck in Chapter 9, Exercise # 9. I updated the def destroy code in the User Controller to:

def destroy user = User.find(params[:id]) if (current_user == user) && (current_user.admin?) flash[:error] = "Can not delete own admin account!" else user.destroy flash[:success] = "User destroyed." end redirect_to users_path end 

This seems to work when I test in the browser by adding the delete link to current_user when the administrator is logged in. But this exercise says first to write a test that I did, but it doesn’t work. Here is what I have for the test:

 describe "as admin user" do let(:user_admin) { FactoryGirl.create(:admin) } before { sign_in user_admin } describe "submitting a DELETE request to destroy own admin account" do before { delete user_path(user_admin) } it { should have_selector('div.alert.alert-error', text: 'delete own admin') } end end 

Maybe what I'm testing should not be tested. How do you check def destroy code modification in user controller?

Answer 1

I tried to get the code in the original message to work, but no luck. Instead, I made it work this way (so that it would fail and then go through). This verifies that the redirect is correct, as well as the correct flash message.

TEST: authentication_pages_spec.rb

  describe "as admin user" do let(:admin) { FactoryGirl.create(:admin) } before { sign_in admin } describe "can't delete self by submitting DELETE request to Users#destroy" do before { delete user_path(admin) } specify { response.should redirect_to(users_path), flash[:error].should =~ /Can not delete own admin account!/i } end end 

IMPLEMENTATION: Users # destroy

 def destroy user = User.find(params[:id]) if (current_user == user) && (current_user.admin?) flash[:error] = "Can not delete own admin account!" else user.destroy flash[:success] = "User destroyed." end redirect_to users_path end 

Perhaps the reason the original test didn't work is because we are issuing a request? I tried to add each of the following elements to the description block, and all failed:

 it { should have_selector('div.alert.alert-error', text: 'delete own admin') } it { should have_selector('title', text: 'All users') } it { should have_selector('h1', text: 'All users') } 

So, it looks like Capybara is not actually being redirected to the page to check for these selectors. I tried the "title" and "h1", thinking maybe there was some problem with the selector "div.alert.alert-error" ... but the "title" and "h1" failed with the same "expected CSS to return that "..."

Does anyone know more about how the style tests specify { response.should ... } ? If they do not follow call forwarding, when do they click on the controller action?

Answer 2

I'm also new to the Rails Tutorial (and Rails in general) and had the same problem, and your question helped me figure out the answer.

I'm still not sure why your code didn’t work, but the following steps definitely worked.

First, modify the test code a bit to use the following structure (I didn’t specify where to place this description block here - you already have a place):

 describe "deleting herself" do it "should not be possible" do expect { delete user_path(admin) }.to_not change(User, :count).by(-1) end end 

Note that I use the wait block {} to track the number of User objects. This definitely makes the tests go red (which is good at this point), while checking for Flash will also make the test go red, but checking for errors does not work here. I really don't know why! Maybe something is related to the double redirection that happens?

Then write a security code to repeat the tests. Your code works (I think), but I think my code is a little more idiomatic in the sense that it uses the session helpers, defined earlier in chapter 9.

 def destroy user = User.find(params[:id]) if (current_user? user) && (current_user.admin?) flash[:error] = "You are not allowed to delete yourself as an admin." else user.destroy flash[:success] = "User destroyed. ID: #{user.id}" end redirect_to users_path end 

This change made my test pass green again, which successfully completed exercise 10.

Answer 3

It’s also worth noting that in the tutorial in Listing 9.43, the partial view in the application is / views / users / _user.html. erb has a check not to show the delete link for the current user with administrator rights on the user index page.

Thus, although the user cannot delete his own account through the web interface, I think that exercise 9.9 goes further, also providing logic at the controller level if someone created and sent an http delete request for the current user.

Probably a good practice is to add these guards around your rail applications to prevent any strange errors from occurring.

Since the filter on the partial view makes it so that you never see the error anyway, you can also simplify the action of the User controller by destroying the action by removing the "else".

 def destroy user = User.find(params[:id]) unless current_user?(user) user.destroy flash[:success] = "User deleted." end redirect_to users_url end 

Answer 4

I am also new to Rails, just doing the tutorial for the first time, your messages help me a lot, but just to contribute, you really do not need to check if the user is an administrator in the destruction, because the destruction will be available only to users admin when adding a line

 before_action :admin_user, only: :destroy 

In the user controller.

So just ask if it matches the current user.

 def destroy usertodestroy = User.find(params[:id]) if (current_user == usertodestroy) flash[:error] = 'Can´t delete own user' else usertodestroy.destroy flash[:success] = "User destroyed. ID: #{usertodestroy.name}" redirect_to users_url end end 

Also in the test you should just ask that the counter has not changed after an attempt to delete

 describe "as admin user" do let(:admin) { FactoryGirl.create(:admin) } before { sign_in(admin) } it "should not be able to delete itself" do expect { delete user_path(admin) }.not_to change(User, :count) end end 

Both do not change the results, but simply simplify things.

Why I really could not understand why the following code in any case removes the user during testing:

 describe "as admin user" do let(:admin) { FactoryGirl.create(:admin) } before { sign_in(admin) } it "should not be able to delete itself" do expect { admin.destroy }.not_to change(User, :count) end end 

In my opinion, this directly calls the UserController, so it should not be deleted.

Answer 5

Despite the above recommendations, my test still failed, with an error:

 undefined method `admin?' for nil:NilClass 

I realized that there is some problem with logging in, as this is only called part of the before_filter 'admin_user' check.

I was able to solve this problem using the non-capybara version of the sign method

 before { signin admin, no_capybara: true } 

Thanks!

Answer 6

I have applied the eblume suggestions forever, I have some comments and doubts:

Firstly, the test can be simplified, since we do not need to check whether the score on one device changes, but if it changes to any number:

 expect { delete user_path(admin) }.not_to change(User, :count) 

As for the code in the controller, it can also be simplified. As we encoded the following before the kill action:

 def admin_user redirect_to(root_url) unless current_user.admin? end 

there is no need to check the destroy method; if the user is an administrator, he must be an administrator.

So the if statement:

 if (current_user? user) 

Now my problem: I do not understand this code, I do not know what this check does.

My first attempt was as follows:

 if (current_user.id == params[:id]) 

But this does not work, I do not understand why.

Answer 7

Controller action

 def destroy usertodestroy = User.find(params[:id]) if (current_user == usertodestroy) flash[:error] = 'Can´t delete own user' redirect_to root_url else usertodestroy.destroy flash[:success] = "User destroyed. ID: #{usertodestroy.name}" redirect_to users_url end end 

and test

 describe "as admin user" do let(:admin) { FactoryGirl.create(:admin) } before { sign_in admin, no_capybara: true } it "attempting to delete self" do expect{ delete user_path(admin) }.not_to change(User, :count) end end 

work for me.