Various code cleaning, mainly on User, Permission and IssueStatus models.

git-svn-id: http://redmine.rubyforge.org/svn/trunk@414 e93f8b46-1217-0410-a6f0-8f06a7374b81

8 files changed, 23 insertions, 33 deletions

diff --git a/app/controllers/application.rb b/app/controllers/application.rb
index 36287ff1c..dd8d71193 100644
--- a/app/controllers/application.rb
+++ b/app/controllers/application.rb
@@ -86,8 +86,8 @@ class ApplicationController < ActionController::Base
     # admin is always authorized
     return true if self.logged_in_user.admin?
     # if not admin, check membership permission    
-    @user_membership ||= Member.find(:first, :conditions => ["user_id=? and project_id=?", self.logged_in_user.id, @project.id])    
-    if @user_membership and Permission.allowed_to_role( "%s/%s" % [ ctrl, action ], @user_membership.role_id )    
+    @user_membership ||= logged_in_user.role_for_project(@project)
+    if @user_membership and Permission.allowed_to_role( "%s/%s" % [ ctrl, action ], @user_membership )    
       return true		
     end		
     render :nothing => true, :status => 403
diff --git a/app/controllers/feeds_controller.rb b/app/controllers/feeds_controller.rb
index 659006c80..d46a63e38 100644
--- a/app/controllers/feeds_controller.rb
+++ b/app/controllers/feeds_controller.rb
@@ -84,7 +84,7 @@ private
       # project feed
       # check if project is public or if the user is a member
       @project = Project.find(params[:project_id])
-      render(:nothing => true, :status => 403) and return false unless @project.is_public? || (@user && @user.role_for_project(@project.id))
+      render(:nothing => true, :status => 403) and return false unless @project.is_public? || (@user && @user.role_for_project(@project))
       scope = ["#{Project.table_name}.id=?", params[:project_id].to_i]
     else
       # global feed
diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 172ab443a..a23837940 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -25,7 +25,7 @@ class IssuesController < ApplicationController
   include IfpdfHelper
 
   def show
-    @status_options = ([@issue.status] + @issue.status.workflows.find(:all, :order => 'position', :include => :new_status, :conditions => ["role_id=? and tracker_id=?", self.logged_in_user.role_for_project(@project.id), @issue.tracker.id]).collect{ |w| w.new_status }) if self.logged_in_user
+    @status_options = @issue.status.find_new_statuses_allowed_to(logged_in_user.role_for_project(@project), @issue.tracker) if logged_in_user
     @custom_values = @issue.custom_values.find(:all, :include => :custom_field)
     @journals_count = @issue.journals.count
     @journals = @issue.journals.find(:all, :include => [:user, :details], :limit => 15, :order => "#{Journal.table_name}.created_on desc")
@@ -67,9 +67,6 @@ class IssuesController < ApplicationController
   def add_note
     unless params[:notes].empty?
       journal = @issue.init_journal(self.logged_in_user, params[:notes])
-      #@history = @issue.histories.build(params[:history])
-      #@history.author_id = self.logged_in_user.id if self.logged_in_user
-      #@history.status = @issue.status
       if @issue.save
         flash[:notice] = l(:notice_successful_update)
         Mailer.deliver_issue_edit(journal) if Permission.find_by_controller_and_action(params[:controller], params[:action]).mail_enabled?
@@ -82,17 +79,10 @@ class IssuesController < ApplicationController
   end
 
   def change_status
-    #@history = @issue.histories.build(params[:history])	
-    @status_options = ([@issue.status] + @issue.status.workflows.find(:all, :order => 'position', :include => :new_status, :conditions => ["role_id=? and tracker_id=?", self.logged_in_user.role_for_project(@project.id), @issue.tracker.id]).collect{ |w| w.new_status }) if self.logged_in_user
+    @status_options = @issue.status.find_new_statuses_allowed_to(logged_in_user.role_for_project(@project), @issue.tracker) if logged_in_user
     @new_status = IssueStatus.find(params[:new_status_id])
     if params[:confirm]
       begin
-        #@history.author_id = self.logged_in_user.id if self.logged_in_user
-        #@issue.status = @history.status
-        #@issue.fixed_version_id = (params[:issue][:fixed_version_id])
-        #@issue.assigned_to_id = (params[:issue][:assigned_to_id])
-        #@issue.done_ratio = (params[:issue][:done_ratio])
-        #@issue.lock_version = (params[:issue][:lock_version])
         journal = @issue.init_journal(self.logged_in_user, params[:notes])
         @issue.status = @new_status
         if @issue.update_attributes(params[:issue])
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index a778e5af2..5d55a8807 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -215,8 +215,7 @@ class ProjectsController < ApplicationController
     
     default_status = IssueStatus.default
     @issue = Issue.new(:project => @project, :tracker => @tracker, :status => default_status)    
-    @allowed_statuses = [default_status] + default_status.workflows.find(:all, :order => 'position', :include => :new_status, :conditions => ["role_id=? and tracker_id=?", self.logged_in_user.role_for_project(@project.id), @issue.tracker.id]).collect{ |w| w.new_status }
-    
+    @allowed_statuses = default_status.find_new_statuses_allowed_to(logged_in_user.role_for_project(@project), @issue.tracker) if logged_in_user
     if request.get?
       @issue.start_date = Date.today
       @custom_values = @project.custom_fields_for_issues(@tracker).collect { |x| CustomValue.new(:custom_field => x, :customized => @issue) }
@@ -349,7 +348,7 @@ class ProjectsController < ApplicationController
     redirect_to :action => 'list_issues', :id => @project and return unless @issues
     @projects = []
     # find projects to which the user is allowed to move the issue
-    @logged_in_user.memberships.each {|m| @projects << m.project if Permission.allowed_to_role("projects/move_issues", m.role_id)}
+    @logged_in_user.memberships.each {|m| @projects << m.project if Permission.allowed_to_role("projects/move_issues", m.role)}
     # issue can be moved to any tracker
     @trackers = Tracker.find(:all)
     if request.post? and params[:new_project_id] and params[:new_tracker_id]    
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index abf5e03d1..b3f981076 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -34,7 +34,7 @@ module ApplicationHelper
       return true
     end
     # check if user is authorized    
-    if @logged_in_user and (@logged_in_user.admin? or Permission.allowed_to_role( "%s/%s" % [ controller, action ], @logged_in_user.role_for_project(@project.id)  )  )
+    if @logged_in_user and (@logged_in_user.admin? or Permission.allowed_to_role( "%s/%s" % [ controller, action ], @logged_in_user.role_for_project(@project)  )  )
       return true
     end
     return false
diff --git a/app/models/issue_status.rb b/app/models/issue_status.rb
index fd6194c9a..337aa5894 100644
--- a/app/models/issue_status.rb
+++ b/app/models/issue_status.rb
@@ -36,12 +36,19 @@ class IssueStatus < ActiveRecord::Base
   end
 
   # Returns an array of all statuses the given role can switch to
+  # Uses association cache when called more than one time
   def new_statuses_allowed_to(role, tracker)
-    statuses = []
-    for workflow in self.workflows
-      statuses << workflow.new_status if workflow.role_id == role.id and workflow.tracker_id == tracker.id
-    end unless role.nil? or tracker.nil?
-    statuses
+    new_statuses = [self] + workflows.select {|w| w.role_id == role.id && w.tracker_id == tracker.id}.collect{|w| w.new_status}
+    new_statuses.sort{|x, y| x.position <=> y.position }
+  end
+  
+  # Same thing as above but uses a database query
+  # More efficient than the previous method if called just once
+  def find_new_statuses_allowed_to(role, tracker)  
+    new_statuses = [self] + workflows.find(:all, 
+                                           :include => :new_status,
+                                           :conditions => ["role_id=? and tracker_id=?", role.id, tracker.id]).collect{ |w| w.new_status }
+    new_statuses.sort{|x, y| x.position <=> y.position }
   end
   
 private
diff --git a/app/models/permission.rb b/app/models/permission.rb
index 23f8a5e91..609d5d561 100644
--- a/app/models/permission.rb
+++ b/app/models/permission.rb
@@ -57,7 +57,7 @@ class Permission < ActiveRecord::Base
         find(:all, :include => :roles).each {|p| perms.store "#{p.controller}/#{p.action}", p.roles.collect {|r| r.id } }
         perms
       end
-    allowed_to_public(action) or (@@cached_perms_for_roles[action] and @@cached_perms_for_roles[action].include? role)
+    allowed_to_public(action) or (role && @@cached_perms_for_roles[action] && @@cached_perms_for_roles[action].include?(role.id))
   end
   
   def self.allowed_to_role_expired
diff --git a/app/models/user.rb b/app/models/user.rb
index 08220beaa..15f857542 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -124,14 +124,8 @@ class User < ActiveRecord::Base
     User.hash_password(clear_password) == self.hashed_password
   end
   
-  def role_for_project(project_id)
-    @role_for_projects ||=
-      begin
-        roles = {}
-        self.memberships.each { |m| roles.store m.project_id, m.role_id }
-        roles
-      end
-    @role_for_projects[project_id]
+  def role_for_project(project)
+    memberships.detect {|m| m.project_id == project.id}
   end
   
   def pref