From 519bac658ae7e92e8454d7ed6445cf286c12fcb6 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 28 Feb 2017 14:23:58 +0530 Subject: [PATCH] Fix time tracking endpoints for API v4 - Use issue/merge_request IID instead of ID - Duplicate the original `TimeTrackingEndpoints` concern (+ specs) for V3, since this is a breaking change. --- lib/api/time_tracking_endpoints.rb | 6 +- lib/api/v3/time_tracking_endpoints.rb | 116 ++++++++++++++++ spec/requests/api/v3/issues_spec.rb | 2 +- spec/requests/api/v3/merge_requests_spec.rb | 2 +- .../api/v3/time_tracking_shared_examples.rb | 128 ++++++++++++++++++ 5 files changed, 249 insertions(+), 5 deletions(-) create mode 100644 lib/api/v3/time_tracking_endpoints.rb create mode 100644 spec/support/api/v3/time_tracking_shared_examples.rb diff --git a/lib/api/time_tracking_endpoints.rb b/lib/api/time_tracking_endpoints.rb index 85b5f7d98b8..05b4b490e27 100644 --- a/lib/api/time_tracking_endpoints.rb +++ b/lib/api/time_tracking_endpoints.rb @@ -5,11 +5,11 @@ module API included do helpers do def issuable_name - declared_params.has_key?(:issue_id) ? 'issue' : 'merge_request' + declared_params.has_key?(:issue_iid) ? 'issue' : 'merge_request' end def issuable_key - "#{issuable_name}_id".to_sym + "#{issuable_name}_iid".to_sym end def update_issuable_key @@ -50,7 +50,7 @@ module API issuable_name = name.end_with?('Issues') ? 'issue' : 'merge_request' issuable_collection_name = issuable_name.pluralize - issuable_key = "#{issuable_name}_id".to_sym + issuable_key = "#{issuable_name}_iid".to_sym desc "Set a time estimate for a project #{issuable_name}" params do diff --git a/lib/api/v3/time_tracking_endpoints.rb b/lib/api/v3/time_tracking_endpoints.rb new file mode 100644 index 00000000000..81ae4e8137d --- /dev/null +++ b/lib/api/v3/time_tracking_endpoints.rb @@ -0,0 +1,116 @@ +module API + module V3 + module TimeTrackingEndpoints + extend ActiveSupport::Concern + + included do + helpers do + def issuable_name + declared_params.has_key?(:issue_id) ? 'issue' : 'merge_request' + end + + def issuable_key + "#{issuable_name}_id".to_sym + end + + def update_issuable_key + "update_#{issuable_name}".to_sym + end + + def read_issuable_key + "read_#{issuable_name}".to_sym + end + + def load_issuable + @issuable ||= begin + case issuable_name + when 'issue' + find_project_issue(params.delete(issuable_key)) + when 'merge_request' + find_project_merge_request(params.delete(issuable_key)) + end + end + end + + def update_issuable(attrs) + custom_params = declared_params(include_missing: false) + custom_params.merge!(attrs) + + issuable = update_service.new(user_project, current_user, custom_params).execute(load_issuable) + if issuable.valid? + present issuable, with: ::API::Entities::IssuableTimeStats + else + render_validation_error!(issuable) + end + end + + def update_service + issuable_name == 'issue' ? ::Issues::UpdateService : ::MergeRequests::UpdateService + end + end + + issuable_name = name.end_with?('Issues') ? 'issue' : 'merge_request' + issuable_collection_name = issuable_name.pluralize + issuable_key = "#{issuable_name}_id".to_sym + + desc "Set a time estimate for a project #{issuable_name}" + params do + requires issuable_key, type: Integer, desc: "The ID of a project #{issuable_name}" + requires :duration, type: String, desc: 'The duration to be parsed' + end + post ":id/#{issuable_collection_name}/:#{issuable_key}/time_estimate" do + authorize! update_issuable_key, load_issuable + + status :ok + update_issuable(time_estimate: Gitlab::TimeTrackingFormatter.parse(params.delete(:duration))) + end + + desc "Reset the time estimate for a project #{issuable_name}" + params do + requires issuable_key, type: Integer, desc: "The ID of a project #{issuable_name}" + end + post ":id/#{issuable_collection_name}/:#{issuable_key}/reset_time_estimate" do + authorize! update_issuable_key, load_issuable + + status :ok + update_issuable(time_estimate: 0) + end + + desc "Add spent time for a project #{issuable_name}" + params do + requires issuable_key, type: Integer, desc: "The ID of a project #{issuable_name}" + requires :duration, type: String, desc: 'The duration to be parsed' + end + post ":id/#{issuable_collection_name}/:#{issuable_key}/add_spent_time" do + authorize! update_issuable_key, load_issuable + + update_issuable(spend_time: { + duration: Gitlab::TimeTrackingFormatter.parse(params.delete(:duration)), + user: current_user + }) + end + + desc "Reset spent time for a project #{issuable_name}" + params do + requires issuable_key, type: Integer, desc: "The ID of a project #{issuable_name}" + end + post ":id/#{issuable_collection_name}/:#{issuable_key}/reset_spent_time" do + authorize! update_issuable_key, load_issuable + + status :ok + update_issuable(spend_time: { duration: :reset, user: current_user }) + end + + desc "Show time stats for a project #{issuable_name}" + params do + requires issuable_key, type: Integer, desc: "The ID of a project #{issuable_name}" + end + get ":id/#{issuable_collection_name}/:#{issuable_key}/time_stats" do + authorize! read_issuable_key, load_issuable + + present load_issuable, with: ::API::Entities::IssuableTimeStats + end + end + end + end +end diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb index 2a8105d5a2b..1941ca0d7d8 100644 --- a/spec/requests/api/v3/issues_spec.rb +++ b/spec/requests/api/v3/issues_spec.rb @@ -1288,6 +1288,6 @@ describe API::V3::Issues, api: true do describe 'time tracking endpoints' do let(:issuable) { issue } - include_examples 'time tracking endpoints', 'issue' + include_examples 'V3 time tracking endpoints', 'issue' end end diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb index b7ed643bc21..d73e9635c9b 100644 --- a/spec/requests/api/v3/merge_requests_spec.rb +++ b/spec/requests/api/v3/merge_requests_spec.rb @@ -712,7 +712,7 @@ describe API::MergeRequests, api: true do describe 'Time tracking' do let(:issuable) { merge_request } - include_examples 'time tracking endpoints', 'merge_request' + include_examples 'V3 time tracking endpoints', 'merge_request' end def mr_with_later_created_and_updated_at_time diff --git a/spec/support/api/v3/time_tracking_shared_examples.rb b/spec/support/api/v3/time_tracking_shared_examples.rb new file mode 100644 index 00000000000..f982b10d999 --- /dev/null +++ b/spec/support/api/v3/time_tracking_shared_examples.rb @@ -0,0 +1,128 @@ +shared_examples 'V3 time tracking endpoints' do |issuable_name| + issuable_collection_name = issuable_name.pluralize + + describe "POST /projects/:id/#{issuable_collection_name}/:#{issuable_name}_id/time_estimate" do + context 'with an unauthorized user' do + subject { post(v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/time_estimate", non_member), duration: '1w') } + + it_behaves_like 'an unauthorized API user' + end + + it "sets the time estimate for #{issuable_name}" do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/time_estimate", user), duration: '1w' + + expect(response).to have_http_status(200) + expect(json_response['human_time_estimate']).to eq('1w') + end + + describe 'updating the current estimate' do + before do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/time_estimate", user), duration: '1w' + end + + context 'when duration has a bad format' do + it 'does not modify the original estimate' do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/time_estimate", user), duration: 'foo' + + expect(response).to have_http_status(400) + expect(issuable.reload.human_time_estimate).to eq('1w') + end + end + + context 'with a valid duration' do + it 'updates the estimate' do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/time_estimate", user), duration: '3w1h' + + expect(response).to have_http_status(200) + expect(issuable.reload.human_time_estimate).to eq('3w 1h') + end + end + end + end + + describe "POST /projects/:id/#{issuable_collection_name}/:#{issuable_name}_id/reset_time_estimate" do + context 'with an unauthorized user' do + subject { post(v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/reset_time_estimate", non_member)) } + + it_behaves_like 'an unauthorized API user' + end + + it "resets the time estimate for #{issuable_name}" do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/reset_time_estimate", user) + + expect(response).to have_http_status(200) + expect(json_response['time_estimate']).to eq(0) + end + end + + describe "POST /projects/:id/#{issuable_collection_name}/:#{issuable_name}_id/add_spent_time" do + context 'with an unauthorized user' do + subject do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/add_spent_time", non_member), + duration: '2h' + end + + it_behaves_like 'an unauthorized API user' + end + + it "add spent time for #{issuable_name}" do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/add_spent_time", user), + duration: '2h' + + expect(response).to have_http_status(201) + expect(json_response['human_total_time_spent']).to eq('2h') + end + + context 'when subtracting time' do + it 'subtracts time of the total spent time' do + issuable.update_attributes!(spend_time: { duration: 7200, user: user }) + + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/add_spent_time", user), + duration: '-1h' + + expect(response).to have_http_status(201) + expect(json_response['total_time_spent']).to eq(3600) + end + end + + context 'when time to subtract is greater than the total spent time' do + it 'does not modify the total time spent' do + issuable.update_attributes!(spend_time: { duration: 7200, user: user }) + + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/add_spent_time", user), + duration: '-1w' + + expect(response).to have_http_status(400) + expect(json_response['message']['time_spent'].first).to match(/exceeds the total time spent/) + end + end + end + + describe "POST /projects/:id/#{issuable_collection_name}/:#{issuable_name}_id/reset_spent_time" do + context 'with an unauthorized user' do + subject { post(v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/reset_spent_time", non_member)) } + + it_behaves_like 'an unauthorized API user' + end + + it "resets spent time for #{issuable_name}" do + post v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/reset_spent_time", user) + + expect(response).to have_http_status(200) + expect(json_response['total_time_spent']).to eq(0) + end + end + + describe "GET /projects/:id/#{issuable_collection_name}/:#{issuable_name}_id/time_stats" do + it "returns the time stats for #{issuable_name}" do + issuable.update_attributes!(spend_time: { duration: 1800, user: user }, + time_estimate: 3600) + + get v3_api("/projects/#{project.id}/#{issuable_collection_name}/#{issuable.id}/time_stats", user) + + expect(response).to have_http_status(200) + expect(json_response['total_time_spent']).to eq(1800) + expect(json_response['time_estimate']).to eq(3600) + end + end +end