Merge branch 'state-machine-stage-2' of https://github.com/Undev/gitlabhq into U…

…ndev-state-machine-stage-2 Conflicts: app/models/merge_request.rb

Merge branch 'state-machine-stage-2' of https://github.com/Undev/gitlabhq into U…
…ndev-state-machine-stage-2 Conflicts: app/models/merge_request.rb
Dmitriy Zaporozhets
2 parents 0cc95ef2 6d68923e
Showing 10 changed files with 97 additions and 82 deletions Show diff stats
app/controllers/merge_requests_controller.rb
app/models/merge_request.rb
app/models/project.rb
app/views/merge_requests/_show.html.haml
db/migrate/20130220124204_add_new_merge_status_to_merge_request.rb
db/migrate/20130220125544_convert_merge_status_in_merge_request.rb
db/migrate/20130220125545_remove_merge_status_from_merge_request.rb
db/migrate/20130220133245_rename_new_merge_status_to_merge_status_in_milestone.rb
db/schema.rb
spec/models/merge_request_spec.rb
@@ -75,7 +75,7 @@ class MergeRequestsController &lt; ProjectResourceController
     if @merge_request.unchecked?
       @merge_request.check_if_can_be_merged
     end
-    render json: {merge_status: @merge_request.human_merge_status}
+    render json: {merge_status: @merge_request.human_merge_status_name}
   rescue Gitlab::SatelliteNotExistError
     render json: {merge_status: :no_satellite}
   end
@@ -24,6 +24,8 @@ require Rails.root.join(&quot;lib/static_model&quot;)
 class MergeRequest < ActiveRecord::Base
   include Issuable
+  BROKEN_DIFF = "--broken-diff"
+
   attr_accessible :title, :assignee_id, :target_branch, :source_branch, :milestone_id,
                   :author_id_of_changes, :state_event
@@ -51,52 +53,42 @@ class MergeRequest &lt; ActiveRecord::Base
     state :merged
   end
-  BROKEN_DIFF = "--broken-diff"
+  state_machine :merge_status, initial: :unchecked do
+    event :mark_as_unchecked do
+      transition [:can_be_merged, :cannot_be_merged] => :unchecked
+    end
+
+    event :mark_as_mergeable do
+      transition unchecked: :can_be_merged
+    end
+
+    event :mark_as_unmergeable do
+      transition unchecked: :cannot_be_merged
+    end
+
+    state :unchecked
-  UNCHECKED = 1
-  CAN_BE_MERGED = 2
-  CANNOT_BE_MERGED = 3
+    state :can_be_merged
+
+    state :cannot_be_merged
+  end
   serialize :st_commits
   serialize :st_diffs
   validates :source_branch, presence: true
   validates :target_branch, presence: true
-  validate :validate_branches
+  validate  :validate_branches
   scope :merged, -> { with_state(:merged) }
+  scope :by_branch, ->(branch_name) { where("source_branch LIKE :branch OR target_branch LIKE :branch", branch: branch_name) }
+  scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) }
+  scope :by_milestone, ->(milestone) { where(milestone_id: milestone) }
   # Closed scope for merge request should return
   # both merged and closed mr's
   scope :closed, -> { with_states(:closed, :merged) }
-  class << self
-    def find_all_by_branch(branch_name)
-      where("source_branch LIKE :branch OR target_branch LIKE :branch", branch: branch_name)
-    end
-
-    def cared(user)
-      where('assignee_id = :user OR author_id = :user', user: user.id)
-    end
-
-    def find_all_by_branch(branch_name)
-      where("source_branch LIKE :branch OR target_branch LIKE :branch", branch: branch_name)
-    end
-
-    def find_all_by_milestone(milestone)
-      where("milestone_id = :milestone_id", milestone_id: milestone)
-    end
-  end
-
-  def human_merge_status
-    merge_statuses = {
-      CAN_BE_MERGED =>  "can_be_merged",
-      CANNOT_BE_MERGED => "cannot_be_merged",
-      UNCHECKED => "unchecked"
-    }
-    merge_statuses[self.merge_status]
-  end
-
   def validate_branches
     if target_branch == source_branch
       errors.add :base, "You can not use same branch for source and target branches"
@@ -108,26 +100,12 @@ class MergeRequest &lt; ActiveRecord::Base
     self.reloaded_diffs
   end
-  def unchecked?
-    merge_status == UNCHECKED
-  end
-
-  def mark_as_unchecked
-    self.merge_status = UNCHECKED
-    self.save
-  end
-
-  def can_be_merged?
-    merge_status == CAN_BE_MERGED
-  end
-
   def check_if_can_be_merged
-    self.merge_status = if Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged?
-                   CAN_BE_MERGED
-                 else
-                   CANNOT_BE_MERGED
-                 end
-    self.save
+    if Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged?
+      mark_as_mergeable
+    else
+      mark_as_unmergeable
+    end
   end
   def diffs
@@ -182,11 +160,6 @@ class MergeRequest &lt; ActiveRecord::Base
       commits.any? && opened?
   end
-  def mark_as_unmergable
-    self.merge_status = CANNOT_BE_MERGED
-    self.save
-  end
-
   def reloaded_commits
     if opened? && unmerged_commits.any?
       self.st_commits = unmerged_commits
@@ -221,7 +194,7 @@ class MergeRequest &lt; ActiveRecord::Base
       true
     end
   rescue
-    self.mark_as_unmergable
+    mark_as_unmergeable
     false
   end
@@ -321,7 +321,7 @@ class Project &lt; ActiveRecord::Base
     c_ids = self.repository.commits_between(oldrev, newrev).map(&:id)
     # Update code for merge requests
-    mrs = self.merge_requests.opened.find_all_by_branch(branch_name).all
+    mrs = self.merge_requests.opened.by_branch(branch_name).all
     mrs.each { |merge_request| merge_request.reload_code; merge_request.mark_as_unchecked }
     # Close merge requests
@@ -29,10 +29,10 @@
   $(function(){
     merge_request = new MergeRequest({
       url_to_automerge_check: "#{automerge_check_project_merge_request_path(@project, @merge_request)}",
-      check_enable: #{@merge_request.merge_status == MergeRequest::UNCHECKED ? "true" : "false"},
+      check_enable: #{@merge_request.unchecked? ? "true" : "false"},
       url_to_ci_check: "#{ci_status_project_merge_request_path(@project, @merge_request)}",
       ci_enable: #{@project.gitlab_ci? ? "true" : "false"},
-      current_status: "#{@merge_request.human_merge_status}",
+      current_status: "#{@merge_request.human_merge_status_name}",
       action: "#{controller.action_name}"
     });
   });
@@ -0,0 +1,5 @@
+class AddNewMergeStatusToMergeRequest < ActiveRecord::Migration
+  def change
+    add_column :merge_requests, :new_merge_status, :string
+  end
+end
@@ -0,0 +1,17 @@
+class ConvertMergeStatusInMergeRequest < ActiveRecord::Migration
+  def up
+    MergeRequest.transaction do
+      MergeRequest.where(merge_status: 1).update_all("new_merge_status = 'unchecked'")
+      MergeRequest.where(merge_status: 2).update_all("new_merge_status = 'can_be_merged'")
+      MergeRequest.where(merge_status: 3).update_all("new_merge_status = 'cannot_be_merged'")
+    end
+  end
+
+  def down
+    MergeRequest.transaction do
+      MergeRequest.where(new_merge_status: :unchecked).update_all("merge_status = 1")
+      MergeRequest.where(new_merge_status: :can_be_merged).update_all("merge_status = 2")
+      MergeRequest.where(new_merge_status: :cannot_be_merged).update_all("merge_status = 3")
+    end
+  end
+end
@@ -0,0 +1,9 @@
+class RemoveMergeStatusFromMergeRequest < ActiveRecord::Migration
+  def up
+    remove_column :merge_requests, :merge_status
+  end
+
+  def down
+    add_column :merge_requests, :merge_status, :integer
+  end
+end
@@ -0,0 +1,5 @@
+class RenameNewMergeStatusToMergeStatusInMilestone < ActiveRecord::Migration
+  def change
+    rename_column :merge_requests, :new_merge_status, :merge_status
+  end
+end
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
-ActiveRecord::Schema.define(:version => 20130218141554) do
+ActiveRecord::Schema.define(:version => 20130220133245) do
   create_table "events", :force => true do |t|
     t.string   "target_type"
@@ -68,19 +68,19 @@ ActiveRecord::Schema.define(:version =&gt; 20130218141554) do
   add_index "keys", ["user_id"], :name => "index_keys_on_user_id"
   create_table "merge_requests", :force => true do |t|
-    t.string   "target_branch",                                      :null => false
-    t.string   "source_branch",                                      :null => false
-    t.integer  "project_id",                                         :null => false
+    t.string   "target_branch",                       :null => false
+    t.string   "source_branch",                       :null => false
+    t.integer  "project_id",                          :null => false
     t.integer  "author_id"
     t.integer  "assignee_id"
     t.string   "title"
-    t.datetime "created_at",                                         :null => false
-    t.datetime "updated_at",                                         :null => false
+    t.datetime "created_at",                          :null => false
+    t.datetime "updated_at",                          :null => false
     t.text     "st_commits",    :limit => 2147483647
     t.text     "st_diffs",      :limit => 2147483647
-    t.integer  "merge_status",                        :default => 1, :null => false
     t.integer  "milestone_id"
     t.string   "state"
+    t.string   "merge_status"
   end
   add_index "merge_requests", ["assignee_id"], :name => "index_merge_requests_on_assignee_id"
@@ -106,11 +106,11 @@ ActiveRecord::Schema.define(:version =&gt; 20130218141554) do
   add_index "milestones", ["project_id"], :name => "index_milestones_on_project_id"
   create_table "namespaces", :force => true do |t|
-    t.string   "name",       :null => false
-    t.string   "path",       :null => false
-    t.integer  "owner_id",   :null => false
-    t.datetime "created_at", :null => false
-    t.datetime "updated_at", :null => false
+    t.string   "name",                        :null => false
+    t.string   "path",                        :null => false
+    t.integer  "owner_id",                    :null => false
+    t.datetime "created_at",                  :null => false
+    t.datetime "updated_at",                  :null => false
     t.string   "type"
   end
@@ -142,16 +142,16 @@ ActiveRecord::Schema.define(:version =&gt; 20130218141554) do
     t.string   "name"
     t.string   "path"
     t.text     "description"
-    t.datetime "created_at",                                :null => false
-    t.datetime "updated_at",                                :null => false
+    t.datetime "created_at",                                   :null => false
+    t.datetime "updated_at",                                   :null => false
     t.integer  "creator_id"
     t.string   "default_branch"
-    t.boolean  "issues_enabled",         :default => true,  :null => false
-    t.boolean  "wall_enabled",           :default => true,  :null => false
-    t.boolean  "merge_requests_enabled", :default => true,  :null => false
-    t.boolean  "wiki_enabled",           :default => true,  :null => false
+    t.boolean  "issues_enabled",         :default => true,     :null => false
+    t.boolean  "wall_enabled",           :default => true,     :null => false
+    t.boolean  "merge_requests_enabled", :default => true,     :null => false
+    t.boolean  "wiki_enabled",           :default => true,     :null => false
     t.integer  "namespace_id"
-    t.boolean  "public",                 :default => false, :null => false
+    t.boolean  "public",                 :default => false,    :null => false
   end
   add_index "projects", ["creator_id"], :name => "index_projects_on_owner_id"
@@ -230,8 +230,8 @@ ActiveRecord::Schema.define(:version =&gt; 20130218141554) do
     t.string   "name"
     t.string   "path"
     t.integer  "owner_id"
-    t.datetime "created_at", :null => false
-    t.datetime "updated_at", :null => false
+    t.datetime "created_at",                  :null => false
+    t.datetime "updated_at",                  :null => false
   end
   create_table "users", :force => true do |t|
@@ -32,6 +32,12 @@ describe MergeRequest do
     it { should_not allow_mass_assignment_of(:project_id) }
   end
+  describe "Respond to" do
+    it { should respond_to(:unchecked?) }
+    it { should respond_to(:can_be_merged?) }
+    it { should respond_to(:cannot_be_merged?) }
+  end
+ 
   describe 'modules' do
     it { should include_module(Issuable) }
   end
	@@ -75,7 +75,7 @@ class MergeRequestsController < ProjectResourceController		@@ -75,7 +75,7 @@ class MergeRequestsController < ProjectResourceController
75	if @merge_request.unchecked?	75	if @merge_request.unchecked?
76	@merge_request.check_if_can_be_merged	76	@merge_request.check_if_can_be_merged
77	end	77	end
78	- render json: {merge_status: @merge_request.human_merge_status}	78	+ render json: {merge_status: @merge_request.human_merge_status_name}
79	rescue Gitlab::SatelliteNotExistError	79	rescue Gitlab::SatelliteNotExistError
80	render json: {merge_status: :no_satellite}	80	render json: {merge_status: :no_satellite}
81	end	81	end
	@@ -321,7 +321,7 @@ class Project < ActiveRecord::Base		@@ -321,7 +321,7 @@ class Project < ActiveRecord::Base
321	c_ids = self.repository.commits_between(oldrev, newrev).map(&:id)	321	c_ids = self.repository.commits_between(oldrev, newrev).map(&:id)
322		322
323	# Update code for merge requests	323	# Update code for merge requests
324	- mrs = self.merge_requests.opened.find_all_by_branch(branch_name).all	324	+ mrs = self.merge_requests.opened.by_branch(branch_name).all
325	mrs.each { \|merge_request\| merge_request.reload_code; merge_request.mark_as_unchecked }	325	mrs.each { \|merge_request\| merge_request.reload_code; merge_request.mark_as_unchecked }
326		326
327	# Close merge requests	327	# Close merge requests
	@@ -29,10 +29,10 @@		@@ -29,10 +29,10 @@
29	$(function(){	29	$(function(){
30	merge_request = new MergeRequest({	30	merge_request = new MergeRequest({
31	url_to_automerge_check: "#{automerge_check_project_merge_request_path(@project, @merge_request)}",	31	url_to_automerge_check: "#{automerge_check_project_merge_request_path(@project, @merge_request)}",
32	- check_enable: #{@merge_request.merge_status == MergeRequest::UNCHECKED ? "true" : "false"},	32	+ check_enable: #{@merge_request.unchecked? ? "true" : "false"},
33	url_to_ci_check: "#{ci_status_project_merge_request_path(@project, @merge_request)}",	33	url_to_ci_check: "#{ci_status_project_merge_request_path(@project, @merge_request)}",
34	ci_enable: #{@project.gitlab_ci? ? "true" : "false"},	34	ci_enable: #{@project.gitlab_ci? ? "true" : "false"},
35	- current_status: "#{@merge_request.human_merge_status}",	35	+ current_status: "#{@merge_request.human_merge_status_name}",
36	action: "#{controller.action_name}"	36	action: "#{controller.action_name}"
37	});	37	});
38	});	38	});
@@ -0,0 +1,5 @@		@@ -0,0 +1,5 @@
	1	+class AddNewMergeStatusToMergeRequest < ActiveRecord::Migration
	2	+ def change
	3	+ add_column :merge_requests, :new_merge_status, :string
	4	+ end
	5	+end
@@ -0,0 +1,17 @@		@@ -0,0 +1,17 @@
	1	+class ConvertMergeStatusInMergeRequest < ActiveRecord::Migration
	2	+ def up
	3	+ MergeRequest.transaction do
	4	+ MergeRequest.where(merge_status: 1).update_all("new_merge_status = 'unchecked'")
	5	+ MergeRequest.where(merge_status: 2).update_all("new_merge_status = 'can_be_merged'")
	6	+ MergeRequest.where(merge_status: 3).update_all("new_merge_status = 'cannot_be_merged'")
	7	+ end
	8	+ end
	9	+
	10	+ def down
	11	+ MergeRequest.transaction do
	12	+ MergeRequest.where(new_merge_status: :unchecked).update_all("merge_status = 1")
	13	+ MergeRequest.where(new_merge_status: :can_be_merged).update_all("merge_status = 2")
	14	+ MergeRequest.where(new_merge_status: :cannot_be_merged).update_all("merge_status = 3")
	15	+ end
	16	+ end
	17	+end
@@ -0,0 +1,9 @@		@@ -0,0 +1,9 @@
	1	+class RemoveMergeStatusFromMergeRequest < ActiveRecord::Migration
	2	+ def up
	3	+ remove_column :merge_requests, :merge_status
	4	+ end
	5	+
	6	+ def down
	7	+ add_column :merge_requests, :merge_status, :integer
	8	+ end
	9	+end
@@ -0,0 +1,5 @@		@@ -0,0 +1,5 @@
	1	+class RenameNewMergeStatusToMergeStatusInMilestone < ActiveRecord::Migration
	2	+ def change
	3	+ rename_column :merge_requests, :new_merge_status, :merge_status
	4	+ end
	5	+end
	@@ -32,6 +32,12 @@ describe MergeRequest do		@@ -32,6 +32,12 @@ describe MergeRequest do
32	it { should_not allow_mass_assignment_of(:project_id) }	32	it { should_not allow_mass_assignment_of(:project_id) }
33	end	33	end
34		34
		35	+ describe "Respond to" do
		36	+ it { should respond_to(:unchecked?) }
		37	+ it { should respond_to(:can_be_merged?) }
		38	+ it { should respond_to(:cannot_be_merged?) }
		39	+ end
		40	+
35	describe 'modules' do	41	describe 'modules' do
36	it { should include_module(Issuable) }	42	it { should include_module(Issuable) }
37	end	43	end