Download as
Browse Code »

Commit b45e9aefd3207c00f9d83a0cbfcca457c9562a59

Authored by Andrew8xx8
1 parent f9729659
Exists in master and in 4 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb, spb-stable

Merge Request uses StateMachine now

Showing 15 changed files with 107 additions and 132 deletions   Show diff stats
app/contexts/merge_requests_load_context.rb
  Diff comments   View file @b45e9ae
... ... @@ -14,7 +14,7 @@ class MergeRequestsLoadContext < BaseContext
14 14 end
15 15  
16 16 merge_requests = merge_requests.page(params[:page]).per(20)
17   - merge_requests = merge_requests.includes(:author, :project).order("closed, created_at desc")
  17 + merge_requests = merge_requests.includes(:author, :project).order("state, created_at desc")
18 18  
19 19 # Filter by specific assignee_id (or lack thereof)?
20 20 if params[:assignee_id].present?
... ...
app/controllers/merge_requests_controller.rb
  Diff comments   View file @b45e9ae
... ... @@ -80,7 +80,7 @@ class MergeRequestsController < ProjectResourceController
80 80  
81 81 def automerge
82 82 return access_denied! unless can?(current_user, :accept_mr, @project)
83   - if @merge_request.open? && @merge_request.can_be_merged?
  83 + if @merge_request.opened? && @merge_request.can_be_merged?
84 84 @merge_request.should_remove_source_branch = params[:should_remove_source_branch]
85 85 @merge_request.automerge!(current_user)
86 86 @status = true
... ...
app/helpers/merge_requests_helper.rb
  Diff comments   View file @b45e9ae
... ... @@ -12,7 +12,7 @@ module MergeRequestsHelper
12 12  
13 13 def mr_css_classes mr
14 14 classes = "merge_request"
15   - classes << " closed" if mr.closed
  15 + classes << " closed" if mr.closed?
16 16 classes << " merged" if mr.merged?
17 17 classes
18 18 end
... ...
app/models/merge_request.rb
  Diff comments   View file @b45e9ae
... ... @@ -9,15 +9,14 @@
9 9 # author_id :integer
10 10 # assignee_id :integer
11 11 # title :string(255)
12   -# closed :boolean default(FALSE), not null
  12 +# state :string(255) not null
13 13 # created_at :datetime not null
14 14 # updated_at :datetime not null
15 15 # st_commits :text(2147483647)
16 16 # st_diffs :text(2147483647)
17   -# merged :boolean default(FALSE), not null
18 17 # merge_status :integer default(1), not null
19   -# milestone_id :integer
20 18 #
  19 +# milestone_id :integer
21 20  
22 21 require Rails.root.join("app/models/commit")
23 22 require Rails.root.join("lib/static_model")
... ... @@ -25,11 +24,33 @@ require Rails.root.join(&quot;lib/static_model&quot;)
25 24 class MergeRequest < ActiveRecord::Base
26 25 include Issuable
27 26  
28   - attr_accessible :title, :assignee_id, :closed, :target_branch, :source_branch, :milestone_id,
  27 + attr_accessible :title, :assignee_id, :target_branch, :source_branch, :milestone_id,
29 28 :author_id_of_changes
30 29  
31 30 attr_accessor :should_remove_source_branch
32 31  
  32 + state_machine :state, :initial => :opened do
  33 + event :close do
  34 + transition [:reopened, :opened] => :closed
  35 + end
  36 +
  37 + event :merge do
  38 + transition [:reopened, :opened] => :merged
  39 + end
  40 +
  41 + event :reopen do
  42 + transition :closed => :reopened
  43 + end
  44 +
  45 + state :opened
  46 +
  47 + state :reopened
  48 +
  49 + state :closed
  50 +
  51 + state :merged
  52 + end
  53 +
33 54 BROKEN_DIFF = "--broken-diff"
34 55  
35 56 UNCHECKED = 1
... ... @@ -43,6 +64,8 @@ class MergeRequest &lt; ActiveRecord::Base
43 64 validates :target_branch, presence: true
44 65 validate :validate_branches
45 66  
  67 + scope :merged, -> { with_state(:merged) }
  68 +
46 69 def self.find_all_by_branch(branch_name)
47 70 where("source_branch LIKE :branch OR target_branch LIKE :branch", branch: branch_name)
48 71 end
... ... @@ -98,7 +121,7 @@ class MergeRequest &lt; ActiveRecord::Base
98 121 end
99 122  
100 123 def reloaded_diffs
101   - if open? && unmerged_diffs.any?
  124 + if opened? && unmerged_diffs.any?
102 125 self.st_diffs = unmerged_diffs
103 126 self.save
104 127 end
... ... @@ -128,10 +151,6 @@ class MergeRequest &lt; ActiveRecord::Base
128 151 commits.first
129 152 end
130 153  
131   - def merged?
132   - merged && merge_event
133   - end
134   -
135 154 def merge_event
136 155 self.project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::MERGED).last
137 156 end
... ... @@ -146,17 +165,7 @@ class MergeRequest &lt; ActiveRecord::Base
146 165  
147 166 def probably_merged?
148 167 unmerged_commits.empty? &&
149   - commits.any? && open?
150   - end
151   -
152   - def open?
153   - !closed
154   - end
155   -
156   - def mark_as_merged!
157   - self.merged = true
158   - self.closed = true
159   - save
  168 + commits.any? && opened?
160 169 end
161 170  
162 171 def mark_as_unmergable
... ... @@ -165,7 +174,7 @@ class MergeRequest &lt; ActiveRecord::Base
165 174 end
166 175  
167 176 def reloaded_commits
168   - if open? && unmerged_commits.any?
  177 + if opened? && unmerged_commits.any?
169 178 self.st_commits = unmerged_commits
170 179 save
171 180 end
... ... @@ -181,7 +190,8 @@ class MergeRequest &lt; ActiveRecord::Base
181 190 end
182 191  
183 192 def merge!(user_id)
184   - self.mark_as_merged!
  193 + self.merge
  194 +
185 195 Event.create(
186 196 project: self.project,
187 197 action: Event::MERGED,
... ...
app/observers/merge_request_observer.rb
  Diff comments   View file @b45e9ae
... ... @@ -7,15 +7,20 @@ class MergeRequestObserver &lt; ActiveRecord::Observer
7 7 end
8 8 end
9 9  
10   - def after_update(merge_request)
  10 + def after_close(merge_request, transition)
11 11 send_reassigned_email(merge_request) if merge_request.is_being_reassigned?
12 12  
13   - status = nil
14   - status = 'closed' if merge_request.is_being_closed?
15   - status = 'reopened' if merge_request.is_being_reopened?
16   - if status
17   - Note.create_status_change_note(merge_request, current_user, status)
18   - end
  13 + Note.create_status_change_note(merge_request, current_user, merge_request.state)
  14 + end
  15 +
  16 + def after_reopen(merge_request, transition)
  17 + send_reassigned_email(merge_request) if merge_request.is_being_reassigned?
  18 +
  19 + Note.create_status_change_note(merge_request, current_user, merge_request.state)
  20 + end
  21 +
  22 + def after_update(merge_request)
  23 + send_reassigned_email(merge_request) if merge_request.is_being_reassigned?
19 24 end
20 25  
21 26 protected
... ...
app/views/merge_requests/show/_mr_accept.html.haml
  Diff comments   View file @b45e9ae
... ... @@ -3,7 +3,7 @@
3 3 %strong Only masters can accept MR
4 4  
5 5  
6   -- if @merge_request.open? && @commits.any? && can?(current_user, :accept_mr, @project)
  6 +- if @merge_request.opened? && @commits.any? && can?(current_user, :accept_mr, @project)
7 7 .automerge_widget.can_be_merged{style: "display:none"}
8 8 .alert.alert-success
9 9 %span
... ...
app/views/merge_requests/show/_mr_box.html.haml
  Diff comments   View file @b45e9ae
1 1 .ui-box.ui-box-show
2 2 .ui-box-head
3 3 %h4.box-title
4   - - if @merge_request.merged
  4 + - if @merge_request.merged?
5 5 .error.status_info
6 6 %i.icon-ok
7 7 Merged
8   - - elsif @merge_request.closed
  8 + - elsif @merge_request.closed?
9 9 .error.status_info Closed
10 10 = gfm escape_once(@merge_request.title)
11 11  
... ... @@ -21,7 +21,7 @@
21 21 %strong= link_to_gfm truncate(milestone.title, length: 20), project_milestone_path(milestone.project, milestone)
22 22  
23 23  
24   - - if @merge_request.closed
  24 + - if @merge_request.closed?
25 25 .ui-box-bottom
26 26 - if @merge_request.merged?
27 27 %span
... ...
app/views/merge_requests/show/_mr_ci.html.haml
  Diff comments   View file @b45e9ae
1   -- if @merge_request.open? && @commits.any?
  1 +- if @merge_request.opened? && @commits.any?
2 2 .ci_widget.ci-success{style: "display:none"}
3 3 .alert.alert-success
4 4 %i.icon-ok
... ...
app/views/merge_requests/show/_mr_title.html.haml
  Diff comments   View file @b45e9ae
... ... @@ -7,7 +7,7 @@
7 7  
8 8 %span.pull-right
9 9 - if can?(current_user, :modify_merge_request, @merge_request)
10   - - if @merge_request.open?
  10 + - if @merge_request.opened?
11 11 .left.btn-group
12 12 %a.btn.grouped.dropdown-toggle{ data: {toggle: :dropdown} }
13 13 %i.icon-download-alt
... ...
app/views/milestones/show.html.haml
  Diff comments   View file @b45e9ae
... ... @@ -77,7 +77,7 @@
77 77 %li=link_to('All Merge Requests', '#')
78 78 %ul.well-list
79 79 - @merge_requests.each do |merge_request|
80   - %li{data: {closed: merge_request.closed}}
  80 + %li{data: {closed: merge_request.closed?}}
81 81 = link_to [@project, merge_request] do
82 82 %span.badge.badge-info ##{merge_request.id}
83 83 &ndash;
... ...
spec/factories.rb
  Diff comments   View file @b45e9ae
... ... @@ -67,10 +67,6 @@ FactoryGirl.define do
67 67 source_branch "master"
68 68 target_branch "stable"
69 69  
70   - trait :closed do
71   - closed true
72   - end
73   -
74 70 # pick 3 commits "at random" (from bcf03b5d~3 to bcf03b5d)
75 71 trait :with_diffs do
76 72 target_branch "master" # pretend bcf03b5d~3
... ... @@ -85,7 +81,16 @@ FactoryGirl.define do
85 81 end
86 82 end
87 83  
  84 + trait :closed do
  85 + state :closed
  86 + end
  87 +
  88 + trait :reopened do
  89 + state :reopened
  90 + end
  91 +
88 92 factory :closed_merge_request, traits: [:closed]
  93 + factory :reopened_merge_request, traits: [:reopened]
89 94 factory :merge_request_with_diffs, traits: [:with_diffs]
90 95 end
91 96  
... ...
spec/models/merge_request_spec.rb
  Diff comments   View file @b45e9ae
... ... @@ -37,6 +37,10 @@ describe MergeRequest do
37 37 end
38 38  
39 39 describe "#mr_and_commit_notes" do
  40 +
  41 + end
  42 +
  43 + describe "#mr_and_commit_notes" do
40 44 let!(:merge_request) { create(:merge_request) }
41 45  
42 46 before do
... ...
spec/models/milestone_spec.rb
  Diff comments   View file @b45e9ae
... ... @@ -7,7 +7,7 @@
7 7 # project_id :integer not null
8 8 # description :text
9 9 # due_date :date
10   -# closed :boolean default(FALSE), not null
  10 +# state :string default(FALSE), not null
11 11 # created_at :datetime not null
12 12 # updated_at :datetime not null
13 13 #
... ...
spec/models/project_spec.rb
  Diff comments   View file @b45e9ae
... ... @@ -121,10 +121,7 @@ describe Project do
121 121 let(:project) { create(:project) }
122 122  
123 123 before do
124   - @merge_request = create(:merge_request,
125   - project: project,
126   - merged: false,
127   - closed: false)
  124 + @merge_request = create(:merge_request, project: project)
128 125 @key = create(:key, user_id: project.owner.id)
129 126 end
130 127  
... ... @@ -133,8 +130,7 @@ describe Project do
133 130 @merge_request.last_commit.id.should == "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a"
134 131 project.update_merge_requests("8716fc78f3c65bbf7bcf7b574febd583bc5d2812", "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a", "refs/heads/stable", @key.user)
135 132 @merge_request.reload
136   - @merge_request.merged.should be_true
137   - @merge_request.closed.should be_true
  133 + @merge_request.merged?.should be_true
138 134 end
139 135  
140 136 it "should update merge request commits with new one if pushed to source branch" do
... ...
spec/observers/merge_request_observer_spec.rb
  Diff comments   View file @b45e9ae
1 1 require 'spec_helper'
2 2  
3 3 describe MergeRequestObserver do
4   - let(:some_user) { double(:user, id: 1) }
5   - let(:assignee) { double(:user, id: 2) }
6   - let(:author) { double(:user, id: 3) }
7   - let(:mr) { double(:merge_request, id: 42, assignee: assignee, author: author) }
  4 + let(:some_user) { create :user }
  5 + let(:assignee) { create :user }
  6 + let(:author) { create :user }
  7 + let(:mr_mock) { double(:merge_request, id: 42, assignee: assignee, author: author) }
  8 + let(:assigned_mr) { create(:merge_request, assignee: assignee, author: author) }
  9 + let(:unassigned_mr) { create(:merge_request, author: author) }
  10 + let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author) }
  11 + let(:closed_unassigned_mr) { create(:closed_merge_request, author: author) }
8 12  
9 13 before(:each) { subject.stub(:current_user).and_return(some_user) }
10 14  
... ... @@ -21,23 +25,21 @@ describe MergeRequestObserver do
21 25 end
22 26  
23 27 it 'sends an email to the assignee' do
24   - Notify.should_receive(:new_merge_request_email).with(mr.id)
25   - subject.after_create(mr)
  28 + Notify.should_receive(:new_merge_request_email).with(mr_mock.id)
  29 + subject.after_create(mr_mock)
26 30 end
27 31  
28 32 it 'does not send an email to the assignee if assignee created the merge request' do
29 33 subject.stub(:current_user).and_return(assignee)
30 34 Notify.should_not_receive(:new_merge_request_email)
31 35  
32   - subject.after_create(mr)
  36 + subject.after_create(mr_mock)
33 37 end
34 38 end
35 39  
36 40 context '#after_update' do
37 41 before(:each) do
38   - mr.stub(:is_being_reassigned?).and_return(false)
39   - mr.stub(:is_being_closed?).and_return(false)
40   - mr.stub(:is_being_reopened?).and_return(false)
  42 + mr_mock.stub(:is_being_reassigned?).and_return(false)
41 43 end
42 44  
43 45 it 'is called when a merge request is changed' do
... ... @@ -52,97 +54,50 @@ describe MergeRequestObserver do
52 54  
53 55 context 'a reassigned email' do
54 56 it 'is sent if the merge request is being reassigned' do
55   - mr.should_receive(:is_being_reassigned?).and_return(true)
56   - subject.should_receive(:send_reassigned_email).with(mr)
  57 + mr_mock.should_receive(:is_being_reassigned?).and_return(true)
  58 + subject.should_receive(:send_reassigned_email).with(mr_mock)
57 59  
58   - subject.after_update(mr)
  60 + subject.after_update(mr_mock)
59 61 end
60 62  
61 63 it 'is not sent if the merge request is not being reassigned' do
62   - mr.should_receive(:is_being_reassigned?).and_return(false)
  64 + mr_mock.should_receive(:is_being_reassigned?).and_return(false)
63 65 subject.should_not_receive(:send_reassigned_email)
64 66  
65   - subject.after_update(mr)
  67 + subject.after_update(mr_mock)
66 68 end
67 69 end
68 70  
  71 + end
  72 +
  73 + context '#after_close' do
69 74 context 'a status "closed"' do
70 75 it 'note is created if the merge request is being closed' do
71   - mr.should_receive(:is_being_closed?).and_return(true)
72   - Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed')
73   -
74   - subject.after_update(mr)
75   - end
76   -
77   - it 'note is not created if the merge request is not being closed' do
78   - mr.should_receive(:is_being_closed?).and_return(false)
79   - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed')
80   -
81   - subject.after_update(mr)
82   - end
83   -
84   - it 'notification is delivered if the merge request being closed' do
85   - mr.stub(:is_being_closed?).and_return(true)
86   - Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed')
87   -
88   - subject.after_update(mr)
89   - end
90   -
91   - it 'notification is not delivered if the merge request not being closed' do
92   - mr.stub(:is_being_closed?).and_return(false)
93   - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed')
  76 + Note.should_receive(:create_status_change_note).with(assigned_mr, some_user, 'closed')
94 77  
95   - subject.after_update(mr)
  78 + assigned_mr.close
96 79 end
97 80  
98 81 it 'notification is delivered only to author if the merge request is being closed' do
99   - mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil)
100   - mr_without_assignee.stub(:is_being_reassigned?).and_return(false)
101   - mr_without_assignee.stub(:is_being_closed?).and_return(true)
102   - mr_without_assignee.stub(:is_being_reopened?).and_return(false)
103   - Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'closed')
  82 + Note.should_receive(:create_status_change_note).with(unassigned_mr, some_user, 'closed')
104 83  
105   - subject.after_update(mr_without_assignee)
  84 + unassigned_mr.close
106 85 end
107 86 end
  87 + end
108 88  
  89 + context '#after_reopen' do
109 90 context 'a status "reopened"' do
110 91 it 'note is created if the merge request is being reopened' do
111   - mr.should_receive(:is_being_reopened?).and_return(true)
112   - Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened')
113   -
114   - subject.after_update(mr)
115   - end
116   -
117   - it 'note is not created if the merge request is not being reopened' do
118   - mr.should_receive(:is_being_reopened?).and_return(false)
119   - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened')
120   -
121   - subject.after_update(mr)
122   - end
123   -
124   - it 'notification is delivered if the merge request being reopened' do
125   - mr.stub(:is_being_reopened?).and_return(true)
126   - Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened')
127   -
128   - subject.after_update(mr)
129   - end
130   -
131   - it 'notification is not delivered if the merge request is not being reopened' do
132   - mr.stub(:is_being_reopened?).and_return(false)
133   - Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened')
  92 + Note.should_receive(:create_status_change_note).with(closed_assigned_mr, some_user, 'reopened')
134 93  
135   - subject.after_update(mr)
  94 + closed_assigned_mr.reopen
136 95 end
137 96  
138 97 it 'notification is delivered only to author if the merge request is being reopened' do
139   - mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil)
140   - mr_without_assignee.stub(:is_being_reassigned?).and_return(false)
141   - mr_without_assignee.stub(:is_being_closed?).and_return(false)
142   - mr_without_assignee.stub(:is_being_reopened?).and_return(true)
143   - Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'reopened')
  98 + Note.should_receive(:create_status_change_note).with(closed_unassigned_mr, some_user, 'reopened')
144 99  
145   - subject.after_update(mr_without_assignee)
  100 + closed_unassigned_mr.reopen
146 101 end
147 102 end
148 103 end
... ... @@ -151,23 +106,23 @@ describe MergeRequestObserver do
151 106 let(:previous_assignee) { double(:user, id: 3) }
152 107  
153 108 before(:each) do
154   - mr.stub(:assignee_id).and_return(assignee.id)
155   - mr.stub(:assignee_id_was).and_return(previous_assignee.id)
  109 + mr_mock.stub(:assignee_id).and_return(assignee.id)
  110 + mr_mock.stub(:assignee_id_was).and_return(previous_assignee.id)
156 111 end
157 112  
158 113 def it_sends_a_reassigned_email_to(recipient)
159   - Notify.should_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id)
  114 + Notify.should_receive(:reassigned_merge_request_email).with(recipient, mr_mock.id, previous_assignee.id)
160 115 end
161 116  
162 117 def it_does_not_send_a_reassigned_email_to(recipient)
163   - Notify.should_not_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id)
  118 + Notify.should_not_receive(:reassigned_merge_request_email).with(recipient, mr_mock.id, previous_assignee.id)
164 119 end
165 120  
166 121 it 'sends a reassigned email to the previous and current assignees' do
167 122 it_sends_a_reassigned_email_to assignee.id
168 123 it_sends_a_reassigned_email_to previous_assignee.id
169 124  
170   - subject.send(:send_reassigned_email, mr)
  125 + subject.send(:send_reassigned_email, mr_mock)
171 126 end
172 127  
173 128 context 'does not send an email to the user who made the reassignment' do
... ... @@ -176,14 +131,14 @@ describe MergeRequestObserver do
176 131 it_sends_a_reassigned_email_to previous_assignee.id
177 132 it_does_not_send_a_reassigned_email_to assignee.id
178 133  
179   - subject.send(:send_reassigned_email, mr)
  134 + subject.send(:send_reassigned_email, mr_mock)
180 135 end
181 136 it 'if the user is the previous assignee' do
182 137 subject.stub(:current_user).and_return(previous_assignee)
183 138 it_sends_a_reassigned_email_to assignee.id
184 139 it_does_not_send_a_reassigned_email_to previous_assignee.id
185 140  
186   - subject.send(:send_reassigned_email, mr)
  141 + subject.send(:send_reassigned_email, mr_mock)
187 142 end
188 143 end
189 144 end
... ...