librelist archives

« back to archive

newb question. really confusing.

newb question. really confusing.

From:
Dean Michael Ancajas
Date:
2012-08-17 @ 06:44
Hi guys,
  I have a confusing situation. Been trying to solve this. Basically I am
doing Chap 9 of Hartl's tutorial. I am doing the supplemental exercise
where the Admin user should not be allowed to delete itself.

Btw, just ignore the $ signs in my code. It is an artifact on my VIM that i
hadn't have the time to remove.

So here's the code the the rspec test that I came up with:

 51     describe "an admin user" do
 52       let(:admin) { FactoryGirl.create(:admin) }
 53 $
 54       before{sign_in admin}
 55 $
 56 ~~~~~~
 57       describe "deletes himself" do
 58         before{ delete user_path(admin) }
 59         specify { response.should redirect_to (root_path) }
 60       end


And this is my controller code:

4   before_filter :admin_user, only: :destroy$
  5 $
  6 $
  7 $
  8   def destroy$
  9    if (params[:id] == current_user.id)$
 10      flash[:error] = "cannot delete own self"$
 11      redirect_to(root_path)$
 12    else~$
 13      User.find(params[:id]).destroy$
 14      flash[:success] = "User destroyed."$
 15      redirect_to users_path$
 16    end$
 17   end$

73     def admin_user~$
 74       redirect_to(root_path) unless current_user.admin?$
 75     end$

The factory code:

1 FactoryGirl.define do$
  2   factory :user do$
  3 #name     "Michael Hartl"$
  4 #    email    "michael@example.com"$
  5 #    password "foobar"$
  6 #    password_confirmation "foobar"$
  7     sequence(:name)  { |n| "Person #{n}" }$
  8     sequence(:email) { |n| "person_#{n}@example.com"}~~~$
  9     password "foobar"~$
 10     password_confirmation "foobar"$
 11 $
 12     factory :admin do$
 13       admin true$
 14     end$
 15 $
 16   end$
 17 end$

When I run the rspec test. I get the error

 1) Authentication authorization an admin user deletes himself
     Failure/Error: specify { response.should redirect_to (root_path) }
       Expected response to be a redirect to <http://www.example.com/> but
was a redirect to <http://www.example.com/users>


The way I understand this is that it is directing to the rootpath/users
instead of the rootpath because the controller is behaving in reverse. To
verify this, I changed line 9 of the controller code into *"if params[:id]
!= current_user.id"* from* "if params[:id]==current_user.id"* and the test
suddenly works even though it is the reverse of what I wanted to. Of
course, it breaks some other RSPEC tests I have written beforehand. The
variable current_user is defined from a helper which only means that it is
the variable for the user who is currently logged in.

I basically cannot understand why this is happening and would like someone
to kindly shed light on it.

Thanks!

Re: [getarailsjob] newb question. really confusing.

From:
Jason Olson
Date:
2012-08-17 @ 08:24
Dean,

I can't answer your question as to why the conditional you have doesn't 
work, but I can offer a solution.

I have moved the if statement to after the user is found and then used the
current_user?(user) method check to see if you are deleting yourself.

  def destroy
    @user = User.find(params[:id])
    
    if current_user?(@user)
      flash[:error] = "Can't Delete Self"
      redirect_to root_path
    else
      @user.destroy

      respond_to do |format|
        format.html { redirect_to users_url }
        format.json { head :no_content }
      end
    end
  end

This gets your test to pass.

But here is another thought.  In this exercise, there is also a section 
that makes sure the delete link is not available for the current user 
(9.44 and 9.45).  This means that a current user cannot delete themselves 
through the web interface, but there is still a security risk where a 
hacker could spoof the url to delete himself, thus the additional test and
controller modification.

But since there is no web interface available to delete yourself, I would 
argue that the destroy action does not need a redirect, just the 
conditional.  With this in mind, here is my version of the controller and 
revised test.

  def destroy
    @user = User.find(params[:id])
    @user.destroy unless current_user?(@user)

    respond_to do |format|
      format.html { redirect_to users_url }
      format.json { head :no_content }
    end
  end

____________________________

    let(:admin) { FactoryGirl.create(:admin) }
    before { sign_in admin }
        
    it "should not delete current user" do
      expect { delete user_path(admin) }.not_to change(User, :count)
    end


Any other thoughts from the group?  Critique?

- JMO

Re: [getarailsjob] newb question. really confusing.

From:
Allen Maxwell
Date:
2012-08-17 @ 12:16
I'm just guessing based on Jason's successful results:

It may be that the conditional: params[:id] == @current_user.id doesn't
work because the params[:id] is a string value (this is my guess) and the
user.id is an integer so the conditional doesn't work properly.

When Jason first finds the user record then it's comparing apples to
apples and works as desired.

Total stab in the dark since I can't run any tests on the code in
development to verify.  One thing that I've found helpful that David
showed me a while ago is the pry gem.  You can put a "binding.pry" on the
first line of the destroy method and see at run time exactly what you're
comparing.  That may help make sense of it.

Max

On 8/17/12 2:24 AM, "Jason Olson" <jmophoto@me.com> wrote:

>Dean,
>
>I can't answer your question as to why the conditional you have doesn't
>work, but I can offer a solution.
>
>I have moved the if statement to after the user is found and then used
>the current_user?(user) method check to see if you are deleting yourself.
>
>  def destroy
>    @user = User.find(params[:id])
>    
>    if current_user?(@user)
>      flash[:error] = "Can't Delete Self"
>      redirect_to root_path
>    else
>      @user.destroy
>
>      respond_to do |format|
>        format.html { redirect_to users_url }
>        format.json { head :no_content }
>      end
>    end
>  end
>
>This gets your test to pass.
>
>But here is another thought.  In this exercise, there is also a section
>that makes sure the delete link is not available for the current user
>(9.44 and 9.45).  This means that a current user cannot delete themselves
>through the web interface, but there is still a security risk where a
>hacker could spoof the url to delete himself, thus the additional test
>and controller modification.
>
>But since there is no web interface available to delete yourself, I would
>argue that the destroy action does not need a redirect, just the
>conditional.  With this in mind, here is my version of the controller and
>revised test.
>
>  def destroy
>    @user = User.find(params[:id])
>    @user.destroy unless current_user?(@user)
>
>    respond_to do |format|
>      format.html { redirect_to users_url }
>      format.json { head :no_content }
>    end
>  end
>
>____________________________
>
>    let(:admin) { FactoryGirl.create(:admin) }
>    before { sign_in admin }
>        
>    it "should not delete current user" do
>      expect { delete user_path(admin) }.not_to change(User, :count)
>    end
>
>
>Any other thoughts from the group?  Critique?
>
>- JMO

Re: [getarailsjob] newb question. really confusing.

From:
Dean Michael Ancajas
Date:
2012-08-21 @ 00:00
Jason and Max,
  Thanks for the help. Both suggestions are actually correct as I tried
them. I appended a "to_i" in the conditional and that made it worked! I
also found Jason's versions were more elegant and polished.

Thanks everybody

On Fri, Aug 17, 2012 at 6:16 AM, Allen Maxwell <aamax@xmission.com> wrote:

> Total stab in the dark since I can't run any tests on the code in
> development to verify.  One thing that I've found helpful that David
>