Merge pull request #14351 from opf/fix-time-logging-issue

Fix time logging issue

app/policies/work_package_policy.rb

@@ -31,12 +31,10 @@ class WorkPackagePolicy < BasePolicy
 
   def cache(work_package)
     @cache ||= Hash.new do |wp_hash, wp|
-      wp_hash[wp] = Hash.new do |project_hash, project|
-        project_hash[project] = allowed_hash(wp)
-      end
+      wp_hash[wp] = allowed_hash(wp)
     end
 
-    @cache[work_package][work_package.project]
+    @cache[work_package]
   end
 
   def allowed_hash(work_package)

lib/api/caching/cached_representer.rb

@@ -190,7 +190,7 @@ def json_key_representer_parts
           cacheable << json_key_parts_of_represented
           cacheable << json_key_dependencies
 
-          OpenProject::Cache::CacheKey.expand(cacheable.flatten.compact)
+          OpenProject::Cache::CacheKey.expand(OpenProject::Cache::CacheKey.key(cacheable.flatten.compact))
         end
 
         def json_key_part_represented

lib/api/v3/work_packages/schema/work_package_schema_representer.rb

@@ -62,21 +62,21 @@ def schema(property, *args)
               opts, = args
               opts[:attribute_group] = attribute_group property
 
-              super property, **opts
+              super(property, **opts)
             end
 
             def schema_with_allowed_link(property, *args)
               opts, = args
               opts[:attribute_group] = attribute_group property
 
-              super property, **opts
+              super(property, **opts)
             end
 
             def schema_with_allowed_collection(property, *args)
               opts, = args
               opts[:attribute_group] = attribute_group property
 
-              super property, **opts
+              super(property, **opts)
             end
           end
 
@@ -168,9 +168,9 @@ def initialize(schema, self_link:, **context)
                  type: 'Duration',
                  required: false,
                  show_if: ->(*) {
-                   current_user.allowed_in_project?(:view_time_entries, represented.project) ||
-                   current_user.allowed_in_work_package?(:view_own_time_entries, represented)
-                 }
+                            current_user.allowed_in_project?(:view_time_entries, represented.project) ||
+                            current_user.allowed_in_any_work_package?(:view_own_time_entries, in_project: represented.project)
+                          }
 
           schema :percentage_done,
                  type: 'Integer',

lib/api/v3/work_packages/work_package_representer.rb

@@ -573,7 +573,16 @@ def current_user_update_allowed?
         def view_time_entries_allowed?
           @view_time_entries_allowed ||=
             current_user.allowed_in_project?(:view_time_entries, represented.project) ||
-              current_user.allowed_in_work_package?(:view_own_time_entries, represented)
+            view_own_time_entries_allowed?
+        end
+
+        def view_own_time_entries_allowed?
+          @view_own_time_entries_allowed ||= if represented.new_record?
+                                               current_user.allowed_in_any_work_package?(:view_own_time_entries,
+                                                                                         in_project: represented.project)
+                                             else
+                                               current_user.allowed_in_work_package?(:view_own_time_entries, represented)
+                                             end
         end
 
         def log_time_allowed?

modules/costs/app/models/cost_scopes.rb

@@ -37,11 +37,9 @@ def with_visible_costs_on(scope, user: User.current, project: nil)
   def with_visible_entries_on(scope, user: User.current, project: nil)
     table = arel_table
 
-    view_allowed = Project.allowed_to(user, view_allowed_entries_permission).select(:id)
-    # TODO: We either should move all _own_ permissions to be work package scoped and then change this
-    # query, or make this method less generic
-    view_own_allowed = Project.allowed_to(user, view_allowed_own_entries_permission).select(:id)
-    visible_scope = scope.where view_or_view_own(table, view_allowed, view_own_allowed, user)
+    visible_scope = scope.where(
+      view_or_view_own(table, view_allowed_entries_permission, view_allowed_own_entries_permission, user)
+    )
 
     if project
       visible_scope.where(project_id: project.id)
@@ -50,14 +48,26 @@ def with_visible_entries_on(scope, user: User.current, project: nil)
     end
   end
 
-  def view_or_view_own(table, view_allowed, view_own_allowed, user)
-    table[:project_id]
-      .in(view_allowed.arel)
-      .or(
+  def view_or_view_own(table, allowed_permission, allowed_own_permission, user) # rubocop:disable Metrics/AbcSize
+    project_allowed_scope = table[:project_id].in(Project.allowed_to(user, allowed_permission).select(:id).arel)
+
+    # We allow some of the `_own_` permissions on the WorkPackage, but others only on the project,
+    # so we need to figure out the correct scope to use
+    wp_scoped_permission = Authorization.permissions_for(allowed_own_permission).any?(&:work_package?)
+
+    if wp_scoped_permission
+      project_allowed_scope.or(
+        table[:work_package_id]
+        .in(WorkPackage.allowed_to(user, allowed_own_permission).select(:id).arel)
+        .and(table[:user_id].eq(user.id))
+      )
+    else
+      project_allowed_scope.or(
         table[:project_id]
-          .in(view_own_allowed.arel)
-          .and(table[:user_id].eq(user.id))
+        .in(Project.allowed_to(user, allowed_own_permission).select(:id).arel)
+        .and(table[:user_id].eq(user.id))
       )
+    end
   end
 
   def with_visible_rates_on(scope, user: User.current)

modules/costs/app/models/time_entries/time_entry_scopes.rb

@@ -12,11 +12,7 @@ def view_allowed_own_entries_permission
 
     def with_visible_rates_on(scope, user: User.current)
       table = arel_table
-
-      view_allowed = Project.allowed_to(user, :view_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)
+      scope.where(view_or_view_own(table, :view_hourly_rates, :view_own_hourly_rate, user))
     end
   end
 end

modules/costs/app/models/time_entry.rb

@@ -52,7 +52,6 @@ class TimeEntry < ApplicationRecord
   include Entry::SplashedDates
 
   scopes :of_user_and_day,
-         :visible,
          :ongoing
 
   # TODO: move into service

modules/costs/spec/features/view_own_rates_spec.rb

@@ -28,20 +28,20 @@
 
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
-RSpec.describe 'Only see your own rates', js: true do
+RSpec.describe 'Only see your own rates', :js do
   let(:project) { work_package.project }
   let(:user) do
     create(:user,
            member_with_roles: { project => role })
   end
   let(:role) do
     create(:project_role, permissions: %i[view_own_hourly_rate
-                                  view_work_packages
-                                  view_work_packages
-                                  view_own_time_entries
-                                  view_own_cost_entries
-                                  view_cost_rates
-                                  log_costs])
+                                          view_work_packages
+                                          view_work_packages
+                                          view_own_time_entries
+                                          view_own_cost_entries
+                                          view_cost_rates
+                                          log_costs])
   end
   let(:work_package) { create(:work_package) }
   let(:wp_page) { Pages::FullWorkPackage.new(work_package) }

modules/costs/spec/models/time_entries/scopes/visible_spec.rb

@@ -28,13 +28,8 @@
 
 require 'spec_helper'
 
-RSpec.describe TimeEntries::Scopes::Visible do
+RSpec.describe TimeEntry, "visible scope" do
   let(:project) { create(:project) }
-  let(:user) do
-    create(:user,
-           member_with_permissions: { project => permissions })
-  end
-  let(:permissions) { [:view_time_entries] }
 
   let(:work_package) do
     create(:work_package,
@@ -68,18 +63,20 @@
     subject { TimeEntry.visible(user) }
 
     context 'for a user having the view_time_entries permission' do
+      let(:user) { create(:user, member_with_permissions: { project => [:view_time_entries] }) }
+
       it 'retrieves all the time entries of projects the user has the permissions in' do
         expect(subject)
-          .to match_array([own_project_time_entry, project_time_entry])
+          .to contain_exactly(own_project_time_entry, project_time_entry)
       end
     end
 
-    context 'for a user having the view_own_time_entries permission' do
-      let(:permissions) { [:view_own_time_entries] }
+    context 'for a user having the view_own_time_entries permission on a work package' do
+      let(:user) { create(:user, member_with_permissions: { work_package => [:view_own_time_entries] }) }
 
       it 'retrieves all the time entries of the user in projects the user has the permissions in' do
         expect(subject)
-          .to match_array([own_project_time_entry])
+          .to contain_exactly(own_project_time_entry)
       end
     end
   end

spec/factories/principal_factory.rb

@@ -77,7 +77,7 @@
           create(:member, principal:, project: object, roles: [role])
         elsif Member.can_be_member_of?(object)
           role = create(:work_package_role, permissions:)
-          create(:member, principal:, entity: object, project: entity.project, roles: [role])
+          create(:member, principal:, entity: object, project: object.project, roles: [role])
         end
       end