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

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

app/assets/javascripts/merge_request.js.coffee

 #= 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)
   #
   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'

app/assets/javascripts/merge_request_tabs.js.coffee

+# 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)

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

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'),
+  # 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 }
 
-      haml_tag :script, "$('." + html_class + "').timeago().tooltip()"
-    end.html_safe
+    element += javascript_tag "$('.js-timeago').timeago()" unless skip_js
+
+    element
   end
 
   def render_markup(file_name, file_content)

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

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

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

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)

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)}  
+      #{time_ago_with_tooltip(commit.committed_date, skip_js: true)}  
     = link_to_browse_code(project, commit)

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')}

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')}

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

@@ -56,6 +56,7 @@
     #notes.notes.tab-pane.voting_notes
       = render "projects/merge_requests/discussion"
     #commits.commits.tab-pane
+      - if current_page?(action: 'commits')
         = render "projects/merge_requests/show/commits"
     #diffs.diffs.tab-pane
       - if current_page?(action: 'diffs')
@@ -64,7 +65,6 @@
   .mr-loading-status
     = spinner
 
-
 :javascript
   var merge_request;
 

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

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

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

spec/controllers/merge_requests_controller_spec.rb → spec/controllers/projects/merge_requests_controller_spec.rb

@@ -79,8 +79,32 @@ describe Projects::MergeRequestsController do
     end
   end
 
-  context '#diffs with forked projects with submodules' do
+  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
+
+    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) }
@@ -91,11 +115,36 @@ describe Projects::MergeRequestsController do
         merge_request.reload
       end
 
-    it '#diffs' do
-      get(:diffs, namespace_id: project.namespace.to_param,
-          project_id: project.to_param, id: merge_request.iid, format: 'json')
+      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

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' }
 

spec/javascripts/fixtures/merge_request_tabs.html.haml

+%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

spec/javascripts/merge_request_tabs_spec.js.coffee

+#= 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')

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') }