diff --git a/.travis.yml b/.travis.yml index c1c92339e3..91890d7123 100644 --- a/.travis.yml +++ b/.travis.yml @@ -59,8 +59,8 @@ env: - COVERAGE=true matrix: - - "TEST_SUITE=plugins:spec DB=mysql" - - "TEST_SUITE=plugins:cucumber DB=mysql" + - "TEST_SUITE=plugins:spec DB=mysql GROUP_SIZE=1 GROUP=1" + - "TEST_SUITE=plugins:cucumber DB=mysql GROUP_SIZE=1 GROUP=1" - "TEST_SUITE=npm" diff --git a/app/controllers/cost_objects_controller.rb b/app/controllers/cost_objects_controller.rb index defc8073d2..40f2d0a7dd 100644 --- a/app/controllers/cost_objects_controller.rb +++ b/app/controllers/cost_objects_controller.rb @@ -18,16 +18,16 @@ #++ class CostObjectsController < ApplicationController - before_filter :find_cost_object, only: [:show, :edit, :update, :copy] - before_filter :find_cost_objects, only: :destroy - before_filter :find_project, only: [ + before_action :find_cost_object, only: [:show, :edit, :update, :copy] + before_action :find_cost_objects, only: :destroy + before_action :find_project, only: [ :new, :create, :update_material_budget_item, :update_labor_budget_item ] - before_filter :find_optional_project, only: :index + before_action :find_optional_project, only: :index - before_filter :authorize_global, only: :index - before_filter :authorize, except: [ + before_action :authorize_global, only: :index + before_action :authorize, except: [ # unrestricted actions :index, :update_material_budget_item, :update_labor_budget_item diff --git a/app/controllers/cost_types_controller.rb b/app/controllers/cost_types_controller.rb index c0988ad16a..f737c18e74 100644 --- a/app/controllers/cost_types_controller.rb +++ b/app/controllers/cost_types_controller.rb @@ -19,8 +19,8 @@ class CostTypesController < ApplicationController # Allow only admins here - before_filter :require_admin - before_filter :find_cost_type, only: [:edit, :update, :set_rate, :destroy, :restore] + before_action :require_admin + before_action :find_cost_type, only: [:edit, :update, :set_rate, :destroy, :restore] layout 'admin' helper :sort diff --git a/app/controllers/costlog_controller.rb b/app/controllers/costlog_controller.rb index a8d9a6fc70..645d36bd34 100644 --- a/app/controllers/costlog_controller.rb +++ b/app/controllers/costlog_controller.rb @@ -19,14 +19,14 @@ class CostlogController < ApplicationController menu_item :work_packages - before_filter :find_project, :authorize, only: [:edit, + before_action :find_project, :authorize, only: [:edit, :new, :create, :update, :destroy] - before_filter :find_associated_objects, only: [:create, + before_action :find_associated_objects, only: [:create, :update] - before_filter :find_optional_project, only: [:report, + before_action :find_optional_project, only: [:report, :index] helper :sort diff --git a/app/controllers/hourly_rates_controller.rb b/app/controllers/hourly_rates_controller.rb index c1efd8d543..795b2a207a 100644 --- a/app/controllers/hourly_rates_controller.rb +++ b/app/controllers/hourly_rates_controller.rb @@ -24,13 +24,13 @@ class HourlyRatesController < ApplicationController helper :hourly_rates include HourlyRatesHelper - before_filter :find_user, only: [:show, :edit, :update, :set_rate] + before_action :find_user, only: [:show, :edit, :update, :set_rate] - before_filter :find_optional_project, only: [:show, :edit, :update] - before_filter :find_project, only: [:set_rate] + before_action :find_optional_project, only: [:show, :edit, :update] + before_action :find_project, only: [:set_rate] # #show, #edit have their own authorization - before_filter :authorize, except: [:show, :edit, :update] + before_action :authorize, except: [:show, :edit, :update] # TODO: this should be an index def show diff --git a/app/models/work_package/abstract_costs.rb b/app/models/work_package/abstract_costs.rb index 6c0b36661b..19bddc4cce 100644 --- a/app/models/work_package/abstract_costs.rb +++ b/app/models/work_package/abstract_costs.rb @@ -17,6 +17,12 @@ class WorkPackage add_costs_to work_package_scope end + ## + # Adds to the given WorkPackage collection query an extra costs column + def add_to_work_package_collection(wp_collection_scope) + add_costs_to wp_collection_scope + end + ## # For the given work packages calculates the sum of all costs. # @@ -50,6 +56,10 @@ class WorkPackage raise NotImplementedError, "subclass responsiblity" end + def subselect_alias + raise NotImplementedError, "subclass responsiblity" + end + private def work_package_ids(work_packages) @@ -64,15 +74,10 @@ class WorkPackage costs_model.table_name end - def table_alias - "#{costs_table_name}_sum_per_wp" - end - def add_costs_to(scope) scope - .joins(sum_arel.join_sources) - .select("#{costs_sum} AS #{costs_sum_alias}") - .group(wp_table[:id]) + .joins(sum_arel(scope).join_sources) + .select(costs_sum_alias) end def costs_sum @@ -92,8 +97,19 @@ class WorkPackage scope # allow all end - def sum_arel - wp_table + def sum_arel(base_scope) + subselect = sum_subselect(base_scope) + .as(subselect_alias) + wp_table. + outer_join(subselect).on(subselect[:id].eq(wp_table[:id])) + end + + def sum_subselect(base_scope) + base_scope + .dup + .except(:select) + .select("#{costs_sum} AS #{costs_sum_alias}") + .select(wp_table[:id]) .outer_join(ce_table).on(ce_table_join_condition) .group(wp_table[:id]) end @@ -102,6 +118,10 @@ class WorkPackage WorkPackage.arel_table end + def wp_table_descendants + wp_table.alias 'descendants' + end + def ce_table costs_model.arel_table end @@ -110,7 +130,9 @@ class WorkPackage authorization_scope = filter_authorized costs_model.all authorization_where = authorization_scope.ast.cores.last.wheres.last - ce_table[:work_package_id].eq(wp_table[:id]).and(authorization_where) + # relies on the scope having the wp descendants joined at least + # when #to_sql is called. + ce_table[:work_package_id].eq(wp_table_descendants[:id]).and(authorization_where) end def projects_table diff --git a/app/models/work_package/labor_costs.rb b/app/models/work_package/labor_costs.rb index 06b18f2266..cc0a9d7e29 100644 --- a/app/models/work_package/labor_costs.rb +++ b/app/models/work_package/labor_costs.rb @@ -8,12 +8,16 @@ class WorkPackage TimeEntry.with_visible_costs_on scope end - def ce_table - super.alias 'time_entry_labor' - end - def costs_sum_alias 'time_entries_sum' end + + def subselect_alias + 'time_entries' + end + + def sum_subselect(base_scope) + super.project('SUM(hours) hours') + end end end diff --git a/app/models/work_package/material_costs.rb b/app/models/work_package/material_costs.rb index 0ca21bbb14..fb1042dfe2 100644 --- a/app/models/work_package/material_costs.rb +++ b/app/models/work_package/material_costs.rb @@ -11,5 +11,9 @@ class WorkPackage def costs_sum_alias 'cost_entries_sum' end + + def subselect_alias + 'cost_entries' + end end end diff --git a/app/views/cost_types/edit.html.erb b/app/views/cost_types/edit.html.erb index 01bb72d63a..431066a8ce 100644 --- a/app/views/cost_types/edit.html.erb +++ b/app/views/cost_types/edit.html.erb @@ -106,7 +106,6 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - <%= t(:button_add_rate) %> <%= styled_button_tag t(:button_save), class: '-with-icon icon-checkmark' %> diff --git a/app/views/costlog/edit.html.erb b/app/views/costlog/edit.html.erb index 3f8058648d..b417274b22 100644 --- a/app/views/costlog/edit.html.erb +++ b/app/views/costlog/edit.html.erb @@ -93,16 +93,18 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
- <% if User.current.allowed_to? :view_cost_rates, @cost_entry.project %> - - <%= number_to_currency(@cost_entry.calculated_costs) %> - - <% else %> - - " size="7" name="cost_entry[overridden_costs]" id="cost_entry_costs_edit"/> <%= Setting.plugin_openproject_costs['costs_currency'] %> - -
<%= t(:help_override_rate) %> - <% end %> + + <% if User.current.allowed_to? :view_cost_rates, @cost_entry.project %> + + <%= number_to_currency(@cost_entry.calculated_costs) %> + + <% else %> + + " size="7" name="cost_entry[overridden_costs]" id="cost_entry_costs_edit"/> <%= Setting.plugin_openproject_costs['costs_currency'] %> + +
<%= t(:help_override_rate) %> + <% end %> +
diff --git a/features/manage_budget.feature b/features/manage_budget.feature deleted file mode 100644 index 89d1f0d9d0..0000000000 --- a/features/manage_budget.feature +++ /dev/null @@ -1,64 +0,0 @@ -#-- copyright -# OpenProject Costs Plugin -# -# Copyright (C) 2009 - 2014 the OpenProject Foundation (OPF) -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# version 3. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -#++ - -Feature: Managing Budgets - - Background: - Given there is 1 User with: - | Login | testuser | - | firstname | Chuck | - | lastname | Testa | - | default rate | 37 | - And there is 1 Project with the following: - | name | project1 | - | identifier | project1 | - And there is a role "manager" - And the role "manager" may have the following rights: - | edit_cost_objects | - | view_cost_rates | - | view_hourly_rates | - And there is 1 cost type with the following: - | name | cost_type_1 | - | unit | single_unit | - | unit_plural | multi_unit | - | cost_rate | 40 | - And the user "testuser" is a "manager" in the project "project1" - And I am already logged in as "testuser" - -@javascript - Scenario: Budgets can be copied - Given there is a budget with the following: - | subject | budget1 | - | author | testuser | - | project | project1 | - And the budget "budget1" has the following material items: - | units | comment | cost_type | - | 10 | materialtestcomment | cost_type_1 | - | 6 | materialtestcomment2 | cost_type_1 | - And the budget "budget1" has the following labor items: - | hours | comment | user | - | 8 | labortestcomment | testuser | - | 5 | labortestcomment2 | testuser | - And I go to the show page of the budget "budget1" - When I click on "Copy" - Then I should see "New budget" - And the planned material costs in row 1 should be "400.00 EUR" - And the planned labor costs in row 1 should be "296.00 EUR" - And the planned material costs in row 2 should be "240.00 EUR" - And the planned labor costs in row 2 should be "185.00 EUR" diff --git a/frontend/app/components/budget/cost-unit-subform.directive.ts b/frontend/app/components/budget/cost-unit-subform.directive.ts new file mode 100644 index 0000000000..e367a2ecb9 --- /dev/null +++ b/frontend/app/components/budget/cost-unit-subform.directive.ts @@ -0,0 +1,107 @@ +// -- copyright +// OpenProject is a project management system. +// Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License version 3. +// +// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +// Copyright (C) 2006-2013 Jean-Philippe Lang +// Copyright (C) 2010-2013 the ChiliProject Team +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License +// as published by the Free Software Foundation; either version 2 +// of the License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +// +// See doc/COPYRIGHT.rdoc for more details. +// ++ + +export class CostUnitSubformController { + + public objId: string; + public objName: string; + + constructor(public $element) { + // Add new row handler + $element.find('#'+this.objId).click(() => { + this.makeEditable('#'+this.objId, this.objName); + }); + } + + private getCurrencyValue(str) { + var result = str.match(/^\s*(([0-9]+[.,])+[0-9]+) (.+)\s*/); + return result ? new Array(result[1], result[3]) : new Array(str, ""); + } + + public makeEditable(id, name){ + var obj = jQuery(id); + + jQuery(id).on('click', this.edit_and_focus(obj, name)); + } + + private edit_and_focus(obj, name) { + this.edit(obj, name); + + jQuery('#'+obj[0].id+'_edit').focus(); + jQuery('#'+obj[0].id+'_edit').select(); + } + + private edit(obj, name, obj_value) { + obj.hide(); + + var obj_value = typeof(obj_value) != 'undefined' ? obj_value : obj[0].innerHTML; + var parsed = this.getCurrencyValue(obj_value); + var value = parsed[0]; + var currency = parsed[1]; + + var form_start = '
'; + var button = '
'; + var span = '
'; + span += ' '; + span += '
'; + + var affix = '
' + + currency + + '
'; + var form_end = '
'; + + jQuery(form_start + button + span + affix + form_end).insertAfter(obj); + + var that = this; + jQuery('#'+obj[0].id+'_cancel').on('click', function() { + that.cleanUp(obj) + }); + } + + private cleanUp(obj){ + jQuery('#'+obj[0].id+'_section').remove(); + obj.show(); + } +} + +function costUnitSubform() { + return { + restrict: 'E', + scope: { + objId: '@', + objName: '@' + }, + bindToController: true, + controller: CostUnitSubformController, + controllerAs: '$ctrl' + }; +} + +angular.module('openproject').directive('costUnitSubform', costUnitSubform); diff --git a/lib/open_project/costs/engine.rb b/lib/open_project/costs/engine.rb index a30092e012..f7d62cfc74 100644 --- a/lib/open_project/costs/engine.rb +++ b/lib/open_project/costs/engine.rb @@ -88,7 +88,7 @@ module OpenProject::Costs end end - patches [:WorkPackage, :Project, :Query, :User, :TimeEntry, :PermittedParams, + patches [:Project, :Query, :User, :TimeEntry, :PermittedParams, :ProjectsController, :ApplicationHelper, :UsersHelper] patch_with_namespace :API, :V3, :WorkPackages, :Schema, :SpecificWorkPackageSchema patch_with_namespace :BasicData, :RoleSeeder @@ -352,10 +352,56 @@ module OpenProject::Costs module EagerLoadedCosts def add_eager_loading(*args) + EagerLoadedCosts.join_costs(super) + end + + def self.join_costs(scope) material = WorkPackage::MaterialCosts.new labor = WorkPackage::LaborCosts.new - material.add_to_work_packages(labor.add_to_work_packages(super)) + # The core adds a "LEFT OUTER JOIN time_entries" where the on clause + # allows all time entries to be joined if he has the :view_time_entries. + # Because the cost scopes add another "LEFT OUTER JOIN time_entries" + # where the on clause allows all time entries to be joined if he has + # the :view_time_entries permission and additionally those which are + # his and for which he has the :view_own_time_entries permission. + # Because costs join includes the values of the core, entries are joined twice. + # We therefore have to remove core's join. + # + # This is very hacky. + # + # We also have to remove the sum calcualtion for time_entries.hours as + # the calculation is later on performed within the subquery added by + # LaborCosts. With it, we can use the value as it is calculated by the subquery. + scope.joins_values.reject! do |join| + join.is_a?(Arel::Nodes::OuterJoin) && + join.left.is_a?(Arel::Table) && + join.left.name == 'time_entries' + end + scope.select_values.reject! do |select| + select == "SUM(time_entries.hours) AS hours" + end + + material_scope = material.add_to_work_package_collection(scope.dup) + labor_scope = labor.add_to_work_package_collection(scope.dup) + + target_scope = scope.joins(material_scope.join_sources) + .joins(labor_scope.join_sources) + .select(material_scope.select_values) + .select(labor_scope.select_values) + .select('time_entries.hours') + + target_scope.joins_values.reject! do |join| + join.is_a?(Arel::Nodes::OuterJoin) && + join.left.is_a?(Arel::Nodes::TableAlias) && + join.left.right == 'descendants' + end + + target_scope.group_values.reject! do |group| + group == :id + end + + target_scope end end @@ -363,6 +409,9 @@ module OpenProject::Costs require 'open_project/costs/patches/members_patch' OpenProject::Costs::Members.mixin! + require 'open_project/costs/patches/work_package_patch' + OpenProject::Costs::Patches::WorkPackagePatch.mixin! + # loading the class so that acts_as_journalized gets registered VariableCostObject @@ -372,11 +421,7 @@ module OpenProject::Costs require 'api/v3/work_packages/work_package_representer' - API::V3::WorkPackages::WorkPackageRepresenter.to_eager_load += [{ time_entries: [:project, - :user] }, - { cost_entries: [:project, - :user] }, - :cost_object] + API::V3::WorkPackages::WorkPackageRepresenter.to_eager_load += [:cost_object] API::V3::WorkPackages::WorkPackageCollectionRepresenter.prepend EagerLoadedCosts end diff --git a/lib/open_project/costs/patches/projects_controller_patch.rb b/lib/open_project/costs/patches/projects_controller_patch.rb index 05e6193162..1a807d54c8 100644 --- a/lib/open_project/costs/patches/projects_controller_patch.rb +++ b/lib/open_project/costs/patches/projects_controller_patch.rb @@ -22,7 +22,7 @@ module OpenProject::Costs::Patches::ProjectsControllerPatch base.send(:include, InstanceMethods) base.class_eval do - before_filter :own_total_hours, only: [:show] + before_action :own_total_hours, only: [:show] end end diff --git a/lib/open_project/costs/patches/work_package_patch.rb b/lib/open_project/costs/patches/work_package_patch.rb index 9b1530f354..443c492444 100644 --- a/lib/open_project/costs/patches/work_package_patch.rb +++ b/lib/open_project/costs/patches/work_package_patch.rb @@ -17,142 +17,140 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. #++ -module OpenProject::Costs::Patches::WorkPackagePatch - def self.included(base) # :nodoc: - base.extend(ClassMethods) - - base.send(:include, InstanceMethods) - - # Same as typing in the class - base.class_eval do - belongs_to :cost_object, inverse_of: :work_packages - has_many :cost_entries, dependent: :delete_all - - # disabled for now, implements part of ticket blocking - validate :validate_cost_object - - after_update :move_cost_entries - - register_journal_formatter(:cost_association) do |value, journable, field| - association = journable.class.reflect_on_association(field.to_sym) - if association - record = association.class_name.constantize.find_by_id(value.to_i) - record.subject if record +module OpenProject::Costs::Patches + module WorkPackagePatch + def self.mixin! + ::WorkPackage.prepend InstanceMethods + ::WorkPackage.singleton_class.prepend ClassMethods + + ::WorkPackage.class_eval do + belongs_to :cost_object, inverse_of: :work_packages + has_many :cost_entries, dependent: :delete_all + + # disabled for now, implements part of ticket blocking + validate :validate_cost_object + + after_update :move_cost_entries + + register_journal_formatter(:cost_association) do |value, journable, field| + association = journable.class.reflect_on_association(field.to_sym) + if association + record = association.class_name.constantize.find_by_id(value.to_i) + record.subject if record + end end - end - register_on_journal_formatter(:cost_association, 'cost_object_id') + register_on_journal_formatter(:cost_association, 'cost_object_id') - associated_to_ask_before_destruction CostEntry, - ->(work_packages) { CostEntry.on_work_packages(work_packages).count > 0 }, - method(:cleanup_cost_entries_before_destruction_of) + associated_to_ask_before_destruction CostEntry, + ->(work_packages) { CostEntry.on_work_packages(work_packages).count > 0 }, + method(:cleanup_cost_entries_before_destruction_of) + end end - end - module ClassMethods - protected + module ClassMethods + protected - def cleanup_cost_entries_before_destruction_of(work_packages, user, to_do = { action: 'destroy' }) - work_packages = Array(work_packages) + def cleanup_cost_entries_before_destruction_of(work_packages, user, to_do = { action: 'destroy' }) + work_packages = Array(work_packages) - return false unless to_do.present? + return false unless to_do.present? - case to_do[:action] - when 'destroy' - true - # nothing to do - when 'nullify' - work_packages.each do |wp| - wp.errors.add(:base, :nullify_is_not_valid_for_cost_entries) - end + case to_do[:action] + when 'destroy' + true + # nothing to do + when 'nullify' + work_packages.each do |wp| + wp.errors.add(:base, :nullify_is_not_valid_for_cost_entries) + end - false - when 'reassign' - reassign_cost_entries_before_destruction(work_packages, user, to_do[:reassign_to_id]) - else - false + false + when 'reassign' + reassign_cost_entries_before_destruction(work_packages, user, to_do[:reassign_to_id]) + else + false + end 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) + 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 - - 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 + protected - def update_cost_entries(work_packages, action) - CostEntry.where(work_package_id: work_packages).update_all(action) + def update_cost_entries(work_packages, action) + CostEntry.where(work_package_id: work_packages).update_all(action) + end end - end - module InstanceMethods - def costs_enabled? - project && project.costs_enabled? - end + module InstanceMethods + def costs_enabled? + project && project.costs_enabled? + end - def validate_cost_object - if cost_object_id_changed? - unless cost_object_id.blank? || project.cost_object_ids.include?(cost_object_id) - errors.add :cost_object, :inclusion + def validate_cost_object + if cost_object_id_changed? + unless cost_object_id.blank? || project.cost_object_ids.include?(cost_object_id) + errors.add :cost_object, :inclusion + end end end - end - def material_costs - if respond_to?(:cost_entries_sum) # column has been eager loaded into result set - cost_entries_sum.to_f - else - WorkPackage::MaterialCosts.new(user: User.current).costs_of work_packages: self + def material_costs + if respond_to?(:cost_entries_sum) # column has been eager loaded into result set + cost_entries_sum.to_f + else + ::WorkPackage::MaterialCosts.new(user: User.current).costs_of work_packages: self_and_descendants + end end - end - def labor_costs - if respond_to?(:time_entries_sum) # column has been eager loaded into result set - time_entries_sum.to_f - else - WorkPackage::LaborCosts.new(user: User.current).costs_of work_packages: self + def labor_costs + if respond_to?(:time_entries_sum) # column has been eager loaded into result set + time_entries_sum.to_f + else + ::WorkPackage::LaborCosts.new(user: User.current).costs_of work_packages: self_and_descendants + end end - end - def overall_costs - labor_costs + material_costs - end + def overall_costs + labor_costs + material_costs + end - # Wraps the association to get the Cost Object subject. Needed for the - # Query and filtering - def cost_object_subject - unless cost_object.nil? - return cost_object.subject + # Wraps the association to get the Cost Object subject. Needed for the + # Query and filtering + def cost_object_subject + unless cost_object.nil? + return cost_object.subject + end end - end - def update_costs! - # This methods ist referenced from some migrations but does nothing - # anymore. - end + def update_costs! + # This methods ist referenced from some migrations but does nothing + # anymore. + end - def move_cost_entries - return unless project_id_changed? - # TODO: This only works with the global cost_rates - CostEntry - .where(work_package_id: id) - .update_all(project_id: project_id) + def move_cost_entries + return unless project_id_changed? + # TODO: This only works with the global cost_rates + CostEntry + .where(work_package_id: id) + .update_all(project_id: project_id) + end end end end - -WorkPackage::SAFE_ATTRIBUTES << 'cost_object_id' if WorkPackage.const_defined? 'SAFE_ATTRIBUTES' diff --git a/spec/controllers/cost_types_controller_spec.rb b/spec/controllers/cost_types_controller_spec.rb index 6b9938e2a8..7ddff02eea 100644 --- a/spec/controllers/cost_types_controller_spec.rb +++ b/spec/controllers/cost_types_controller_spec.rb @@ -26,7 +26,7 @@ describe CostTypesController, type: :controller do describe 'DELETE destroy' do it 'allows an admin to delete' do as_logged_in_user admin do - delete :destroy, id: cost_type.id + delete :destroy, params: { id: cost_type.id } end expect(assigns(:cost_type).deleted_at).to be_a Time @@ -42,7 +42,7 @@ describe CostTypesController, type: :controller do it 'allows an admin to restore' do as_logged_in_user admin do - patch :restore, id: cost_type.id + patch :restore, params: { id: cost_type.id } end expect(assigns(:cost_type).deleted_at).to be_nil diff --git a/spec/controllers/costlog_controller_spec.rb b/spec/controllers/costlog_controller_spec.rb index f821092238..de80d4433b 100644 --- a/spec/controllers/costlog_controller_spec.rb +++ b/spec/controllers/costlog_controller_spec.rb @@ -57,13 +57,15 @@ describe CostlogController, type: :controller do end shared_examples_for 'assigns' do - it { expect(assigns(:cost_entry).project).to eq(expected_project) } - it { expect(assigns(:cost_entry).work_package).to eq(expected_work_package) } - it { expect(assigns(:cost_entry).user).to eq(expected_user) } - it { expect(assigns(:cost_entry).spent_on).to eq(expected_spent_on) } - it { expect(assigns(:cost_entry).cost_type).to eq(expected_cost_type) } - it { expect(assigns(:cost_entry).units).to eq(expected_units) } - it { expect(assigns(:cost_entry).overridden_costs).to eq(expected_overridden_costs) } + it do + expect(assigns(:cost_entry).project).to eq(expected_project) + expect(assigns(:cost_entry).work_package).to eq(expected_work_package) + expect(assigns(:cost_entry).user).to eq(expected_user) + expect(assigns(:cost_entry).spent_on).to eq(expected_spent_on) + expect(assigns(:cost_entry).cost_type).to eq(expected_cost_type) + expect(assigns(:cost_entry).units).to eq(expected_units) + expect(assigns(:cost_entry).overridden_costs).to eq(expected_overridden_costs) + end end before do @@ -88,7 +90,7 @@ describe CostlogController, type: :controller do shared_examples_for 'successful new' do before do - get :new, params + get :new, params: params end it { expect(response).to be_success } @@ -98,7 +100,7 @@ describe CostlogController, type: :controller do shared_examples_for 'forbidden new' do before do - get :new, params + get :new, params: params end it { expect(response.response_code).to eq(403) } @@ -152,7 +154,7 @@ describe CostlogController, type: :controller do shared_examples_for 'successful edit' do before do - get :edit, params + get :edit, params: params end it { expect(response).to be_success } @@ -163,7 +165,7 @@ describe CostlogController, type: :controller do shared_examples_for 'forbidden edit' do before do - get :edit, params + get :edit, params: params end it { expect(response.response_code).to eq(403) } @@ -239,7 +241,7 @@ describe CostlogController, type: :controller do params['id'] = (cost_entry.id + 1).to_s - get :edit, params + get :edit, params: params end it { expect(response.response_code).to eq(404) } @@ -276,7 +278,7 @@ describe CostlogController, type: :controller do shared_examples_for 'successful create' do before do - post :create, params + post :create, params: params end # is this really usefull, shouldn't it redirect to the creating work_package by default? @@ -288,7 +290,7 @@ describe CostlogController, type: :controller do shared_examples_for 'invalid create' do before do - post :create, params + post :create, params: params end it { expect(response).to be_success } @@ -298,7 +300,7 @@ describe CostlogController, type: :controller do shared_examples_for 'forbidden create' do before do - post :create, params + post :create, params: params end it { expect(response.response_code).to eq(403) } @@ -502,7 +504,7 @@ describe CostlogController, type: :controller do shared_examples_for 'successful update' do before do - put :update, params + put :update, params: params end it { expect(response).to redirect_to(controller: 'costlog', action: 'index', project_id: project) } @@ -513,7 +515,9 @@ describe CostlogController, type: :controller do end shared_examples_for 'invalid update' do - before { put :update, params } + before do + put :update, params: params + end it_should_behave_like 'assigns' it { expect(response).to be_success } @@ -522,7 +526,7 @@ describe CostlogController, type: :controller do shared_examples_for 'forbidden update' do before do - put :update, params + put :update, params: params end it { expect(response.response_code).to eq(403) } diff --git a/spec/controllers/hourly_rates_controller_spec.rb b/spec/controllers/hourly_rates_controller_spec.rb index 788b00db52..63626b28f1 100644 --- a/spec/controllers/hourly_rates_controller_spec.rb +++ b/spec/controllers/hourly_rates_controller_spec.rb @@ -34,7 +34,7 @@ describe HourlyRatesController do } before do as_logged_in_user admin do - post :update, params + post :update, params: params end end diff --git a/spec/controllers/work_packages_bulk_controller_spec.rb b/spec/controllers/work_packages_bulk_controller_spec.rb index 6ed6731c9e..d4e62af375 100644 --- a/spec/controllers/work_packages_bulk_controller_spec.rb +++ b/spec/controllers/work_packages_bulk_controller_spec.rb @@ -32,7 +32,10 @@ describe WorkPackages::BulkController, type: :controller do describe '#update' do context 'when a cost report is assigned' do - before do put :update, ids: [work_package.id], work_package: { cost_object_id: cost_object.id } end + before do + put :update, params: { ids: [work_package.id], + work_package: { cost_object_id: cost_object.id } } + end subject { work_package.reload.cost_object.try :id } diff --git a/spec/features/add_budget_spec.rb b/spec/features/add_budget_spec.rb index 348db6a4ba..d7356bd726 100644 --- a/spec/features/add_budget_spec.rb +++ b/spec/features/add_budget_spec.rb @@ -77,11 +77,13 @@ describe 'adding a new budget', type: :feature, js: true do expect(page).to have_content('Successful creation') new_budget_page.toggle_unit_costs! + expect(page).to have_selector('td.currency', text: '150.00 EUR') expect(new_budget_page.unit_costs_at(1)).to have_content '150.00 EUR' expect(new_budget_page.unit_costs_at(2)).to have_content '100.00 EUR' expect(new_budget_page.overall_unit_costs).to have_content '250.00 EUR' new_budget_page.toggle_labor_costs! + expect(page).to have_selector('td.currency', text: '125.00 EUR') expect(new_budget_page.labor_costs_at(1)).to have_content '125.00 EUR' expect(new_budget_page.labor_costs_at(2)).to have_content '50.00 EUR' expect(new_budget_page.overall_labor_costs).to have_content '175.00 EUR' diff --git a/spec/features/time_entries_spec.rb b/spec/features/time_entries_spec.rb new file mode 100644 index 0000000000..22f28129c1 --- /dev/null +++ b/spec/features/time_entries_spec.rb @@ -0,0 +1,70 @@ +#-- copyright +# OpenProject Costs Plugin +# +# Copyright (C) 2009 - 2014 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# version 3. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +#++ + +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb') + +describe 'Work Package table cost sums', type: :feature, js: true do + let(:project) { FactoryGirl.create :project } + let(:user) { FactoryGirl.create :admin } + + let(:parent) { FactoryGirl.create :work_package, project: project } + let(:work_package) { FactoryGirl.create :work_package, project: project, parent: parent } + let(:hourly_rate) { FactoryGirl.create :default_hourly_rate, user: user, rate: 1.00 } + + let!(:time_entry1) { + FactoryGirl.create :time_entry, + user: user, + work_package: parent, + project: project, + hours: 10 + } + + let!(:time_entry2) { + FactoryGirl.create :time_entry, + user: user, + work_package: work_package, + project: project, + hours: 2.50 + } + + let(:wp_table) { ::Pages::WorkPackagesTable.new(project) } + let!(:query) do + query = FactoryGirl.build(:query, user: user, project: project) + query.column_names = %w(id subject spent_hours) + + query.save! + query + end + + before do + login_as(user) + + wp_table.visit_query(query) + wp_table.expect_work_package_listed(parent) + wp_table.expect_work_package_listed(work_package) + end + + it 'shows the correct sum of the time entries' do + parent_row = wp_table.row(parent) + wp_row = wp_table.row(work_package) + + expect(parent_row).to have_selector('.wp-edit-field.spentTime', text: '12.5 hours') + expect(wp_row).to have_selector('.wp-edit-field.spentTime', text: '2.5 hours') + end +end diff --git a/spec/features/view_own_rates_spec.rb b/spec/features/view_own_rates_spec.rb index 1806f90d72..b09945c0c9 100644 --- a/spec/features/view_own_rates_spec.rb +++ b/spec/features/view_own_rates_spec.rb @@ -32,6 +32,7 @@ describe 'Only see your own rates', type: :feature, js: true do :view_cost_rates, :log_costs] } let(:work_package) {FactoryGirl.create :work_package } + let(:wp_page) { ::Pages::FullWorkPackage.new(work_package) } let(:hourly_rate) { FactoryGirl.create :default_hourly_rate, user: user, rate: 10.00 } let(:time_entry) { FactoryGirl.create :time_entry, user: user, @@ -65,8 +66,8 @@ describe 'Only see your own rates', type: :feature, js: true do user: other_user, cost_type: cost_type } - it 'only displays own entries and rates' do - allow(User).to receive(:current).and_return(user) + before do + login_as(user) work_package hourly_rate @@ -76,9 +77,11 @@ describe 'Only see your own rates', type: :feature, js: true do other_time_entry other_cost_entry - wp_page = Pages::FullWorkPackage.new(work_package) wp_page.visit! + wp_page.ensure_page_loaded + end + it 'only displays own entries and rates' do # All the values do not include the entries made by the other user wp_page.expect_attributes spent_time: '1 hour', costs_by_type: '2 Translations', diff --git a/spec/lib/api/v3/work_packages/work_package_representer_spec.rb b/spec/lib/api/v3/work_packages/work_package_representer_spec.rb index ebcb71892a..085bc646db 100644 --- a/spec/lib/api/v3/work_packages/work_package_representer_spec.rb +++ b/spec/lib/api/v3/work_packages/work_package_representer_spec.rb @@ -435,18 +435,6 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do end describe '.to_eager_load' do - it 'includes cost entries with dependencies' do - expect(described_class.to_eager_load.any? { |el| - el.is_a?(Hash) && el[:cost_entries] == [:project, :user] - }).to be_truthy - end - - it 'includes time entries with dependencies' do - expect(described_class.to_eager_load.any? { |el| - el.is_a?(Hash) && el[:time_entries] == [:project, :user] - }).to be_truthy - end - it 'includes the cost objects' do expect(described_class.to_eager_load.any? { |el| el == :cost_object diff --git a/spec/models/work_package/cost_eager_loading_spec.rb b/spec/models/work_package/cost_eager_loading_spec.rb new file mode 100644 index 0000000000..8ce06b2a64 --- /dev/null +++ b/spec/models/work_package/cost_eager_loading_spec.rb @@ -0,0 +1,123 @@ +#-- copyright +# OpenProject Costs Plugin +# +# Copyright (C) 2009 - 2014 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# version 3. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +#++ + +require 'spec_helper' + +describe WorkPackage, 'cost eager loading', type: :model do + let(:project) do + work_package.project + end + let(:role) do + FactoryGirl.create(:role, + permissions: [:view_work_packages, + :view_cost_entries, + :view_cost_rates, + :view_time_entries, + :log_time, + :log_costs, + :view_hourly_rates]) + end + let(:user) do + FactoryGirl.create(:user, + member_in_project: project, + member_through_role: role) + end + + let(:cost_type) do + FactoryGirl.create(:cost_type) + end + let(:work_package) do + FactoryGirl.create(:work_package) + end + let(:cost_entry1) do + FactoryGirl.create(:cost_entry, + cost_type: cost_type, + user: user, + work_package: work_package, + project: project) + end + let(:cost_entry2) do + FactoryGirl.create(:cost_entry, + cost_type: cost_type, + user: user, + work_package: work_package, + project: project) + + end + let(:time_entry1) do + FactoryGirl.create(:time_entry, + user: user, + project: project, + work_package: work_package) + end + let(:time_entry2) do + FactoryGirl.create(:time_entry, + user: user, + project: project, + work_package: work_package) + end + let(:user_rates) do + FactoryGirl.create(:hourly_rate, + user: user, + project: project) + end + let(:cost_rate) do + FactoryGirl.create(:cost_rate, + cost_type: cost_type) + end + + context "combining core's and cost's eager loading" do + let(:scope) do + + scope = WorkPackage + .include_spent_hours(user) + .where(id: [work_package.id]) + + OpenProject::Costs::Engine::EagerLoadedCosts.join_costs(scope) + end + + before do + allow(User) + .to receive(:current) + .and_return(user) + + user_rates + project.reload + cost_rate + cost_entry1 + cost_entry2 + time_entry1 + time_entry2 + end + + subject { scope.first } + + it 'correctly calculates spent time' do + expect(scope.to_a.first.hours).to eql time_entry1.hours + time_entry2.hours + end + + it 'correctly calculates labor costs' do + expect(scope.first.labor_costs).to eql (user_rates.rate * (time_entry1.hours + time_entry2.hours)).to_f + end + + it 'correctly calculates material costs' do + expect(scope.first.material_costs).to eql (cost_entry1.costs + cost_entry2.costs).to_f + end + end +end diff --git a/spec/support/pages/budget_form.rb b/spec/support/pages/budget_form.rb index 6dc68283db..35525cf130 100644 --- a/spec/support/pages/budget_form.rb +++ b/spec/support/pages/budget_form.rb @@ -71,13 +71,13 @@ module Pages end def add_unit_costs_row! - find('#material_budget_items_fieldset a', text: 'Add planned costs').click + find('#material_budget_items_fieldset .wp-inline-create--add-link').click @unit_rows = unit_rows + 1 end def add_labor_costs_row! - find('#labor_budget_items_fieldset a', text: 'Add planned costs').click + find('#labor_budget_items_fieldset .wp-inline-create--add-link').click @labor_rows = labor_rows + 1 end diff --git a/spec/support/pages/edit_budget.rb b/spec/support/pages/edit_budget.rb index e87882d29f..3822a681f1 100644 --- a/spec/support/pages/edit_budget.rb +++ b/spec/support/pages/edit_budget.rb @@ -27,10 +27,11 @@ #++ require 'support/pages/page' +require_relative 'budget_form' module Pages class EditBudget < Page - include BudgetForm + include ::Pages::BudgetForm attr_reader :cost_object_id # cost_object == budget diff --git a/spec/support/pages/new_budget.rb b/spec/support/pages/new_budget.rb index 91b1eabdfd..fb3060ba62 100644 --- a/spec/support/pages/new_budget.rb +++ b/spec/support/pages/new_budget.rb @@ -27,10 +27,11 @@ #++ require 'support/pages/page' +require_relative 'budget_form' module Pages class NewBudget < Page - include BudgetForm + include ::Pages::BudgetForm attr_reader :project_identifier