diff --git a/app/controllers/cost_objects_controller.rb b/app/controllers/cost_objects_controller.rb index 5b6a0c94c1..127bec6037 100644 --- a/app/controllers/cost_objects_controller.rb +++ b/app/controllers/cost_objects_controller.rb @@ -60,13 +60,11 @@ class CostObjectsController < ApplicationController sort_init 'id', 'desc' sort_update sort_columns - allowed_condition = Project.allowed_to(User.current, - :view_cost_objects, - project: @project) - - @cost_objects = CostObject.order(sort_clause) - .includes(:project, :author) - .merge(allowed_condition) + @cost_objects = CostObject + .visible(current_user) + .order(sort_clause) + .includes(:author) + .where(project_id: @project.id) .page(page_param) .per_page(per_page_param) diff --git a/app/controllers/costlog_controller.rb b/app/controllers/costlog_controller.rb index 255661db77..a8d9a6fc70 100644 --- a/app/controllers/costlog_controller.rb +++ b/app/controllers/costlog_controller.rb @@ -47,7 +47,7 @@ class CostlogController < ApplicationController cond = ARCondition.new if @project.nil? - cond << Project.allowed_to_condition(User.current, :view_cost_entries) + cond << "project_id IN (#{Project.allowed_to(User.current, :view_cost_entries).to_sql})" elsif @work_package.nil? cond << @project.project_condition(Setting.display_subprojects_work_packages?) else @@ -65,7 +65,7 @@ class CostlogController < ApplicationController respond_to do |format| format.html { @entries = CostEntry.includes(:project, :cost_type, :user, work_package: :type) - .merge(Project.allowed_to(User.current, :view_cost_entries, project: @project)) + .merge(Project.allowed_to(User.current, :view_cost_entries)) .where(cond.conditions) .order(sort_clause) .page(page_param) @@ -244,11 +244,11 @@ class CostlogController < ApplicationController @from, @to = @to, @from if @from && @to && @from > @to @from ||= (CostEntry.includes([:project, :user]) - .where(Project.allowed_to_condition(User.current, :view_cost_entries)) - .minimum(:spent_on) || Date.today) - 1 + .visible + .minimum(:spent_on) || Date.today) - 1 @to ||= (CostEntry.includes([:project, :user]) - .where(Project.allowed_to_condition(User.current, :view_cost_entries)) - .maximum(:spent_on) || Date.today) + .visible + .maximum(:spent_on) || Date.today) end def new_default_cost_entry diff --git a/app/models/cost_entry.rb b/app/models/cost_entry.rb index 3ee6fa8f71..ca6d0d3c51 100644 --- a/app/models/cost_entry.rb +++ b/app/models/cost_entry.rb @@ -37,27 +37,43 @@ class CostEntry < ActiveRecord::Base after_initialize :after_initialize validate :validate - scope :visible, lambda { |*args| - where(CostEntry.visible_condition(args[0] || User.current, args[1])) - .includes([:project, :user]) - .references(:project) - } + scope :visible, -> (*args) { + user = args.first || User.current + project = args[1] - scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) } + table = self.arel_table - def self.visible_condition(user, project) - %{ (#{Project.allowed_to_condition(user, :view_cost_entries, project: project)} OR - (#{Project.allowed_to_condition(user, :view_own_cost_entries, project: project)} AND #{CostEntry.table_name}.user_id = #{user.id})) } - end + view_allowed = Project.allowed_to(user, :view_cost_entries).select(:id) + view_own_allowed = Project.allowed_to(user, :view_own_cost_entries).select(:id) + + view_or_view_own = table[:project_id] + .in(view_allowed.arel) + .or(table[:project_id] + .in(view_own_allowed.arel) + .and(table[:user_id].eq(user.id))) + + scope = where(view_or_view_own) + + if project + scope = scope.where(project_id: project.id) + end + + scope + } scope :visible_costs, lambda{|*args| - view_cost_rates = Project.allowed_to_condition((args.first || User.current), :view_cost_rates, project: args[1]) - view_cost_entries = CostEntry.visible_condition((args.first || User.current), args[1]) + user = args.first || User.current + project = args[1] + + table = self.arel_table - where([view_cost_entries, view_cost_rates].join(' AND ')) - .includes([:project, :user]) + view_allowed = Project.allowed_to(user, :view_cost_rates).select(:id) + + visible(user, project).where(table[:project_id].in(view_allowed.arel)) } + scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) } + def self.costs_of(work_packages:) # N.B. Because of an AR quirks the code below uses statements like # where(work_package_id: ids) diff --git a/app/models/cost_object.rb b/app/models/cost_object.rb index 3d59dd0f32..3fc0040e07 100644 --- a/app/models/cost_object.rb +++ b/app/models/cost_object.rb @@ -44,6 +44,12 @@ class CostObject < ActiveRecord::Base CostObject.replace_author_with_deleted_user user end + def self.visible(user) + includes(:project) + .references(:projects) + .merge(Project.allowed_to(user, :view_cost_objects)) + end + def initialize(attributes = nil) super self.author = User.current if self.new_record? diff --git a/app/models/labor_budget_item.rb b/app/models/labor_budget_item.rb index fcb4b96077..83bbb3c929 100644 --- a/app/models/labor_budget_item.rb +++ b/app/models/labor_budget_item.rb @@ -32,19 +32,29 @@ class LaborBudgetItem < ActiveRecord::Base include ActiveModel::ForbiddenAttributesProtection # user_id correctness is ensured in VariableCostObject#*_labor_budget_item_attributes= - def self.visible_condition(user, project) - %{ (#{Project.allowed_to_condition(user, - :view_hourly_rates, - project: project)} OR - (#{Project.allowed_to_condition(user, - :view_own_hourly_rate, - project: project)} AND #{LaborBudgetItem.table_name}.user_id = #{user.id})) } + def self.visible(user, project) + table = self.arel_table + + view_allowed = Project.allowed_to(user, :view_hourly_rates).select(:id) + view_own_allowed = Project.allowed_to(user, :view_own_hourly_rates).select(:id) + + view_or_view_own = table[:project_id] + .in(view_allowed.arel) + .or(table[:project_id] + .in(view_own_allowed.arel) + .and(table[:user_id].eq(user.id))) + + scope = includes([{ cost_object: :project }, :user]) + .references(:projects) + where(view_or_view_own) + + if project + scope = scope.where(cost_object: { projects_id: project.id }) + end end scope :visible_costs, lambda{|*args| - includes([{ cost_object: :project }, :user]) - .where(LaborBudgetItem.visible_condition((args.first || User.current), args[1])) - .references(:projects) + visible((args.first || User.current), args[1]) } def costs diff --git a/app/models/material_budget_item.rb b/app/models/material_budget_item.rb index d29c4cb5ad..55ddeea3ff 100644 --- a/app/models/material_budget_item.rb +++ b/app/models/material_budget_item.rb @@ -26,16 +26,20 @@ class MaterialBudgetItem < ActiveRecord::Base include ActiveModel::ForbiddenAttributesProtection - def self.visible_condition(user, project) - Project.allowed_to_condition(user, - :view_cost_rates, - project: project) + def self.visible(user) + includes(cost_object: :project) + .references(:projects) + .merge(Project.allowed_to(user, :view_cost_rates)) end scope :visible_costs, lambda { |*args| - where(MaterialBudgetItem.visible_condition((args.first || User.current), args[1])) - .includes(cost_object: :project) - .references(:projects) + scope = visible(args.first || User.current) + + if args[1] + scope = scope.where(cost_object: { projects_id: args[1].id }) + end + + scope } def costs diff --git a/lib/open_project/costs/patches/time_entry_patch.rb b/lib/open_project/costs/patches/time_entry_patch.rb index 03e5d4acdb..21526ff3ce 100644 --- a/lib/open_project/costs/patches/time_entry_patch.rb +++ b/lib/open_project/costs/patches/time_entry_patch.rb @@ -24,36 +24,52 @@ module OpenProject::Costs::Patches::TimeEntryPatch base.send(:include, InstanceMethods) - # Same as typing in the class t.update_costs + # Same as typing in the class base.class_eval do belongs_to :rate, -> { where(type: ['HourlyRate', 'DefaultHourlyRate']) }, class_name: 'Rate' before_save :update_costs - def self.visible_condition(user, table_alias: nil, project: nil) - options = {} - options[:project_alias] = table_alias if table_alias - options[:project] = project if project - - %{ (#{Project.allowed_to_condition(user, - :view_time_entries, - options)} OR - (#{Project.allowed_to_condition(user, - :view_own_time_entries, - options)} AND - #{TimeEntry.table_name}.user_id = #{user.id})) } - end + scope :visible, -> (*args) { + user = args.first || User.current + project = args[1] + + table = self.arel_table + + view_allowed = Project.allowed_to(user, :view_time_entries).select(:id) + view_own_allowed = Project.allowed_to(user, :view_own_time_entries).select(:id) + + view_or_view_own = table[:project_id] + .in(view_allowed.arel) + .or(table[:project_id] + .in(view_own_allowed.arel) + .and(table[:user_id].eq(user.id))) + + scope = where(view_or_view_own) + + if project + scope = scope.where(project_id: project.id) + end + + scope + } scope :visible_costs, lambda{|*args| user = args.first || User.current project = args[1] - view_hourly_rates = %{ (#{Project.allowed_to_condition(user, :view_hourly_rates, project: project)} OR - (#{Project.allowed_to_condition(user, :view_own_hourly_rate, project: project)} AND #{TimeEntry.table_name}.user_id = #{user.id})) } - view_time_entries = TimeEntry.visible_condition(user, project: project) + table = self.arel_table + + view_allowed = Project.allowed_to(user, :view_hourly_rates).select(:id) + view_own_allowed = Project.allowed_to(user, :view_own_hourly_rates).select(:id) + + view_or_view_own = table[:project_id] + .in(view_allowed.arel) + .or(table[:project_id] + .in(view_own_allowed.arel) + .and(table[:user_id].eq(user.id))) - includes(:project, :user) - .where([view_time_entries, view_hourly_rates].join(' AND ')) + visible(user, project).where(view_or_view_own) } def self.costs_of(work_packages:) diff --git a/lib/open_project/costs/patches/user_patch.rb b/lib/open_project/costs/patches/user_patch.rb index 15ee0905ac..ffeef97903 100644 --- a/lib/open_project/costs/patches/user_patch.rb +++ b/lib/open_project/costs/patches/user_patch.rb @@ -31,9 +31,10 @@ module OpenProject::Costs::Patches::UserPatch module InstanceMethods def allowed_to_condition_with_project_id(permission, projects = nil) - project_condition = Project.allowed_to_condition(self, permission, project: projects) + scope = Project.allowed_to(self, permission) + scope = scope.where(id: projects) if projects - ids = Project.where(project_condition).pluck(:id) + ids = scope.pluck(:id) ids.empty? ? '1=0' : diff --git a/lib/open_project/costs/patches/work_package_patch.rb b/lib/open_project/costs/patches/work_package_patch.rb index 46519d53b2..621ba3efa6 100644 --- a/lib/open_project/costs/patches/work_package_patch.rb +++ b/lib/open_project/costs/patches/work_package_patch.rb @@ -66,26 +66,30 @@ module OpenProject::Costs::Patches::WorkPackagePatch false when 'reassign' - reassign_to = WorkPackage - .where(Project.allowed_to_condition(user, :edit_cost_entries)) - .includes(:project) - .references(:projects) - .find_by_id(to_do[:reassign_to_id]) - - if reassign_to.nil? - work_packages.each do |wp| - wp.errors.add(:base, :is_not_a_valid_target_for_cost_entries, id: to_do[:reassign_to_id]) - end - - false - else - WorkPackage.update_cost_entries(work_packages.map(&:id), "work_package_id = #{reassign_to.id}, project_id = #{reassign_to.project_id}") - end + reassign_cost_entries_before_destruction(work_packages, user, to_do[:reassign_to_id]) else false end end + def reassign_cost_entries_before_destruction(work_packages, user, ids) + reassign_to = WorkPackage + .joins(:project) + .merge(Project.allowed_to(user, :edit_cost_entries)) + .find_by_id(ids) + + if reassign_to.nil? + work_packages.each do |wp| + wp.errors.add(:base, :is_not_a_valid_target_for_cost_entries, id: ids) + end + + false + else + condition = "work_package_id = #{reassign_to.id}, project_id = #{reassign_to.project_id}" + WorkPackage.update_cost_entries(work_packages.map(&:id), condition) + end + end + protected def update_cost_entries(work_packages, action)