Merge Request -> show. Refactored. f5 support for diff

Gemfile

@@ -39,6 +39,7 @@ gem "charlock_holmes"
 gem "foreman"
 gem "colored"
 gem 'resque_mailer'
+gem 'tabs_on_rails'
 
 group :assets do
   gem "sass-rails",   "3.2.5"

Gemfile.lock

@@ -331,6 +331,7 @@ GEM
       tilt (~> 1.1, != 1.3.0)
     sqlite3 (1.3.6)
     stamp (0.1.6)
+    tabs_on_rails (2.1.1)
     therubyracer (0.10.1)
       libv8 (~> 3.3.10)
     thin (1.3.1)
@@ -420,6 +421,7 @@ DEPENDENCIES
   six
   sqlite3
   stamp
+  tabs_on_rails
   therubyracer
   thin
   turn

app/assets/javascripts/merge_requests.js

@@ -8,36 +8,77 @@ var MergeRequest = {
       var self = this;
       self.opts = opts;
 
+      self.initTabs();
+      self.initMergeWidget();
+
+      $(".mr_show_all_commits").bind("click", function() { 
+        self.showAllCommits();
+      });
+
+      $(".line_note_link, .line_note_reply_link").live("click", function(e) {
+        var form = $(".per_line_form");
+        $(this).parent().parent().after(form);
+        form.find("#note_line_code").val($(this).attr("line_code"));
+        form.show();
+        return false;
+      });
+    },
+
+  initMergeWidget: 
+    function() { 
+      var self = this;
       self.showState(self.opts.current_state);
+
       if($(".automerge_widget").length && self.opts.check_enable){
         $.get(opts.url_to_automerge_check, function(data){
           self.showState(data.state);
         }, "json");
       }
+    },
 
-      $(".nav-tabs a").live("click", function() { 
-        $(".nav-tabs a").parent().removeClass("active");
+  initTabs:
+    function() { 
+      $(".mr_nav_tabs a").live("click", function() { 
+        $(".mr_nav_tabs a").parent().removeClass("active");
         $(this).parent().addClass("active");
       });
 
-      $(".nav-tabs a.merge-notes-tab").live("click", function(e) { 
+      var current_tab;
+      if(this.opts.action == "diffs") { 
+        current_tab = $(".mr_nav_tabs .merge-diffs-tab");
+      } else {
+        current_tab = $(".mr_nav_tabs .merge-notes-tab");
+      }
+      current_tab.parent().addClass("active");
+
+      this.initNotesTab();
+      this.initDiffTab();
+    },
+
+  initNotesTab: 
+    function() { 
+      $(".mr_nav_tabs a.merge-notes-tab").live("click", function(e) { 
         $(".merge-request-diffs").hide();
         $(".merge_request_notes").show();
+        var mr_path = $(".merge-notes-tab").attr("data-url");
+        history.pushState({ path: mr_path }, '', mr_path);
         e.preventDefault();
       });
+    },
 
-      $(".nav-tabs a.merge-diffs-tab").live("click", function(e) { 
+  initDiffTab: 
+    function() { 
+      $(".mr_nav_tabs a.merge-diffs-tab").live("click", function(e) { 
         if(!MergeRequest.diffs_loaded) { 
           MergeRequest.loadDiff(); 
         }
         $(".merge_request_notes").hide();
         $(".merge-request-diffs").show();
+        var mr_diff_path = $(".merge-diffs-tab").attr("data-url");
+        history.pushState({ path: mr_diff_path }, '', mr_diff_path);
         e.preventDefault();
       });
 
-      $(".mr_show_all_commits").bind("click", function() { 
-        MergeRequest.showAllCommits();
-      })
     },
 
   showState:

app/assets/stylesheets/sections/merge_requests.scss

@@ -72,3 +72,13 @@
     @extend .primary;
   }
 }
+
+.mr_nav_tabs { 
+  li { 
+    a { 
+      font-weight:bold;
+      padding:8px 20px;
+      text-align:center;
+    }
+  }
+}

app/controllers/merge_requests_controller.rb

@@ -3,6 +3,8 @@ class MergeRequestsController < ApplicationController
   before_filter :project
   before_filter :module_enabled
   before_filter :merge_request, :only => [:edit, :update, :destroy, :show, :commits, :diffs, :automerge, :automerge_check]
+  before_filter :validates_merge_request, :only => [:show, :diffs]
+  before_filter :define_show_vars, :only => [:show, :diffs]
   layout "project"
 
   # Authorize
@@ -20,6 +22,7 @@ class MergeRequestsController < ApplicationController
   # Allow destroy merge_request
   before_filter :authorize_admin_merge_request!, :only => [:destroy]
 
+
   def index
     @merge_requests = @project.merge_requests
 
@@ -34,20 +37,6 @@ class MergeRequestsController < ApplicationController
   end
 
   def show
-    # Show git not found page if target branch doesnt exist
-    return git_not_found! unless @project.repo.heads.map(&:name).include?(@merge_request.target_branch) 
-
-    # Show git not found page if source branch doesnt exist
-    # and there is no saved commits between source & target branch
-    return git_not_found! if !@project.repo.heads.map(&:name).include?(@merge_request.source_branch) && @merge_request.commits.blank?
-    
-    # Build a note object for comment form
-    @note = @project.notes.new(:noteable => @merge_request)
-
-    # Get commits from repository 
-    # or from cache if already merged
-    @commits = @merge_request.commits
-
     respond_to do |format|
       format.html
       format.js
@@ -142,4 +131,22 @@ class MergeRequestsController < ApplicationController
   def module_enabled
     return render_404 unless @project.merge_requests_enabled
   end
+
+  def validates_merge_request
+    # Show git not found page if target branch doesnt exist
+    return git_not_found! unless @project.repo.heads.map(&:name).include?(@merge_request.target_branch) 
+
+    # Show git not found page if source branch doesnt exist
+    # and there is no saved commits between source & target branch
+    return git_not_found! if !@project.repo.heads.map(&:name).include?(@merge_request.source_branch) && @merge_request.commits.blank?
+  end
+
+  def define_show_vars
+    # Build a note object for comment form
+    @note = @project.notes.new(:noteable => @merge_request)
+
+    # Get commits from repository 
+    # or from cache if already merged
+    @commits = @merge_request.commits
+  end
 end

app/views/commits/_diffs.html.haml

@@ -35,5 +35,4 @@
           .diff_file_content_image{:class => image_diff_class(diff)}
             %img{:src => "data:#{file.mime_type};base64,#{Base64.encode64(file.data)}"}
         - else
-          %p
-            %center No preview for this file type
+          %p.nothing_here_message No preview for this file type

app/views/merge_requests/_show.html.haml

@@ -0,0 +1,39 @@
+= render "merge_requests/show/mr_title"
+= render "merge_requests/show/how_to_merge"
+= render "merge_requests/show/mr_box"
+= render "merge_requests/show/mr_accept"
+= render "merge_requests/show/commits"
+
+- if @commits.present?
+  %ul.nav.nav-tabs.mr_nav_tabs
+    %li
+      = link_to "#notes", "data-url" => project_merge_request_path(@project, @merge_request), :class => "merge-notes-tab tab" do
+        %i.icon-comment
+        Comments
+    %li
+      = link_to "#diffs", "data-url" => diffs_project_merge_request_path(@project, @merge_request), :class => "merge-diffs-tab tab" do
+        %i.icon-list-alt
+        Diff
+
+.merge_request_notes#notes{ :class => (controller.action_name == 'show') ? "" : "hide" }
+  = render("notes/notes", :tid => @merge_request.id, :tt => "merge_request")
+.merge-request-diffs
+  = render "merge_requests/show/diffs" if @diffs
+.status
+
+= render "notes/per_line_form"
+
+:javascript
+  $(function(){
+    MergeRequest.init({
+      url_to_automerge_check: "#{automerge_check_project_merge_request_path(@project, @merge_request)}",
+      check_enable: #{@merge_request.state == MergeRequest::UNCHECKED ? "true" : "false"},
+      current_state: "#{@merge_request.human_state}",
+      action: "#{controller.action_name}"
+    });
+
+    $(".edit_merge_request").live("ajax:beforeSend", function() {
+      $(this).replaceWith('#{image_tag "ajax_loader.gif"}');
+    })
+  })
+

app/views/merge_requests/diffs.html.haml

@@ -0,0 +1,2 @@
+= render "show"
+

app/views/merge_requests/diffs.js.haml

@@ -1,4 +1,4 @@
 :plain
-  $(".merge-request-diffs").html("#{escape_javascript(render(:partial => "diffs"))}");
+  $(".merge-request-diffs").html("#{escape_javascript(render(:partial => "merge_requests/show/diffs"))}");
 
 

app/views/merge_requests/show.html.haml

@@ -1,144 +1 @@
-%h3
-  = "Merge Request ##{@merge_request.id}:"
-   
-  %span.pretty_label.branch= @merge_request.source_branch
-  →
-  %span.pretty_label.branch= @merge_request.target_branch
-
-  %span.right
-    - if @merge_request.merged?
-      %span.btn.small.disabled.padded
-        %strong
-          %i.icon-ok
-          = "MERGED"
-    - if can?(current_user, :modify_merge_request, @merge_request)
-      - if @merge_request.open?
-        = link_to 'Close', project_merge_request_path(@project, @merge_request, :merge_request => {:closed => true }, :status_only => true), :method => :put, :class => "btn small padded danger", :title => "Close merge request"
-        = link_to edit_project_merge_request_path(@project, @merge_request), :class => "btn small padded" do
-          %i.icon-edit
-          Edit
-
-    %br
-    - if @merge_request.upvotes > 0
-      .upvotes#upvotes= "+#{pluralize @merge_request.upvotes, 'upvote'}"
-
-= render "merge_requests/how_to_merge"
-.back_link
-  = link_to project_merge_requests_path(@project) do
-    ← To merge requests
-
-.main_box
-  .top_box_content
-    %h4
-      - if @merge_request.closed
-        .alert-message.error.status_info Closed
-      - else
-        .alert-message.success.status_info Open
-      = @merge_request.title
-
-  .middle_box_content
-    %div
-      %cite.cgray Created at #{@merge_request.created_at.stamp("Aug 21, 2011")} by
-      = image_tag gravatar_icon(@merge_request.author_email), :width => 16, :class => "lil_av"
-      %strong.author= link_to_merge_request_author(@merge_request)
-
-      %cite.cgray and currently assigned to
-      = image_tag gravatar_icon(@merge_request.assignee_email), :width => 16, :class => "lil_av"
-      %strong.author= link_to_merge_request_assignee(@merge_request)
-
-
-  - if @merge_request.closed
-    .bottom_box_content
-      - if @merge_request.merged?
-        %span
-          Merged by #{@merge_request.merge_event.author_name}
-          %small #{time_ago_in_words(@merge_request.merge_event.created_at)} ago.
-      - elsif @merge_request.closed_event
-        %span
-          Closed by #{@merge_request.closed_event.author_name}
-          %small #{time_ago_in_words(@merge_request.closed_event.created_at)} ago.
-
-- unless can?(current_user, :accept_mr, @project)
-  .alert-message
-    %strong Only masters can accept MR
-
-
-- if @merge_request.open? && @commits.any? && can?(current_user, :accept_mr, @project)
-  .automerge_widget.can_be_merged{:style => "display:none"}
-    .alert.alert-success
-      %span
-        = form_for [:automerge, @project, @merge_request], :remote => true, :method => :get do |f|
-          %p
-            You can accept this request automatically. 
-            If you still want to do it manually - 
-            %strong= link_to "click here", "#", :class => "how_to_merge_link vlink", :title => "How To Merge"
-            for instructions
-          .accept_group
-            = f.submit "Accept Merge Request", :class => "btn small success accept_merge_request"
-            - unless @project.root_ref? @merge_request.source_branch 
-              .remove_branch_holder
-                = label_tag :should_remove_source_branch, :class => "checkbox" do 
-                  = check_box_tag :should_remove_source_branch
-                  Remove source-branch
-          .clearfix
-             
-
-  .automerge_widget.cannot_be_merged{:style => "display:none"}
-    .alert.alert-info
-      %span
-        = link_to "Show how to merge", "#", :class => "how_to_merge_link btn small padded", :title => "How To Merge"
-         
-        %strong This request cant be merged with GitLab. You should do it manually
-
-  .automerge_widget.unchecked
-    .alert-message
-      %strong
-        %i.icon-refresh
-        Checking for ability to automatically merge…
-
-  .automerge_widget.already_cannot_be_merged{:style => "display:none"}
-    .alert.alert-info
-      %strong This merge request already can not be merged. Try to reload page.
-
-= render "merge_requests/commits"
-
-- unless @commits.empty?
-  .nav.nav-tabs
-    %li.active
-      = link_to "#notes", :class => "merge-notes-tab tab" do
-        Notes
-    %li
-      = link_to "#diffs", "data-url" => diffs_project_merge_request_path(@project, @merge_request), :class => "merge-diffs-tab tab" do
-        Diff
-
-
-.merge_request_notes#notes= render "notes/notes", :tid => @merge_request.id, :tt => "merge_request"
-
-.merge-request-diffs
-.status
-
-:javascript
-  $(function(){
-    MergeRequest.init({
-      url_to_automerge_check: "#{automerge_check_project_merge_request_path(@project, @merge_request)}",
-      check_enable: #{@merge_request.state == MergeRequest::UNCHECKED ? "true" : "false"},
-      current_state: "#{@merge_request.human_state}"
-    });
-
-    $(".edit_merge_request").live("ajax:beforeSend", function() {
-      $(this).replaceWith('#{image_tag "ajax_loader.gif"}');
-    })
-  })
-
-= render "notes/per_line_form"
-
-:javascript
-  $(document).ready(function(){
-    $(".line_note_link, .line_note_reply_link").live("click", function(e) {
-      var form = $(".per_line_form");
-      $(this).parent().parent().after(form);
-      form.find("#note_line_code").val($(this).attr("line_code"));
-      form.show();
-      return false;
-    });
-  });
+= render "show"

app/views/merge_requests/show.js.haml

@@ -0,0 +1,2 @@
+:plain
+  $(".merge-request-notes").html("#{escape_javascript(render("notes/notes", :tid => @merge_request.id, :tt => "merge_request"))}");

app/views/merge_requests/show/_mr_accept.html.haml

@@ -0,0 +1,42 @@
+- unless can?(current_user, :accept_mr, @project)
+  .alert-message
+    %strong Only masters can accept MR
+
+
+- if @merge_request.open? && @commits.any? && can?(current_user, :accept_mr, @project)
+  .automerge_widget.can_be_merged{:style => "display:none"}
+    .alert.alert-success
+      %span
+        = form_for [:automerge, @project, @merge_request], :remote => true, :method => :get do |f|
+          %p
+            You can accept this request automatically. 
+            If you still want to do it manually - 
+            %strong= link_to "click here", "#", :class => "how_to_merge_link vlink", :title => "How To Merge"
+            for instructions
+          .accept_group
+            = f.submit "Accept Merge Request", :class => "btn small success accept_merge_request"
+            - unless @project.root_ref? @merge_request.source_branch 
+              .remove_branch_holder
+                = label_tag :should_remove_source_branch, :class => "checkbox" do 
+                  = check_box_tag :should_remove_source_branch
+                  Remove source-branch
+          .clearfix
+             
+
+  .automerge_widget.cannot_be_merged{:style => "display:none"}
+    .alert.alert-info
+      %span
+        = link_to "Show how to merge", "#", :class => "how_to_merge_link btn small padded", :title => "How To Merge"
+         
+        %strong This request cant be merged with GitLab. You should do it manually
+
+  .automerge_widget.unchecked
+    .alert-message
+      %strong
+        %i.icon-refresh
+        Checking for ability to automatically merge…
+
+  .automerge_widget.already_cannot_be_merged{:style => "display:none"}
+    .alert.alert-info
+      %strong This merge request already can not be merged. Try to reload page.
+

app/views/merge_requests/show/_mr_box.html.haml

@@ -0,0 +1,31 @@
+.main_box
+  .top_box_content
+    %h4
+      - if @merge_request.closed
+        .alert-message.error.status_info Closed
+      - else
+        .alert-message.success.status_info Open
+      = @merge_request.title
+
+  .middle_box_content
+    %div
+      %cite.cgray Created at #{@merge_request.created_at.stamp("Aug 21, 2011")} by
+      = image_tag gravatar_icon(@merge_request.author_email), :width => 16, :class => "lil_av"
+      %strong.author= link_to_merge_request_author(@merge_request)
+
+      %cite.cgray and currently assigned to
+      = image_tag gravatar_icon(@merge_request.assignee_email), :width => 16, :class => "lil_av"
+      %strong.author= link_to_merge_request_assignee(@merge_request)
+
+
+  - if @merge_request.closed
+    .bottom_box_content
+      - if @merge_request.merged?
+        %span
+          Merged by #{@merge_request.merge_event.author_name}
+          %small #{time_ago_in_words(@merge_request.merge_event.created_at)} ago.
+      - elsif @merge_request.closed_event
+        %span
+          Closed by #{@merge_request.closed_event.author_name}
+          %small #{time_ago_in_words(@merge_request.closed_event.created_at)} ago.
+

app/views/merge_requests/show/_mr_title.html.haml

@@ -0,0 +1,28 @@
+%h3
+  = "Merge Request ##{@merge_request.id}:"
+   
+  %span.pretty_label.branch= @merge_request.source_branch
+  →
+  %span.pretty_label.branch= @merge_request.target_branch
+
+  %span.right
+    - if @merge_request.merged?
+      %span.btn.small.disabled.padded
+        %strong
+          %i.icon-ok
+          = "MERGED"
+    - if can?(current_user, :modify_merge_request, @merge_request)
+      - if @merge_request.open?
+        = link_to 'Close', project_merge_request_path(@project, @merge_request, :merge_request => {:closed => true }, :status_only => true), :method => :put, :class => "btn small padded danger", :title => "Close merge request"
+        = link_to edit_project_merge_request_path(@project, @merge_request), :class => "btn small padded" do
+          %i.icon-edit
+          Edit
+
+    %br
+    - if @merge_request.upvotes > 0
+      .upvotes#upvotes= "+#{pluralize @merge_request.upvotes, 'upvote'}"
+
+
+.back_link
+  = link_to project_merge_requests_path(@project) do
+    ← To merge requests

db/schema.rb

@@ -61,8 +61,8 @@ ActiveRecord::Schema.define(:version => 20120627145613) do
     t.boolean  "closed",                              :default => false, :null => false
     t.datetime "created_at",                                             :null => false
     t.datetime "updated_at",                                             :null => false
-    t.text     "st_commits",    :limit => 4294967295
-    t.text     "st_diffs",      :limit => 4294967295
+    t.text     "st_commits",    :limit => 2147483647
+    t.text     "st_diffs",      :limit => 2147483647
     t.boolean  "merged",                              :default => false, :null => false
     t.integer  "state",                               :default => 1,     :null => false
   end