From 29c6918f3adc5995c0d1fb26bfb54bbeab1a8065 Mon Sep 17 00:00:00 2001
From: Missy Davies <ms.melissadavies@gmail.com>
Date: Mon, 24 Jul 2023 23:02:25 +0000
Subject: [PATCH] Add negative validation to estimate mutations
Fixed the issue and merge request update mutations to include a
validation to ensure time estimates are greater than or equal
to zero.
Changelog: fixed
---
.../mutations/validate_time_estimate.rb | 22 +++++++
app/graphql/mutations/issues/update.rb | 5 +-
.../mutations/merge_requests/update.rb | 7 +--
spec/graphql/mutations/issues/update_spec.rb | 13 +++-
.../mutations/merge_requests/update_spec.rb | 14 ++++-
.../gitlab/time_tracking_formatter_spec.rb | 2 +-
.../graphql/mutations/issues/update_spec.rb | 5 ++
.../merge_requests/set_time_estimate_spec.rb | 41 +++++++++++++
.../update_time_estimate_shared_examples.rb | 59 +++++++++++++++++++
9 files changed, 158 insertions(+), 10 deletions(-)
create mode 100644 app/graphql/mutations/concerns/mutations/validate_time_estimate.rb
create mode 100644 spec/requests/api/graphql/mutations/merge_requests/set_time_estimate_spec.rb
create mode 100644 spec/support/shared_examples/graphql/mutations/update_time_estimate_shared_examples.rb
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 0000000000000..82a56fd04f326
--- /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 2a863893cf1a9..35deb9e0af83d 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 da4db7342a391..470292df86c60 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 ac82037b7e2b9..622ccb86b2ed6 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 8a10f6cadd0e6..6ced71c5f4c30 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 4203a76cbfbf3..aa755d64a7a24 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 97ead687a82f8..ff100d9962871 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 0000000000000..6bc130a97cf9f
--- /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 0000000000000..d6d360bb413a9
--- /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
--
GitLab