diff options
| -rw-r--r-- | app/contexts/merge_requests_load_context.rb | 2 | ||||
| -rw-r--r-- | app/controllers/merge_requests_controller.rb | 2 | ||||
| -rw-r--r-- | app/helpers/merge_requests_helper.rb | 2 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 54 | ||||
| -rw-r--r-- | app/observers/merge_request_observer.rb | 19 | ||||
| -rw-r--r-- | app/views/merge_requests/show/_mr_accept.html.haml | 2 | ||||
| -rw-r--r-- | app/views/merge_requests/show/_mr_box.html.haml | 6 | ||||
| -rw-r--r-- | app/views/merge_requests/show/_mr_ci.html.haml | 2 | ||||
| -rw-r--r-- | app/views/merge_requests/show/_mr_title.html.haml | 2 | ||||
| -rw-r--r-- | app/views/milestones/show.html.haml | 2 | ||||
| -rw-r--r-- | spec/factories.rb | 13 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 4 | ||||
| -rw-r--r-- | spec/models/milestone_spec.rb | 2 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 8 | ||||
| -rw-r--r-- | spec/observers/merge_request_observer_spec.rb | 119 |
15 files changed, 107 insertions, 132 deletions
diff --git a/app/contexts/merge_requests_load_context.rb b/app/contexts/merge_requests_load_context.rb index 4ec66cd9b78..683f4c83e77 100644 --- a/app/contexts/merge_requests_load_context.rb +++ b/app/contexts/merge_requests_load_context.rb @@ -14,7 +14,7 @@ class MergeRequestsLoadContext < BaseContext end merge_requests = merge_requests.page(params[:page]).per(20) - merge_requests = merge_requests.includes(:author, :project).order("closed, created_at desc") + merge_requests = merge_requests.includes(:author, :project).order("state, created_at desc") # Filter by specific assignee_id (or lack thereof)? if params[:assignee_id].present? diff --git a/app/controllers/merge_requests_controller.rb b/app/controllers/merge_requests_controller.rb index c1cf66d5656..bf6652726d2 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -80,7 +80,7 @@ class MergeRequestsController < ProjectResourceController def automerge return access_denied! unless can?(current_user, :accept_mr, @project) - if @merge_request.open? && @merge_request.can_be_merged? + if @merge_request.opened? && @merge_request.can_be_merged? @merge_request.should_remove_source_branch = params[:should_remove_source_branch] @merge_request.automerge!(current_user) @status = true diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index ca0a89c3749..155d03d1147 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -12,7 +12,7 @@ module MergeRequestsHelper def mr_css_classes mr classes = "merge_request" - classes << " closed" if mr.closed + classes << " closed" if mr.closed? classes << " merged" if mr.merged? classes end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index cf4f75aaf41..67f3fb5162a 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -9,15 +9,14 @@ # author_id :integer # assignee_id :integer # title :string(255) -# closed :boolean default(FALSE), not null +# state :string(255) not null # created_at :datetime not null # updated_at :datetime not null # st_commits :text(2147483647) # st_diffs :text(2147483647) -# merged :boolean default(FALSE), not null # merge_status :integer default(1), not null -# milestone_id :integer # +# milestone_id :integer require Rails.root.join("app/models/commit") require Rails.root.join("lib/static_model") @@ -25,11 +24,33 @@ require Rails.root.join("lib/static_model") class MergeRequest < ActiveRecord::Base include Issuable - attr_accessible :title, :assignee_id, :closed, :target_branch, :source_branch, :milestone_id, + attr_accessible :title, :assignee_id, :target_branch, :source_branch, :milestone_id, :author_id_of_changes attr_accessor :should_remove_source_branch + state_machine :state, :initial => :opened do + event :close do + transition [:reopened, :opened] => :closed + end + + event :merge do + transition [:reopened, :opened] => :merged + end + + event :reopen do + transition :closed => :reopened + end + + state :opened + + state :reopened + + state :closed + + state :merged + end + BROKEN_DIFF = "--broken-diff" UNCHECKED = 1 @@ -43,6 +64,8 @@ class MergeRequest < ActiveRecord::Base validates :target_branch, presence: true validate :validate_branches + scope :merged, -> { with_state(:merged) } + def self.find_all_by_branch(branch_name) where("source_branch LIKE :branch OR target_branch LIKE :branch", branch: branch_name) end @@ -98,7 +121,7 @@ class MergeRequest < ActiveRecord::Base end def reloaded_diffs - if open? && unmerged_diffs.any? + if opened? && unmerged_diffs.any? self.st_diffs = unmerged_diffs self.save end @@ -128,10 +151,6 @@ class MergeRequest < ActiveRecord::Base commits.first end - def merged? - merged && merge_event - end - def merge_event self.project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::MERGED).last end @@ -146,17 +165,7 @@ class MergeRequest < ActiveRecord::Base def probably_merged? unmerged_commits.empty? && - commits.any? && open? - end - - def open? - !closed - end - - def mark_as_merged! - self.merged = true - self.closed = true - save + commits.any? && opened? end def mark_as_unmergable @@ -165,7 +174,7 @@ class MergeRequest < ActiveRecord::Base end def reloaded_commits - if open? && unmerged_commits.any? + if opened? && unmerged_commits.any? self.st_commits = unmerged_commits save end @@ -181,7 +190,8 @@ class MergeRequest < ActiveRecord::Base end def merge!(user_id) - self.mark_as_merged! + self.merge + Event.create( project: self.project, action: Event::MERGED, diff --git a/app/observers/merge_request_observer.rb b/app/observers/merge_request_observer.rb index 6d3c2bdd186..d89e7734c1a 100644 --- a/app/observers/merge_request_observer.rb +++ b/app/observers/merge_request_observer.rb @@ -7,15 +7,20 @@ class MergeRequestObserver < ActiveRecord::Observer end end - def after_update(merge_request) + def after_close(merge_request, transition) send_reassigned_email(merge_request) if merge_request.is_being_reassigned? - status = nil - status = 'closed' if merge_request.is_being_closed? - status = 'reopened' if merge_request.is_being_reopened? - if status - Note.create_status_change_note(merge_request, current_user, status) - end + Note.create_status_change_note(merge_request, current_user, merge_request.state) + end + + def after_reopen(merge_request, transition) + send_reassigned_email(merge_request) if merge_request.is_being_reassigned? + + Note.create_status_change_note(merge_request, current_user, merge_request.state) + end + + def after_update(merge_request) + send_reassigned_email(merge_request) if merge_request.is_being_reassigned? end protected diff --git a/app/views/merge_requests/show/_mr_accept.html.haml b/app/views/merge_requests/show/_mr_accept.html.haml index c2c04b863e7..64f25a5118c 100644 --- a/app/views/merge_requests/show/_mr_accept.html.haml +++ b/app/views/merge_requests/show/_mr_accept.html.haml @@ -3,7 +3,7 @@ %strong Only masters can accept MR -- if @merge_request.open? && @commits.any? && can?(current_user, :accept_mr, @project) +- if @merge_request.opened? && @commits.any? && can?(current_user, :accept_mr, @project) .automerge_widget.can_be_merged{style: "display:none"} .alert.alert-success %span diff --git a/app/views/merge_requests/show/_mr_box.html.haml b/app/views/merge_requests/show/_mr_box.html.haml index 644d7fcc58e..fdc61a97924 100644 --- a/app/views/merge_requests/show/_mr_box.html.haml +++ b/app/views/merge_requests/show/_mr_box.html.haml @@ -1,11 +1,11 @@ .ui-box.ui-box-show .ui-box-head %h4.box-title - - if @merge_request.merged + - if @merge_request.merged? .error.status_info %i.icon-ok Merged - - elsif @merge_request.closed + - elsif @merge_request.closed? .error.status_info Closed = gfm escape_once(@merge_request.title) @@ -21,7 +21,7 @@ %strong= link_to_gfm truncate(milestone.title, length: 20), project_milestone_path(milestone.project, milestone) - - if @merge_request.closed + - if @merge_request.closed? .ui-box-bottom - if @merge_request.merged? %span diff --git a/app/views/merge_requests/show/_mr_ci.html.haml b/app/views/merge_requests/show/_mr_ci.html.haml index dd1e78a0205..a8faa6ba617 100644 --- a/app/views/merge_requests/show/_mr_ci.html.haml +++ b/app/views/merge_requests/show/_mr_ci.html.haml @@ -1,4 +1,4 @@ -- if @merge_request.open? && @commits.any? +- if @merge_request.opened? && @commits.any? .ci_widget.ci-success{style: "display:none"} .alert.alert-success %i.icon-ok diff --git a/app/views/merge_requests/show/_mr_title.html.haml b/app/views/merge_requests/show/_mr_title.html.haml index 8119728dcb9..26a8296d96b 100644 --- a/app/views/merge_requests/show/_mr_title.html.haml +++ b/app/views/merge_requests/show/_mr_title.html.haml @@ -7,7 +7,7 @@ %span.pull-right - if can?(current_user, :modify_merge_request, @merge_request) - - if @merge_request.open? + - if @merge_request.opened? .left.btn-group %a.btn.grouped.dropdown-toggle{ data: {toggle: :dropdown} } %i.icon-download-alt diff --git a/app/views/milestones/show.html.haml b/app/views/milestones/show.html.haml index 43d82a54dd6..3133c2011ba 100644 --- a/app/views/milestones/show.html.haml +++ b/app/views/milestones/show.html.haml @@ -77,7 +77,7 @@ %li=link_to('All Merge Requests', '#') %ul.well-list - @merge_requests.each do |merge_request| - %li{data: {closed: merge_request.closed}} + %li{data: {closed: merge_request.closed?}} = link_to [@project, merge_request] do %span.badge.badge-info ##{merge_request.id} – diff --git a/spec/factories.rb b/spec/factories.rb index d2e9f48c47b..92d250d6d60 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -67,10 +67,6 @@ FactoryGirl.define do source_branch "master" target_branch "stable" - trait :closed do - closed true - end - # pick 3 commits "at random" (from bcf03b5d~3 to bcf03b5d) trait :with_diffs do target_branch "master" # pretend bcf03b5d~3 @@ -85,7 +81,16 @@ FactoryGirl.define do end end + trait :closed do + state :closed + end + + trait :reopened do + state :reopened + end + factory :closed_merge_request, traits: [:closed] + factory :reopened_merge_request, traits: [:reopened] factory :merge_request_with_diffs, traits: [:with_diffs] end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a7799f68604..e61bf44ce53 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -37,6 +37,10 @@ describe MergeRequest do end describe "#mr_and_commit_notes" do + + end + + describe "#mr_and_commit_notes" do let!(:merge_request) { create(:merge_request) } before do diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 2ea2c56a6f7..8871bf4a156 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -7,7 +7,7 @@ # project_id :integer not null # description :text # due_date :date -# closed :boolean default(FALSE), not null +# state :string default(FALSE), not null # created_at :datetime not null # updated_at :datetime not null # diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 4b620a2fa3e..5c27f363401 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -121,10 +121,7 @@ describe Project do let(:project) { create(:project) } before do - @merge_request = create(:merge_request, - project: project, - merged: false, - closed: false) + @merge_request = create(:merge_request, project: project) @key = create(:key, user_id: project.owner.id) end @@ -133,8 +130,7 @@ describe Project do @merge_request.last_commit.id.should == "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a" project.update_merge_requests("8716fc78f3c65bbf7bcf7b574febd583bc5d2812", "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a", "refs/heads/stable", @key.user) @merge_request.reload - @merge_request.merged.should be_true - @merge_request.closed.should be_true + @merge_request.merged?.should be_true end it "should update merge request commits with new one if pushed to source branch" do diff --git a/spec/observers/merge_request_observer_spec.rb b/spec/observers/merge_request_observer_spec.rb index 4841bf88fc5..9d702107a89 100644 --- a/spec/observers/merge_request_observer_spec.rb +++ b/spec/observers/merge_request_observer_spec.rb @@ -1,10 +1,14 @@ require 'spec_helper' describe MergeRequestObserver do - let(:some_user) { double(:user, id: 1) } - let(:assignee) { double(:user, id: 2) } - let(:author) { double(:user, id: 3) } - let(:mr) { double(:merge_request, id: 42, assignee: assignee, author: author) } + let(:some_user) { create :user } + let(:assignee) { create :user } + let(:author) { create :user } + let(:mr_mock) { double(:merge_request, id: 42, assignee: assignee, author: author) } + let(:assigned_mr) { create(:merge_request, assignee: assignee, author: author) } + let(:unassigned_mr) { create(:merge_request, author: author) } + let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author) } + let(:closed_unassigned_mr) { create(:closed_merge_request, author: author) } before(:each) { subject.stub(:current_user).and_return(some_user) } @@ -21,23 +25,21 @@ describe MergeRequestObserver do end it 'sends an email to the assignee' do - Notify.should_receive(:new_merge_request_email).with(mr.id) - subject.after_create(mr) + Notify.should_receive(:new_merge_request_email).with(mr_mock.id) + subject.after_create(mr_mock) end it 'does not send an email to the assignee if assignee created the merge request' do subject.stub(:current_user).and_return(assignee) Notify.should_not_receive(:new_merge_request_email) - subject.after_create(mr) + subject.after_create(mr_mock) end end context '#after_update' do before(:each) do - mr.stub(:is_being_reassigned?).and_return(false) - mr.stub(:is_being_closed?).and_return(false) - mr.stub(:is_being_reopened?).and_return(false) + mr_mock.stub(:is_being_reassigned?).and_return(false) end it 'is called when a merge request is changed' do @@ -52,97 +54,50 @@ describe MergeRequestObserver do context 'a reassigned email' do it 'is sent if the merge request is being reassigned' do - mr.should_receive(:is_being_reassigned?).and_return(true) - subject.should_receive(:send_reassigned_email).with(mr) + mr_mock.should_receive(:is_being_reassigned?).and_return(true) + subject.should_receive(:send_reassigned_email).with(mr_mock) - subject.after_update(mr) + subject.after_update(mr_mock) end it 'is not sent if the merge request is not being reassigned' do - mr.should_receive(:is_being_reassigned?).and_return(false) + mr_mock.should_receive(:is_being_reassigned?).and_return(false) subject.should_not_receive(:send_reassigned_email) - subject.after_update(mr) + subject.after_update(mr_mock) end end + end + + context '#after_close' do context 'a status "closed"' do it 'note is created if the merge request is being closed' do - mr.should_receive(:is_being_closed?).and_return(true) - Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed') - - subject.after_update(mr) - end - - it 'note is not created if the merge request is not being closed' do - mr.should_receive(:is_being_closed?).and_return(false) - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed') - - subject.after_update(mr) - end - - it 'notification is delivered if the merge request being closed' do - mr.stub(:is_being_closed?).and_return(true) - Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed') - - subject.after_update(mr) - end - - it 'notification is not delivered if the merge request not being closed' do - mr.stub(:is_being_closed?).and_return(false) - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed') + Note.should_receive(:create_status_change_note).with(assigned_mr, some_user, 'closed') - subject.after_update(mr) + assigned_mr.close end it 'notification is delivered only to author if the merge request is being closed' do - mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil) - mr_without_assignee.stub(:is_being_reassigned?).and_return(false) - mr_without_assignee.stub(:is_being_closed?).and_return(true) - mr_without_assignee.stub(:is_being_reopened?).and_return(false) - Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'closed') + Note.should_receive(:create_status_change_note).with(unassigned_mr, some_user, 'closed') - subject.after_update(mr_without_assignee) + unassigned_mr.close end end + end + context '#after_reopen' do context 'a status "reopened"' do it 'note is created if the merge request is being reopened' do - mr.should_receive(:is_being_reopened?).and_return(true) - Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened') - - subject.after_update(mr) - end - - it 'note is not created if the merge request is not being reopened' do - mr.should_receive(:is_being_reopened?).and_return(false) - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened') - - subject.after_update(mr) - end - - it 'notification is delivered if the merge request being reopened' do - mr.stub(:is_being_reopened?).and_return(true) - Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened') - - subject.after_update(mr) - end - - it 'notification is not delivered if the merge request is not being reopened' do - mr.stub(:is_being_reopened?).and_return(false) - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened') + Note.should_receive(:create_status_change_note).with(closed_assigned_mr, some_user, 'reopened') - subject.after_update(mr) + closed_assigned_mr.reopen end it 'notification is delivered only to author if the merge request is being reopened' do - mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil) - mr_without_assignee.stub(:is_being_reassigned?).and_return(false) - mr_without_assignee.stub(:is_being_closed?).and_return(false) - mr_without_assignee.stub(:is_being_reopened?).and_return(true) - Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'reopened') + Note.should_receive(:create_status_change_note).with(closed_unassigned_mr, some_user, 'reopened') - subject.after_update(mr_without_assignee) + closed_unassigned_mr.reopen end end end @@ -151,23 +106,23 @@ describe MergeRequestObserver do let(:previous_assignee) { double(:user, id: 3) } before(:each) do - mr.stub(:assignee_id).and_return(assignee.id) - mr.stub(:assignee_id_was).and_return(previous_assignee.id) + mr_mock.stub(:assignee_id).and_return(assignee.id) + mr_mock.stub(:assignee_id_was).and_return(previous_assignee.id) end def it_sends_a_reassigned_email_to(recipient) - Notify.should_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id) + Notify.should_receive(:reassigned_merge_request_email).with(recipient, mr_mock.id, previous_assignee.id) end def it_does_not_send_a_reassigned_email_to(recipient) - Notify.should_not_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id) + Notify.should_not_receive(:reassigned_merge_request_email).with(recipient, mr_mock.id, previous_assignee.id) end it 'sends a reassigned email to the previous and current assignees' do it_sends_a_reassigned_email_to assignee.id it_sends_a_reassigned_email_to previous_assignee.id - subject.send(:send_reassigned_email, mr) + subject.send(:send_reassigned_email, mr_mock) end context 'does not send an email to the user who made the reassignment' do @@ -176,14 +131,14 @@ describe MergeRequestObserver do it_sends_a_reassigned_email_to previous_assignee.id it_does_not_send_a_reassigned_email_to assignee.id - subject.send(:send_reassigned_email, mr) + subject.send(:send_reassigned_email, mr_mock) end it 'if the user is the previous assignee' do subject.stub(:current_user).and_return(previous_assignee) it_sends_a_reassigned_email_to assignee.id it_does_not_send_a_reassigned_email_to previous_assignee.id - subject.send(:send_reassigned_email, mr) + subject.send(:send_reassigned_email, mr_mock) end end end |