Merge branch 'remove-group-timelog-mandatory-arguments' into 'master'

Improve timelog graphQL functionality

See merge request gitlab-org/gitlab!61847

app/graphql/resolvers/timelog_resolver.rb

@@ -23,90 +23,72 @@ module Resolvers
              description: 'List time-logs within a time range where the logged time is equal to or before endTime.'
 
     def resolve_with_lookahead(**args)
-      return Timelog.none unless timelogs_available_for_user?
+      build_timelogs
 
-      validate_params_presence!(args)
-      transformed_args = transform_args(args)
-      validate_time_difference!(transformed_args)
+      if args.any?
+        validate_args!(args)
+        build_parsed_args(args)
+        validate_time_difference!
+        apply_time_filter
+      end
 
-      find_timelogs(transformed_args)
+      apply_lookahead(timelogs)
     end
 
     private
 
+    attr_reader :parsed_args, :timelogs
+
     def preloads
       {
         note: [:note]
       }
     end
 
-    def find_timelogs(args)
-      apply_lookahead(group.timelogs(args[:start_time], args[:end_time]))
+    def validate_args!(args)
+      if args[:start_time] && args[:start_date]
+        raise_argument_error('Provide either a start date or time, but not both')
+      elsif args[:end_time] && args[:end_date]
+        raise_argument_error('Provide either an end date or time, but not both')
       end
-
-    def timelogs_available_for_user?
-      group&.user_can_access_group_timelogs?(context[:current_user])
     end
 
-    def validate_params_presence!(args)
-      message = case time_params_count(args)
-                when 0
-                  'Start and End arguments must be present'
-                when 1
-                  'Both Start and End arguments must be present'
-                when 2
-                  validate_duplicated_args(args)
-                when 3 || 4
-                  'Only Time or Date arguments must be present'
-                end
+    def build_parsed_args(args)
+      if times_provided?(args)
+        @parsed_args = args
+      else
+        @parsed_args = args.except(:start_date, :end_date)
 
-      raise_argument_error(message) if message
+        @parsed_args[:start_time] = args[:start_date].beginning_of_day if args[:start_date]
+        @parsed_args[:end_time] = args[:end_date].end_of_day if args[:end_date]
       end
-
-    def validate_time_difference!(args)
-      message = if args[:end_time] < args[:start_time]
-                  'Start argument must be before End argument'
-                elsif args[:end_time] - args[:start_time] > 60.days
-                  'The time range period cannot contain more than 60 days'
     end
 
-      raise_argument_error(message) if message
+    def times_provided?(args)
+      args[:start_time] && args[:end_time]
     end
 
-    def transform_args(args)
-      return args if args.keys == [:start_time, :end_time]
+    def validate_time_difference!
+      return unless end_time_before_start_time?
 
-      time_args = args.except(:start_date, :end_date)
-
-      if time_args.empty?
-        time_args[:start_time] = args[:start_date].beginning_of_day
-        time_args[:end_time] = args[:end_date].end_of_day
-      elsif time_args.key?(:start_time)
-        time_args[:end_time] = args[:end_date].end_of_day
-      elsif time_args.key?(:end_time)
-        time_args[:start_time] = args[:start_date].beginning_of_day
+      raise_argument_error('Start argument must be before End argument')
     end
 
-      time_args
+    def end_time_before_start_time?
+      times_provided?(parsed_args) && parsed_args[:end_time] < parsed_args[:start_time]
     end
 
-    def time_params_count(args)
-      [:start_time, :end_time, :start_date, :end_date].count { |param| args.key?(param) }
+    def build_timelogs
+      @timelogs = Timelog.in_group(object)
     end
 
-    def validate_duplicated_args(args)
-      if args.key?(:start_time) && args.key?(:start_date) ||
-        args.key?(:end_time) && args.key?(:end_date)
-        'Both Start and End arguments must be present'
-      end
+    def apply_time_filter
+      @timelogs = timelogs.at_or_after(parsed_args[:start_time]) if parsed_args[:start_time]
+      @timelogs = timelogs.at_or_before(parsed_args[:end_time]) if parsed_args[:end_time]
     end
 
     def raise_argument_error(message)
       raise Gitlab::Graphql::Errors::ArgumentError, message
     end
-
-    def group
-      @group ||= object.respond_to?(:sync) ? object.sync : object
-    end
   end
 end

app/graphql/types/timelog_type.rb

@@ -4,7 +4,7 @@ module Types
   class TimelogType < BaseObject
     graphql_name 'Timelog'
 
-    authorize :read_group_timelogs
+    authorize :read_issue
 
     field :spent_at,
           Types::TimeType,

app/models/concerns/has_timelogs_report.rb

-# frozen_string_literal: true
-
-module HasTimelogsReport
-  extend ActiveSupport::Concern
-  include Gitlab::Utils::StrongMemoize
-
-  def timelogs(start_time, end_time)
-    strong_memoize(:timelogs) { timelogs_for(start_time, end_time) }
-  end
-
-  def user_can_access_group_timelogs?(current_user)
-    Ability.allowed?(current_user, :read_group_timelogs, self)
-  end
-
-  private
-
-  def timelogs_for(start_time, end_time)
-    Timelog.between_times(start_time, end_time).in_group(self)
-  end
-end

app/models/group.rb

@@ -16,7 +16,6 @@ class Group < Namespace
   include Gitlab::Utils::StrongMemoize
   include GroupAPICompatibility
   include EachBatch
-  include HasTimelogsReport
   include BulkMemberAccessLoad
 
   has_many :all_group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent

app/models/timelog.rb

@@ -18,8 +18,12 @@ class Timelog < ApplicationRecord
     joins(:project).where(projects: { namespace: group.self_and_descendants })
   end
 
-  scope :between_times, -> (start_time, end_time) do
-    where('spent_at BETWEEN ? AND ?', start_time, end_time)
+  scope :at_or_after, -> (start_time) do
+    where('spent_at >= ?', start_time)
+  end
+
+  scope :at_or_before, -> (end_time) do
+    where('spent_at <= ?', end_time)
   end
 
   def issuable

app/policies/group_policy.rb

@@ -131,7 +131,6 @@ class GroupPolicy < BasePolicy
     enable :read_prometheus
     enable :read_package
     enable :read_package_settings
-    enable :read_group_timelogs
   end
 
   rule { maintainer }.policy do

app/policies/project_policy.rb

@@ -263,7 +263,6 @@ class ProjectPolicy < BasePolicy
     enable :read_confidential_issues
     enable :read_package
     enable :read_product_analytics
-    enable :read_group_timelogs
   end
 
   # We define `:public_user_access` separately because there are cases in gitlab-ee

app/policies/timelog_policy.rb

 # frozen_string_literal: true
 
 class TimelogPolicy < BasePolicy
-  delegate { @subject.issuable.resource_parent }
+  delegate { @subject.issuable }
 end

doc/user/permissions.md

@@ -80,6 +80,7 @@ The following table lists project permissions available for each role:
 | Label issues                                      |         | ✓          | ✓           | ✓        | ✓      |
 | Set issue weight                                  |         | ✓          | ✓           | ✓        | ✓      |
 | [Set issue estimate and record time spent](project/time_tracking.md) | | ✓ | ✓         | ✓        | ✓      |
+| View a time tracking report                       | ✓ (*1*) | ✓          | ✓           | ✓        | ✓      |
 | Lock issue threads                                |         | ✓          | ✓           | ✓        | ✓      |
 | Manage issue tracker                              |         | ✓          | ✓           | ✓        | ✓      |
 | Manage linked issues                              |         | ✓          | ✓           | ✓        | ✓      |

doc/user/project/time_tracking.md

@@ -82,6 +82,10 @@ To remove all the time spent at once, use `/remove_time_spent`.
 
 You can view a breakdown of time spent on an issue or merge request.
 
+Prerequisites:
+
+- A minimum of [Reporter](../permissions.md#project-members-permissions) access to a private project in GitLab.
+
 To view a time tracking report, go to an issue or a merge request and select **Time tracking report**
 in the right sidebar.
 

spec/graphql/resolvers/timelog_resolver_spec.rb

@@ -12,41 +12,35 @@ RSpec.describe Resolvers::TimelogResolver do
   context "with a group" do
     let_it_be(:current_user) { create(:user) }
     let_it_be(:group) { create(:group) }
-    let_it_be(:project)            { create(:project, :public, group: group) }
-
-    before_all do
-      group.add_developer(current_user)
-      project.add_developer(current_user)
-    end
-
-    before do
-      group.clear_memoization(:timelogs)
-    end
+    let_it_be(:project) { create(:project, :empty_repo, :public, group: group) }
 
     describe '#resolve' do
+      let_it_be(:short_time_ago) { 5.days.ago.beginning_of_day }
+      let_it_be(:medium_time_ago) { 15.days.ago.beginning_of_day }
+
       let_it_be(:issue) { create(:issue, project: project) }
-      let_it_be(:issue2) { create(:issue, project: project) }
-      let_it_be(:timelog1) { create(:issue_timelog, issue: issue, spent_at: 2.days.ago.beginning_of_day) }
-      let_it_be(:timelog2) { create(:issue_timelog, issue: issue2, spent_at: 2.days.ago.end_of_day) }
-      let_it_be(:timelog3) { create(:issue_timelog, issue: issue2, spent_at: 10.days.ago) }
+      let_it_be(:merge_request) { create(:merge_request, source_project: project) }
 
-      let(:args) { { start_time: 6.days.ago, end_time: 2.days.ago.noon } }
+      let_it_be(:timelog1) { create(:issue_timelog, issue: issue, spent_at: short_time_ago.beginning_of_day) }
+      let_it_be(:timelog2) { create(:issue_timelog, issue: issue, spent_at: short_time_ago.end_of_day) }
+      let_it_be(:timelog3) { create(:merge_request_timelog, merge_request: merge_request, spent_at: medium_time_ago) }
 
-      it 'finds all timelogs within given dates' do
-        timelogs = resolve_timelogs(**args)
+      let(:args) { { start_time: short_time_ago, end_time: short_time_ago.noon } }
 
-        expect(timelogs).to contain_exactly(timelog1)
+      it 'finds all timelogs' do
+        timelogs = resolve_timelogs
+
+        expect(timelogs).to contain_exactly(timelog1, timelog2, timelog3)
       end
 
-      it 'return nothing when user has insufficient permissions' do
-        user = create(:user)
-        group.add_guest(current_user)
+      it 'finds all timelogs within given dates' do
+        timelogs = resolve_timelogs(**args)
 
-        expect(resolve_timelogs(user: user, **args)).to be_empty
+        expect(timelogs).to contain_exactly(timelog1)
       end
 
-      context 'when start_time and end_date are present' do
-        let(:args) { { start_time: 6.days.ago, end_date: 2.days.ago } }
+      context 'when only start_date is present' do
+        let(:args) { { start_date: short_time_ago } }
 
         it 'finds timelogs until the end of day of end_date' do
           timelogs = resolve_timelogs(**args)
@@ -55,108 +49,65 @@ RSpec.describe Resolvers::TimelogResolver do
         end
       end
 
-      context 'when start_date and end_time are present' do
-        let(:args) { { start_date: 6.days.ago, end_time: 2.days.ago.noon } }
+      context 'when only end_date is present' do
+        let(:args) { { end_date: medium_time_ago } }
 
-        it 'finds all timelogs within start_date and end_time' do
+        it 'finds timelogs until the end of day of end_date' do
           timelogs = resolve_timelogs(**args)
 
-          expect(timelogs).to contain_exactly(timelog1)
+          expect(timelogs).to contain_exactly(timelog3)
         end
       end
 
-      context 'when arguments are invalid' do
-        let_it_be(:error_class) { Gitlab::Graphql::Errors::ArgumentError }
-
-        context 'when no time or date arguments are present' do
-          let(:args) { {} }
-
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Start and End arguments must be present/)
-          end
-        end
+      context 'when start_time and end_date are present' do
+        let(:args) { { start_time: short_time_ago, end_date: short_time_ago } }
 
-        context 'when only start_time is present' do
-          let(:args) { { start_time: 6.days.ago } }
+        it 'finds timelogs until the end of day of end_date' do
+          timelogs = resolve_timelogs(**args)
 
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Both Start and End arguments must be present/)
+          expect(timelogs).to contain_exactly(timelog1, timelog2)
         end
       end
 
-        context 'when only end_time is present' do
-          let(:args) { { end_time: 2.days.ago } }
-
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Both Start and End arguments must be present/)
-          end
-        end
+      context 'when start_date and end_time are present' do
+        let(:args) { { start_date: short_time_ago, end_time: short_time_ago.noon } }
 
-        context 'when only start_date is present' do
-          let(:args) { { start_date: 6.days.ago } }
+        it 'finds all timelogs within start_date and end_time' do
+          timelogs = resolve_timelogs(**args)
 
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Both Start and End arguments must be present/)
+          expect(timelogs).to contain_exactly(timelog1)
         end
       end
 
-        context 'when only end_date is present' do
-          let(:args) { { end_date: 2.days.ago } }
-
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Both Start and End arguments must be present/)
-          end
-        end
+      context 'when arguments are invalid' do
+        let_it_be(:error_class) { Gitlab::Graphql::Errors::ArgumentError }
 
         context 'when start_time and start_date are present' do
-          let(:args) { { start_time: 6.days.ago, start_date: 6.days.ago } }
+          let(:args) { { start_time: short_time_ago, start_date: short_time_ago } }
 
           it 'returns correct error' do
             expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Both Start and End arguments must be present/)
+              .to raise_error(error_class, /Provide either a start date or time, but not both/)
           end
         end
 
         context 'when end_time and end_date are present' do
-          let(:args) { { end_time: 2.days.ago, end_date: 2.days.ago } }
-
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Both Start and End arguments must be present/)
-          end
-        end
-
-        context 'when three arguments are present' do
-          let(:args) { { start_date: 6.days.ago, end_date: 2.days.ago, end_time: 2.days.ago } }
+          let(:args) { { end_time: short_time_ago, end_date: short_time_ago } }
 
           it 'returns correct error' do
             expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /Only Time or Date arguments must be present/)
+              .to raise_error(error_class, /Provide either an end date or time, but not both/)
           end
         end
 
         context 'when start argument is after end argument' do
-          let(:args) { { start_time: 2.days.ago, end_time: 6.days.ago } }
+          let(:args) { { start_time: short_time_ago, end_time: medium_time_ago } }
 
           it 'returns correct error' do
             expect { resolve_timelogs(**args) }
               .to raise_error(error_class, /Start argument must be before End argument/)
           end
         end
-
-        context 'when time range is more than 60 days' do
-          let(:args) { { start_time: 3.months.ago, end_time: 2.days.ago } }
-
-          it 'returns correct error' do
-            expect { resolve_timelogs(**args) }
-              .to raise_error(error_class, /The time range period cannot contain more than 60 days/)
-          end
-        end
       end
     end
   end

spec/graphql/types/timelog_type_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe GitlabSchema.types['Timelog'] do
 
   it { expect(described_class.graphql_name).to eq('Timelog') }
   it { expect(described_class).to have_graphql_fields(fields) }
-  it { expect(described_class).to require_graphql_authorizations(:read_group_timelogs) }
+  it { expect(described_class).to require_graphql_authorizations(:read_issue) }
 
   describe 'user field' do
     subject { described_class.fields['user'] }

spec/models/concerns/has_timelogs_report_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe HasTimelogsReport do
-  let_it_be(:user) { create(:user) }
-
-  let(:group)     { create(:group) }
-  let(:project)   { create(:project, :public, group: group) }
-  let(:issue1)    { create(:issue, project: project) }
-  let(:merge_request1) { create(:merge_request, source_project: project) }
-
-  describe '#timelogs' do
-    let_it_be(:start_time) { 20.days.ago }
-    let_it_be(:end_time) { 8.days.ago }
-
-    let!(:timelog1) { create_timelog(15.days.ago, issue: issue1) }
-    let!(:timelog2) { create_timelog(10.days.ago, merge_request: merge_request1) }
-    let!(:timelog3) { create_timelog(5.days.ago, issue: issue1) }
-
-    before do
-      group.add_developer(user)
-    end
-
-    it 'returns collection of timelogs between given times' do
-      expect(group.timelogs(start_time, end_time).to_a).to match_array([timelog1, timelog2])
-    end
-
-    it 'returns empty collection if times are not present' do
-      expect(group.timelogs(nil, nil)).to be_empty
-    end
-
-    it 'returns empty collection if time range is invalid' do
-      expect(group.timelogs(end_time, start_time)).to be_empty
-    end
-  end
-
-  describe '#user_can_access_group_timelogs?' do
-    it 'returns true if user can access group timelogs' do
-      group.add_developer(user)
-
-      expect(group).to be_user_can_access_group_timelogs(user)
-    end
-
-    it 'returns false if user has insufficient permissions' do
-      group.add_guest(user)
-
-      expect(group).not_to be_user_can_access_group_timelogs(user)
-    end
-  end
-
-  def create_timelog(time, issue: nil, merge_request: nil)
-    create(:timelog, issue: issue, merge_request: merge_request, user: user, spent_at: time)
-  end
-end

spec/models/timelog_spec.rb

@@ -64,25 +64,57 @@ RSpec.describe Timelog do
     let_it_be(:subgroup_issue) { create(:issue, project: subgroup_project) }
     let_it_be(:subgroup_merge_request) { create(:merge_request, source_project: subgroup_project) }
 
-    let_it_be(:timelog) { create(:issue_timelog, spent_at: 65.days.ago) }
-    let_it_be(:timelog1) { create(:issue_timelog, spent_at: 15.days.ago, issue: group_issue) }
-    let_it_be(:timelog2) { create(:issue_timelog, spent_at: 5.days.ago, issue: subgroup_issue) }
-    let_it_be(:timelog3) { create(:merge_request_timelog, spent_at: 65.days.ago) }
-    let_it_be(:timelog4) { create(:merge_request_timelog, spent_at: 15.days.ago, merge_request: group_merge_request) }
-    let_it_be(:timelog5) { create(:merge_request_timelog, spent_at: 5.days.ago, merge_request: subgroup_merge_request) }
-
-    describe 'in_group' do
+    let_it_be(:short_time_ago) { 5.days.ago }
+    let_it_be(:medium_time_ago) { 15.days.ago }
+    let_it_be(:long_time_ago) { 65.days.ago }
+
+    let_it_be(:timelog) { create(:issue_timelog, spent_at: long_time_ago) }
+    let_it_be(:timelog1) { create(:issue_timelog, spent_at: medium_time_ago, issue: group_issue) }
+    let_it_be(:timelog2) { create(:issue_timelog, spent_at: short_time_ago, issue: subgroup_issue) }
+    let_it_be(:timelog3) { create(:merge_request_timelog, spent_at: long_time_ago) }
+    let_it_be(:timelog4) { create(:merge_request_timelog, spent_at: medium_time_ago, merge_request: group_merge_request) }
+    let_it_be(:timelog5) { create(:merge_request_timelog, spent_at: short_time_ago, merge_request: subgroup_merge_request) }
+
+    describe '.in_group' do
       it 'return timelogs created for group issues and merge requests' do
         expect(described_class.in_group(group)).to contain_exactly(timelog1, timelog2, timelog4, timelog5)
       end
     end
 
-    describe 'between_times' do
-      it 'returns collection of timelogs within given times' do
-        timelogs = described_class.between_times(20.days.ago, 10.days.ago)
+    describe '.at_or_after' do
+      it 'returns timelogs at the time limit' do
+        timelogs = described_class.at_or_after(short_time_ago)
 
-        expect(timelogs).to contain_exactly(timelog1, timelog4)
+        expect(timelogs).to contain_exactly(timelog2, timelog5)
       end
+
+      it 'returns timelogs after given time' do
+        timelogs = described_class.at_or_after(just_before(short_time_ago))
+
+        expect(timelogs).to contain_exactly(timelog2, timelog5)
+      end
+    end
+
+    describe '.at_or_before' do
+      it 'returns timelogs at the time limit' do
+        timelogs = described_class.at_or_before(long_time_ago)
+
+        expect(timelogs).to contain_exactly(timelog, timelog3)
+      end
+
+      it 'returns timelogs before given time' do
+        timelogs = described_class.at_or_before(just_after(long_time_ago))
+
+        expect(timelogs).to contain_exactly(timelog, timelog3)
+      end
+    end
+
+    def just_before(time)
+      time - 1.day
+    end
+
+    def just_after(time)
+      time + 1.day
     end
   end
 end

spec/policies/group_policy_spec.rb

@@ -923,54 +923,4 @@ RSpec.describe GroupPolicy do
       it { expect(described_class.new(current_user, subgroup)).to be_allowed(:read_label) }
     end
   end
-
-  context 'timelogs' do
-    context 'with admin' do
-      let(:current_user) { admin }
-
-      context 'when admin mode is enabled', :enable_admin_mode do
-        it { is_expected.to be_allowed(:read_group_timelogs) }
-      end
-
-      context 'when admin mode is disabled' do
-        it { is_expected.to be_disallowed(:read_group_timelogs) }
-      end
-    end
-
-    context 'with owner' do
-      let(:current_user) { owner }
-
-      it { is_expected.to be_allowed(:read_group_timelogs) }
-    end
-
-    context 'with maintainer' do
-      let(:current_user) { maintainer }
-
-      it { is_expected.to be_allowed(:read_group_timelogs) }
-    end
-
-    context 'with reporter' do
-      let(:current_user) { reporter }
-
-      it { is_expected.to be_allowed(:read_group_timelogs) }
-    end
-
-    context 'with guest' do
-      let(:current_user) { guest }
-
-      it { is_expected.to be_disallowed(:read_group_timelogs) }
-    end
-
-    context 'with non member' do
-      let(:current_user) { create(:user) }
-
-      it { is_expected.to be_disallowed(:read_group_timelogs) }
-    end
-
-    context 'with anonymous' do
-      let(:current_user) { nil }
-
-      it { is_expected.to be_disallowed(:read_group_timelogs) }
-    end
-  end
 end

spec/policies/project_policy_spec.rb

@@ -1385,54 +1385,4 @@ RSpec.describe ProjectPolicy do
       end
     end
   end
-
-  context 'timelogs' do
-    context 'with admin' do
-      let(:current_user) { admin }
-
-      context 'when admin mode enabled', :enable_admin_mode do
-        it { is_expected.to be_allowed(:read_group_timelogs) }
-      end
-
-      context 'when admin mode disabled' do
-        it { is_expected.to be_disallowed(:read_group_timelogs) }
-      end
-    end
-
-    context 'with owner' do
-      let(:current_user) { owner }
-
-      it { is_expected.to be_allowed(:read_group_timelogs) }
-    end
-
-    context 'with maintainer' do
-      let(:current_user) { maintainer }
-
-      it { is_expected.to be_allowed(:read_group_timelogs) }
-    end
-
-    context 'with reporter' do
-      let(:current_user) { reporter }
-
-      it { is_expected.to be_allowed(:read_group_timelogs) }
-    end
-
-    context 'with guest' do
-      let(:current_user) { guest }
-
-      it { is_expected.to be_disallowed(:read_group_timelogs) }
-    end
-
-    context 'with non member' do
-      let(:current_user) { non_member }
-
-      it { is_expected.to be_disallowed(:read_group_timelogs) }
-    end
-
-    context 'with anonymous' do
-      let(:current_user) { anonymous }
-
-      it { is_expected.to be_disallowed(:read_group_timelogs) }
-    end
-  end
 end

spec/requests/api/graphql/group/timelogs_spec.rb

@@ -17,8 +17,37 @@ RSpec.describe 'Timelogs through GroupQuery' do
 
     let(:timelogs_data)   { graphql_data['group']['timelogs']['nodes'] }
 
+    context 'when the project is private' do
+      let_it_be(:group2)    { create(:group) }
+      let_it_be(:project2)  { create(:project, :private, group: group2) }
+      let_it_be(:issue2)    { create(:issue, project: project2) }
+      let_it_be(:timelog3)  { create(:timelog, issue: issue2, spent_at: '2019-08-13 14:00:00') }
+
+      subject { post_graphql(query(full_path: group2.full_path), current_user: user) }
+
+      context 'when the user is not a member of the project' do
+        it 'returns no timelogs' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(graphql_errors).to be_nil
+          expect(timelog_array.size).to eq 0
+        end
+      end
+
+      context 'when the user is a member of the project' do
         before do
-      group.add_developer(user)
+          project2.add_developer(user)
+        end
+
+        it 'returns timelogs' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(graphql_errors).to be_nil
+          expect(timelog_array.size).to eq 1
+        end
+      end
     end
 
     context 'when the request is correct' do
@@ -74,18 +103,6 @@ RSpec.describe 'Timelogs through GroupQuery' do
           expect(timelogs_data).to be_empty
         end
       end
-
-      context 'when user has no permission to read group timelogs' do
-        it 'returns empty result' do
-          guest = create(:user)
-          group.add_guest(guest)
-          post_graphql(query, current_user: guest)
-
-          expect(response).to have_gitlab_http_status(:success)
-          expect(graphql_errors).to be_nil
-          expect(timelogs_data).to be_empty
-        end
-      end
     end
   end
 
@@ -95,7 +112,7 @@ RSpec.describe 'Timelogs through GroupQuery' do
     end
   end
 
-  def query(timelog_params = params)
+  def query(timelog_params: params, full_path: group.full_path)
     timelog_nodes = <<~NODE
       nodes {
         spentAt
@@ -114,7 +131,7 @@ RSpec.describe 'Timelogs through GroupQuery' do
 
     graphql_query_for(
       :group,
-      { full_path: group.full_path },
+      { full_path: full_path },
       query_graphql_field(:timelogs, timelog_params, timelog_nodes)
     )
   end