Merge branch 'md-time-tracking-graphql-mutations-validation' into 'master'

Add negative validation to time estimate update mutations

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126619



Merged-by: charlie ablett <cablett@gitlab.com>
Approved-by: Ross Byrne <robyrne@gitlab.com>
Approved-by: Marcin Sedlak-Jakubowski <msedlakjakubowski@gitlab.com>
Approved-by: Marco Zille <marco.zille@gmail.com>
Approved-by: charlie ablett <cablett@gitlab.com>
Reviewed-by: Marco Zille <marco.zille@gmail.com>
Reviewed-by: charlie ablett <cablett@gitlab.com>
Co-authored-by: Missy Davies <ms.melissadavies@gmail.com>

app/graphql/mutations/concerns/mutations/validate_time_estimate.rb

+# frozen_string_literal: true
+
+module Mutations
+  module ValidateTimeEstimate
+    private
+
+    def validate_time_estimate(time_estimate)
+      return unless time_estimate
+
+      parsed_time_estimate = Gitlab::TimeTrackingFormatter.parse(time_estimate, keep_zero: true)
+
+      if parsed_time_estimate.nil?
+        raise Gitlab::Graphql::Errors::ArgumentError,
+          'timeEstimate must be formatted correctly, for example `1h 30m`'
+      elsif parsed_time_estimate < 0
+        raise Gitlab::Graphql::Errors::ArgumentError,
+          'timeEstimate must be greater than or equal to zero. ' \
+          'Remember that every new timeEstimate overwrites the previous value.'
+      end
+    end
+  end
+end

app/graphql/mutations/issues/update.rb

@@ -6,6 +6,7 @@ class Update < Base
       graphql_name 'UpdateIssue'
 
       include CommonMutationArguments
+      include ValidateTimeEstimate
 
       argument :title, GraphQL::Types::String,
                required: false,
@@ -54,9 +55,7 @@ def ready?(label_ids: [], add_label_ids: [], remove_label_ids: [], time_estimate
           raise Gitlab::Graphql::Errors::ArgumentError, 'labelIds is mutually exclusive with any of addLabelIds or removeLabelIds'
         end
 
-        if !time_estimate.nil? && Gitlab::TimeTrackingFormatter.parse(time_estimate, keep_zero: true).nil?
-          raise Gitlab::Graphql::Errors::ArgumentError, 'timeEstimate must be formatted correctly, for example `1h 30m`'
-        end
+        validate_time_estimate(time_estimate)
 
         super
       end

app/graphql/mutations/merge_requests/update.rb

@@ -5,6 +5,8 @@ module MergeRequests
     class Update < Base
       graphql_name 'MergeRequestUpdate'
 
+      include ValidateTimeEstimate
+
       description 'Update attributes of a merge request'
 
       argument :title, GraphQL::Types::String,
@@ -45,10 +47,7 @@ def resolve(project_path:, iid:, **args)
       end
 
       def ready?(time_estimate: nil, **args)
-        if !time_estimate.nil? && Gitlab::TimeTrackingFormatter.parse(time_estimate, keep_zero: true).nil?
-          raise Gitlab::Graphql::Errors::ArgumentError,
-                'timeEstimate must be formatted correctly, for example `1h 30m`'
-        end
+        validate_time_estimate(time_estimate)
 
         super
       end

spec/graphql/mutations/issues/update_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Mutations::Issues::Update do
+RSpec.describe Mutations::Issues::Update, feature_category: :team_planning do
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
   let_it_be(:project_label) { create(:label, project: project) }
@@ -177,6 +177,17 @@
           end
         end
 
+        context 'when timeEstimate is negative' do
+          let(:time_estimate) { '-1h' }
+
+          it 'raises an argument error and changes are not applied' do
+            expect { mutation.ready?(time_estimate: time_estimate) }
+              .to raise_error(Gitlab::Graphql::Errors::ArgumentError,
+                'timeEstimate must be greater than or equal to zero. Remember that every new timeEstimate overwrites the previous value.')
+            expect { subject }.not_to change { issue.time_estimate }
+          end
+        end
+
         context 'when timeEstimate is 0' do
           let(:time_estimate) { '0' }
 

spec/graphql/mutations/merge_requests/update_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Mutations::MergeRequests::Update do
+RSpec.describe Mutations::MergeRequests::Update, feature_category: :team_planning do
   let(:merge_request) { create(:merge_request) }
   let(:user) { create(:user) }
 
@@ -59,6 +59,18 @@
           end
         end
 
+        context 'when timeEstimate is negative' do
+          let(:time_estimate) { '-1h' }
+
+          it 'raises an argument error and changes are not applied' do
+            expect { mutation.ready?(time_estimate: time_estimate) }
+            .to raise_error(Gitlab::Graphql::Errors::ArgumentError,
+              'timeEstimate must be greater than or equal to zero. ' \
+              'Remember that every new timeEstimate overwrites the previous value.')
+            expect { subject }.not_to change { merge_request.time_estimate }
+          end
+        end
+
         context 'when timeEstimate is 0' do
           let(:time_estimate) { '0' }
 

spec/lib/gitlab/time_tracking_formatter_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::TimeTrackingFormatter do
+RSpec.describe Gitlab::TimeTrackingFormatter, feature_category: :team_planning do
   describe '#parse' do
     let(:keep_zero) { false }
 

spec/requests/api/graphql/mutations/issues/update_spec.rb

@@ -147,5 +147,10 @@
         end
       end
     end
+
+    it_behaves_like 'updating time estimate' do
+      let(:resource) { issue }
+      let(:mutation_name) { 'updateIssue' }
+    end
   end
 end

spec/requests/api/graphql/mutations/merge_requests/set_time_estimate_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Setting time estimate of a merge request', feature_category: :code_review_workflow do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project) }
+
+  let(:input) do
+    {
+      iid: merge_request.iid.to_s
+    }
+  end
+
+  let(:extra_params) { { project_path: project.full_path } }
+  let(:input_params) { input.merge(extra_params) }
+  let(:mutation) { graphql_mutation(:merge_request_update, input_params, nil, ['productAnalyticsState']) }
+  let(:mutation_response) { graphql_mutation_response(:merge_request_update) }
+
+  context 'when the user is not allowed to update a merge request' do
+    before_all do
+      project.add_reporter(current_user)
+    end
+
+    it_behaves_like 'a mutation that returns a top-level access error'
+  end
+
+  context 'when updating a time estimate' do
+    before_all do
+      project.add_developer(current_user)
+    end
+
+    it_behaves_like 'updating time estimate' do
+      let(:resource) { merge_request }
+      let(:mutation_name) { 'mergeRequestUpdate' }
+    end
+  end
+end

spec/support/shared_examples/graphql/mutations/update_time_estimate_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'updating time estimate' do
+  context 'when setting time estimate', :aggregate_failures do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:input_params) { input.merge(extra_params).merge({ timeEstimate: time_estimate }) }
+
+    context 'when time estimate is not a valid numerical value' do
+      let(:time_estimate) { '-3.5d' }
+
+      it 'does not update' do
+        expect { post_graphql_mutation(mutation, current_user: current_user) }.not_to change { resource.time_estimate }
+      end
+
+      it 'returns error' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(graphql_errors).to include(a_hash_including('message' => /must be greater than or equal to zero/))
+      end
+    end
+
+    context 'when time estimate is not a number' do
+      let(:time_estimate) { 'nonsense' }
+
+      it 'does not update' do
+        expect { post_graphql_mutation(mutation, current_user: current_user) }.not_to change { resource.time_estimate }
+      end
+
+      it 'returns error' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(graphql_errors).to include(a_hash_including('message' => /must be formatted correctly/))
+      end
+    end
+
+    context 'when time estimate is valid' do
+      let(:time_estimate) { "1h" }
+
+      before do
+        post_graphql_mutation(mutation, current_user: current_user)
+      end
+
+      it_behaves_like 'a working GraphQL mutation'
+
+      where(:time_estimate, :value) do
+        '1h'              | 3600
+        '0h'              | 0
+        '-0h'             | 0
+      end
+
+      with_them do
+        specify do
+          expect(graphql_data_at(mutation_name, resource.class.to_s.underscore, 'timeEstimate')).to eq(value)
+        end
+      end
+    end
+  end
+end