Merge branch 'id-expose-approvals-endpoints' into 'master'

[RUN AS-IF-FOSS] Expose approvals fields for FOSS FE

See merge request gitlab-org/gitlab!36564

app/models/concerns/approvable_base.rb

+# frozen_string_literal: true
+
+module ApprovableBase
+  extend ActiveSupport::Concern
+
+  included do
+    has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+    has_many :approved_by_users, through: :approvals, source: :user
+  end
+
+  def has_approved?(user)
+    return false unless user
+
+    approved_by_users.include?(user)
+  end
+end

app/models/merge_request.rb

@@ -20,6 +20,7 @@ class MergeRequest < ApplicationRecord
   include IgnorableColumns
   include MilestoneEventable
   include StateEventable
+  include ApprovableBase
 
   extend ::Gitlab::Utils::Override
 
@@ -92,9 +93,6 @@ class MergeRequest < ApplicationRecord
   has_many :draft_notes
   has_many :reviews, inverse_of: :merge_request
 
-  has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
-  has_many :approved_by_users, through: :approvals, source: :user
-
   KNOWN_MERGE_PARAMS = [
     :auto_merge_strategy,
     :should_remove_source_branch,

app/presenters/merge_request_presenter.rb

@@ -8,6 +8,8 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
   include ChecksCollaboration
   include Gitlab::Utils::StrongMemoize
 
+  APPROVALS_WIDGET_BASE_TYPE = 'base'
+
   presents :merge_request
 
   def ci_status
@@ -224,6 +226,22 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
     end
   end
 
+  def api_approvals_path
+    expose_path(api_v4_projects_merge_requests_approvals_path(id: project.id, merge_request_iid: merge_request.iid))
+  end
+
+  def api_approve_path
+    expose_path(api_v4_projects_merge_requests_approve_path(id: project.id, merge_request_iid: merge_request.iid))
+  end
+
+  def api_unapprove_path
+    expose_path(api_v4_projects_merge_requests_unapprove_path(id: project.id, merge_request_iid: merge_request.iid))
+  end
+
+  def approvals_widget_type
+    APPROVALS_WIDGET_BASE_TYPE
+  end
+
   private
 
   def cached_can_be_reverted?

app/serializers/merge_request_poll_cached_widget_entity.rb

@@ -86,6 +86,18 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity
     presenter(merge_request).squash_on_merge?
   end
 
+  expose :api_approvals_path do |merge_request|
+    presenter(merge_request).api_approvals_path
+  end
+
+  expose :api_approve_path do |merge_request|
+    presenter(merge_request).api_approve_path
+  end
+
+  expose :api_unapprove_path do |merge_request|
+    presenter(merge_request).api_unapprove_path
+  end
+
   private
 
   delegate :current_user, to: :request

app/serializers/merge_request_poll_widget_entity.rb

@@ -157,6 +157,10 @@ class MergeRequestPollWidgetEntity < Grape::Entity
     presenter(merge_request).squash_on_merge?
   end
 
+  expose :approvals_widget_type do |merge_request|
+    presenter(merge_request).approvals_widget_type
+  end
+
   private
 
   delegate :current_user, to: :request

app/services/merge_requests/remove_approval_service.rb

@@ -4,7 +4,7 @@ module MergeRequests
   class RemoveApprovalService < MergeRequests::BaseService
     # rubocop: disable CodeReuse/ActiveRecord
     def execute(merge_request)
-      return unless approved_by_user?(merge_request)
+      return unless merge_request.has_approved?(current_user)
 
       # paranoid protection against running wrong deletes
       return unless merge_request.id && current_user.id
@@ -24,10 +24,6 @@ module MergeRequests
 
     private
 
-    def approved_by_user?(merge_request)
-      merge_request.approved_by_users.include?(current_user)
-    end
-
     def reset_approvals_cache(merge_request)
       merge_request.approvals.reset
     end

changelogs/unreleased/id-expose-approvals-endpoints.yml

+---
+title: Expose approvals fields for FOSS FE
+merge_request: 36564
+author:
+type: changed

ee/app/presenters/ee/merge_request_presenter.rb

@@ -3,18 +3,9 @@
 module EE
   module MergeRequestPresenter
     include ::VisibleApprovable
+    extend ::Gitlab::Utils::Override
 
-    def approvals_path
-      if expose_mr_approval_path?
-        expose_path(approvals_project_merge_request_path(project, merge_request))
-      end
-    end
-
-    def api_approvals_path
-      if expose_mr_approval_path?
-        expose_path(api_v4_projects_merge_requests_approvals_path(id: project.id, merge_request_iid: merge_request.iid))
-      end
-    end
+    APPROVALS_WIDGET_FULL_TYPE = 'full'
 
     def api_approval_settings_path
       if expose_mr_approval_path?
@@ -28,18 +19,6 @@ module EE
       end
     end
 
-    def api_approve_path
-      if expose_mr_approval_path?
-        expose_path(api_v4_projects_merge_requests_approve_path(id: project.id, merge_request_iid: merge_request.iid))
-      end
-    end
-
-    def api_unapprove_path
-      if expose_mr_approval_path?
-        expose_path(api_v4_projects_merge_requests_unapprove_path(id: project.id, merge_request_iid: merge_request.iid))
-      end
-    end
-
     def merge_train_when_pipeline_succeeds_docs_path
       help_page_path('ci/merge_request_pipelines/pipelines_for_merged_results/merge_trains/index.md', anchor: 'add-a-merge-request-to-a-merge-train')
     end
@@ -64,6 +43,11 @@ module EE
       merge_request.approval_state.suggested_approvers(current_user: current_user)
     end
 
+    override :approvals_widget_type
+    def approvals_widget_type
+      expose_mr_approval_path? ? APPROVALS_WIDGET_FULL_TYPE : super
+    end
+
     private
 
     def expose_mr_approval_path?

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -109,22 +109,10 @@ module EE
         merge_request.approval_feature_available?
       end
 
-      expose :api_approvals_path do |merge_request|
-        presenter(merge_request).api_approvals_path
-      end
-
       expose :api_approval_settings_path do |merge_request|
         presenter(merge_request).api_approval_settings_path
       end
 
-      expose :api_approve_path do |merge_request|
-        presenter(merge_request).api_approve_path
-      end
-
-      expose :api_unapprove_path do |merge_request|
-        presenter(merge_request).api_unapprove_path
-      end
-
       expose :merge_train_when_pipeline_succeeds_docs_path do |merge_request|
         presenter(merge_request).merge_train_when_pipeline_succeeds_docs_path
       end

ee/app/services/ee/merge_requests/remove_approval_service.rb

@@ -7,11 +7,6 @@ module EE
 
       private
 
-      override :approved_by_user?
-      def approved_by_user?(merge_request)
-        merge_request.has_approved?(current_user)
-      end
-
       override :reset_approvals_cache
       def reset_approvals_cache(merge_request)
         merge_request.reset_approval_cache!

ee/lib/ee/api/entities/approval_state.rb

@@ -20,7 +20,7 @@ module EE
           approval_state.project.require_password_to_approve?
         end
 
-        expose :approved_by, using: EE::API::Entities::Approvals do |approval_state|
+        expose :approved_by, using: ::API::Entities::Approvals do |approval_state|
           approval_state.merge_request.approvals
         end
 

ee/lib/ee/api/entities/approvals.rb

-# frozen_string_literal: true
-
-module EE
-  module API
-    module Entities
-      class Approvals < Grape::Entity
-        expose :user, using: ::API::Entities::UserBasic
-      end
-    end
-  end
-end

ee/spec/presenters/merge_request_presenter_spec.rb

@@ -31,22 +31,6 @@ RSpec.describe MergeRequestPresenter do
     end
   end
 
-  describe '#approvals_path' do
-    subject { described_class.new(merge_request, current_user: user).approvals_path }
-
-    it_behaves_like 'is nil when needed'
-
-    it { is_expected.to eq(expose_path("/#{merge_request.project.full_path}/-/merge_requests/#{merge_request.iid}/approvals")) }
-  end
-
-  describe '#api_approvals_path' do
-    subject { described_class.new(merge_request, current_user: user).api_approvals_path }
-
-    it_behaves_like 'is nil when needed'
-
-    it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approvals")) }
-  end
-
   describe '#api_approval_settings_path' do
     subject { described_class.new(merge_request, current_user: user).api_approval_settings_path }
 
@@ -67,22 +51,6 @@ RSpec.describe MergeRequestPresenter do
     end
   end
 
-  describe '#api_approve_path' do
-    subject { described_class.new(merge_request, current_user: user).api_approve_path }
-
-    it_behaves_like 'is nil when needed'
-
-    it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approve")) }
-  end
-
-  describe '#api_unapprove_path' do
-    subject { described_class.new(merge_request, current_user: user).api_unapprove_path }
-
-    it_behaves_like 'is nil when needed'
-
-    it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/unapprove")) }
-  end
-
   describe '#suggested_approvers' do
     subject { described_class.new(merge_request, current_user: user).suggested_approvers }
 
@@ -118,4 +86,24 @@ RSpec.describe MergeRequestPresenter do
       end
     end
   end
+
+  describe '#approvals_widget_type' do
+    subject { described_class.new(merge_request, current_user: user).approvals_widget_type }
+
+    context 'when approvals feature is available for a project' do
+      let(:approval_feature_available) { true }
+
+      it 'returns full' do
+        is_expected.to eq('full')
+      end
+    end
+
+    context 'when approvals feature is not available for a project' do
+      let(:approval_feature_available) { false }
+
+      it 'returns base' do
+        is_expected.to eq('base')
+      end
+    end
+  end
 end

lib/api/entities/approvals.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    class Approvals < Grape::Entity
+      expose :user, using: ::API::Entities::UserBasic
+    end
+  end
+end

lib/api/entities/merge_request_approvals.rb

@@ -3,6 +3,22 @@
 module API
   module Entities
     class MergeRequestApprovals < Grape::Entity
+      expose :user_has_approved do |merge_request, options|
+        merge_request.has_approved?(options[:current_user])
+      end
+
+      expose :user_can_approve do |merge_request, options|
+        !merge_request.has_approved?(options[:current_user]) &&
+          options[:current_user].can?(:approve_merge_request, merge_request)
+      end
+
+      expose :approved do |merge_request|
+        merge_request.approvals.present?
+      end
+
+      expose :approved_by, using: ::API::Entities::Approvals do |merge_request|
+        merge_request.approvals
+      end
     end
   end
 end

spec/lib/api/entities/merge_request_approvals_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Entities::MergeRequestApprovals do
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request) }
+
+  subject { described_class.new(merge_request, current_user: user).as_json }
+
+  before do
+    merge_request.project.add_developer(user)
+  end
+
+  it 'serializes an approved merge request' do
+    create(:approval, merge_request: merge_request, user: user)
+
+    is_expected.to eq({
+      user_has_approved: true,
+      user_can_approve: false,
+      approved: true,
+      approved_by: [{
+        user: API::Entities::UserBasic.new(user).as_json
+      }]
+    })
+  end
+
+  it 'serializes a merge request that is not approved' do
+    is_expected.to eq({
+      user_has_approved: false,
+      user_can_approve: true,
+      approved: false,
+      approved_by: []
+    })
+  end
+end

spec/models/concerns/approvable_base_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ApprovableBase do
+  describe '#has_approved?' do
+    let(:merge_request) { create(:merge_request) }
+    let(:user) { create(:user) }
+
+    subject { merge_request.has_approved?(user) }
+
+    context 'when a user has not approved' do
+      it 'returns false' do
+        is_expected.to be_falsy
+      end
+    end
+
+    context 'when a user has approved' do
+      let!(:approval) { create(:approval, merge_request: merge_request, user: user) }
+
+      it 'returns false' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when a user is nil' do
+      let(:user) { nil }
+
+      it 'returns false' do
+        is_expected.to be_falsy
+      end
+    end
+  end
+end

spec/presenters/merge_request_presenter_spec.rb

@@ -613,4 +613,22 @@ RSpec.describe MergeRequestPresenter do
       end
     end
   end
+
+  describe '#api_approvals_path' do
+    subject { described_class.new(resource, current_user: user).api_approvals_path }
+
+    it { is_expected.to eq(expose_path("/api/v4/projects/#{project.id}/merge_requests/#{resource.iid}/approvals")) }
+  end
+
+  describe '#api_approve_path' do
+    subject { described_class.new(resource, current_user: user).api_approve_path }
+
+    it { is_expected.to eq(expose_path("/api/v4/projects/#{project.id}/merge_requests/#{resource.iid}/approve")) }
+  end
+
+  describe '#api_unapprove_path' do
+    subject { described_class.new(resource, current_user: user).api_unapprove_path }
+
+    it { is_expected.to eq(expose_path("/api/v4/projects/#{project.id}/merge_requests/#{resource.iid}/unapprove")) }
+  end
 end