Commit 778b1f46 authored by Rémy Coutable's avatar Rémy Coutable Committed by Yorick Peterse

Merge branch 'rs-diff_view' into 'master'

Always read diff_view setting from the cookie

Prior, when the user had their view set to "parallel" and then visited a
merge request's changes tab _without_ passing the `view` parameter via
query string, the view would be parallel but the `Notes` class was
always instantiated with the default value from `diff_view` ("inline"),
resulting in broken markup when the form to add a line note was
dynamically inserted.

The cookie is set whenever the view is changed, so this value should
always be up-to-date.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/14557 and https://gitlab.com/gitlab-org/gitlab-ce/issues/15285

See merge request !3732
parent dd5a29e8
...@@ -83,8 +83,7 @@ class Projects::ApplicationController < ApplicationController ...@@ -83,8 +83,7 @@ class Projects::ApplicationController < ApplicationController
end end
def apply_diff_view_cookie! def apply_diff_view_cookie!
view = params[:view] || cookies[:diff_view] cookies.permanent[:diff_view] = params.delete(:view) if params[:view].present?
cookies.permanent[:diff_view] = params[:view] = view if view
end end
def builds_enabled def builds_enabled
......
...@@ -9,7 +9,13 @@ module DiffHelper ...@@ -9,7 +9,13 @@ module DiffHelper
end end
def diff_view def diff_view
params[:view] == 'parallel' ? 'parallel' : 'inline' diff_views = %w(inline parallel)
if diff_views.include?(cookies[:diff_view])
cookies[:diff_view]
else
diff_views.first
end
end end
def diff_hard_limit_enabled? def diff_hard_limit_enabled?
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
- page_card_attributes @merge_request.card_attributes - page_card_attributes @merge_request.card_attributes
- header_title project_title(@project, "Merge Requests", namespace_project_merge_requests_path(@project.namespace, @project)) - header_title project_title(@project, "Merge Requests", namespace_project_merge_requests_path(@project.namespace, @project))
- if params[:view] == 'parallel' - if diff_view == 'parallel'
- fluid_layout true - fluid_layout true
.merge-request{'data-url' => merge_request_path(@merge_request)} .merge-request{'data-url' => merge_request_path(@merge_request)}
......
...@@ -300,14 +300,6 @@ describe Projects::MergeRequestsController do ...@@ -300,14 +300,6 @@ describe Projects::MergeRequestsController do
expect(response.cookies['diff_view']).to eq('parallel') expect(response.cookies['diff_view']).to eq('parallel')
end end
it 'assigns :view param based on cookie' do
request.cookies['diff_view'] = 'parallel'
go
expect(controller.params[:view]).to eq 'parallel'
end
end end
describe 'GET commits' do describe 'GET commits' do
......
...@@ -11,6 +11,26 @@ describe DiffHelper do ...@@ -11,6 +11,26 @@ describe DiffHelper do
let(:diff_refs) { [commit.parent, commit] } let(:diff_refs) { [commit.parent, commit] }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) } let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs) }
describe 'diff_view' do
it 'returns a valid value when cookie is set' do
helper.request.cookies[:diff_view] = 'parallel'
expect(helper.diff_view).to eq 'parallel'
end
it 'returns a default value when cookie is invalid' do
helper.request.cookies[:diff_view] = 'invalid'
expect(helper.diff_view).to eq 'inline'
end
it 'returns a default value when cookie is nil' do
expect(helper.request.cookies).to be_empty
expect(helper.diff_view).to eq 'inline'
end
end
describe 'diff_hard_limit_enabled?' do describe 'diff_hard_limit_enabled?' do
it 'should return true if param is provided' do it 'should return true if param is provided' do
allow(controller).to receive(:params) { { force_show_diff: true } } allow(controller).to receive(:params) { { force_show_diff: true } }
......
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