Merge branch 'dz-refactor-mr-widget-serializer' into 'master'

Move feature expose logic from serializer to the model

Closes #4336

See merge request gitlab-org/gitlab-ee!4690

ee/app/models/ee/merge_request.rb

@@ -63,5 +63,25 @@ module EE
     def has_dast_data?
       dast_artifact&.success?
     end
+
+    def expose_performance_data?
+      project.feature_available?(:merge_request_performance_metrics) &&
+        has_performance_data?
+    end
+
+    def expose_sast_data?
+      project.feature_available?(:sast) &&
+        has_sast_data?
+    end
+
+    def expose_dast_data?
+      project.feature_available?(:dast) &&
+        has_dast_data?
+    end
+
+    def expose_sast_container_data?
+      project.feature_available?(:sast_container) &&
+        has_sast_container_data?
+    end
   end
 end

ee/app/serializers/ee/merge_request_widget_entity.rb

@@ -27,7 +27,7 @@ module EE
         end
       end
 
-      expose :performance, if: -> (mr, _) { expose_performance_data?(mr) } do
+      expose :performance, if: -> (mr, _) { mr.expose_performance_data? } do
         expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_performance_artifact) } do |merge_request|
           raw_project_build_artifacts_url(merge_request.source_project,
                                           merge_request.head_performance_artifact,
@@ -41,60 +41,35 @@ module EE
         end
       end
 
-      expose :sast, if: -> (mr, _) { expose_sast_data?(mr, current_user) } do
-        expose :head_path do |merge_request|
+      expose :sast, if: -> (mr, _) { mr.expose_sast_data? } do
+        expose :head_path, if: -> (mr, _) { can?(current_user, :read_build, mr.head_sast_artifact) } do |merge_request|
           raw_project_build_artifacts_url(merge_request.source_project,
                                           merge_request.head_sast_artifact,
                                           path: Ci::Build::SAST_FILE)
         end
 
-        expose :base_path, if: -> (mr, _) { mr.has_base_sast_data? } do |merge_request|
+        expose :base_path, if: -> (mr, _) { mr.has_base_sast_data? && can?(current_user, :read_build, mr.base_sast_artifact)} do |merge_request|
           raw_project_build_artifacts_url(merge_request.target_project,
                                           merge_request.base_sast_artifact,
                                           path: Ci::Build::SAST_FILE)
         end
       end
 
-      expose :sast_container, if: -> (mr, _) { expose_sast_container_data?(mr, current_user) } do
-        expose :path do |merge_request|
+      expose :sast_container, if: -> (mr, _) { mr.expose_sast_container_data? } do
+        expose :path, if: -> (mr, _) { can?(current_user, :read_build, mr.sast_container_artifact) } do |merge_request|
           raw_project_build_artifacts_url(merge_request.source_project,
                                           merge_request.sast_container_artifact,
                                           path: Ci::Build::SAST_CONTAINER_FILE)
         end
       end
 
-      expose :dast, if: -> (mr, _) { expose_dast_data?(mr, current_user) } do
-        expose :path do |merge_request|
+      expose :dast, if: -> (mr, _) { mr.expose_dast_data? } do
+        expose :path, if: -> (mr, _) { can?(current_user, :read_build, mr.dast_artifact) } do |merge_request|
           raw_project_build_artifacts_url(merge_request.source_project,
                                           merge_request.dast_artifact,
                                           path: Ci::Build::DAST_FILE)
         end
       end
     end
-
-    private
-
-    def expose_sast_data?(mr, current_user)
-      mr.project.feature_available?(:sast) &&
-        mr.has_sast_data? &&
-        can?(current_user, :read_build, mr.head_sast_artifact)
-    end
-
-    def expose_performance_data?(mr)
-      mr.project.feature_available?(:merge_request_performance_metrics) &&
-        mr.has_performance_data?
-    end
-
-    def expose_sast_container_data?(mr, current_user)
-      mr.project.feature_available?(:sast_container) &&
-        mr.has_sast_container_data? &&
-        can?(current_user, :read_build, mr.sast_container_artifact)
-    end
-
-    def expose_dast_data?(mr, current_user)
-      mr.project.feature_available?(:dast) &&
-        mr.has_dast_data? &&
-        can?(current_user, :read_build, mr.dast_artifact)
-    end
   end
 end

ee/spec/models/merge_request_spec.rb

@@ -225,4 +225,17 @@ describe MergeRequest do
   describe '#dast_artifact' do
     it { is_expected.to delegate_method(:dast_artifact).to(:head_pipeline) }
   end
+
+  %w(sast dast performance sast_container).each do |type|
+    method = "expose_#{type}_data?"
+
+    describe "##{method}" do
+      before do
+        allow(merge_request).to receive(:"has_#{type}_data?").and_return(true)
+        allow(merge_request.project).to receive(:feature_available?).and_return(true)
+      end
+
+      it { expect(merge_request.send(method.to_sym)).to be_truthy }
+    end
+  end
 end

ee/spec/serializers/merge_request_widget_entity_spec.rb

@@ -7,6 +7,10 @@ describe MergeRequestWidgetEntity do
   let(:request) { double('request', current_user: user) }
   let(:pipeline) { create(:ci_empty_pipeline, project: project) }
 
+  before do
+    project.add_developer(user)
+  end
+
   subject do
     described_class.new(merge_request, request: request)
   end
@@ -23,7 +27,7 @@ describe MergeRequestWidgetEntity do
   it 'has performance data' do
     build = create(:ci_build, name: 'job')
 
-    allow(subject).to receive(:expose_performance_data?).and_return(true)
+    allow(merge_request).to receive(:expose_performance_data?).and_return(true)
     allow(merge_request).to receive(:base_performance_artifact).and_return(build)
     allow(merge_request).to receive(:head_performance_artifact).and_return(build)
 
@@ -31,9 +35,9 @@ describe MergeRequestWidgetEntity do
   end
 
   it 'has sast data' do
-    build = create(:ci_build, name: 'sast')
+    build = create(:ci_build, name: 'sast', pipeline: pipeline)
 
-    allow(subject).to receive(:expose_sast_data?).and_return(true)
+    allow(merge_request).to receive(:expose_sast_data?).and_return(true)
     allow(merge_request).to receive(:has_base_sast_data?).and_return(true)
     allow(merge_request).to receive(:base_sast_artifact).and_return(build)
     allow(merge_request).to receive(:head_sast_artifact).and_return(build)
@@ -44,9 +48,9 @@ describe MergeRequestWidgetEntity do
   end
 
   it 'has sast_container data' do
-    build = create(:ci_build, name: 'sast:image')
+    build = create(:ci_build, name: 'sast:image', pipeline: pipeline)
 
-    allow(subject).to receive(:expose_sast_container_data?).and_return(true)
+    allow(merge_request).to receive(:expose_sast_container_data?).and_return(true)
     allow(merge_request).to receive(:sast_container_artifact).and_return(build)
 
     expect(subject.as_json).to include(:sast_container)