Merge branch 'rs-dev-issue-2355' into 'master'

MergeRequest#show performance improvements

This is a first pass on improving the performance of the `MergeRequests#show` page. Notable changes:

- The "Commits" tab is loaded lazily, so the initial page load should be much faster for MRs with many commits.
- Relative timestamps via `timeago` are only initialized once per load instead of `O(n^2)`. This greatly improves frontend rendering times for a large number of commits.
- Refactored `User.find_for_commit` to use a single ARel-generated SQL query instead of the old method which resulted in one query, and could result in up to three.

See merge request !838

21 files changed, 470 insertions, 134 deletions

diff --git a/app/assets/javascripts/application.js.coffee b/app/assets/javascripts/application.js.coffee
index 6a3f7386d5b..c18ea929506 100644
--- a/app/assets/javascripts/application.js.coffee
+++ b/app/assets/javascripts/application.js.coffee
@@ -141,8 +141,7 @@ $ ->
   $('.trigger-submit').on 'change', ->
     $(@).parents('form').submit()
 
-  $("abbr.timeago").timeago()
-  $('.js-timeago').timeago()
+  $('abbr.timeago, .js-timeago').timeago()
 
   # Flash
   if (flash = $(".flash-container")).length > 0
diff --git a/app/assets/javascripts/merge_request.js.coffee b/app/assets/javascripts/merge_request.js.coffee
index 25a7815dba2..5c0bc686111 100644
--- a/app/assets/javascripts/merge_request.js.coffee
+++ b/app/assets/javascripts/merge_request.js.coffee
@@ -1,29 +1,25 @@
 #= require jquery.waitforimages
 #= require task_list
 
+#= require merge_request_tabs
+
 class @MergeRequest
   # Initialize MergeRequest behavior
   #
   # Options:
-  #   action         - String, current controller action
-  #   diffs_loaded   - Boolean, have diffs been pre-rendered server-side?
-  #                    (default: true if `action` is 'diffs', otherwise false)
-  #   commits_loaded - Boolean, have commits been pre-rendered server-side?
-  #                    (default: false)
+  #   action - String, current controller action
   #
   constructor: (@opts) ->
     @initContextWidget()
     this.$el = $('.merge-request')
 
-    @diffs_loaded = @opts.diffs_loaded or @opts.action == 'diffs'
-    @commits_loaded = @opts.commits_loaded or false
-
-    this.bindEvents()
-    this.activateTabFromPath()
-
     this.$('.show-all-commits').on 'click', =>
       this.showAllCommits()
 
+    # `MergeRequests#new` has no tab-persisting or lazy-loading behavior
+    unless @opts.action == 'new'
+      new MergeRequestTabs(@opts)
+
     # Prevent duplicate event bindings
     @disableTaskList()
 
@@ -52,83 +48,6 @@ class @MergeRequest
     $(".context .inline-update").on "change", "#merge_request_assignee_id", ->
       $(this).submit()
 
-
-  bindEvents: ->
-    this.$('.merge-request-tabs a[data-toggle="tab"]').on 'shown.bs.tab', (e) =>
-      $target = $(e.target)
-      tab_action = $target.data('action')
-
-      # Lazy-load diffs
-      if tab_action == 'diffs'
-        this.loadDiff() unless @diffs_loaded
-        $('.diff-header').trigger('sticky_kit:recalc')
-
-      # Skip tab-persisting behavior on MergeRequests#new
-      unless @opts.action == 'new'
-        @setCurrentAction(tab_action)
-
-  # 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 the action is "notes", the URL is reset to the standard
-  # `MergeRequests#show` route.
-  #
-  # Examples:
-  #
-  #   location.pathname # => "/namespace/project/merge_requests/1"
-  #   setCurrentAction('diffs')
-  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
-  #
-  #   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}"
-
-    # Ensure parameters and hash come along for the ride
-    new_state += location.search + location.hash
-
-    # 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
-
-  loadDiff: (event) ->
-    $.ajax
-      type: 'GET'
-      url: this.$('.merge-request-tabs .diffs-tab a').attr('href') + ".json"
-      beforeSend: =>
-        this.$('.mr-loading-status .loading').show()
-      complete: =>
-        @diffs_loaded = true
-        this.$('.mr-loading-status .loading').hide()
-      success: (data) =>
-        this.$(".diffs").html(data.html)
-      dataType: 'json'
-
   showAllCommits: ->
     this.$('.first-commits').remove()
     this.$('.all-commits').removeClass 'hide'
diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee
new file mode 100644
index 00000000000..de9a4c2cc2f
--- /dev/null
+++ b/app/assets/javascripts/merge_request_tabs.js.coffee
@@ -0,0 +1,153 @@
+# MergeRequestTabs
+#
+# Handles persisting and restoring the current tab selection and lazily-loading
+# content on the MergeRequests#show page.
+#
+# ### Example Markup
+#
+#   <ul class="nav nav-tabs merge-request-tabs">
+#     <li class="notes-tab active">
+#       <a data-action="notes" data-target="#notes" data-toggle="tab" href="/foo/bar/merge_requests/1">
+#         Discussion
+#       </a>
+#     </li>
+#     <li class="commits-tab">
+#       <a data-action="commits" data-target="#commits" data-toggle="tab" href="/foo/bar/merge_requests/1/commits">
+#         Commits
+#       </a>
+#     </li>
+#     <li class="diffs-tab">
+#       <a data-action="diffs" data-target="#diffs" data-toggle="tab" href="/foo/bar/merge_requests/1/diffs">
+#         Diffs
+#       </a>
+#     </li>
+#   </ul>
+#
+#   <div class="tab-content">
+#     <div class="notes tab-pane active" id="notes">
+#       Notes Content
+#     </div>
+#     <div class="commits tab-pane" id="commits">
+#       Commits Content
+#     </div>
+#     <div class="diffs tab-pane" id="diffs">
+#       Diffs Content
+#     </div>
+#   </div>
+#
+#   <div class="mr-loading-status">
+#     <div class="loading">
+#       Loading Animation
+#     </div>
+#   </div>
+#
+class @MergeRequestTabs
+  diffsLoaded: false
+  commitsLoaded: false
+
+  constructor: (@opts = {}) ->
+    # Store the `location` object, allowing for easier stubbing in tests
+    @_location = location
+
+    @bindEvents()
+    @activateTab(@opts.action)
+
+    switch @opts.action
+      when 'commits' then @commitsLoaded = true
+      when 'diffs'   then @diffsLoaded = true
+
+  bindEvents: ->
+    $(document).on 'shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', @tabShown
+
+  tabShown: (event) =>
+    $target = $(event.target)
+    action = $target.data('action')
+
+    if action == 'commits'
+      @loadCommits($target.attr('href'))
+    else if action == 'diffs'
+      @loadDiff($target.attr('href'))
+
+    @setCurrentAction(action)
+
+  # Activate a tab based on the current action
+  activateTab: (action) ->
+    action = 'notes' if action == 'show'
+    $(".merge-request-tabs a[data-action='#{action}']").tab('show')
+
+  # Replaces the current Merge Request-specific action in the URL with a new one
+  #
+  # If the action is "notes", the URL is reset to the standard
+  # `MergeRequests#show` route.
+  #
+  # Examples:
+  #
+  #   location.pathname # => "/namespace/project/merge_requests/1"
+  #   setCurrentAction('diffs')
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
+  #
+  #   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"
+  #
+  # Returns the new URL String
+  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}"
+
+    # Ensure parameters and hash come along for the ride
+    new_state += @_location.search + @_location.hash
+
+    # 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}, document.title, new_state
+
+    new_state
+
+  loadCommits: (source) ->
+    return if @commitsLoaded
+
+    @_get
+      url: "#{source}.json"
+      success: (data) =>
+        document.getElementById('commits').innerHTML = data.html
+        $('.js-timeago').timeago()
+        @commitsLoaded = true
+
+  loadDiff: (source) ->
+    return if @diffsLoaded
+
+    @_get
+      url: "#{source}.json"
+      success: (data) =>
+        document.getElementById('diffs').innerHTML = data.html
+        $('.diff-header').trigger('sticky_kit:recalc')
+        @diffsLoaded = true
+
+  toggleLoading: ->
+    $('.mr-loading-status .loading').toggle()
+
+  _get: (options) ->
+    defaults = {
+      beforeSend: @toggleLoading
+      complete: @toggleLoading
+      dataType: 'json'
+      type: 'GET'
+    }
+
+    options = $.extend({}, defaults, options)
+
+    $.ajax(options)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 14069bafe71..51ecbfd561a 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -71,7 +71,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def commits
-    render 'show'
+    respond_to do |format|
+      format.html { render 'show' }
+      format.json { render json: { html: view_to_html_string('projects/merge_requests/show/_commits') } }
+    end
   end
 
   def new
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 10d7aa11209..c1342331119 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -179,14 +179,33 @@ module ApplicationHelper
     BroadcastMessage.current
   end
 
-  def time_ago_with_tooltip(date, placement = 'top', html_class = 'time_ago')
-    capture_haml do
-      haml_tag :time, date.to_s,
-        class: html_class, datetime: date.getutc.iso8601, title: date.in_time_zone.stamp('Aug 21, 2011 9:23pm'),
-        data: { toggle: 'tooltip', placement: placement }
-
-      haml_tag :script, "$('." + html_class + "').timeago().tooltip()"
-    end.html_safe
+  # Render a `time` element with Javascript-based relative date and tooltip
+  #
+  # time       - Time object
+  # placement  - Tooltip placement String (default: "top")
+  # html_class - Custom class for `time` element (default: "time_ago")
+  # skip_js    - When true, exclude the `script` tag (default: false)
+  #
+  # By default also includes a `script` element with Javascript necessary to
+  # initialize the `timeago` jQuery extension. If this method is called many
+  # times, for example rendering hundreds of commits, it's advisable to disable
+  # this behavior using the `skip_js` argument and re-initializing `timeago`
+  # manually once all of the elements have been rendered.
+  #
+  # A `js-timeago` class is always added to the element, even when a custom
+  # `html_class` argument is provided.
+  #
+  # Returns an HTML-safe String
+  def time_ago_with_tooltip(time, placement: 'top', html_class: 'time_ago', skip_js: false)
+    element = content_tag :time, time.to_s,
+      class: "#{html_class} js-timeago",
+      datetime: time.getutc.iso8601,
+      title: time.in_time_zone.stamp('Aug 21, 2011 9:23pm'),
+      data: { toggle: 'tooltip', placement: placement }
+
+    element += javascript_tag "$('.js-timeago').timeago()" unless skip_js
+
+    element
   end
 
   def render_markup(file_name, file_content)
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 36d3f371c1b..d4c345fe431 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -45,13 +45,13 @@ module IssuesHelper
 
   def issue_timestamp(issue)
     # Shows the created at time and the updated at time if different
-    ts = "#{time_ago_with_tooltip(issue.created_at, 'bottom', 'note_created_ago')}"
+    ts = time_ago_with_tooltip(issue.created_at, placement: 'bottom', html_class: 'note_created_ago')
     if issue.updated_at != issue.created_at
       ts << capture_haml do
         haml_tag :span do
           haml_concat '&middot;'
           haml_concat icon('edit', title: 'edited')
-          haml_concat time_ago_with_tooltip(issue.updated_at, 'bottom', 'issue_edited_ago')
+          haml_concat time_ago_with_tooltip(issue.updated_at, placement: 'bottom', html_class: 'issue_edited_ago')
         end
       end
     end
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index a7c1fa0b071..dda9b17d61d 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -25,13 +25,13 @@ module NotesHelper
 
   def note_timestamp(note)
     # Shows the created at time and the updated at time if different
-    ts = "#{time_ago_with_tooltip(note.created_at, 'bottom', 'note_created_ago')}"
+    ts = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note_created_ago')
     if note.updated_at != note.created_at
       ts << capture_haml do
         haml_tag :span do
           haml_concat '&middot;'
           haml_concat icon('edit', title: 'edited')
-          haml_concat time_ago_with_tooltip(note.updated_at, 'bottom', 'note_edited_ago')
+          haml_concat time_ago_with_tooltip(note.updated_at, placement: 'bottom', html_class: 'note_edited_ago')
         end
       end
     end
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 94ce6646634..ec65e473919 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -211,7 +211,7 @@ module ProjectsHelper
 
   def project_last_activity(project)
     if project.last_activity_at
-      time_ago_with_tooltip(project.last_activity_at, 'bottom', 'last_activity_time_ago')
+      time_ago_with_tooltip(project.last_activity_at, placement: 'bottom', html_class: 'last_activity_time_ago')
     else
       "Never"
     end
diff --git a/app/models/user.rb b/app/models/user.rb
index a2e2d220b3a..29f43051464 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -223,10 +223,26 @@ class User < ActiveRecord::Base
     end
 
     def find_for_commit(email, name)
-      # Prefer email match over name match
-      User.where(email: email).first ||
-        User.joins(:emails).where(emails: { email: email }).first ||
-        User.where(name: name).first
+      user_table = arel_table
+      email_table = Email.arel_table
+
+      # Use ARel to build a query:
+      query = user_table.
+        # SELECT "users".* FROM "users"
+        project(user_table[Arel.star]).
+        # LEFT OUTER JOIN "emails"
+        join(email_table, Arel::Nodes::OuterJoin).
+        # ON "users"."id" = "emails"."user_id"
+        on(user_table[:id].eq(email_table[:user_id])).
+        # WHERE ("user"."email" = '<email>' OR "user"."name" = '<name>')
+        # OR "emails"."email" = '<email>'
+        where(
+          user_table[:email].eq(email).
+          or(user_table[:name].eq(name)).
+          or(email_table[:email].eq(email))
+        )
+
+      find_by_sql(query.to_sql).first
     end
 
     def filter(filter_name)
diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml
index 083fca9b658..f9106564a27 100644
--- a/app/views/projects/commits/_commit.html.haml
+++ b/app/views/projects/commits/_commit.html.haml
@@ -29,5 +29,5 @@
     = commit_author_link(commit, avatar: true, size: 24)
     authored
     .committed_ago
-      #{time_ago_with_tooltip(commit.committed_date)} &nbsp;
+      #{time_ago_with_tooltip(commit.committed_date, skip_js: true)} &nbsp;
     = link_to_browse_code(project, commit)
diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml
index 64d62b45657..2c296cab977 100644
--- a/app/views/projects/issues/_issue.html.haml
+++ b/app/views/projects/issues/_issue.html.haml
@@ -28,7 +28,7 @@
           = 0
 
   .issue-info
-    = "##{issue.iid} opened #{time_ago_with_tooltip(issue.created_at, 'bottom')} by #{link_to_member(@project, issue.author, avatar: false)}".html_safe
+    = "##{issue.iid} opened #{time_ago_with_tooltip(issue.created_at, placement: 'bottom')} by #{link_to_member(@project, issue.author, avatar: false)}".html_safe
     - if issue.votes_count > 0
       = render 'votes/votes_inline', votable: issue
     - if issue.milestone
@@ -41,4 +41,4 @@
         = issue.task_status
 
     .pull-right.issue-updated-at
-      %small updated #{time_ago_with_tooltip(issue.updated_at, 'bottom', 'issue_update_ago')}
+      %small updated #{time_ago_with_tooltip(issue.updated_at, placement: 'bottom', html_class: 'issue_update_ago')}
diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml
index c16df27ee8f..d79c4548247 100644
--- a/app/views/projects/merge_requests/_merge_request.html.haml
+++ b/app/views/projects/merge_requests/_merge_request.html.haml
@@ -35,7 +35,7 @@
           = 0
 
   .merge-request-info
-    = "##{merge_request.iid} opened #{time_ago_with_tooltip(merge_request.created_at, 'bottom')} by #{link_to_member(@project, merge_request.author, avatar: false)}".html_safe
+    = "##{merge_request.iid} opened #{time_ago_with_tooltip(merge_request.created_at, placement: 'bottom')} by #{link_to_member(@project, merge_request.author, avatar: false)}".html_safe
     - if merge_request.votes_count > 0
       = render 'votes/votes_inline', votable: merge_request
     - if merge_request.milestone_id?
@@ -48,4 +48,4 @@
         = merge_request.task_status
 
     .pull-right.hidden-xs
-      %small updated #{time_ago_with_tooltip(merge_request.updated_at, 'bottom', 'merge_request_updated_ago')}
+      %small updated #{time_ago_with_tooltip(merge_request.updated_at, placement: 'bottom', html_class: 'merge_request_updated_ago')}
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 5d7e73f2b28..9dc4a47258e 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -56,7 +56,8 @@
     #notes.notes.tab-pane.voting_notes
       = render "projects/merge_requests/discussion"
     #commits.commits.tab-pane
-      = render "projects/merge_requests/show/commits"
+      - if current_page?(action: 'commits')
+        = render "projects/merge_requests/show/commits"
     #diffs.diffs.tab-pane
       - if current_page?(action: 'diffs')
         = render "projects/merge_requests/show/diffs"
@@ -64,7 +65,6 @@
   .mr-loading-status
     = spinner
 
-
 :javascript
   var merge_request;
 
diff --git a/app/views/projects/notes/discussions/_active.html.haml b/app/views/projects/notes/discussions/_active.html.haml
index e7a3854701c..4f15a99d061 100644
--- a/app/views/projects/notes/discussions/_active.html.haml
+++ b/app/views/projects/notes/discussions/_active.html.haml
@@ -16,7 +16,7 @@
       = link_to_member(@project, last_note.author, avatar: false)
 
       %span.discussion-last-update
-        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
-        
+        #{time_ago_with_tooltip(last_note.updated_at, placement: 'bottom', html_class: 'discussion_updated_ago')}
+
   .discussion-body.js-toggle-content
     = render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
diff --git a/app/views/projects/notes/discussions/_commit.html.haml b/app/views/projects/notes/discussions/_commit.html.haml
index 62609cfc1c8..6903fad4a0a 100644
--- a/app/views/projects/notes/discussions/_commit.html.haml
+++ b/app/views/projects/notes/discussions/_commit.html.haml
@@ -14,7 +14,7 @@
       last updated by
       = link_to_member(@project, last_note.author, avatar: false)
       %span.discussion-last-update
-        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
+        #{time_ago_with_tooltip(last_note.updated_at, placement: 'bottom', html_class: 'discussion_updated_ago')}
   .discussion-body.js-toggle-content
     - if note.for_diff_line?
       = render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
diff --git a/app/views/projects/notes/discussions/_outdated.html.haml b/app/views/projects/notes/discussions/_outdated.html.haml
index 52a1d342f55..218b0da3977 100644
--- a/app/views/projects/notes/discussions/_outdated.html.haml
+++ b/app/views/projects/notes/discussions/_outdated.html.haml
@@ -14,6 +14,6 @@
       last updated by
       = link_to_member(@project, last_note.author, avatar: false)
       %span.discussion-last-update
-        #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')}
+        #{time_ago_with_tooltip(last_note.updated_at, placement: 'bottom', html_class: 'discussion_updated_ago')}
   .discussion-body.js-toggle-content.hide
     = render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
diff --git a/spec/controllers/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c94ef1629ae..5cc5bd78cd7 100644
--- a/spec/controllers/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -79,23 +79,72 @@ describe Projects::MergeRequestsController do
     end
   end
 
-  context '#diffs with forked projects with submodules' do
-    render_views
-    let(:project) { create(:project) }
-    let(:fork_project) { create(:forked_project_with_submodules) }
-    let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) }
-
-    before do
-      fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
-      fork_project.save
-      merge_request.reload
+  describe 'GET diffs' do
+    def go(format: 'html')
+      get :diffs, namespace_id: project.namespace.to_param,
+        project_id: project.to_param, id: merge_request.iid, format: format
     end
 
-    it '#diffs' do
-      get(:diffs, namespace_id: project.namespace.to_param,
-          project_id: project.to_param, id: merge_request.iid, format: 'json')
-      expect(response).to be_success
-      expect(response.body).to have_content('Subproject commit')
+    context 'as html' do
+      it 'renders the diff template' do
+        go
+
+        expect(response).to render_template('diffs')
+      end
+    end
+
+    context 'as json' do
+      it 'renders the diffs template to a string' do
+        go format: 'json'
+
+        expect(response).to render_template('projects/merge_requests/show/_diffs')
+        expect(JSON.parse(response.body)).to have_key('html')
+      end
+    end
+
+    context 'with forked projects with submodules' do
+      render_views
+
+      let(:project) { create(:project) }
+      let(:fork_project) { create(:forked_project_with_submodules) }
+      let(:merge_request) { create(:merge_request_with_diffs, source_project: fork_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) }
+
+      before do
+        fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
+        fork_project.save
+        merge_request.reload
+      end
+
+      it 'renders' do
+        go format: 'json'
+
+        expect(response).to be_success
+        expect(response.body).to have_content('Subproject commit')
+      end
+    end
+  end
+
+  describe 'GET commits' do
+    def go(format: 'html')
+      get :commits, namespace_id: project.namespace.to_param,
+        project_id: project.to_param, id: merge_request.iid, format: format
+    end
+
+    context 'as html' do
+      it 'renders the show template' do
+        go
+
+        expect(response).to render_template('show')
+      end
+    end
+
+    context 'as json' do
+      it 'renders the commits template to a string' do
+        go format: 'json'
+
+        expect(response).to render_template('projects/merge_requests/show/_commits')
+        expect(JSON.parse(response.body)).to have_key('html')
+      end
     end
   end
 end
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 47e10197f5c..a62c801e6a2 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -240,6 +240,55 @@ describe ApplicationHelper do
     end
   end
 
+  describe 'time_ago_with_tooltip' do
+    def element(*arguments)
+      Time.zone = 'UTC'
+      time = Time.zone.parse('2015-07-02 08:00')
+      element = time_ago_with_tooltip(time, *arguments)
+
+      Nokogiri::HTML::DocumentFragment.parse(element).first_element_child
+    end
+
+    it 'returns a time element' do
+      expect(element.name).to eq 'time'
+    end
+
+    it 'includes the date string' do
+      expect(element.text).to eq '2015-07-02 08:00:00 UTC'
+    end
+
+    it 'has a datetime attribute' do
+      expect(element.attr('datetime')).to eq '2015-07-02T08:00:00Z'
+    end
+
+    it 'has a formatted title attribute' do
+      expect(element.attr('title')).to eq 'Jul 02, 2015 8:00am'
+    end
+
+    it 'includes a default js-timeago class' do
+      expect(element.attr('class')).to eq 'time_ago js-timeago'
+    end
+
+    it 'accepts a custom html_class' do
+      expect(element(html_class: 'custom_class').attr('class')).to eq 'custom_class js-timeago'
+    end
+
+    it 'accepts a custom tooltip placement' do
+      expect(element(placement: 'bottom').attr('data-placement')).to eq 'bottom'
+    end
+
+    it 're-initializes timeago Javascript' do
+      el = element.next_element
+
+      expect(el.name).to eq 'script'
+      expect(el.text).to include "$('.js-timeago').timeago()"
+    end
+
+    it 'allows the script tag to be excluded' do
+      expect(element(skip_js: true)).not_to include 'script'
+    end
+  end
+
   describe 'render_markup' do
     let(:content) { 'Noël' }
 
diff --git a/spec/javascripts/fixtures/merge_request_tabs.html.haml b/spec/javascripts/fixtures/merge_request_tabs.html.haml
new file mode 100644
index 00000000000..7624a713948
--- /dev/null
+++ b/spec/javascripts/fixtures/merge_request_tabs.html.haml
@@ -0,0 +1,22 @@
+%ul.nav.nav-tabs.merge-request-tabs
+  %li.notes-tab
+    %a{href: '/foo/bar/merge_requests/1', data: {target: '#notes', action: 'notes', toggle: 'tab'}}
+      Discussion
+  %li.commits-tab
+    %a{href: '/foo/bar/merge_requests/1/commits', data: {target: '#commits', action: 'commits', toggle: 'tab'}}
+      Commits
+  %li.diffs-tab
+    %a{href: '/foo/bar/merge_requests/1/diffs', data: {target: '#diffs', action: 'diffs', toggle: 'tab'}}
+      Diffs
+
+.tab-content
+  #notes.notes.tab-pane
+    Notes Content
+  #commits.commits.tab-pane
+    Commits Content
+  #diffs.diffs.tab-pane
+    Diffs Content
+
+.mr-loading-status
+  .loading
+    Loading Animation
diff --git a/spec/javascripts/merge_request_tabs_spec.js.coffee b/spec/javascripts/merge_request_tabs_spec.js.coffee
new file mode 100644
index 00000000000..6cc96fb68a0
--- /dev/null
+++ b/spec/javascripts/merge_request_tabs_spec.js.coffee
@@ -0,0 +1,82 @@
+#= require merge_request_tabs
+
+describe 'MergeRequestTabs', ->
+  stubLocation = (stubs) ->
+    defaults = {pathname: '', search: '', hash: ''}
+    $.extend(defaults, stubs)
+
+  fixture.preload('merge_request_tabs.html')
+
+  beforeEach ->
+    @class = new MergeRequestTabs()
+    @spies = {
+      ajax:    spyOn($, 'ajax').and.callFake ->
+      history: spyOn(history, 'replaceState').and.callFake ->
+    }
+
+  describe '#activateTab', ->
+    beforeEach ->
+      fixture.load('merge_request_tabs.html')
+      @subject = @class.activateTab
+
+    it 'shows the first tab when action is show', ->
+      @subject('show')
+      expect($('#notes')).toHaveClass('active')
+
+    it 'shows the notes tab when action is notes', ->
+      @subject('notes')
+      expect($('#notes')).toHaveClass('active')
+
+    it 'shows the commits tab when action is commits', ->
+      @subject('commits')
+      expect($('#commits')).toHaveClass('active')
+
+    it 'shows the diffs tab when action is diffs', ->
+      @subject('diffs')
+      expect($('#diffs')).toHaveClass('active')
+
+  describe '#setCurrentAction', ->
+    beforeEach ->
+      @subject = @class.setCurrentAction
+
+    it 'changes from commits', ->
+      @class._location = stubLocation(pathname: '/foo/bar/merge_requests/1/commits')
+
+      expect(@subject('notes')).toBe('/foo/bar/merge_requests/1')
+      expect(@subject('diffs')).toBe('/foo/bar/merge_requests/1/diffs')
+
+    it 'changes from diffs', ->
+      @class._location = stubLocation(pathname: '/foo/bar/merge_requests/1/diffs')
+
+      expect(@subject('notes')).toBe('/foo/bar/merge_requests/1')
+      expect(@subject('commits')).toBe('/foo/bar/merge_requests/1/commits')
+
+    it 'changes from notes', ->
+      @class._location = stubLocation(pathname: '/foo/bar/merge_requests/1')
+
+      expect(@subject('diffs')).toBe('/foo/bar/merge_requests/1/diffs')
+      expect(@subject('commits')).toBe('/foo/bar/merge_requests/1/commits')
+
+    it 'includes search parameters and hash string', ->
+      @class._location = stubLocation({
+        pathname: '/foo/bar/merge_requests/1/diffs'
+        search:   '?view=parallel'
+        hash:     '#L15-35'
+      })
+
+      expect(@subject('show')).toBe('/foo/bar/merge_requests/1?view=parallel#L15-35')
+
+    it 'replaces the current history state', ->
+      @class._location = stubLocation(pathname: '/foo/bar/merge_requests/1')
+      new_state = @subject('commits')
+
+      expect(@spies.history).toHaveBeenCalledWith(
+        {turbolinks: true, url: new_state},
+        document.title,
+        new_state
+      )
+
+    it 'treats "show" like "notes"', ->
+      @class._location = stubLocation(pathname: '/foo/bar/merge_requests/1/commits')
+
+      expect(@subject('show')).toBe('/foo/bar/merge_requests/1')
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index fa7680fbbec..afdd9f71af2 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -364,6 +364,31 @@ describe User do
     end
   end
 
+  describe '.find_for_commit' do
+    it 'finds by primary email' do
+      user = create(:user, email: 'foo@example.com')
+
+      expect(User.find_for_commit(user.email, '')).to eq user
+    end
+
+    it 'finds by secondary email' do
+      email = create(:email, email: 'foo@example.com')
+      user  = email.user
+
+      expect(User.find_for_commit(email.email, '')).to eq user
+    end
+
+    it 'finds by name' do
+      user = create(:user, name: 'Joey JoJo')
+
+      expect(User.find_for_commit('', 'Joey JoJo')).to eq user
+    end
+
+    it 'returns nil when nothing found' do
+      expect(User.find_for_commit('', '')).to be_nil
+    end
+  end
+
   describe 'search' do
     let(:user1) { create(:user, username: 'James', email: 'james@testing.com') }
     let(:user2) { create(:user, username: 'jameson', email: 'jameson@example.com') }