Commit ca3c0c6c authored by Paco Guzman's avatar Paco Guzman

MergeRequest new form load diff asynchronously

parent 0bbeff3d
...@@ -30,6 +30,7 @@ v 8.13.0 (unreleased) ...@@ -30,6 +30,7 @@ v 8.13.0 (unreleased)
- Replace `alias_method_chain` with `Module#prepend` - Replace `alias_method_chain` with `Module#prepend`
- Enable GitLab Import/Export for non-admin users. - Enable GitLab Import/Export for non-admin users.
- Preserve label filters when sorting !6136 (Joseph Frazier) - Preserve label filters when sorting !6136 (Joseph Frazier)
- MergeRequest#new form load diff asynchronously
- Only update issuable labels if they have been changed - Only update issuable labels if they have been changed
- Take filters in account in issuable counters. !6496 - Take filters in account in issuable counters. !6496
- Use custom Ruby images to test builds (registry.dev.gitlab.org/gitlab/gitlab-build-images:*) - Use custom Ruby images to test builds (registry.dev.gitlab.org/gitlab/gitlab-build-images:*)
......
...@@ -38,6 +38,11 @@ ...@@ -38,6 +38,11 @@
gl.utils.getPagePath = function() { gl.utils.getPagePath = function() {
return $('body').data('page').split(':')[0]; return $('body').data('page').split(':')[0];
}; };
gl.utils.parseUrl = function (url) {
var parser = document.createElement('a');
parser.href = url;
return parser;
};
return jQuery.timefor = function(time, suffix, expiredLabel) { return jQuery.timefor = function(time, suffix, expiredLabel) {
var suffixFromNow, timefor; var suffixFromNow, timefor;
if (!time) { if (!time) {
......
...@@ -61,6 +61,9 @@ ...@@ -61,6 +61,9 @@
function MergeRequestTabs(opts) { function MergeRequestTabs(opts) {
this.opts = opts != null ? opts : {}; this.opts = opts != null ? opts : {};
this.opts.setUrl = this.opts.setUrl !== undefined ? this.opts.setUrl : true; this.opts.setUrl = this.opts.setUrl !== undefined ? this.opts.setUrl : true;
this.buildsLoaded = this.opts.buildsLoaded || false;
this.setCurrentAction = bind(this.setCurrentAction, this); this.setCurrentAction = bind(this.setCurrentAction, this);
this.tabShown = bind(this.tabShown, this); this.tabShown = bind(this.tabShown, this);
this.showTab = bind(this.showTab, this); this.showTab = bind(this.showTab, this);
...@@ -93,7 +96,7 @@ ...@@ -93,7 +96,7 @@
this.loadCommits($target.attr('href')); this.loadCommits($target.attr('href'));
this.expandView(); this.expandView();
this.resetViewContainer(); this.resetViewContainer();
} else if (action === 'diffs') { } else if (this.isDiffAction(action)) {
this.loadDiff($target.attr('href')); this.loadDiff($target.attr('href'));
if ((typeof bp !== "undefined" && bp !== null) && bp.getBreakpointSize() !== 'lg') { if ((typeof bp !== "undefined" && bp !== null) && bp.getBreakpointSize() !== 'lg') {
this.shrinkView(); this.shrinkView();
...@@ -170,8 +173,9 @@ ...@@ -170,8 +173,9 @@
action = 'notes'; action = 'notes';
} }
this.currentAction = action; this.currentAction = action;
// Remove a trailing '/commits' or '/diffs' // Remove a trailing '/commits' '/diffs' '/builds' '/pipelines' '/new' '/new/diffs'
new_state = this._location.pathname.replace(/\/(commits|diffs|builds|pipelines)(\.html)?\/?$/, ''); new_state = this._location.pathname.replace(/\/(commits|diffs|builds|pipelines|new|new\/diffs)(\.html)?\/?$/, '');
// Append the new action if we're on a tab other than 'notes' // Append the new action if we're on a tab other than 'notes'
if (action !== 'notes') { if (action !== 'notes') {
new_state += "/" + action; new_state += "/" + action;
...@@ -210,8 +214,13 @@ ...@@ -210,8 +214,13 @@
if (this.diffsLoaded) { if (this.diffsLoaded) {
return; return;
} }
// We extract pathname for the current Changes tab anchor href
// some pages like MergeRequestsController#new has query parameters on that anchor
var url = gl.utils.parseUrl(source);
return this._get({ return this._get({
url: (source + ".json") + this._location.search, url: (url.pathname + ".json") + this._location.search,
success: (function(_this) { success: (function(_this) {
return function(data) { return function(data) {
$('#diffs').html(data.html); $('#diffs').html(data.html);
...@@ -223,7 +232,7 @@ ...@@ -223,7 +232,7 @@
gl.utils.localTimeAgo($('.js-timeago', 'div#diffs')); gl.utils.localTimeAgo($('.js-timeago', 'div#diffs'));
$('#diffs .js-syntax-highlight').syntaxHighlight(); $('#diffs .js-syntax-highlight').syntaxHighlight();
$('#diffs .diff-file').singleFileDiff(); $('#diffs .diff-file').singleFileDiff();
if (_this.diffViewType() === 'parallel' && _this.currentAction === 'diffs') { if (_this.diffViewType() === 'parallel' && (_this.isDiffAction(_this.currentAction)) ) {
_this.expandViewContainer(); _this.expandViewContainer();
} }
_this.diffsLoaded = true; _this.diffsLoaded = true;
...@@ -324,6 +333,10 @@ ...@@ -324,6 +333,10 @@
return $('.inline-parallel-buttons a.active').data('view-type'); return $('.inline-parallel-buttons a.active').data('view-type');
}; };
MergeRequestTabs.prototype.isDiffAction = function(action) {
return action === 'diffs' || action === 'new/diffs'
};
MergeRequestTabs.prototype.expandViewContainer = function() { MergeRequestTabs.prototype.expandViewContainer = function() {
var $wrapper = $('.content-wrapper .container-fluid'); var $wrapper = $('.content-wrapper .container-fluid');
if (this.fixedLayoutPref === null) { if (this.fixedLayoutPref === null) {
......
...@@ -19,6 +19,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -19,6 +19,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
before_action :define_diff_comment_vars, only: [:diffs] before_action :define_diff_comment_vars, only: [:diffs]
before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :pipelines] before_action :ensure_ref_fetched, only: [:show, :diffs, :commits, :builds, :conflicts, :pipelines]
before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines] before_action :close_merge_request_without_source_project, only: [:show, :diffs, :commits, :builds, :pipelines]
before_action :apply_diff_view_cookie!, only: [:new_diffs]
before_action :build_merge_request, only: [:new, :new_diffs]
# Allow read any merge_request # Allow read any merge_request
before_action :authorize_read_merge_request! before_action :authorize_read_merge_request!
...@@ -210,29 +212,26 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -210,29 +212,26 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end end
def new def new
apply_diff_view_cookie! define_new_vars
end
build_merge_request
@noteable = @merge_request
@target_branches = if @merge_request.target_project
@merge_request.target_project.repository.branch_names
else
[]
end
@target_project = merge_request.target_project
@source_project = merge_request.source_project
@commits = @merge_request.compare_commits.reverse
@commit = @merge_request.diff_head_commit
@base_commit = @merge_request.diff_base_commit
@diffs = @merge_request.diffs(diff_options) if @merge_request.compare
@diff_notes_disabled = true
@pipeline = @merge_request.pipeline
@statuses = @pipeline.statuses.relevant if @pipeline
@note_counts = Note.where(commit_id: @commits.map(&:id)). def new_diffs
group(:commit_id).count respond_to do |format|
format.html do
define_new_vars
render "new"
end
format.json do
@diffs = if @merge_request.can_be_created
@merge_request.diffs(diff_options)
else
[]
end
@diff_notes_disabled = true
render json: { html: view_to_html_string('projects/merge_requests/_new_diffs', diffs: @diffs) }
end
end
end end
def create def create
...@@ -490,6 +489,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -490,6 +489,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController
) )
end end
def define_new_vars
@noteable = @merge_request
@target_branches = if @merge_request.target_project
@merge_request.target_project.repository.branch_names
else
[]
end
@target_project = merge_request.target_project
@source_project = merge_request.source_project
@commits = @merge_request.compare_commits.reverse
@commit = @merge_request.diff_head_commit
@base_commit = @merge_request.diff_base_commit
@pipeline = @merge_request.pipeline
@statuses = @pipeline.statuses.relevant if @pipeline
@note_counts = Note.where(commit_id: @commits.map(&:id)).
group(:commit_id).count
end
def invalid_mr def invalid_mr
# Render special view for MR with removed target branch # Render special view for MR with removed target branch
render 'invalid' render 'invalid'
...@@ -521,7 +541,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController ...@@ -521,7 +541,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def build_merge_request def build_merge_request
params[:merge_request] ||= ActionController::Parameters.new(source_project: @project) params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
@merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params.merge(diff_options: diff_options)).execute
end end
def compared_diff_version def compared_diff_version
......
...@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base
# Temporary fields to store compare vars # Temporary fields to store compare vars
# when creating new merge request # when creating new merge request
attr_accessor :can_be_created, :compare_commits, :compare attr_accessor :can_be_created, :compare_commits, :diff_options, :compare
state_machine :state, initial: :opened do state_machine :state, initial: :opened do
event :close do event :close do
...@@ -196,7 +196,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -196,7 +196,7 @@ class MergeRequest < ActiveRecord::Base
end end
def diff_size def diff_size
merge_request_diff.size diffs(diff_options).size
end end
def diff_base_commit def diff_base_commit
......
- show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true) - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
- can_create_note = !@diff_notes_disabled && can?(current_user, :create_note, diffs.project)
- diff_files = diffs.diff_files - diff_files = diffs.diff_files
.content-block.oneline-block.files-changed .content-block.oneline-block.files-changed
...@@ -20,7 +21,7 @@ ...@@ -20,7 +21,7 @@
- if diff_files.overflow? - if diff_files.overflow?
= render 'projects/diffs/warning', diff_files: diff_files = render 'projects/diffs/warning', diff_files: diff_files
.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, diffs.project))}} .files{ data: { can_create_note: can_create_note } }
- diff_files.each_with_index do |diff_file, index| - diff_files.each_with_index do |diff_file, index|
- diff_commit = commit_for_diff(diff_file) - diff_commit = commit_for_diff(diff_file)
- blob = diff_file.blob(diff_commit) - blob = diff_file.blob(diff_commit)
......
= render "projects/diffs/diffs", diffs: @diffs, show_whitespace_toggle: false
...@@ -19,34 +19,32 @@ ...@@ -19,34 +19,32 @@
.mr-compare.merge-request .mr-compare.merge-request
%ul.merge-request-tabs.nav-links.no-top.no-bottom %ul.merge-request-tabs.nav-links.no-top.no-bottom
%li.commits-tab %li.commits-tab.active
= link_to url_for(params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do = link_to url_for(params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do
Commits Commits
%span.badge= @commits.size %span.badge= @commits.size
- if @pipeline - if @pipeline
%li.builds-tab.active %li.builds-tab
= link_to url_for(params), data: {target: 'div#builds', action: 'builds', toggle: 'tab'} do = link_to url_for(params), data: {target: 'div#builds', action: 'builds', toggle: 'tab'} do
Builds Builds
%span.badge= @statuses.size %span.badge= @statuses.size
%li.diffs-tab.active %li.diffs-tab
= link_to url_for(params), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do = link_to url_for(params.merge(action: 'new_diffs')), data: {target: 'div#diffs', action: 'new/diffs', toggle: 'tab'} do
Changes Changes
%span.badge= @diffs.real_size %span.badge= @merge_request.diff_size
.tab-content .tab-content
#commits.commits.tab-pane #commits.commits.tab-pane.active
= render "projects/merge_requests/show/commits" = render "projects/merge_requests/show/commits"
#diffs.diffs.tab-pane.active #diffs.diffs.tab-pane
- if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE - # This tab is always loaded via AJAX
.alert.alert-danger
%h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
%p To preserve performance the line changes are not shown.
- else
= render "projects/diffs/diffs", diffs: @diffs, show_whitespace_toggle: false
- if @pipeline - if @pipeline
#builds.builds.tab-pane #builds.builds.tab-pane
= render "projects/merge_requests/show/builds" = render "projects/merge_requests/show/builds"
.mr-loading-status
= spinner
:javascript :javascript
$('.assign-to-me-link').on('click', function(e){ $('.assign-to-me-link').on('click', function(e){
$('#merge_request_assignee_id').val("#{current_user.id}").trigger("change"); $('#merge_request_assignee_id').val("#{current_user.id}").trigger("change");
...@@ -54,6 +52,6 @@ ...@@ -54,6 +52,6 @@
}); });
:javascript :javascript
var merge_request = new MergeRequest({ var merge_request = new MergeRequest({
action: "#{(@show_changes_tab ? 'diffs' : 'new')}", action: "#{(@show_changes_tab ? 'new/diffs' : 'new')}",
setUrl: false buildsLoaded: "#{@pipeline ? 'true' : 'false'}"
}); });
...@@ -285,6 +285,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only: ...@@ -285,6 +285,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
get :update_branches get :update_branches
get :diff_for_path get :diff_for_path
post :bulk_update post :bulk_update
get :new_diffs, path: 'new/diffs'
end end
resources :discussions, only: [], constraints: { id: /\h{40}/ } do resources :discussions, only: [], constraints: { id: /\h{40}/ } do
......
...@@ -71,6 +71,7 @@ Feature: Project Source Browse Files ...@@ -71,6 +71,7 @@ Feature: Project Source Browse Files
And I fill the new branch name And I fill the new branch name
And I click on "Commit Changes" And I click on "Commit Changes"
Then I am redirected to the new merge request page Then I am redirected to the new merge request page
When I click on "Changes" tab
And I should see its new content And I should see its new content
@javascript @javascript
...@@ -80,9 +81,10 @@ Feature: Project Source Browse Files ...@@ -80,9 +81,10 @@ Feature: Project Source Browse Files
And I fill the upload file commit message And I fill the upload file commit message
And I fill the new branch name And I fill the new branch name
And I click on "Upload file" And I click on "Upload file"
Then I can see the new text file Then I can see the new commit message
And I am redirected to the new merge request page And I am redirected to the new merge request page
And I can see the new commit message When I click on "Changes" tab
Then I can see the new text file
@javascript @javascript
Scenario: I can upload file and commit when I don't have write access Scenario: I can upload file and commit when I don't have write access
...@@ -93,9 +95,10 @@ Feature: Project Source Browse Files ...@@ -93,9 +95,10 @@ Feature: Project Source Browse Files
And I upload a new text file And I upload a new text file
And I fill the upload file commit message And I fill the upload file commit message
And I click on "Upload file" And I click on "Upload file"
Then I can see the new text file Then I can see the new commit message
And I am redirected to the fork's new merge request page And I am redirected to the fork's new merge request page
And I can see the new commit message When I click on "Changes" tab
Then I can see the new text file
@javascript @javascript
Scenario: I can replace file and commit Scenario: I can replace file and commit
...@@ -119,9 +122,10 @@ Feature: Project Source Browse Files ...@@ -119,9 +122,10 @@ Feature: Project Source Browse Files
And I replace it with a text file And I replace it with a text file
And I fill the replace file commit message And I fill the replace file commit message
And I click on "Replace file" And I click on "Replace file"
Then I can see the new text file
And I am redirected to the fork's new merge request page
And I can see the replacement commit message And I can see the replacement commit message
And I am redirected to the fork's new merge request page
When I click on "Changes" tab
Then I can see the new text file
@javascript @javascript
Scenario: If I enter an illegal file name I see an error message Scenario: If I enter an illegal file name I see an error message
...@@ -191,6 +195,7 @@ Feature: Project Source Browse Files ...@@ -191,6 +195,7 @@ Feature: Project Source Browse Files
And I fill the new branch name And I fill the new branch name
And I click on "Commit Changes" And I click on "Commit Changes"
Then I am redirected to the new merge request page Then I am redirected to the new merge request page
Then I click on "Changes" tab
And I should see its new content And I should see its new content
@javascript @wip @javascript @wip
......
...@@ -105,6 +105,10 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps ...@@ -105,6 +105,10 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
click_button 'Commit Changes' click_button 'Commit Changes'
end end
step 'I click on "Changes" tab' do
click_link 'Changes'
end
step 'I click on "Create directory"' do step 'I click on "Create directory"' do
click_button 'Create directory' click_button 'Create directory'
end end
......
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