CODEWITHSUNDEEP
ruby-on-railsruby-on-rails-3if-statement
holyredbeard
holyredbeard

Reputation: 21238

Issue while using || (OR) in if statement

In my project controller I have the actions below. As you can see 'check_if_owner_or_member' is called when the 'show' view is rendered, which checks whether or not a user is a member of the project or administrator. If that's not the case the user get's an error message and gets redirected to the root.

When trying out the action it works if the user is admin, but not if the user is a member. So, something is apparently wrong with 'if !is_owner || !is_member', because it works if I only try with 'if !is_member'.

What am I doing wrong?

before_filter :check_if_owner_or_member, :only => [:show]

def is_owner
    Project.where("id = ? AND user_id = ?", params[:id], current_user.id).count > 0
end

def is_member
    ProjectsUser.where("project_id = ? AND user_id = ?", params[:id], current_user.id).count > 0
end

def check_if_owner_or_member
    if !is_owner || !is_member
        redirect_to root_path
        flash[:error] = "You don't have permission to the project!"
    end
end

Upvotes: 0

Views: 80

Answers (2)

MrYoshiji
MrYoshiji

Reputation: 54882

You should refactor your code like this:

before_filter :check_if_owner_or_member, :only => [:show]

def is_owner?
  Project.exists?(id: params[:id], user_id: current_user.id)
end

def is_member?
  ProjectsUser.exists?(project_id: params[:id], user_id: current_user.id)
end

def check_if_owner_or_member
  unless is_owner? || is_member? # as TheDude said, you probably meant && here
    redirect_to root_path
    flash[:error] = "You don't have permission to the project!"
  end
end

It is more readable and using the exists? method which is faster to execute than finding, counting and comparing to zero.

Upvotes: 1

TheDude
TheDude

Reputation: 3952

Since a member is not an admin, the first part would be true and the second part won't be executed. I imagine that you would want to use && here.

Upvotes: 1

Related Questions