From e227b92972522ad24818d2e69877dbdb84f40884 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Wed, 4 Apr 2007 17:26:05 +0000 Subject: [PATCH] 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 --- app/controllers/application.rb | 4 ++-- app/controllers/feeds_controller.rb | 2 +- app/controllers/issues_controller.rb | 14 ++------------ app/controllers/projects_controller.rb | 5 ++--- app/helpers/application_helper.rb | 2 +- app/models/issue_status.rb | 17 ++++++++++++----- app/models/permission.rb | 2 +- app/models/user.rb | 10 ++-------- 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