Merge branch 'new-merge-requests-commit-tab-active' into 'master'

MergeRequest new form load diff asynchronously

## What does this MR do?

Don't return diff tab content from the server when request for a new merge request

## Are there points in the code the reviewer needs to double check?

## Why was this MR needed?

To avoid 502 errors due to this controller action takes to much time

## What are the relevant issue numbers?

Relates #13912

## Screenshots (if relevant)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- ~~[ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- ~~[ ] API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5844

CHANGELOG

@@ -33,6 +33,7 @@ v 8.13.0 (unreleased)
   - Replace `alias_method_chain` with `Module#prepend`
   - Enable GitLab Import/Export for non-admin users.
   - Preserve label filters when sorting !6136 (Joseph Frazier)
+  - MergeRequest#new form load diff asynchronously
   - Only update issuable labels if they have been changed
   - Take filters in account in issuable counters. !6496
   - Use custom Ruby images to test builds (registry.dev.gitlab.org/gitlab/gitlab-build-images:*)

app/assets/javascripts/lib/utils/common_utils.js

@@ -38,6 +38,11 @@
     gl.utils.getPagePath = function() {
       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) {
       var suffixFromNow, timefor;
       if (!time) {

app/assets/javascripts/merge_request_tabs.js

@@ -61,6 +61,9 @@
     function MergeRequestTabs(opts) {
       this.opts = opts != null ? opts : {};
       this.opts.setUrl = this.opts.setUrl !== undefined ? this.opts.setUrl : true;
+
+      this.buildsLoaded = this.opts.buildsLoaded || false;
+
       this.setCurrentAction = bind(this.setCurrentAction, this);
       this.tabShown = bind(this.tabShown, this);
       this.showTab = bind(this.showTab, this);
@@ -93,7 +96,7 @@
         this.loadCommits($target.attr('href'));
         this.expandView();
         this.resetViewContainer();
-      } else if (action === 'diffs') {
+      } else if (this.isDiffAction(action)) {
         this.loadDiff($target.attr('href'));
         if ((typeof bp !== "undefined" && bp !== null) && bp.getBreakpointSize() !== 'lg') {
           this.shrinkView();
@@ -170,8 +173,9 @@
         action = 'notes';
       }
       this.currentAction = action;
-      // Remove a trailing '/commits' or '/diffs'
-      new_state = this._location.pathname.replace(/\/(commits|diffs|builds|pipelines)(\.html)?\/?$/, '');
+      // Remove a trailing '/commits' '/diffs' '/builds' '/pipelines' '/new' '/new/diffs'
+      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'
       if (action !== 'notes') {
         new_state += "/" + action;
@@ -210,8 +214,13 @@
       if (this.diffsLoaded) {
         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({
-        url: (source + ".json") + this._location.search,
+        url: (url.pathname + ".json") + this._location.search,
         success: (function(_this) {
           return function(data) {
             $('#diffs').html(data.html);
@@ -223,7 +232,7 @@
             gl.utils.localTimeAgo($('.js-timeago', 'div#diffs'));
             $('#diffs .js-syntax-highlight').syntaxHighlight();
             $('#diffs .diff-file').singleFileDiff();
-            if (_this.diffViewType() === 'parallel' && _this.currentAction === 'diffs') {
+            if (_this.diffViewType() === 'parallel' && (_this.isDiffAction(_this.currentAction)) ) {
               _this.expandViewContainer();
             }
             _this.diffsLoaded = true;
@@ -324,6 +333,10 @@
       return $('.inline-parallel-buttons a.active').data('view-type');
     };
 
+    MergeRequestTabs.prototype.isDiffAction = function(action) {
+      return action === 'diffs' || action === 'new/diffs'
+    };
+
     MergeRequestTabs.prototype.expandViewContainer = function() {
       var $wrapper = $('.content-wrapper .container-fluid');
       if (this.fixedLayoutPref === null) {

app/controllers/projects/merge_requests_controller.rb

@@ -19,6 +19,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :define_diff_comment_vars, only: [:diffs]
   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 :apply_diff_view_cookie!, only: [:new_diffs]
+  before_action :build_merge_request, only: [:new, :new_diffs]
 
   # Allow read any merge_request
   before_action :authorize_read_merge_request!
@@ -210,29 +212,26 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def new
-    apply_diff_view_cookie!
-
-    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
+    define_new_vars
+  end
 
-    @note_counts = Note.where(commit_id: @commits.map(&:id)).
-      group(:commit_id).count
+  def new_diffs
+    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
 
   def create
@@ -490,6 +489,27 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     )
   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
     # Render special view for MR with removed target branch
     render 'invalid'
@@ -521,7 +541,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def build_merge_request
     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
 
   def compared_diff_version

app/models/merge_request.rb

@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base
 
   # Temporary fields to store compare vars
   # 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
     event :close do
@@ -196,7 +196,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def diff_size
-    merge_request_diff.size
+    diffs(diff_options).size
   end
 
   def diff_base_commit

app/views/projects/diffs/_diffs.html.haml

 - 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
 
 .content-block.oneline-block.files-changed
@@ -20,7 +21,7 @@
 - if diff_files.overflow?
   = 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_commit = commit_for_diff(diff_file)
     - blob = diff_file.blob(diff_commit)

app/views/projects/merge_requests/_new_diffs.html.haml

+= render "projects/diffs/diffs", diffs: @diffs, show_whitespace_toggle: false

app/views/projects/merge_requests/_new_submit.html.haml

@@ -19,34 +19,32 @@
 
 .mr-compare.merge-request
   %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
         Commits
         %span.badge= @commits.size
     - if @pipeline
-      %li.builds-tab.active
+      %li.builds-tab
         = link_to url_for(params), data: {target: 'div#builds', action: 'builds', toggle: 'tab'} do
           Builds
           %span.badge= @statuses.size
-    %li.diffs-tab.active
-      = link_to url_for(params), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
+    %li.diffs-tab
+      = link_to url_for(params.merge(action: 'new_diffs')), data: {target: 'div#diffs', action: 'new/diffs', toggle: 'tab'} do
         Changes
-        %span.badge= @diffs.real_size
+        %span.badge= @merge_request.diff_size
 
   .tab-content
-    #commits.commits.tab-pane
+    #commits.commits.tab-pane.active
       = render "projects/merge_requests/show/commits"
-    #diffs.diffs.tab-pane.active
-      - if @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
-        .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
+    #diffs.diffs.tab-pane
+      - # This tab is always loaded via AJAX
     - if @pipeline
       #builds.builds.tab-pane
         = render "projects/merge_requests/show/builds"
 
+  .mr-loading-status
+    = spinner
+
 :javascript
   $('.assign-to-me-link').on('click', function(e){
     $('#merge_request_assignee_id').val("#{current_user.id}").trigger("change");
@@ -54,6 +52,6 @@
   });
 :javascript
   var merge_request = new MergeRequest({
-    action: "#{(@show_changes_tab ? 'diffs' : 'new')}",
-    setUrl: false
+    action: "#{(@show_changes_tab ? 'new/diffs' : 'new')}",
+    buildsLoaded: "#{@pipeline ? 'true' : 'false'}"
   });

config/routes/project.rb

@@ -285,6 +285,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only:
           get :update_branches
           get :diff_for_path
           post :bulk_update
+          get :new_diffs, path: 'new/diffs'
         end
 
         resources :discussions, only: [], constraints: { id: /\h{40}/ } do

features/project/source/browse_files.feature

@@ -71,6 +71,7 @@ Feature: Project Source Browse Files
     And I fill the new branch name
     And I click on "Commit Changes"
     Then I am redirected to the new merge request page
+    When I click on "Changes" tab
     And I should see its new content
 
   @javascript
@@ -80,9 +81,10 @@ Feature: Project Source Browse Files
     And I fill the upload file commit message
     And I fill the new branch name
     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 can see the new commit message
+    When I click on "Changes" tab
+    Then I can see the new text file
 
   @javascript
   Scenario: I can upload file and commit when I don't have write access
@@ -93,9 +95,10 @@ Feature: Project Source Browse Files
     And I upload a new text file
     And I fill the upload file commit message
     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 can see the new commit message
+    When I click on "Changes" tab
+    Then I can see the new text file
 
   @javascript
   Scenario: I can replace file and commit
@@ -119,9 +122,10 @@ Feature: Project Source Browse Files
     And I replace it with a text file
     And I fill the replace file commit message
     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 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
   Scenario: If I enter an illegal file name I see an error message
@@ -191,6 +195,7 @@ Feature: Project Source Browse Files
     And I fill the new branch name
     And I click on "Commit Changes"
     Then I am redirected to the new merge request page
+    Then I click on "Changes" tab
     And I should see its new content
 
   @javascript  @wip

features/steps/project/source/browse_files.rb

@@ -105,6 +105,10 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
     click_button 'Commit Changes'
   end
 
+  step 'I click on "Changes" tab' do
+    click_link 'Changes'
+  end
+
   step 'I click on "Create directory"' do
     click_button 'Create directory'
   end