Merge branch '196834-confidential-issue' into 'master'

Add GraphQL group timelogs time arguments

See merge request gitlab-org/gitlab!28560

app/models/timelog.rb

@@ -16,8 +16,8 @@ class Timelog < ApplicationRecord
     )
   end
 
-  scope :between_dates, -> (start_date, end_date) do
-    where('spent_at BETWEEN ? AND ?', start_date, end_date)
+  scope :between_times, -> (start_time, end_time) do
+    where('spent_at BETWEEN ? AND ?', start_time, end_time)
   end
 
   def issuable

doc/api/graphql/reference/gitlab_schema.graphql

@@ -4097,9 +4097,14 @@ type Group {
     before: String
 
     """
-    List time logs within a time range where the logged date is before end_date parameter.
+    List time logs within a date range where the logged date is equal to or before endDate
     """
-    endDate: Time!
+    endDate: Time
+
+    """
+    List time-logs within a time range where the logged time is equal to or before endTime
+    """
+    endTime: Time
 
     """
     Returns the first _n_ elements from the list.
@@ -4112,9 +4117,14 @@ type Group {
     last: Int
 
     """
-    List time logs within a time range where the logged date is after start_date parameter.
+    List time logs within a date range where the logged date is equal to or after startDate
+    """
+    startDate: Time
+
+    """
+    List time-logs within a time range where the logged time is equal to or after startTime
     """
-    startDate: Time!
+    startTime: Time
   ): TimelogConnection!
 
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -11530,29 +11530,41 @@
               "args": [
                 {
                   "name": "startDate",
-                  "description": "List time logs within a time range where the logged date is after start_date parameter.",
+                  "description": "List time logs within a date range where the logged date is equal to or after startDate",
                   "type": {
-                    "kind": "NON_NULL",
-                    "name": null,
-                    "ofType": {
                     "kind": "SCALAR",
                     "name": "Time",
                     "ofType": null
-                    }
                   },
                   "defaultValue": null
                 },
                 {
                   "name": "endDate",
-                  "description": "List time logs within a time range where the logged date is before end_date parameter.",
+                  "description": "List time logs within a date range where the logged date is equal to or before endDate",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Time",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "startTime",
+                  "description": "List time-logs within a time range where the logged time is equal to or after startTime",
+                  "type": {
+                    "kind": "SCALAR",
+                    "name": "Time",
+                    "ofType": null
+                  },
+                  "defaultValue": null
+                },
+                {
+                  "name": "endTime",
+                  "description": "List time-logs within a time range where the logged time is equal to or before endTime",
                   "type": {
-                    "kind": "NON_NULL",
-                    "name": null,
-                    "ofType": {
                     "kind": "SCALAR",
                     "name": "Time",
                     "ofType": null
-                    }
                   },
                   "defaultValue": null
                 },

ee/app/graphql/resolvers/timelog_resolver.rb

@@ -3,61 +3,93 @@
 module Resolvers
   class TimelogResolver < BaseResolver
     argument :start_date, Types::TimeType,
-              required: true,
-              description: 'List time logs within a time range where the logged date is after start_date parameter.'
+              required: false,
+              description: 'List time logs within a date range where the logged date is equal to or after startDate'
 
     argument :end_date, Types::TimeType,
-              required: true,
-              description: 'List time logs within a time range where the logged date is before end_date parameter.'
+              required: false,
+              description: 'List time logs within a date range where the logged date is equal to or before endDate'
+
+    argument :start_time, Types::TimeType,
+              required: false,
+              description: 'List time-logs within a time range where the logged time is equal to or after startTime'
+
+    argument :end_time, Types::TimeType,
+              required: false,
+              description: 'List time-logs within a time range where the logged time is equal to or before endTime'
 
     def resolve(**args)
-      validate_date_params!(args)
-      authorize_group_timelogs!
+      return Timelog.none unless timelogs_available_for_user?
+
+      validate_params_presence!(args)
+      transformed_args = transform_args(args)
+      validate_time_difference!(transformed_args)
 
-      find_timelogs(args)
+      find_timelogs(transformed_args)
     end
 
     private
 
     def find_timelogs(args)
-      group.timelogs(args[:start_date], args[:end_date])
+      group.timelogs(args[:start_time], args[:end_time])
     end
 
-    def validate_date_params!(args)
-      validate_dates_present!(args[:start_date], args[:end_date])
-      validate_dates_difference!(args[:start_date], args[:end_date])
-      validate_date_range!(args[:start_date], args[:end_date])
-    end
-
-    def valid_object?
-      group.present? &&
+    def timelogs_available_for_user?
       group&.feature_available?(:group_timelogs) &&
         group&.user_can_access_group_timelogs?(context[:current_user])
     end
 
-    def authorize_group_timelogs!
-      unless valid_object?
-        raise Gitlab::Graphql::Errors::ResourceNotAvailable,
-          "The resource is not available or you don't have permission to perform this action"
+    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
+
+      raise_argument_error(message) if message
     end
 
-    def validate_dates_present!(start_date, end_date)
-      return if start_date.present? && end_date.present?
+    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('Both start_date and end_date must be present.')
+      raise_argument_error(message) if message
     end
 
-    def validate_dates_difference!(start_date, end_date)
-      return if end_date > start_date
+    def transform_args(args)
+      return args if args.keys == [:start_time, :end_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
+      end
 
-      raise_argument_error('start_date must be earlier than end_date.')
+      time_args
     end
 
-    def validate_date_range!(start_date, end_date)
-      return if end_date - start_date <= 60.days
+    def time_params_count(args)
+      [:start_time, :end_time, :start_date, :end_date].count { |param| args.key?(param) }
+    end
 
-      raise_argument_error('The date range period cannot contain more than 60 days')
+    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
     end
 
     def raise_argument_error(message)

ee/app/models/concerns/has_timelogs_report.rb

@@ -3,8 +3,8 @@
 module HasTimelogsReport
   extend ActiveSupport::Concern
 
-  def timelogs(start_date, end_date)
-    @timelogs ||= timelogs_for(start_date, end_date)
+  def timelogs(start_time, end_time)
+    @timelogs ||= timelogs_for(start_time, end_time)
   end
 
   def user_can_access_group_timelogs?(current_user)
@@ -15,7 +15,7 @@ module HasTimelogsReport
 
   private
 
-  def timelogs_for(start_date, end_date)
-    Timelog.between_dates(start_date, end_date).for_issues_in_group(self)
+  def timelogs_for(start_time, end_time)
+    Timelog.between_times(start_time, end_time).for_issues_in_group(self)
   end
 end

ee/changelogs/unreleased/196834-confidential-issue.yml

+---
+title: Change GraphQL arguments in group.timelogs query to use startTime and endTime
+merge_request: 28560
+author:
+type: changed

ee/spec/graphql/ee/types/group_type_spec.rb

@@ -16,8 +16,8 @@ describe GitlabSchema.types['Group'] do
   describe 'timelogs field' do
     subject { described_class.fields['timelogs'] }
 
-    it 'finds timelogs between start date and end date' do
-      is_expected.to have_graphql_arguments(:start_date, :end_date, :after, :before, :first, :last)
+    it 'finds timelogs between start time and end time' do
+      is_expected.to have_graphql_arguments(:start_time, :end_time, :start_date, :end_date, :after, :before, :first, :last)
       is_expected.to have_graphql_resolver(Resolvers::TimelogResolver)
       is_expected.to have_non_null_graphql_type(Types::TimelogType.connection_type)
     end

ee/spec/graphql/resolvers/timelog_resolver_spec.rb

@@ -6,93 +6,153 @@ describe Resolvers::TimelogResolver do
   include GraphqlHelpers
 
   context "within a group" do
-    let(:current_user) { create(:user) }
-    let(:user)         { create(:user) }
+    let_it_be(:current_user) { create(:user) }
     let(:group)              { create(:group) }
     let(:project)            { create(:project, :public, group: group) }
 
     before do
-      group.add_users([current_user, user], :developer)
-      project.add_developer(user)
+      group.add_developer(current_user)
+      project.add_developer(current_user)
       stub_licensed_features(group_timelogs: true)
     end
 
     describe '#resolve' do
       let(:issue) { create(:issue, project: project) }
       let(:issue2) { create(:issue, project: project) }
-      let!(:timelog1)   { create(:timelog, issue: issue, user: user, spent_at: 5.days.ago) }
-      let!(:timelog2)   { create(:timelog, issue: issue2, user: user, spent_at: 10.days.ago) }
-      let(:start_date)  { 6.days.ago }
-      let(:end_date)    { 2.days.ago }
+      let(:args) { { start_time: 6.days.ago, end_time: 2.days.ago.noon } }
+      let!(:timelog1) { create(:timelog, issue: issue, spent_at: 2.days.ago.beginning_of_day) }
+      let!(:timelog2) { create(:timelog, issue: issue2, spent_at: 2.days.ago.end_of_day) }
+      let!(:timelog3) { create(:timelog, issue: issue2, spent_at: 10.days.ago) }
 
-      shared_examples 'validation fails with error' do
-        it 'raises error with correct message' do
-          expect { resolve_timelogs(start_date: start_date, end_date: end_date) }
-            .to raise_error(
-              error_type,
-              message
-            )
+      it 'finds all timelogs within given dates' do
+        timelogs = resolve_timelogs(args)
+
+        expect(timelogs).to contain_exactly(timelog1)
       end
+
+      it 'return nothing when user has insufficient permissions' do
+        group.add_guest(current_user)
+
+        expect(resolve_timelogs(args)).to be_empty
       end
 
-      it 'finds all timelogs within given dates' do
-        timelogs = resolve_timelogs(start_date: start_date, end_date: end_date)
+      it 'returns nothing when feature is disabled' do
+        stub_licensed_features(group_timelogs: false)
+
+        expect(resolve_timelogs(args)).to be_empty
+      end
+
+      context 'when start_time and end_date are present' do
+        let(:args) { { start_time: 6.days.ago, end_date: 2.days.ago } }
+
+        it 'finds timelogs until the end of day of end_date' do
+          timelogs = resolve_timelogs(args)
+
+          expect(timelogs).to contain_exactly(timelog1, timelog2)
+        end
+      end
+
+      context 'finds timelogs until the time specified on end_time' do
+        let(:args) { { start_date: 6.days.ago, end_time: 2.days.ago.noon } }
+
+        it 'finds all timelogs within start_date and end_time' do
+          timelogs = resolve_timelogs(args)
 
           expect(timelogs).to contain_exactly(timelog1)
         end
+      end
 
       context 'when arguments are invalid' do
-        let(:error_type) { Gitlab::Graphql::Errors::ArgumentError }
+        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 only start_time is present' do
+          let(:args) { { start_time: 6.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 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 only start_date is present' do
-          let(:end_date) { nil }
-          let(:message) { 'Both start_date and end_date must be present.' }
+          let(:args) { { start_date: 6.days.ago } }
 
-          it_behaves_like 'validation fails with error'
+          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 only end_date is present' do
-          let(:start_date) { nil }
-          let(:message) { 'Both start_date and end_date must be present.' }
+          let(:args) { { end_date: 2.days.ago } }
 
-          it_behaves_like 'validation fails with error'
+          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 is later than end_date' do
-          let(:start_date) { 3.days.ago }
-          let(:end_date) { 5.days.ago }
-          let(:message) { 'start_date must be earlier than end_date.' }
+        context 'when start_time and start_date are present' do
+          let(:args) { { start_time: 6.days.ago, start_date: 6.days.ago } }
 
-          it_behaves_like 'validation fails with error'
+          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 time range is more than 60 days' do
-          let(:start_date) { 3.months.ago }
-          let(:end_date) { 1.day.ago }
-          let(:message) { 'The date range period cannot contain more than 60 days' }
+        context 'when end_time and end_date are present' do
+          let(:args) { { end_time: 2.days.ago, end_date: 2.days.ago } }
 
-          it_behaves_like 'validation fails with error'
+          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 resource is not available' do
-        let(:error_type) { Gitlab::Graphql::Errors::ResourceNotAvailable }
-        let(:message) { "The resource is not available or you don't have permission to perform this action" }
+        context 'when three arguments are present' do
+          let(:args) { { start_date: 6.days.ago, end_date: 2.days.ago, end_time: 2.days.ago } }
 
-        context 'when feature is disabled' do
-          before do
-            stub_licensed_features(group_timelogs: false)
+          it 'returns correct error' do
+            expect {resolve_timelogs(args)}
+              .to raise_error(error_class, /Only Time or Date arguments must be present/)
           end
-
-          it_behaves_like 'validation fails with error'
         end
 
-        context "when user has insufficient permissions" do
-          before do
-            group.add_guest(current_user)
+        context 'when start argument is after end argument' do
+          let(:args) { { start_time: 2.days.ago, end_time: 6.days.ago } }
+
+          it 'returns correct error' do
+            expect {resolve_timelogs(args)}
+              .to raise_error(error_class, /Start argument must be before End argument/)
+          end
         end
 
-          it_behaves_like 'validation fails with error'
+        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

ee/spec/models/concerns/has_timelogs_report_spec.rb

@@ -11,23 +11,23 @@ describe HasTimelogsReport do
     let!(:timelog1) { create_timelog(15.days.ago) }
     let!(:timelog2) { create_timelog(10.days.ago) }
     let!(:timelog3) { create_timelog(5.days.ago) }
-    let(:start_date) { 20.days.ago }
-    let(:end_date) { 8.days.ago }
+    let(:start_time) { 20.days.ago }
+    let(:end_time) { 8.days.ago }
 
     before do
       group.add_developer(user)
     end
 
-    it 'returns collection of timelogs between given dates' do
-      expect(group.timelogs(start_date, end_date).to_a).to match_array([timelog1, timelog2])
+    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 dates are not present' do
+    it 'returns empty collection if times are not present' do
       expect(group.timelogs(nil, nil)).to be_empty
     end
 
-    it 'returns empty collection if date range is invalid' do
-      expect(group.timelogs(end_date, start_date)).to be_empty
+    it 'returns empty collection if time range is invalid' do
+      expect(group.timelogs(end_time, start_time)).to be_empty
     end
   end
 
@@ -54,7 +54,7 @@ describe HasTimelogsReport do
     end
   end
 
-  def create_timelog(date)
-    create(:timelog, issue: issue, user: user, spent_at: date)
+  def create_timelog(time)
+    create(:timelog, issue: issue, user: user, spent_at: time)
   end
 end

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

@@ -6,44 +6,16 @@ describe 'Timelogs through GroupQuery' do
   include GraphqlHelpers
 
   describe 'Get list of timelogs from a group issues' do
-    let(:user)          { create(:user) }
-    let(:group)         { create(:group) }
-    let(:project)       { create(:project, :public, group: group) }
-    let(:milestone)     { create(:milestone, group: group) }
-    let(:epic)          { create(:epic, group: group) }
-    let(:issue)         { create(:issue, project: project, milestone: milestone, epic: epic) }
-    let!(:timelog1)     { create(:timelog, issue: issue, user: user, spent_at: 10.days.ago) }
-    let!(:timelog2)     { create(:timelog, spent_at: 15.days.ago) }
+    let_it_be(:user)      { create(:user) }
+    let_it_be(:group)     { create(:group) }
+    let_it_be(:project)   { create(:project, :public, group: group) }
+    let_it_be(:milestone) { create(:milestone, group: group) }
+    let_it_be(:epic)      { create(:epic, group: group) }
+    let_it_be(:issue)     { create(:issue, project: project, milestone: milestone, epic: epic) }
+    let_it_be(:timelog1)  { create(:timelog, issue: issue, user: user, spent_at: '2019-08-13 14:00:00') }
+    let_it_be(:timelog2)  { create(:timelog, issue: issue, user: user, spent_at: '2019-08-10 08:00:00') }
+    let_it_be(:params)    { { startTime: '2019-08-10 12:00:00', endTime: '2019-08-21 12:00:00' } }
     let(:timelogs_data)   { graphql_data['group']['timelogs']['nodes'] }
-    let(:query) do
-      timelog_nodes = <<~NODE
-      nodes {
-        date
-        spentAt
-        timeSpent
-        user {
-          username
-        }
-        issue {
-          title
-          milestone {
-            title
-          }
-          epic {
-            title
-          }
-        }
-      }
-      NODE
-
-      graphql_query_for("group", { "fullPath" => group.full_path },
-        ['groupTimelogsEnabled', query_graphql_field(
-          "timelogs",
-          { startDate: "#{13.days.ago.to_date}", endDate: "#{2.days.ago.to_date}" },
-          timelog_nodes
-        )]
-      )
-    end
 
     before do
       group.add_developer(user)
@@ -81,13 +53,20 @@ describe 'Timelogs through GroupQuery' do
         expect(milestone_title).to eq([milestone.title])
         expect(epic_title).to eq([epic.title])
       end
-    end
 
-    context 'when requests has errors' do
-      let(:error_message) do
-        "The resource is not available or you don't have permission to perform this action"
+      context 'when arguments with no time are present' do
+        let!(:timelog3) { create(:timelog, issue: issue, user: user, spent_at: '2019-08-10 15:00:00') }
+        let!(:timelog4) { create(:timelog, issue: issue, user: user, spent_at: '2019-08-21 15:00:00') }
+        let(:params) { { startDate: '2019-08-10', endDate: '2019-08-21' }}
+
+        it 'sets times as start of day and end of day' do
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(timelog_array.size).to eq 2
+        end
+      end
     end
 
+    context 'when requests has errors' do
       context 'when group_timelogs feature is disabled' do
         before do
           stub_licensed_features(group_timelogs: false)
@@ -97,8 +76,9 @@ describe 'Timelogs through GroupQuery' do
           post_graphql(query, current_user: user)
 
           expect(response).to have_gitlab_http_status(:success)
-          expect(graphql_errors).to include(a_hash_including('message' => error_message))
-          expect(graphql_data['group']).to be_nil
+          expect(graphql_errors).to be_nil
+          expect(timelogs_data).to be_empty
+          expect(graphql_data['group']['groupTimelogsEnabled']).to be_falsey
         end
       end
 
@@ -124,8 +104,9 @@ describe 'Timelogs through GroupQuery' do
           post_graphql(query, current_user: guest)
 
           expect(response).to have_gitlab_http_status(:success)
-          expect(graphql_errors).to include(a_hash_including('message' => error_message))
-          expect(graphql_data['group']).to be_nil
+          expect(graphql_errors).to be_nil
+          expect(timelogs_data).to be_empty
+          expect(graphql_data['group']['groupTimelogsEnabled']).to be_truthy
         end
       end
     end
@@ -136,4 +117,34 @@ describe 'Timelogs through GroupQuery' do
       extract_attribute ? item[extract_attribute] : item
     end
   end
+
+  def query(timelog_params = params)
+    timelog_nodes = <<~NODE
+      nodes {
+        date
+        spentAt
+        timeSpent
+        user {
+          username
+        }
+        issue {
+          title
+          milestone {
+            title
+          }
+          epic {
+            title
+          }
+        }
+      }
+    NODE
+
+    graphql_query_for("group", { "fullPath" => group.full_path },
+      ['groupTimelogsEnabled', query_graphql_field(
+        "timelogs",
+        timelog_params,
+        timelog_nodes
+      )]
+    )
+  end
 end

spec/models/timelog_spec.rb

@@ -56,12 +56,12 @@ RSpec.describe Timelog do
       end
     end
 
-    describe 'between_dates' do
-      it 'returns collection of timelogs within given dates' do
+    describe 'between_times' do
+      it 'returns collection of timelogs within given times' do
         create(:timelog, spent_at: 65.days.ago)
         timelog1 = create(:timelog, spent_at: 15.days.ago)
         timelog2 = create(:timelog, spent_at: 5.days.ago)
-        timelogs = described_class.between_dates(20.days.ago, 1.day.ago)
+        timelogs = described_class.between_times(20.days.ago, 1.day.ago)
 
         expect(timelogs).to contain_exactly(timelog1, timelog2)
       end