Merge pull request #212 from ulferts/feature/improve_apiv3_wp_index

Feature/improve apiv3 wp index

lib/open_project/costs/attributes_helper.rb

@@ -33,11 +33,23 @@ module OpenProject::Costs
     end
 
     def time_entries
-      @work_package.time_entries.visible(@user, @work_package.project)
+      if @work_package.time_entries.loaded?
+        @work_package.time_entries.select do |time_entry|
+          time_entry.visible_by?(@user)
+        end
+      else
+        @work_package.time_entries.visible(@user, @work_package.project)
+      end
     end
 
     def cost_entries
-      @cost_entries ||= @work_package.cost_entries.visible(@user, @work_package.project)
+      @cost_entries ||= if @work_package.cost_entries.loaded?
+                          @work_package.cost_entries.select do |cost_entry|
+                            cost_entry.visible_by?(@user)
+                          end
+                        else
+                          @work_package.cost_entries.visible(@user, @work_package.project)
+                        end
     end
 
     private

lib/open_project/costs/engine.rb

@@ -286,6 +286,14 @@ module OpenProject::Costs
       # TODO: this recreates the original behaviour
       # however, it might not be desirable to allow assigning of cost_object regardless of the permissions
       PermittedParams.permit(:new_work_package, :cost_object_id)
+
+      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]
     end
 
     config.to_prepare do |_app|

lib/open_project/costs/patches/time_entry_patch.rb

@@ -28,17 +28,20 @@ module OpenProject::Costs::Patches::TimeEntryPatch
     base.class_eval do
       belongs_to :rate, -> { where(type: ['HourlyRate', 'DefaultHourlyRate']) }, class_name: 'Rate'
 
-      scope :visible, lambda { |*args|
-        where(TimeEntry.visible_condition(args[0] || User.current, args[1]))
-          .includes(:project, :user)
-          .references(:project)
-      }
-
       before_save :update_costs
 
-      def self.visible_condition(user, project)
-        %{ (#{Project.allowed_to_condition(user, :view_time_entries, project: project)} OR
-             (#{Project.allowed_to_condition(user, :view_own_time_entries, project: project)} AND #{TimeEntry.table_name}.user_id = #{user.id})) }
+      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_costs, lambda{|*args|
@@ -47,7 +50,7 @@ module OpenProject::Costs::Patches::TimeEntryPatch
 
         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)
+        view_time_entries = TimeEntry.visible_condition(user, project: project)
 
         includes(:project, :user)
           .where([view_time_entries, view_hourly_rates].join(' AND '))
@@ -126,7 +129,8 @@ module OpenProject::Costs::Patches::TimeEntryPatch
     end
 
     def visible_by?(usr)
-      usr.allowed_to?(:view_time_entries, project)
+      usr.allowed_to?(:view_time_entries, project) ||
+        (user_id == usr.id && usr.allowed_to?(:view_own_time_entries, project))
     end
 
     def costs_visible_by?(usr)

spec/lib/api/v3/work_packages/work_package_representer_spec.rb

@@ -243,4 +243,24 @@ describe ::API::V3::WorkPackages::WorkPackageRepresenter do
       it { is_expected.not_to have_json_path('_links/log_costs') }
     end
   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
+      }).to be_truthy
+    end
+  end
 end

spec/models/time_entry_spec.rb

@@ -295,6 +295,54 @@ describe TimeEntry, type: :model do
     end
   end
 
+  describe 'visible_by?' do
+    context 'when not having the necessary permissions' do
+      before do
+        is_member(project, user, [])
+      end
+
+      it 'is visible' do
+        expect(time_entry.visible_by?(user)).to be_falsey
+      end
+    end
+
+    context 'when having the view_time_entries permission' do
+      before do
+        is_member(project, user, [:view_time_entries])
+      end
+
+      it 'is visible' do
+        expect(time_entry.visible_by?(user)).to be_truthy
+      end
+    end
+
+    context 'when having the view_own_time_entries permission ' +
+      'and being the owner of the time entry' do
+      before do
+        is_member(project, user, [:view_own_time_entries])
+
+        time_entry.user = user
+      end
+
+      it 'is visible' do
+        expect(time_entry.visible_by?(user)).to be_truthy
+      end
+    end
+
+    context 'when having the view_own_time_entries permission ' +
+      'and not being the owner of the time entry' do
+      before do
+        is_member(project, user, [:view_own_time_entries])
+
+        time_entry.user = FactoryGirl.build :user
+      end
+
+      it 'is visible' do
+        expect(time_entry.visible_by?(user)).to be_falsey
+      end
+    end
+  end
+
   describe 'class' do
     describe '#visible' do
       describe "WHEN having the view_time_entries permission