Help needed to DRY helpers and proper MVC with Ruby

rubyonrails-talk
1

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!

2

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

3

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!

4

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

5

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

6

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.

7

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

Thanks for the help!