Merge branch 'md-rest-api-time-tracking-endpoint-validation' into 'master'

Add non negative validation to update time estimate rest api endpoint

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



Merged-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Marco Zille <marco.zille@gmail.com>
Approved-by: Gregorius Marco <gmarco@gitlab.com>
Approved-by: Heinrich Lee Yu <heinrich@gitlab.com>
Reviewed-by: Marco Zille <marco.zille@gmail.com>
Reviewed-by: Gregorius Marco <gmarco@gitlab.com>
Co-authored-by: Missy Davies <ms.melissadavies@gmail.com>

lib/api/time_tracking_endpoints.rb

@@ -55,10 +55,11 @@ def update_service
       issuable_key             = "#{issuable_name}_iid".to_sym
 
       desc "Set a time estimate for a #{issuable_name}" do
-        detail " Sets an estimated time of work for this #{issuable_name}."
+        detail "Sets an estimated time of work for this #{issuable_name}."
         success Entities::IssuableTimeStats
         failure [
           { code: 401, message: 'Unauthorized' },
+          { code: 400, message: 'Bad request' },
           { code: 404, message: 'Not found' }
         ]
         tags [issuable_collection_name]
@@ -70,8 +71,14 @@ def update_service
       post ":id/#{issuable_collection_name}/:#{issuable_key}/time_estimate" do
         authorize! admin_issuable_key, load_issuable
 
-        status :ok
-        update_issuable(time_estimate: Gitlab::TimeTrackingFormatter.parse(params.delete(:duration)))
+        time_estimate = Gitlab::TimeTrackingFormatter.parse(params.delete(:duration), keep_zero: true)
+
+        if time_estimate && time_estimate >= 0
+          status :ok
+          update_issuable(time_estimate: time_estimate)
+        else
+          bad_request!(reason: 'Time estimate must have a valid format and be greater than or equal to zero.')
+        end
       end
 
       desc "Reset the time estimate for a project #{issuable_name}" do

spec/support/shared_examples/requests/api/time_tracking_shared_examples.rb

@@ -20,40 +20,49 @@
   issuable_collection_name = issuable_name.pluralize
 
   describe "POST /projects/:id/#{issuable_collection_name}/:#{issuable_name}_id/time_estimate" do
+    subject(:set_time_estimate) do
+      post(api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", user), params: { duration: duration })
+    end
+
+    let(:duration) { '2h' }
+
     context 'with an unauthorized user' do
-      subject { post(api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", non_member), params: { duration: '1w' }) }
+      let(:user) { non_member }
 
       it_behaves_like 'an unauthorized API user'
       it_behaves_like 'API user with insufficient permissions'
     end
 
-    it "sets the time estimate for #{issuable_name}" do
-      post api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", user), params: { duration: '1w' }
+    context 'with an authorized user' do
+      it "sets the time estimate for #{issuable_name}" do
+        set_time_estimate
 
-      expect(response).to have_gitlab_http_status(:ok)
-      expect(json_response['human_time_estimate']).to eq('1w')
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['time_estimate']).to eq(7200)
+      end
     end
 
     describe 'updating the current estimate' do
       before do
-        post api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", user), params: { duration: '1w' }
+        post(api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", user), params: { duration: '2h' })
       end
 
-      context 'when duration has a bad format' do
-        it 'does not modify the original estimate' do
-          post api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", user), params: { duration: 'foo' }
+      using RSpec::Parameterized::TableSyntax
 
-          expect(response).to have_gitlab_http_status(:bad_request)
-          expect(issuable.reload.human_time_estimate).to eq('1w')
-        end
+      where(:updated_duration, :expected_http_status, :expected_time_estimate) do
+        'foo' | :bad_request | 7200
+        '-1'  | :bad_request | 7200
+        '1h'  | :ok          | 3600
+        '0'   | :ok          | 0
       end
 
-      context 'with a valid duration' do
-        it 'updates the estimate' do
-          post api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.iid}/time_estimate", user), params: { duration: '3w1h' }
+      with_them do
+        let(:duration) { updated_duration }
+        it 'returns expected HTTP status and time estimate' do
+          set_time_estimate
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(issuable.reload.human_time_estimate).to eq('3w 1h')
+          expect(response).to have_gitlab_http_status(expected_http_status)
+          expect(issuable.reload.time_estimate).to eq(expected_time_estimate)
         end
       end
     end