Merge pull request #283 from finnlabs/fix/time_entry_sums

Fix/time entry sums

.travis.yml

@@ -59,7 +59,7 @@ env:
     - COVERAGE=true
 
   matrix:
-    - "TEST_SUITE=plugins:spec      DB=mysql    GROUP_SIZE=1 GROUP=1"
+    - "TEST_SUITE=plugins:specs     DB=mysql    GROUP_SIZE=1 GROUP=1"
     - "TEST_SUITE=plugins:cucumber  DB=mysql    GROUP_SIZE=1 GROUP=1"
 
     - "TEST_SUITE=npm"

app/models/cost_scopes.rb

@@ -3,13 +3,13 @@ module CostScopes
     base_module.class_eval do
       def self.extended(base_class)
         base_class.class_eval do
-          scope :visible, -> (*args) {
+          scope :visible, ->(*args) {
             user = args.first || User.current
 
             with_visible_entries_on self, user: user, project: args[1]
           }
 
-          scope :visible_costs, -> (*args) {
+          scope :visible_costs, ->(*args) {
             user = args.first || User.current
 
             with_visible_costs_on self, user: user, project: args[1]

app/models/labor_budget_item.rb

@@ -36,7 +36,7 @@ class LaborBudgetItem < ActiveRecord::Base
     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_own_allowed = Project.allowed_to(user, :view_own_hourly_rate).select(:id)
 
     view_or_view_own = table[:project_id]
                        .in(view_allowed.arel)

app/models/time_entry/time_entry_scopes.rb

@@ -14,7 +14,7 @@ class TimeEntry
       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_own_allowed = Project.allowed_to(user, :view_own_hourly_rate).select(:id)
 
       scope.where view_or_view_own(table, view_allowed, view_own_allowed, user)
     end

app/models/work_package/labor_costs.rb

@@ -17,7 +17,7 @@ class WorkPackage
     end
 
     def sum_subselect(base_scope)
-      super.project('SUM(hours) hours')
+      super
     end
   end
 end

lib/open_project/costs/engine.rb

@@ -356,27 +356,34 @@ module OpenProject::Costs
       end
 
       def self.join_costs(scope)
-        material = WorkPackage::MaterialCosts.new
-        labor = WorkPackage::LaborCosts.new
-
         # 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"
+        # Costs will add another "LEFT OUTER JOIN time_entries". The two joins
-        # where the on clause allows all time entries to be joined if he has
+        # may or may not include each other's rows depending on the user's and the project's permissions.
+        # This is caused by entries being 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.
+        # Because of that, entries may be joined twice.
-        # We therefore have to remove core's join.
+        # We therefore modify the core's join by placing it in a subquery similar to those of costs.
         #
         # 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.
+        material = WorkPackage::MaterialCosts.new
+        labor = WorkPackage::LaborCosts.new
+        time = scope.dup
+
+        wp_table = WorkPackage.arel_table
+        time_join = wp_table
+                    .outer_join(time.arel.as('spent_time_hours'))
+                    .on(wp_table[:id].eq(time.arel_table.alias('spent_time_hours')[:id]))
+
         scope.joins_values.reject! do |join|
           join.is_a?(Arel::Nodes::OuterJoin) &&
-               join.left.is_a?(Arel::Table) &&
+            join.left.is_a?(Arel::Table) &&
-               join.left.name == 'time_entries'
+            join.left.name == 'time_entries'
         end
         scope.select_values.reject! do |select|
           select == "SUM(time_entries.hours) AS hours"
@@ -385,16 +392,18 @@ module OpenProject::Costs
         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)
+        target_scope = scope
+                       .joins(material_scope.join_sources)
                        .joins(labor_scope.join_sources)
+                       .joins(time_join.join_sources)
                        .select(material_scope.select_values)
                        .select(labor_scope.select_values)
-                       .select('time_entries.hours')
+                       .select('spent_time_hours.hours')
 
         target_scope.joins_values.reject! do |join|
           join.is_a?(Arel::Nodes::OuterJoin) &&
-               join.left.is_a?(Arel::Nodes::TableAlias) &&
+            join.left.is_a?(Arel::Nodes::TableAlias) &&
-               join.left.right == 'descendants'
+            join.left.right == 'descendants'
         end
 
         target_scope.group_values.reject! do |group|

spec/models/work_package/cost_eager_loading_spec.rb

@@ -58,7 +58,6 @@ describe WorkPackage, 'cost eager loading', type: :model do
                        user: user,
                        work_package: work_package,
                        project: project)
-
   end
   let(:time_entry1) do
     FactoryGirl.create(:time_entry,
@@ -84,9 +83,9 @@ describe WorkPackage, 'cost eager loading', type: :model do
 
   context "combining core's and cost's eager loading" do
     let(:scope) do
-
       scope = WorkPackage
               .include_spent_hours(user)
+              .select('work_packages.*')
               .where(id: [work_package.id])
 
       OpenProject::Costs::Engine::EagerLoadedCosts.join_costs(scope)