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
Merge request reports
Activity
Reassigned to @DouweM
@rspeicher I believe the real issue is trickier than that. The
apply_diff_view_cookie!
method is only called inProjects::MergeRequestsController#diffs
, thusparams[:view]
is set in the only two cases:- When present in QS, i.e.
view=parallel
- When visiting the
/merge_request/:id/diffs
URL directly, i.e. not when accessed dynamically when the "Changes" tab is clicked
In all other cases, as you pointed out,
diff_view
would return the defaultinline
value.Your fix works but I think the
apply_diff_view_cookie!
could also be changed to make it clear that we don't rely onparams[:view]
in the view, e.g.:def apply_diff_view_cookie! - view = params[:view] || cookies[:diff_view] - cookies.permanent[:diff_view] = params[:view] = view if view + cookies.permanent[:diff_view] = params.delete(:view) if params[:view].present? end
- When present in QS, i.e.
@rspeicher Thoughts?
Reassigned to @rspeicher
Reassigned to @rymai
Reassigned to @rspeicher
Added 1 commit:
- 8530ce4c - Clarify that the diff view setting always comes from the cookie
@rymai Ready for review. We probably could sneak this into 8.7 final, but I'd be more comfortable waiting for the first patch release. Up to you guys.
Edited by Robert SpeicherReassigned to @rymai
@rspeicher Seeing how the diff is fairly simply I think we can squeeze this into 8.7.0.
mentioned in commit 5a8873f3
mentioned in commit 778b1f46
Mentioned in commit sharcnet-web-team/gitlab-ce@5a8873f3
mentioned in commit 49470353
mentioned in commit 1b80abeb
mentioned in commit 2ea923a4
mentioned in commit 902e9b33
mentioned in commit 314758f5
mentioned in commit c8d09be7
mentioned in commit acf5918b
mentioned in commit 1694efe8
mentioned in merge request !16071 (merged)