Commit d4afb16d authored by Bob Van Landuyt's avatar Bob Van Landuyt Committed by Thong Kuah

Only expose 1 diff style in diffs endpoint

When enabling the `single_mr_diff_view` feature flag, the backend will
only render one kind of diff: inline or parallel. The feature is
disabled for now until the frontend has been updated.

To switch the view, the frontend can pass a `view` param containing
either `inline` or `parallel`. That will get written to a cookie. The
response for diff files will then contain either
`highlighted_diff_lines` or `parallel_diff_lines` but no longer both.
parent b614399e
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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'
......
......@@ -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.'
......
......@@ -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')
......
......@@ -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'
......
......@@ -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)
......
......@@ -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)
......
......@@ -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 }
......
......@@ -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
......
......@@ -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}']"
......
......@@ -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]}']"))
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
# 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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment