CODEWITHSUNDEEP
ruby-on-railsruby
user3379926
user3379926

Reputation: 3945

Helper Method always returning true even when proven wrong

I have the following helper method in rails 4.

  def is_blog_owner?(blog_id)
    if current_user && blog_id
      blog = Blog.find_by(id: blog_id)
      roles = current_user.roles_for_blog(blog)
      roles.each do |role|
        if role.role == 'Blog-Owner'
          true
        end
      end
    end
  end

It has one problem, if the roles for a current user are nil it always seems to return true.

The way this currently works is that if a current user has a role of blog owner for a specific blog, then return true.

So if I visit (as user id 1) users/1/blogs/2 I will see edit and delete as shown below in the show.html.erb How ever if I then log out and log in as user id 2 and visit users/1/blogs/2 I still see edit and delete. Which I should not.

So I threw a binding.pry after roles gets set and found roles for user id 2 on user id 1's blog id of 2 to be nil This should mean that I should not see the edit and delete buttons, but I do ... What is going on?!

<h2><%[email protected]%> Profile</h2>

<p class="text-muted">
    Welcome to your blog porfile page. Here you can manage the blog. You can edit or
    delete the specific blog.
</p>

<% if is_blog_owner?(params[:id]) %>

  <hr>
  <h3>Action</h3>
  <p class="text-muted">You can currently do the following actions: </p>

  <%= link_to "Edit", edit_user_blog_path(current_user.id, @blog.id), :class => 'btn btn-success' %> |
  <%= link_to "Delete", user_blog_path(current_user.id, @blog.id),
  data: { confirm: "This is permenent. Are you sure?" },
  :method => :delete,
  :class => 'btn btn-danger'%>

<% end %>

I should ad that I did a <%= is_blog_owner?(params[:id]).inspect and got a [] returned ... oO. Should it not return false?

Upvotes: 0

Views: 114

Answers (2)

Pavling
Pavling

Reputation: 3963

Your problem is that roles.each ... returns the enumerator it was called on - so essentially your method always returns roles.

To sort it, change it thus:

def is_blog_owner?(blog_id)
  if current_user && blog_id
    blog = Blog.find_by(id: blog_id)
    roles = current_user.roles_for_blog(blog)
    roles.each do |role|
      if role.role == 'Blog-Owner'
        return true
      end
    end
  end
  return false
end

but it might be better to re-write it to make more sense about what it's doing. So it's looking through the roles the current users has for the blog, and returns true if any of them are 'Blog-Owner'?

Firstly, it might be better to use some authorisation process (like CanCan) to isolate this, but if you insist on a method of your own you could streamline it with .detect:

def is_blog_owner?(blog_id)
  if current_user && blog_id
    blog = Blog.find_by(id: blog_id)
    roles = current_user.roles_for_blog(blog)
    roles.detect do |role|
      role.role == 'Blog-Owner'
    end
  end
end

Which returns the first element in the enumerator that matches the block, otherwise it return nil if no element matches.

Upvotes: 0

Neil Slater
Neil Slater

Reputation: 27207

This construct is a problem for you:

  roles.each do |role|
    if role.role == 'Blog-Owner'
      true
    end
  end

it returns the value of roles, which presumably will be an Array and thus always a true value. The standalone true inside the block is not returned, that's not how .each works. Generally you use .each to process items in an array to change, or output based on each item, or perhaps perform some side-effect based on each one. The return value is always the Array object, and not related to what you do inside the block.

Instead, you could use the method .any? which seems to match your intent:

  roles.any? do |role|
    role.role == 'Blog-Owner'
  end

Upvotes: 2

Related Questions