Merge branch 'bvl-single-diff-type' into 'master'

Only expose 1 diff style in diffs endpoint

See merge request gitlab-org/gitlab!19317

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -31,6 +31,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     options = {
       merge_request: @merge_request,
+      diff_view: diff_view,
       pagination_data: diffs.pagination_data
     }
 
@@ -60,7 +61,9 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       render: ->(partial, locals) { view_to_html_string(partial, locals) }
     }
 
-    render json: DiffsSerializer.new(request).represent(@diffs, additional_attributes)
+    options = additional_attributes.merge(diff_view: diff_view)
+
+    render json: DiffsSerializer.new(request).represent(@diffs, options)
   end
 
   def define_diff_vars

app/serializers/diff_file_entity.rb

@@ -53,7 +53,7 @@ class DiffFileEntity < DiffFileBaseEntity
   end
 
   # Used for inline diffs
-  expose :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, _) { diff_file.text? } do |diff_file|
+  expose :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, options) { inline_diff_view?(options) && diff_file.text? } do |diff_file|
     diff_file.diff_lines_for_serializer
   end
 
@@ -62,5 +62,21 @@ class DiffFileEntity < DiffFileBaseEntity
   end
 
   # Used for parallel diffs
-  expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, _) { diff_file.text? }
+  expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, options) { parallel_diff_view?(options) && diff_file.text? }
+
+  private
+
+  def parallel_diff_view?(options)
+    return true unless Feature.enabled?(:single_mr_diff_view)
+
+    # If we're not rendering inline, we must be rendering parallel
+    !inline_diff_view?(options)
+  end
+
+  def inline_diff_view?(options)
+    return true unless Feature.enabled?(:single_mr_diff_view)
+
+    # If nothing is present, inline will be the default.
+    options.fetch(:diff_view, :inline).to_sym == :inline
+  end
 end

ee/spec/features/merge_request/batch_comments_spec.rb

@@ -30,8 +30,11 @@ describe 'Merge request > Batch comments', :js do
     end
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'Feature is enabled' do
     before do
+      stub_feature_flags(single_mr_diff_view: false)
       stub_licensed_features(batch_comments: true)
 
       visit_diffs

ee/spec/features/merge_request/user_reviews_image_spec.rb

@@ -11,6 +11,7 @@ describe 'Merge request > image review', :js do
   let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     stub_licensed_features(batch_comments: true)
 
     sign_in(user)
@@ -23,6 +24,8 @@ describe 'Merge request > image review', :js do
     wait_for_requests
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   it 'leaves review' do
     find('.js-add-image-diff-note-button', match: :first).click
 

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -34,6 +34,16 @@ describe Projects::MergeRequests::DiffsController do
       it 'saves the preferred diff view in a cookie' do
         expect(response.cookies['diff_view']).to eq('parallel')
       end
+
+      it 'only renders the required view', :aggregate_failures do
+        diff_files_without_deletions = json_response['diff_files'].reject { |f| f['deleted_file'] }
+        have_no_inline_diff_lines = satisfy('have no inline diff lines') do |diff_file|
+          !diff_file.has_key?('highlighted_diff_lines')
+        end
+
+        expect(diff_files_without_deletions).to all(have_key('parallel_diff_lines'))
+        expect(diff_files_without_deletions).to all(have_no_inline_diff_lines)
+      end
     end
 
     context 'when the user cannot view the merge request' do
@@ -259,7 +269,7 @@ describe Projects::MergeRequests::DiffsController do
           it 'only renders the diffs for the path given' do
             diff_for_path(old_path: existing_path, new_path: existing_path)
 
-            paths = json_response["diff_files"].map { |file| file['new_path'] }
+            paths = json_response['diff_files'].map { |file| file['new_path'] }
 
             expect(paths).to include(existing_path)
           end
@@ -344,6 +354,7 @@ describe Projects::MergeRequests::DiffsController do
       let(:expected_options) do
         {
           merge_request: merge_request,
+          diff_view: :inline,
           pagination_data: {
             current_page: 1,
             next_page: nil,
@@ -367,6 +378,7 @@ describe Projects::MergeRequests::DiffsController do
       let(:expected_options) do
         {
           merge_request: merge_request,
+          diff_view: :inline,
           pagination_data: {
             current_page: 2,
             next_page: 3,

spec/features/merge_request/maintainer_edits_fork_spec.rb

@@ -20,7 +20,7 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js
   end
 
   before do
-    stub_feature_flags(web_ide_default: false)
+    stub_feature_flags(web_ide_default: false, single_mr_diff_view: false)
 
     target_project.add_maintainer(user)
     sign_in(user)
@@ -32,6 +32,8 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js
     wait_for_requests
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   it 'mentions commits will go to the source branch' do
     expect(page).to have_content('Your changes can be committed to fix because a merge request is open.')
   end

spec/features/merge_request/user_comments_on_diff_spec.rb

@@ -13,12 +13,15 @@ describe 'User comments on a diff', :js do
   let(:user) { create(:user) }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     project.add_maintainer(user)
     sign_in(user)
 
     visit(diffs_project_merge_request_path(project, merge_request))
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'when viewing comments' do
     context 'when toggling inline comments' do
       context 'in a single file' do

spec/features/merge_request/user_creates_image_diff_notes_spec.rb

@@ -9,6 +9,7 @@ describe 'Merge request > User creates image diff notes', :js do
   let(:user) { project.creator }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     sign_in(user)
 
     # Stub helper to return any blob file as image from public app folder.
@@ -17,6 +18,8 @@ describe 'Merge request > User creates image diff notes', :js do
     allow_any_instance_of(DiffHelper).to receive(:diff_file_old_blob_raw_url).and_return('/favicon.png')
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'create commit diff notes' do
     commit_id = '2f63565e7aac07bcdadb654e253078b727143ec4'
 

spec/features/merge_request/user_expands_diff_spec.rb

@@ -7,6 +7,8 @@ describe 'User expands diff', :js do
   let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
+
     allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes)
     allow(Gitlab::Git::Diff).to receive(:collapse_limit).and_return(10.kilobytes)
 
@@ -15,6 +17,8 @@ describe 'User expands diff', :js do
     wait_for_requests
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   it 'allows user to expand diff' do
     page.within find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd"]') do
       click_link 'Click to expand it.'

spec/features/merge_request/user_posts_diff_notes_spec.rb

@@ -14,12 +14,15 @@ describe 'Merge request > User posts diff notes', :js do
   let(:test_note_comment) { 'this is a test note!' }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     set_cookie('sidebar_collapsed', 'true')
 
     project.add_developer(user)
     sign_in(user)
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'when hovering over a parallel view diff file' do
     before do
       visit diffs_project_merge_request_path(project, merge_request, view: 'parallel')

spec/features/merge_request/user_resolves_conflicts_spec.rb

@@ -9,6 +9,7 @@ describe 'Merge request > User resolves conflicts', :js do
   before do
     # In order to have the diffs collapsed, we need to disable the increase feature
     stub_feature_flags(gitlab_git_diff_size_limit_increase: false)
+    stub_feature_flags(single_mr_diff_view: false)
   end
 
   def create_merge_request(source_branch)
@@ -17,7 +18,9 @@ describe 'Merge request > User resolves conflicts', :js do
     end
   end
 
-  shared_examples "conflicts are resolved in Interactive mode" do
+  it_behaves_like 'rendering a single diff version'
+
+  shared_examples 'conflicts are resolved in Interactive mode' do
     it 'conflicts are resolved in Interactive mode' do
       within find('.files-wrapper .diff-file', text: 'files/ruby/popen.rb') do
         click_button 'Use ours'

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -19,6 +19,12 @@ describe 'Merge request > User resolves diff notes and threads', :js do
     )
   end
 
+  before do
+    stub_feature_flags(single_mr_diff_view: false)
+  end
+
+  it_behaves_like 'rendering a single diff version'
+
   context 'no threads' do
     before do
       project.add_maintainer(user)

spec/features/merge_request/user_sees_avatar_on_diff_notes_spec.rb

@@ -7,8 +7,8 @@ describe 'Merge request > User sees avatars on diff notes', :js do
 
   let(:project)       { create(:project, :public, :repository) }
   let(:user)          { project.creator }
-  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") }
-  let(:path)          { "files/ruby/popen.rb" }
+  let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: 'Bug NS-04') }
+  let(:path)          { 'files/ruby/popen.rb' }
   let(:position) do
     Gitlab::Diff::Position.new(
       old_path: path,
@@ -21,12 +21,15 @@ describe 'Merge request > User sees avatars on diff notes', :js do
   let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     project.add_maintainer(user)
     sign_in user
 
     set_cookie('sidebar_collapsed', 'true')
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'discussion tab' do
     before do
       visit project_merge_request_path(project, merge_request)

spec/features/merge_request/user_sees_diff_spec.rb

@@ -9,6 +9,12 @@ describe 'Merge request > User sees diff', :js do
   let(:project) { create(:project, :public, :repository) }
   let(:merge_request) { create(:merge_request, source_project: project) }
 
+  before do
+    stub_feature_flags(single_mr_diff_view: false)
+  end
+
+  it_behaves_like 'rendering a single diff version'
+
   context 'when linking to note' do
     describe 'with unresolved note' do
       let(:note) { create :diff_note_on_merge_request, project: project, noteable: merge_request }

spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb

@@ -11,11 +11,14 @@ describe 'Merge request > User sees MR with deleted source branch', :js do
   let(:user) { project.creator }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     merge_request.update!(source_branch: 'this-branch-does-not-exist')
     sign_in(user)
     visit project_merge_request_path(project, merge_request)
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   it 'shows a message about missing source branch' do
     expect(page).to have_content('Source branch does not exist.')
   end

spec/features/merge_request/user_sees_versions_spec.rb

@@ -16,11 +16,15 @@ describe 'Merge request > User sees versions', :js do
   let!(:params) { {} }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
+
     project.add_maintainer(user)
     sign_in(user)
     visit diffs_project_merge_request_path(project, merge_request, params)
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   shared_examples 'allows commenting' do |file_id:, line_code:, comment:|
     it do
       diff_file_selector = ".diff-file[id='#{file_id}']"

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -25,12 +25,15 @@ describe 'User comments on a diff', :js do
   let(:user) { create(:user) }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     project.add_maintainer(user)
     sign_in(user)
 
     visit(diffs_project_merge_request_path(project, merge_request))
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'single suggestion note' do
     it 'hides suggestion popover' do
       click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))

spec/features/merge_request/user_toggles_whitespace_changes_spec.rb

@@ -8,6 +8,7 @@ describe 'Merge request > User toggles whitespace changes', :js do
   let(:user) { project.creator }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     project.add_maintainer(user)
     sign_in(user)
     visit diffs_project_merge_request_path(project, merge_request)
@@ -15,6 +16,8 @@ describe 'Merge request > User toggles whitespace changes', :js do
     find('.js-show-diff-settings').click
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   it 'has a button to toggle whitespace changes' do
     expect(page).to have_content 'Show whitespace changes'
   end

spec/features/merge_request/user_views_diffs_spec.rb

@@ -9,6 +9,7 @@ describe 'User views diffs', :js do
   let(:project) { create(:project, :public, :repository) }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
     visit(diffs_project_merge_request_path(project, merge_request))
 
     wait_for_requests
@@ -16,6 +17,8 @@ describe 'User views diffs', :js do
     find('.js-toggle-tree-list').click
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   shared_examples 'unfold diffs' do
     it 'unfolds diffs upwards' do
       first('.js-unfold').click

spec/features/projects/blobs/edit_spec.rb

@@ -12,9 +12,11 @@ describe 'Editing file blob', :js do
   let(:readme_file_path) { 'README.md' }
 
   before do
-    stub_feature_flags(web_ide_default: false)
+    stub_feature_flags(web_ide_default: false, single_mr_diff_view: false)
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'as a developer' do
     let(:user) { create(:user) }
     let(:role) { :developer }
@@ -27,14 +29,14 @@ describe 'Editing file blob', :js do
     def edit_and_commit(commit_changes: true)
       wait_for_requests
       find('.js-edit-blob').click
-      fill_editor(content: "class NextFeature\\nend\\n")
+      fill_editor(content: 'class NextFeature\\nend\\n')
 
       if commit_changes
         click_button 'Commit changes'
       end
     end
 
-    def fill_editor(content: "class NextFeature\\nend\\n")
+    def fill_editor(content: 'class NextFeature\\nend\\n')
       wait_for_requests
       find('#editor')
       execute_script("ace.edit('editor').setValue('#{content}')")
@@ -95,13 +97,13 @@ describe 'Editing file blob', :js do
     context 'when rendering the preview' do
       it 'renders content with CommonMark' do
         visit project_edit_blob_path(project, tree_join(branch, readme_file_path))
-        fill_editor(content: "1. one\\n  - sublist\\n")
+        fill_editor(content: '1. one\\n  - sublist\\n')
         click_link 'Preview'
         wait_for_requests
 
         # the above generates two separate lists (not embedded) in CommonMark
-        expect(page).to have_content("sublist")
-        expect(page).not_to have_xpath("//ol//li//ul")
+        expect(page).to have_content('sublist')
+        expect(page).not_to have_xpath('//ol//li//ul')
       end
     end
   end

spec/features/projects/view_on_env_spec.rb

@@ -9,9 +9,13 @@ describe 'View on environment', :js do
   let(:user) { project.creator }
 
   before do
+    stub_feature_flags(single_mr_diff_view: false)
+
     project.add_maintainer(user)
   end
 
+  it_behaves_like 'rendering a single diff version'
+
   context 'when the branch has a route map' do
     let(:route_map) do
       <<-MAP.strip_heredoc
@@ -26,7 +30,7 @@ describe 'View on environment', :js do
         user,
         start_branch: branch_name,
         branch_name: branch_name,
-        commit_message: "Add .gitlab/route-map.yml",
+        commit_message: 'Add .gitlab/route-map.yml',
         file_path: '.gitlab/route-map.yml',
         file_content: route_map
       ).execute
@@ -37,9 +41,9 @@ describe 'View on environment', :js do
         user,
         start_branch: branch_name,
         branch_name: branch_name,
-        commit_message: "Update feature",
+        commit_message: 'Update feature',
         file_path: file_path,
-        file_content: "# Noop"
+        file_content: '# Noop'
       ).execute
     end
 

spec/serializers/diff_file_entity_spec.rb

@@ -11,7 +11,8 @@ describe DiffFileEntity do
   let(:diff_refs) { commit.diff_refs }
   let(:diff) { commit.raw_diffs.first }
   let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
-  let(:entity) { described_class.new(diff_file, request: {}) }
+  let(:options) { {} }
+  let(:entity) { described_class.new(diff_file, options.reverse_merge(request: {})) }
 
   subject { entity.as_json }
 
@@ -23,7 +24,7 @@ describe DiffFileEntity do
     let(:user) { create(:user) }
     let(:request) { EntityRequest.new(project: project, current_user: user) }
     let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-    let(:entity) { described_class.new(diff_file, request: request, merge_request: merge_request) }
+    let(:entity) { described_class.new(diff_file, options.merge(request: request, merge_request: merge_request)) }
     let(:exposed_urls) { %i(edit_path view_path context_lines_path) }
 
     it_behaves_like 'diff file entity'
@@ -49,6 +50,8 @@ describe DiffFileEntity do
   end
 
   context '#parallel_diff_lines' do
+    let(:options) { { diff_view: :parallel } }
+
     it 'exposes parallel diff lines correctly' do
       response = subject
 

spec/support/capybara.rb

@@ -101,8 +101,12 @@ RSpec.configure do |config|
 
   config.after(:example, :js) do |example|
     # when a test fails, display any messages in the browser's console
-    if example.exception
+    # but fail don't add the message if the failure is a pending test that got
+    # fixed. If we raised the `JSException` the fixed test would be marked as
+    # failed again.
+    if example.exception && !example.exception.is_a?(RSpec::Core::Pending::PendingExampleFixedError)
       console = page.driver.browser.manage.logs.get(:browser)&.reject { |log| log.message =~ JS_CONSOLE_FILTER }
+
       if console.present?
         message = "Unexpected browser console output:\n" + console.map(&:message).join("\n")
         raise JSConsoleError, message

spec/support/shared_examples/merge_requests_rendering_a_single_diff_version.rb

+# frozen_string_literal: true
+
+# This pending test can be removed when `single_mr_diff_view` is enabled by default
+# disabling the feature flag above is then not needed anymore.
+RSpec.shared_examples 'rendering a single diff version' do |attribute|
+  pending 'allows editing diff settings single_mr_diff_view is enabled' do
+    project = create(:project, :repository)
+    user = project.creator
+    merge_request = create(:merge_request, source_project: project)
+    stub_feature_flags(single_mr_diff_view: true)
+    sign_in(user)
+
+    visit(diffs_project_merge_request_path(project, merge_request))
+
+    expect(page).to have_selector('.js-show-diff-settings')
+  end
+end

spec/support/shared_examples/serializers/diff_file_entity_examples.rb

@@ -31,14 +31,43 @@ shared_examples 'diff file entity' do
 
   it 'exposes correct attributes' do
     expect(subject).to include(:added_lines, :removed_lines,
-                               :context_lines_path, :highlighted_diff_lines,
-                               :parallel_diff_lines)
+                               :context_lines_path)
   end
 
   it 'includes viewer' do
     expect(subject[:viewer].with_indifferent_access)
         .to match_schema('entities/diff_viewer')
   end
+
+  context 'diff files' do
+    context 'when diff_view is parallel' do
+      let(:options) { { diff_view: :parallel } }
+
+      it 'contains only the parallel diff lines', :aggregate_failures do
+        expect(subject).to include(:parallel_diff_lines)
+        expect(subject).not_to include(:highlighted_diff_lines)
+      end
+    end
+
+    context 'when diff_view is parallel' do
+      let(:options) { { diff_view: :inline } }
+
+      it 'contains only the inline diff lines', :aggregate_failures do
+        expect(subject).not_to include(:parallel_diff_lines)
+        expect(subject).to include(:highlighted_diff_lines)
+      end
+    end
+
+    context 'when the `single_mr_diff_view` feature is disabled' do
+      before do
+        stub_feature_flags(single_mr_diff_view: false)
+      end
+
+      it 'contains both kinds of diffs' do
+        expect(subject).to include(:highlighted_diff_lines, :parallel_diff_lines)
+      end
+    end
+  end
 end
 
 shared_examples 'diff file discussion entity' do