Merge branch 'rs-persist-tab-selection-more-betterer' into 'master'

Improve MergeRequest tab-persisting behavior

Now uses the path instead of the hash.

See discussion in !728

See merge request !737

Gemfile

@@ -186,7 +186,7 @@ gem 'charlock_holmes'
 gem "sass-rails", '~> 4.0.2'
 gem "coffee-rails"
 gem "uglifier"
-gem 'turbolinks'
+gem 'turbolinks', '~> 2.5.0'
 gem 'jquery-turbolinks'
 
 gem 'select2-rails'

Gemfile.lock

@@ -101,13 +101,13 @@ GEM
     coderay (1.1.0)
     coercible (1.0.0)
       descendants_tracker (~> 0.0.1)
-    coffee-rails (4.0.1)
+    coffee-rails (4.1.0)
       coffee-script (>= 2.2.0)
       railties (>= 4.0.0, < 5.0)
-    coffee-script (2.2.0)
+    coffee-script (2.4.1)
       coffee-script-source
       execjs
-    coffee-script-source (1.6.3)
+    coffee-script-source (1.9.1.1)
     colored (1.2)
     colorize (0.5.8)
     columnize (0.9.0)
@@ -418,7 +418,7 @@ GEM
     quiet_assets (1.0.2)
       railties (>= 3.1, < 5.0)
     racc (1.4.10)
-    rack (1.5.2)
+    rack (1.5.3)
     rack-accept (0.4.5)
       rack (>= 0.4)
     rack-attack (4.3.0)
@@ -633,7 +633,7 @@ GEM
       multi_json (~> 1.7)
       twitter-stream (~> 0.1)
     tins (0.13.1)
-    turbolinks (2.0.0)
+    turbolinks (2.5.3)
       coffee-rails
     twitter-stream (0.1.16)
       eventmachine (>= 0.12.8)
@@ -806,7 +806,7 @@ DEPENDENCIES
   test_after_commit
   thin
   tinder (~> 1.9.2)
-  turbolinks
+  turbolinks (~> 2.5.0)
   uglifier
   underscore-rails (~> 1.4.4)
   unf

app/assets/javascripts/merge_request.js.coffee

@@ -26,7 +26,7 @@ class @MergeRequest
     @commits_loaded = @opts.commits_loaded or false
 
     this.bindEvents()
-    this.activateTabFromHash()
+    this.activateTabFromPath()
 
     this.initMergeWidget()
     this.$('.show-all-commits').on 'click', =>
@@ -82,19 +82,16 @@ class @MergeRequest
   bindEvents: ->
     this.$('.merge-request-tabs a[data-toggle="tab"]').on 'shown.bs.tab', (e) =>
       $target = $(e.target)
-
-      # Nothing else to be done if we're on the first tab
-      return if $target.data('action') == 'notes'
-
-      # Persist current tab selection via URL
-      href = $target.attr('href')
-      if href.substr(0,1) == '#'
-        location.replace("#!#{href.substr(1)}")
+      tab_action = $target.data('action')
 
       # Lazy-load diffs
-      if $target.data('action') == 'diffs'
+      if tab_action == 'diffs'
         this.loadDiff() unless @diffs_loaded
-        $('.diff-header').trigger("sticky_kit:recalc")
+        $('.diff-header').trigger('sticky_kit:recalc')
+
+      # Skip tab-persisting behavior on MergeRequests#new
+      unless @opts.action == 'new'
+        @setCurrentAction(tab_action)
 
     this.$('.accept_merge_request').on 'click', ->
       $('.automerge_widget.can_be_merged').hide()
@@ -112,27 +109,51 @@ class @MergeRequest
       this.$('.remove_source_branch_in_progress').hide()
       this.$('.remove_source_branch_widget.failed').show()
 
-  # Activates a tab section based on the `#!` URL hash
+  # Activate a tab based on the current URL path
+  #
+  # If the current action is 'show' or 'new' (i.e., initial page load),
+  # activates the first tab, otherwise activates the tab corresponding to the
+  # current action (diffs, commits).
+  activateTabFromPath: ->
+    if @opts.action == 'show' || @opts.action == 'new'
+      this.$('.merge-request-tabs a[data-toggle="tab"]:first').tab('show')
+    else
+      this.$(".merge-request-tabs a[data-action='#{@opts.action}']").tab('show')
+
+  # Replaces the current Merge Request-specific action in the URL with a new one
   #
-  # If no hash value is present (i.e., on the initial page load), the first tab
-  # is selected by default.
+  # If the action is "notes", the URL is reset to the standard
+  # `MergeRequests#show` route.
   #
-  # ... unless the current controller action is `diffs`, in which case that tab
-  # is selected instead. Fun, right?
+  # Examples:
   #
-  # Note: We use a `#!` instead of a standard URL hash for two reasons:
+  #   location.pathname # => "/namespace/project/merge_requests/1"
+  #   setCurrentAction('diffs')
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
   #
-  # 1. Prevents the hash acting like an anchor and scrolling the page.
-  # 2. Prevents mutating browser history.
-  activateTabFromHash: ->
-    # Correct the hash if we came here directly via the `/diffs` path
-    if location.hash == '' and @opts.action == 'diffs'
-      location.replace('#!diffs')
-
-    if location.hash == ''
-      this.$('.merge-request-tabs a[data-toggle="tab"]:first').tab('show')
-    else if location.hash.substr(0,2) == '#!'
-      this.$(".merge-request-tabs a[href='##{location.hash.substr(2)}']").tab("show")
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
+  #   setCurrentAction('notes')
+  #   location.pathname # => "/namespace/project/merge_requests/1"
+  #
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
+  #   setCurrentAction('commits')
+  #   location.pathname # => "/namespace/project/merge_requests/1/commits"
+  setCurrentAction: (action) ->
+    # Normalize action, just to be safe
+    action = 'notes' if action == 'show'
+
+    # Remove a trailing '/commits' or '/diffs'
+    new_state = location.pathname.replace(/\/(commits|diffs)\/?$/, '')
+
+    # Append the new action if we're on a tab other than 'notes'
+    unless action == 'notes'
+      new_state += "/#{action}"
+
+    # Replace the current history state with the new one without breaking
+    # Turbolinks' history.
+    #
+    # See https://github.com/rails/turbolinks/issues/363
+    history.replaceState {turbolinks: true, url: new_state}, '', new_state
 
   showState: (state) ->
     $('.automerge_widget').hide()
@@ -161,7 +182,7 @@ class @MergeRequest
   loadDiff: (event) ->
     $.ajax
       type: 'GET'
-      url: this.$('.merge-request-tabs .diffs-tab a').data('source') + ".json"
+      url: this.$('.merge-request-tabs .diffs-tab a').attr('href') + ".json"
       beforeSend: =>
         this.$('.mr-loading-status .loading').show()
       complete: =>

app/controllers/projects/merge_requests_controller.rb

@@ -2,10 +2,13 @@ require 'gitlab/satellite/satellite'
 
 class Projects::MergeRequestsController < Projects::ApplicationController
   before_action :module_enabled
-  before_action :merge_request, only: [:edit, :update, :show, :diffs, :automerge, :automerge_check, :ci_status, :toggle_subscription]
-  before_action :closes_issues, only: [:edit, :update, :show, :diffs]
-  before_action :validates_merge_request, only: [:show, :diffs]
-  before_action :define_show_vars, only: [:show, :diffs]
+  before_action :merge_request, only: [
+    :edit, :update, :show, :diffs, :commits, :automerge, :automerge_check,
+    :ci_status, :toggle_subscription
+  ]
+  before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits]
+  before_action :validates_merge_request, only: [:show, :diffs, :commits]
+  before_action :define_show_vars, only: [:show, :diffs, :commits]
 
   # Allow read any merge_request
   before_action :authorize_read_merge_request!
@@ -67,6 +70,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     end
   end
 
+  def commits
+    render 'show'
+  end
+
   def new
     params[:merge_request] ||= ActionController::Parameters.new(source_project: @project)
     @merge_request = MergeRequests::BuildService.new(project, current_user, merge_request_params).execute

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

@@ -20,12 +20,12 @@
 .mr-compare.merge-request
   %ul.nav.nav-tabs.merge-request-tabs
     %li.commits-tab
-      = link_to '#commits', data: {action: 'commits', toggle: 'tab'} do
+      = link_to url_for(params), data: {target: '#commits', action: 'commits', toggle: 'tab'} do
         = icon('history')
         Commits
         %span.badge= @commits.size
     %li.diffs-tab
-      = link_to '#diffs', data: {action: 'diffs', toggle: 'tab'} do
+      = link_to url_for(params), data: {target: '#diffs', action: 'diffs', toggle: 'tab'} do
         = icon('list-alt')
         Changes
         %span.badge= @diffs.size
@@ -56,7 +56,7 @@
 :javascript
   var merge_request
   merge_request = new MergeRequest({
-    action: 'diffs',
+    action: 'new',
     diffs_loaded: true,
     commits_loaded: true
   });

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

@@ -38,17 +38,17 @@
   - if @commits.present?
     %ul.nav.nav-tabs.merge-request-tabs
       %li.notes-tab
-        = link_to '#notes', data: {action: 'notes', toggle: 'tab'} do
+        = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#notes', action: 'notes', toggle: 'tab'} do
           = icon('comments')
           Discussion
           %span.badge= @merge_request.mr_and_commit_notes.user.count
       %li.commits-tab
-        = link_to '#commits', data: {action: 'commits', toggle: 'tab'} do
+        = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#commits', action: 'commits', toggle: 'tab'} do
           = icon('history')
           Commits
           %span.badge= @commits.size
       %li.diffs-tab
-        = link_to '#diffs', data: {source: diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), action: 'diffs', toggle: 'tab'} do
+        = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#diffs', action: 'diffs', toggle: 'tab'} do
           = icon('list-alt')
           Changes
           %span.badge= @merge_request.diffs.size

config/routes.rb

@@ -450,6 +450,7 @@ Gitlab::Application.routes.draw do
         resources :merge_requests, constraints: { id: /\d+/ }, except: [:destroy] do
           member do
             get :diffs
+            get :commits
             post :automerge
             get :automerge_check
             get :ci_status

spec/routing/project_routing_spec.rb

@@ -208,23 +208,31 @@ describe Projects::RefsController, 'routing' do
   end
 end
 
-#           diffs_project_merge_request GET    /:project_id/merge_requests/:id/diffs(.:format)           projects/merge_requests#diffs
-#       automerge_project_merge_request POST   /:project_id/merge_requests/:id/automerge(.:format)       projects/merge_requests#automerge
-# automerge_check_project_merge_request GET    /:project_id/merge_requests/:id/automerge_check(.:format) projects/merge_requests#automerge_check
-#    branch_from_project_merge_requests GET    /:project_id/merge_requests/branch_from(.:format)         projects/merge_requests#branch_from
-#      branch_to_project_merge_requests GET    /:project_id/merge_requests/branch_to(.:format)           projects/merge_requests#branch_to
-#                project_merge_requests GET    /:project_id/merge_requests(.:format)                     projects/merge_requests#index
-#                                       POST   /:project_id/merge_requests(.:format)                     projects/merge_requests#create
-#             new_project_merge_request GET    /:project_id/merge_requests/new(.:format)                 projects/merge_requests#new
-#            edit_project_merge_request GET    /:project_id/merge_requests/:id/edit(.:format)            projects/merge_requests#edit
-#                 project_merge_request GET    /:project_id/merge_requests/:id(.:format)                 projects/merge_requests#show
-#                                       PUT    /:project_id/merge_requests/:id(.:format)                 projects/merge_requests#update
-#                                       DELETE /:project_id/merge_requests/:id(.:format)                 projects/merge_requests#destroy
+#               diffs_namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/:id/diffs(.:format)               projects/merge_requests#diffs
+#             commits_namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/:id/commits(.:format)             projects/merge_requests#commits
+#           automerge_namespace_project_merge_request POST     /:namespace_id/:project_id/merge_requests/:id/automerge(.:format)           projects/merge_requests#automerge
+#     automerge_check_namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/:id/automerge_check(.:format)     projects/merge_requests#automerge_check
+#           ci_status_namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/:id/ci_status(.:format)           projects/merge_requests#ci_status
+# toggle_subscription_namespace_project_merge_request POST     /:namespace_id/:project_id/merge_requests/:id/toggle_subscription(.:format) projects/merge_requests#toggle_subscription
+#        branch_from_namespace_project_merge_requests GET      /:namespace_id/:project_id/merge_requests/branch_from(.:format)             projects/merge_requests#branch_from
+#          branch_to_namespace_project_merge_requests GET      /:namespace_id/:project_id/merge_requests/branch_to(.:format)               projects/merge_requests#branch_to
+#    update_branches_namespace_project_merge_requests GET      /:namespace_id/:project_id/merge_requests/update_branches(.:format)         projects/merge_requests#update_branches
+#                    namespace_project_merge_requests GET      /:namespace_id/:project_id/merge_requests(.:format)                         projects/merge_requests#index
+#                                                     POST     /:namespace_id/:project_id/merge_requests(.:format)                         projects/merge_requests#create
+#                 new_namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/new(.:format)                     projects/merge_requests#new
+#                edit_namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/:id/edit(.:format)                projects/merge_requests#edit
+#                     namespace_project_merge_request GET      /:namespace_id/:project_id/merge_requests/:id(.:format)                     projects/merge_requests#show
+#                                                     PATCH    /:namespace_id/:project_id/merge_requests/:id(.:format)                     projects/merge_requests#update
+#                                                     PUT      /:namespace_id/:project_id/merge_requests/:id(.:format)                     projects/merge_requests#update
 describe Projects::MergeRequestsController, 'routing' do
   it 'to #diffs' do
     expect(get('/gitlab/gitlabhq/merge_requests/1/diffs')).to route_to('projects/merge_requests#diffs', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1')
   end
 
+  it 'to #commits' do
+    expect(get('/gitlab/gitlabhq/merge_requests/1/commits')).to route_to('projects/merge_requests#commits', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1')
+  end
+
   it 'to #automerge' do
     expect(post('/gitlab/gitlabhq/merge_requests/1/automerge')).to route_to(
       'projects/merge_requests#automerge',