Merge branch...

Merge branch '332883-bug-not-showing-warning-when-we-don-t-show-the-full-diff-in-a-merge-request' into 'master'

Resolve "Bug: Not showing warning when we don't show the full diff in a merge request"

See merge request gitlab-org/gitlab!64515

app/helpers/diff_helper.rb

@@ -190,10 +190,8 @@ module DiffHelper
   end
 
   def render_overflow_warning?(diffs_collection)
-    diff_files = diffs_collection.raw_diff_files
-
-    diff_files.overflow?.tap do |overflown|
-      log_overflow_limits(diff_files)
+    diffs_collection.overflow?.tap do |overflown|
+      log_overflow_limits(diff_files: diffs_collection.raw_diff_files, collection_overflow: overflown)
     end
   end
 
@@ -285,12 +283,12 @@ module DiffHelper
     conflicts_service.conflicts.files.index_by(&:our_path)
   end
 
-  def log_overflow_limits(diff_files)
+  def log_overflow_limits(diff_files:, collection_overflow:)
     if diff_files.any?(&:too_large?)
       Gitlab::Metrics.add_event(:diffs_overflow_single_file_limits)
     end
 
-    Gitlab::Metrics.add_event(:diffs_overflow_collection_limits) if diff_files.overflow?
+    Gitlab::Metrics.add_event(:diffs_overflow_collection_limits) if collection_overflow
     Gitlab::Metrics.add_event(:diffs_overflow_max_bytes_limits) if diff_files.overflow_max_bytes?
     Gitlab::Metrics.add_event(:diffs_overflow_max_files_limits) if diff_files.overflow_max_files?
     Gitlab::Metrics.add_event(:diffs_overflow_max_lines_limits) if diff_files.overflow_max_lines?

app/serializers/merge_request_diff_entity.rb

@@ -16,7 +16,6 @@ class MergeRequestDiffEntity < Grape::Entity
   end
 
   expose :created_at
-  expose :state
   expose :commits_count
 
   expose :latest?, as: :latest

lib/gitlab/diff/file_collection/base.rb

@@ -85,6 +85,10 @@ module Gitlab
           # No-op
         end
 
+        def overflow?
+          raw_diff_files.overflow?
+        end
+
         private
 
         def empty_pagination_data

lib/gitlab/diff/file_collection/merge_request_diff_base.rb

@@ -48,6 +48,10 @@ module Gitlab
           @merge_request_diff.real_size
         end
 
+        def overflow?
+          @merge_request_diff.overflow?
+        end
+
         private
 
         def highlight_cache

spec/helpers/diff_helper_spec.rb

@@ -293,23 +293,22 @@ RSpec.describe DiffHelper do
   describe '#render_overflow_warning?' do
     using RSpec::Parameterized::TableSyntax
 
-    let(:diffs_collection) { instance_double(Gitlab::Diff::FileCollection::MergeRequestDiff, raw_diff_files: diff_files) }
+    let(:diffs_collection) { instance_double(Gitlab::Diff::FileCollection::MergeRequestDiff, raw_diff_files: diff_files, overflow?: false) }
     let(:diff_files) { Gitlab::Git::DiffCollection.new(files) }
     let(:safe_file) { { too_large: false, diff: '' } }
     let(:large_file) { { too_large: true, diff: '' } }
     let(:files) { [safe_file, safe_file] }
 
-    before do
-      allow(diff_files).to receive(:overflow?).and_return(false)
-      allow(diff_files).to receive(:overflow_max_bytes?).and_return(false)
-      allow(diff_files).to receive(:overflow_max_files?).and_return(false)
-      allow(diff_files).to receive(:overflow_max_lines?).and_return(false)
-      allow(diff_files).to receive(:collapsed_safe_bytes?).and_return(false)
-      allow(diff_files).to receive(:collapsed_safe_files?).and_return(false)
-      allow(diff_files).to receive(:collapsed_safe_lines?).and_return(false)
-    end
-
     context 'when no limits are hit' do
+      before do
+        allow(diff_files).to receive(:overflow_max_bytes?).and_return(false)
+        allow(diff_files).to receive(:overflow_max_files?).and_return(false)
+        allow(diff_files).to receive(:overflow_max_lines?).and_return(false)
+        allow(diff_files).to receive(:collapsed_safe_bytes?).and_return(false)
+        allow(diff_files).to receive(:collapsed_safe_files?).and_return(false)
+        allow(diff_files).to receive(:collapsed_safe_lines?).and_return(false)
+      end
+
       it 'returns false and does not log any overflow events' do
         expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_collection_limits)
         expect(Gitlab::Metrics).not_to receive(:add_event).with(:diffs_overflow_single_file_limits)
@@ -343,7 +342,7 @@ RSpec.describe DiffHelper do
 
     context 'when the file collection has an overflow' do
       before do
-        allow(diff_files).to receive(:overflow?).and_return(true)
+        allow(diffs_collection).to receive(:overflow?).and_return(true)
       end
 
       it 'returns true and only logs all the correct collection overflow event' do

spec/lib/gitlab/diff/file_collection/base_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Diff::FileCollection::Base do
+  let(:merge_request) { create(:merge_request) }
+  let(:diffable) { merge_request.merge_request_diff }
+  let(:diff_options) { {} }
+
+  describe '#overflow?' do
+    subject(:overflown) { described_class.new(diffable, project: merge_request.project, diff_options: diff_options).overflow? }
+
+    context 'when it is not overflown' do
+      it 'returns false' do
+        expect(overflown).to eq(false)
+      end
+    end
+
+    context 'when it is overflown' do
+      let(:diff_options) { { max_files: 1 } }
+
+      it 'returns true' do
+        expect(overflown).to eq(true)
+      end
+    end
+  end
+end

spec/lib/gitlab/diff/file_collection/merge_request_diff_base_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBase do
+  let(:merge_request) { create(:merge_request) }
+  let(:diffable) { merge_request.merge_request_diff }
+
+  describe '#overflow?' do
+    subject(:overflown) { described_class.new(diffable, diff_options: nil).overflow? }
+
+    context 'when it is not overflown' do
+      it 'returns false' do
+        expect(overflown).to eq(false)
+      end
+    end
+
+    context 'when it is overflown' do
+      before do
+        diffable.update!(state: :overflow)
+      end
+
+      it 'returns true' do
+        expect(overflown).to eq(true)
+      end
+    end
+  end
+end

spec/serializers/merge_request_diff_entity_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe MergeRequestDiffEntity do
       expect(subject).to include(
         :version_index, :created_at, :commits_count,
         :latest, :short_commit_sha, :version_path,
-        :compare_path, :state
+        :compare_path
       )
     end
   end