From 8d18e557f9ce1c7bcf99ea15f2433b2a3f1b8e73 Mon Sep 17 00:00:00 2001 From: Robert May Date: Tue, 4 Aug 2020 13:50:33 +0100 Subject: [PATCH 1/4] Add a basic placeholder field Adds a basic `commented_by` field to the merge request approvals data. This will be populated in a later iteration. --- lib/api/entities/merge_request_approvals.rb | 4 ++++ spec/lib/api/entities/merge_request_approvals_spec.rb | 6 ++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/api/entities/merge_request_approvals.rb b/lib/api/entities/merge_request_approvals.rb index 0c464844ae7ebf..74d909e0b1126a 100644 --- a/lib/api/entities/merge_request_approvals.rb +++ b/lib/api/entities/merge_request_approvals.rb @@ -18,6 +18,10 @@ class MergeRequestApprovals < Grape::Entity expose :approved_by, using: ::API::Entities::Approvals do |merge_request| merge_request.approvals end + + expose :commented_by, using: ::API::Entities::Approvals do |merge_request| + [] + end end end end diff --git a/spec/lib/api/entities/merge_request_approvals_spec.rb b/spec/lib/api/entities/merge_request_approvals_spec.rb index cbbb037100a110..e13271ae0e35ae 100644 --- a/spec/lib/api/entities/merge_request_approvals_spec.rb +++ b/spec/lib/api/entities/merge_request_approvals_spec.rb @@ -21,7 +21,8 @@ approved: true, approved_by: [{ user: API::Entities::UserBasic.new(user).as_json - }] + }], + commented_by: [] }) end @@ -30,7 +31,8 @@ user_has_approved: false, user_can_approve: true, approved: false, - approved_by: [] + approved_by: [], + commented_by: [] }) end end -- GitLab From 49666dba841a07d34b28c3030037614fb267641c Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 6 Aug 2020 15:50:38 +0100 Subject: [PATCH 2/4] Actually add commented_by in right place --- ee/lib/ee/api/entities/merge_request_approval_state_rule.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ee/lib/ee/api/entities/merge_request_approval_state_rule.rb b/ee/lib/ee/api/entities/merge_request_approval_state_rule.rb index 053a5ca3688d18..7dde02d2bfaa1e 100644 --- a/ee/lib/ee/api/entities/merge_request_approval_state_rule.rb +++ b/ee/lib/ee/api/entities/merge_request_approval_state_rule.rb @@ -7,6 +7,8 @@ class MergeRequestApprovalStateRule < MergeRequestApprovalRule expose :code_owner expose :approved_approvers, as: :approved_by, using: ::API::Entities::UserBasic expose :approved?, as: :approved + + expose :approved_approvers, as: :commented_by, using: ::API::Entities::UserBasic end end end -- GitLab From 6821f9420d0313d8308042f334d3c2f93365f9a1 Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 6 Aug 2020 17:08:24 +0100 Subject: [PATCH 3/4] Add basic spec to cover the endpoint --- spec/requests/api/merge_request_approvals_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb index fad5c3fb60ec30..bc9b6c2f5bed66 100644 --- a/spec/requests/api/merge_request_approvals_spec.rb +++ b/spec/requests/api/merge_request_approvals_spec.rb @@ -23,6 +23,19 @@ end end + describe "GET :id/merge_requests/:merge_request_iid/approval_settings" do + it "retrieves the approval settings" do + project.add_developer(approver) + project.add_developer(create(:user)) + + create(:approval, user: approver, merge_request: merge_request) + + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) + + expect(response).to have_gitlab_http_status(:ok) + end + end + describe 'POST :id/merge_requests/:merge_request_iid/approve' do context 'as a valid approver' do let_it_be(:approver) { create(:user) } -- GitLab From 1468dff10750ac03d458448c95a661ef8894f4e3 Mon Sep 17 00:00:00 2001 From: Robert May Date: Fri, 7 Aug 2020 12:54:22 +0100 Subject: [PATCH 4/4] Adjust specs to cover new location --- .../requests/api/merge_request_approvals_spec.rb | 1 + lib/api/entities/merge_request_approvals.rb | 4 ---- .../api/entities/merge_request_approvals_spec.rb | 6 ++---- spec/requests/api/merge_request_approvals_spec.rb | 13 ------------- 4 files changed, 3 insertions(+), 21 deletions(-) diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index 847ffbcb164c82..6aeb0298687c71 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -174,6 +174,7 @@ expect(rule_response['name']).to eq('foo') expect(rule_response['approvers'][0]['username']).to eq(approver.username) expect(rule_response['approved_by'][0]['username']).to eq(approver.username) + expect(rule_response['commented_by'][0]['username']).to eq(approver.username) expect(rule_response['source_rule']).to be_nil expect(rule_response['section']).to be_nil end diff --git a/lib/api/entities/merge_request_approvals.rb b/lib/api/entities/merge_request_approvals.rb index 74d909e0b1126a..0c464844ae7ebf 100644 --- a/lib/api/entities/merge_request_approvals.rb +++ b/lib/api/entities/merge_request_approvals.rb @@ -18,10 +18,6 @@ class MergeRequestApprovals < Grape::Entity expose :approved_by, using: ::API::Entities::Approvals do |merge_request| merge_request.approvals end - - expose :commented_by, using: ::API::Entities::Approvals do |merge_request| - [] - end end end end diff --git a/spec/lib/api/entities/merge_request_approvals_spec.rb b/spec/lib/api/entities/merge_request_approvals_spec.rb index e13271ae0e35ae..cbbb037100a110 100644 --- a/spec/lib/api/entities/merge_request_approvals_spec.rb +++ b/spec/lib/api/entities/merge_request_approvals_spec.rb @@ -21,8 +21,7 @@ approved: true, approved_by: [{ user: API::Entities::UserBasic.new(user).as_json - }], - commented_by: [] + }] }) end @@ -31,8 +30,7 @@ user_has_approved: false, user_can_approve: true, approved: false, - approved_by: [], - commented_by: [] + approved_by: [] }) end end diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb index bc9b6c2f5bed66..fad5c3fb60ec30 100644 --- a/spec/requests/api/merge_request_approvals_spec.rb +++ b/spec/requests/api/merge_request_approvals_spec.rb @@ -23,19 +23,6 @@ end end - describe "GET :id/merge_requests/:merge_request_iid/approval_settings" do - it "retrieves the approval settings" do - project.add_developer(approver) - project.add_developer(create(:user)) - - create(:approval, user: approver, merge_request: merge_request) - - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) - - expect(response).to have_gitlab_http_status(:ok) - end - end - describe 'POST :id/merge_requests/:merge_request_iid/approve' do context 'as a valid approver' do let_it_be(:approver) { create(:user) } -- GitLab