Help needed to DRY helpers and proper MVC with Ruby

Hello.

I'm still fumbling learning RoR, so I apologize in advance.

I've got working code, but I don't think that it's done properly so
any
help to make this more efficient and most of all, correct, would be so
very appreciated.

In my users_helper.rb file

def generate_sub_nav
   if session[:user]
      sub_nav_menu = []
     user = User.find(session[:user])
        for role in user.roles
          for right in role.rights
            if right.on_menu
      sub_nav_menu << right.id
            end
        end # for right
      end # for role
    end # if
     sub_nav_menu.sort!
  end # def

In my application_helper.rb file for the layout and display

def show_sub_nav(rights)
  html_out = '<table><tr><td>'

  for right in rights
    right_array = Right.find(:all, :conditions => ["id = ?", right])
    for menu in right_array
      html_out << link_to_unless_current (menu.name,
:controller => menu.controller, :action => menu.action)
    end
  end #for right in rights

  html_out << '</td></tr></table>'
  html_out
end

In my controllers (not including the user_controller)

helper :users

There's got to be a better, more Ruby like way to do this.

THANKS!

In my users_helper.rb file

def generate_sub_nav
   if session[:user]
      sub_nav_menu = []
     user = User.find(session[:user])
        for role in user.roles
          for right in role.rights
            if right.on_menu
      sub_nav_menu << right.id
            end
        end # for right
      end # for role
    end # if
     sub_nav_menu.sort!
  end # def

First, you might map rights to users through roles:

class User < ActiveDirectory::Base
  has_many :rights, :through :roles
end

Then you could reduce that method to:

def generate_sub_nav
  if session[:user] && user = User.find(session[:user])
    user.rights.select {|right| right.on_menu}.sort
  end
end

note that I'm returning an enumeration of Rights objects, not an
enumeration of Right object ids

In my application_helper.rb file for the layout and display

def show_sub_nav(rights)
  html_out = '<table><tr><td>'

  for right in rights
    right_array = Right.find(:all, :conditions => ["id = ?", right])
    for menu in right_array
      html_out << link_to_unless_current (menu.name,
:controller => menu.controller, :action => menu.action)
    end
  end #for right in rights

  html_out << '</td></tr></table>'
  html_out
end

def show_sub_nav(rights)
  html_out = '<table><tr><td>'
  rights.each do |right|
    html_out << link_to_unless_current(h(right.name),
{:controller=>right.controller, :action=>right.action})
  end
  html_out << '</td></tr></table>'
  html_out
end

Other things you could do include including the roles and/or rights in
the User.find method call, limiting the rights to those with the on_menu
attribute, and ordering the results by right id instead of sorting the
Rights objects.

You might also want to consider loading the User object from the
session[:user] id in a filter instead of doing it in the helper, since
you're likely to want to use your User object in a variety of different
places.

I also wonder if show_sub_nav really merits a helper method at all. If
it were me, I'd just put this chunk of code in your standard layout
wrapper.

- donald

Thanks so much for taking the time to work with me on this. I'm
learning so much from user groups!

Here are a couple of things that I tried with results and questions.

I have this in my user.rb file

has_many :roles
has_many :rights, :through => :roles

But now I get a MySQL error because it's looking for a column that
does not exist

Mysql::Error: #42S22Unknown column 'roles.user_id'

The roles table doesn't have a user_id field. I have roles and users
connected through the roles_users table.

The show_sub_nav is part of the menu and was put into the
application_helper.rb file so that it could be accessed by the entire
application. I wasn't able to figure out where else to put it.

I'll keep working through the rest of your suggestions to see how I
can create better code.

THANKS!

Also as an FYI, the users, roles and rights design was taken from the
Pragmatic Programmers Rails Recipes book.

Thanks so much for taking the time to work with me on this.
I'm learning so much from user groups!

No problem.

Here are a couple of things that I tried with results and questions.

I have this in my user.rb file

has_many :roles
has_many :rights, :through => :roles

But now I get a MySQL error because it's looking for a column
that does not exist

Mysql::Error: #42S22Unknown column 'roles.user_id'

The roles table doesn't have a user_id field. I have roles
and users connected through the roles_users table.

Oops, I've led you astray then. I'm actually a bit surprised what you
have works then, I thought when you did habtm you needed to tag both
ends of the relationship as has_and_belongs_to_many. C'e la vie,
disregard that portion of my suggestions.

It's perhaps worth your while to try using the :include feature of
Base.find. Something like:

User.find(session[:user], :include=>{:roles=>{:rights}})

should load the user data along with its roles and rights all in one
query. I may have the syntax wrong, this is from memory.

The show_sub_nav is part of the menu and was put into the
application_helper.rb file so that it could be accessed by
the entire application. I wasn't able to figure out where
else to put it.

Most folks would use ActionView's layout feature for this, I suspect.

- donald

Would I incorporate this into this block here?

  def generate_sub_nav
  if session[:user] && user =
User.find(session[:user], :include=>{:roles=> :rights})
    user.rights.select {|right| right.on_menu}.sort
  end

If so, then I get this error

undefined method `rights' for #<User:0xc966b4c>

But, I also removed this line

has_many :rights, :through => :roles

Basically, replaced one error with a new one.

I'm getting really lost now.

Does anyone know what needs to change? Do I need to add back in
":throgh" and then make changes elsewhere?

Thanks for the help!