diff --git a/app/graphql/mutations/concerns/mutations/validate_time_estimate.rb b/app/graphql/mutations/concerns/mutations/validate_time_estimate.rb
new file mode 100644
index 0000000000000000000000000000000000000000..82a56fd04f326f359106984a0b9dd1069ef2eea7
--- /dev/null
+++ b/app/graphql/mutations/concerns/mutations/validate_time_estimate.rb
@@ -0,0 +1,22 @@
+# 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
diff --git a/app/graphql/mutations/issues/update.rb b/app/graphql/mutations/issues/update.rb
index 2a863893cf1a9d161d0db600fd0353cee1cef1b2..35deb9e0af83d2ee51aad6ae0f9b473e608a10b3 100644
--- a/app/graphql/mutations/issues/update.rb
+++ b/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
diff --git a/app/graphql/mutations/merge_requests/update.rb b/app/graphql/mutations/merge_requests/update.rb
index da4db7342a39171f8b949302ce3deec054c485e2..470292df86c60f9dd987b7aa8c44d93be5a3a654 100644
--- a/app/graphql/mutations/merge_requests/update.rb
+++ b/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
diff --git a/spec/graphql/mutations/issues/update_spec.rb b/spec/graphql/mutations/issues/update_spec.rb
index ac82037b7e2b97eb82e73f1484bc3e50fda2105d..622ccb86b2ed66dcf64d2cc0099c459f4c233317 100644
--- a/spec/graphql/mutations/issues/update_spec.rb
+++ b/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' }
diff --git a/spec/graphql/mutations/merge_requests/update_spec.rb b/spec/graphql/mutations/merge_requests/update_spec.rb
index 8a10f6cadd0e6a0d11ed40f2d43d4d28dcbdfcbd..6ced71c5f4c30e8f9fd316968e433a271c45e78b 100644
--- a/spec/graphql/mutations/merge_requests/update_spec.rb
+++ b/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' }
diff --git a/spec/lib/gitlab/time_tracking_formatter_spec.rb b/spec/lib/gitlab/time_tracking_formatter_spec.rb
index 4203a76cbfbf35aa91a6770dc71a532d262fe159..aa755d64a7a24480d7d13a92bc2df8a4acab72b7 100644
--- a/spec/lib/gitlab/time_tracking_formatter_spec.rb
+++ b/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 }
diff --git a/spec/requests/api/graphql/mutations/issues/update_spec.rb b/spec/requests/api/graphql/mutations/issues/update_spec.rb
index 97ead687a82f82ccda5e085b754d39c4569a8477..ff100d99628711529fce4b8dcdbc2fcceb4f8b94 100644
--- a/spec/requests/api/graphql/mutations/issues/update_spec.rb
+++ b/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
diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_time_estimate_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_time_estimate_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..6bc130a97cf9fe66c8062320a84b6f625c7089e5
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/merge_requests/set_time_estimate_spec.rb
@@ -0,0 +1,41 @@
+# 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
diff --git a/spec/support/shared_examples/graphql/mutations/update_time_estimate_shared_examples.rb b/spec/support/shared_examples/graphql/mutations/update_time_estimate_shared_examples.rb
new file mode 100644
index 0000000000000000000000000000000000000000..d6d360bb413a94eeedf3240e0bb41520a8af0448
--- /dev/null
+++ b/spec/support/shared_examples/graphql/mutations/update_time_estimate_shared_examples.rb
@@ -0,0 +1,59 @@
+# 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