diff --git a/Gemfile b/Gemfile index 01696152..4a7db0eb 100644 --- a/Gemfile +++ b/Gemfile @@ -70,6 +70,9 @@ gem "github-markup", "~> 0.7.4", require: 'github/markup' # Servers gem "unicorn", "~> 4.4.0" +# State machine +gem "state_machine" + # Issue tags gem "acts-as-taggable-on", "2.3.3" diff --git a/Gemfile.lock b/Gemfile.lock index 1bc7124f..1b8c5837 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -425,6 +425,7 @@ GEM rack (~> 1.0) tilt (~> 1.1, != 1.3.0) stamp (0.3.0) + state_machine (1.1.2) temple (0.5.5) test_after_commit (0.0.1) therubyracer (0.10.2) @@ -536,6 +537,7 @@ DEPENDENCIES slim spinach-rails stamp + state_machine test_after_commit therubyracer thin diff --git a/app/assets/javascripts/merge_requests.js.coffee b/app/assets/javascripts/merge_requests.js.coffee index 65ed817c..496da731 100644 --- a/app/assets/javascripts/merge_requests.js.coffee +++ b/app/assets/javascripts/merge_requests.js.coffee @@ -27,7 +27,7 @@ class MergeRequest this.$el.find(selector) initMergeWidget: -> - this.showState( @opts.current_state ) + this.showState( @opts.current_status ) if this.$('.automerge_widget').length and @opts.check_enable $.get @opts.url_to_automerge_check, (data) => diff --git a/app/contexts/merge_requests_load_context.rb b/app/contexts/merge_requests_load_context.rb index 4ec66cd9..683f4c83 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 ab6bf595..bf665272 100644 --- a/app/controllers/merge_requests_controller.rb +++ b/app/controllers/merge_requests_controller.rb @@ -73,14 +73,14 @@ class MergeRequestsController < ProjectResourceController if @merge_request.unchecked? @merge_request.check_if_can_be_merged end - render json: {state: @merge_request.human_state} + render json: {merge_status: @merge_request.human_merge_status} rescue Gitlab::SatelliteNotExistError - render json: {state: :no_satellite} + render json: {merge_status: :no_satellite} end 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/controllers/milestones_controller.rb b/app/controllers/milestones_controller.rb index a0c824e8..57f1e9e6 100644 --- a/app/controllers/milestones_controller.rb +++ b/app/controllers/milestones_controller.rb @@ -12,7 +12,7 @@ class MilestonesController < ProjectResourceController def index @milestones = case params[:f] - when 'all'; @project.milestones.order("closed, due_date DESC") + when 'all'; @project.milestones.order("state, due_date DESC") when 'closed'; @project.milestones.closed.order("due_date DESC") else @project.milestones.active.order("due_date ASC") end diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 2825787f..ed7e3e86 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -6,7 +6,7 @@ module IssuesHelper def issue_css_classes issue classes = "issue" - classes << " closed" if issue.closed + classes << " closed" if issue.closed? classes << " today" if issue.today? classes end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index ca0a89c3..155d03d1 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/concerns/issuable.rb b/app/models/concerns/issuable.rb index 645b35ec..85337583 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -17,10 +17,9 @@ module Issuable validates :project, presence: true validates :author, presence: true validates :title, presence: true, length: { within: 0..255 } - validates :closed, inclusion: { in: [true, false] } - scope :opened, -> { where(closed: false) } - scope :closed, -> { where(closed: true) } + scope :opened, -> { with_state(:opened) } + scope :closed, -> { with_state(:closed) } scope :of_group, ->(group) { where(project_id: group.project_ids) } scope :of_user_team, ->(team) { where(project_id: team.project_ids, assignee_id: team.member_ids) } scope :assigned, ->(u) { where(assignee_id: u.id)} @@ -62,14 +61,6 @@ module Issuable assignee_id_changed? end - def is_being_closed? - closed_changed? && closed - end - - def is_being_reopened? - closed_changed? && !closed - end - # # Votes # diff --git a/app/models/issue.rb b/app/models/issue.rb index 04c2df05..112f43c4 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -9,7 +9,7 @@ # project_id :integer # created_at :datetime not null # updated_at :datetime not null -# closed :boolean default(FALSE), not null +# state :string default(FALSE), not null # position :integer default(0) # branch_name :string(255) # description :text @@ -19,8 +19,9 @@ class Issue < ActiveRecord::Base include Issuable - attr_accessible :title, :assignee_id, :closed, :position, :description, - :milestone_id, :label_list, :author_id_of_changes + attr_accessible :title, :assignee_id, :position, :description, + :milestone_id, :label_list, :author_id_of_changes, + :state_event acts_as_taggable_on :labels @@ -33,4 +34,20 @@ class Issue < ActiveRecord::Base opened.assigned(user) end end + + state_machine :state, initial: :opened do + event :close do + transition [:reopened, :opened] => :closed + end + + event :reopen do + transition closed: :reopened + end + + state :opened + + state :reopened + + state :closed + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ac7c9f74..06aa9f3c 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 -# state :integer default(1), not null -# milestone_id :integer +# merge_status :integer default(1), not null # +# 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, - :author_id_of_changes + attr_accessible :title, :assignee_id, :target_branch, :source_branch, :milestone_id, + :author_id_of_changes, :state_event 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,8 +64,13 @@ class MergeRequest < ActiveRecord::Base validates :target_branch, presence: true validate :validate_branches + scope :merged, -> { with_state(: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 @@ -58,13 +84,13 @@ class MergeRequest < ActiveRecord::Base end end - def human_state - states = { + def human_merge_status + merge_statuses = { CAN_BE_MERGED => "can_be_merged", CANNOT_BE_MERGED => "cannot_be_merged", UNCHECKED => "unchecked" } - states[self.state] + merge_statuses[self.merge_status] end def validate_branches @@ -79,20 +105,20 @@ class MergeRequest < ActiveRecord::Base end def unchecked? - state == UNCHECKED + merge_status == UNCHECKED end def mark_as_unchecked - self.state = UNCHECKED + self.merge_status = UNCHECKED self.save end def can_be_merged? - state == CAN_BE_MERGED + merge_status == CAN_BE_MERGED end def check_if_can_be_merged - self.state = if Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged? + self.merge_status = if Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged? CAN_BE_MERGED else CANNOT_BE_MERGED @@ -105,7 +131,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 @@ -135,10 +161,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 @@ -153,26 +175,16 @@ 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 - self.state = CANNOT_BE_MERGED + self.merge_status = CANNOT_BE_MERGED self.save end def reloaded_commits - if open? && unmerged_commits.any? + if opened? && unmerged_commits.any? self.st_commits = unmerged_commits save end @@ -188,7 +200,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/models/milestone.rb b/app/models/milestone.rb index 457fe18f..d822a68d 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -13,19 +13,32 @@ # class Milestone < ActiveRecord::Base - attr_accessible :title, :description, :due_date, :closed, :author_id_of_changes + attr_accessible :title, :description, :due_date, :state_event, :author_id_of_changes attr_accessor :author_id_of_changes belongs_to :project has_many :issues has_many :merge_requests - scope :active, -> { where(closed: false) } - scope :closed, -> { where(closed: true) } + scope :active, -> { with_state(:active) } + scope :closed, -> { with_state(:closed) } validates :title, presence: true validates :project, presence: true - validates :closed, inclusion: { in: [true, false] } + + state_machine :state, initial: :active do + event :close do + transition active: :closed + end + + event :activate do + transition closed: :active + end + + state :closed + + state :active + end def expired? if due_date @@ -68,17 +81,13 @@ class Milestone < ActiveRecord::Base end def can_be_closed? - open? && issues.opened.count.zero? + active? && issues.opened.count.zero? end def is_empty? total_items_count.zero? end - def open? - !closed - end - def author_id author_id_of_changes end diff --git a/app/models/project.rb b/app/models/project.rb index 15b2d858..9d61ffc1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -43,7 +43,7 @@ class Project < ActiveRecord::Base has_many :events, dependent: :destroy has_many :merge_requests, dependent: :destroy - has_many :issues, dependent: :destroy, order: "closed, created_at DESC" + has_many :issues, dependent: :destroy, order: "state, created_at DESC" has_many :milestones, dependent: :destroy has_many :users_projects, dependent: :destroy has_many :notes, dependent: :destroy diff --git a/app/observers/activity_observer.rb b/app/observers/activity_observer.rb index b568bb6b..9c72a6d3 100644 --- a/app/observers/activity_observer.rb +++ b/app/observers/activity_observer.rb @@ -20,15 +20,23 @@ class ActivityObserver < ActiveRecord::Observer end end - def after_save(record) - if record.changed.include?("closed") && record.author_id_of_changes + def after_close(record, transition) Event.create( project: record.project, target_id: record.id, target_type: record.class.name, - action: (record.closed ? Event::CLOSED : Event::REOPENED), + action: Event::CLOSED, + author_id: record.author_id_of_changes + ) + end + + def after_reopen(record, transition) + Event.create( + project: record.project, + target_id: record.id, + target_type: record.class.name, + action: Event::REOPENED, author_id: record.author_id_of_changes ) - end end end diff --git a/app/observers/issue_observer.rb b/app/observers/issue_observer.rb index 262d0f89..592e2950 100644 --- a/app/observers/issue_observer.rb +++ b/app/observers/issue_observer.rb @@ -7,22 +7,31 @@ class IssueObserver < ActiveRecord::Observer end end - def after_update(issue) + def after_close(issue, transition) send_reassigned_email(issue) if issue.is_being_reassigned? - status = nil - status = 'closed' if issue.is_being_closed? - status = 'reopened' if issue.is_being_reopened? - if status - Note.create_status_change_note(issue, current_user, status) - [issue.author, issue.assignee].compact.each do |recipient| - Notify.delay.issue_status_changed_email(recipient.id, issue.id, status, current_user.id) - end - end + create_note(issue) + end + + def after_reopen(issue, transition) + send_reassigned_email(issue) if issue.is_being_reassigned? + + create_note(issue) + end + + def after_update(issue) + send_reassigned_email(issue) if issue.is_being_reassigned? end protected + def create_note(issue) + Note.create_status_change_note(issue, current_user, issue.state) + [issue.author, issue.assignee].compact.each do |recipient| + Notify.delay.issue_status_changed_email(recipient.id, issue.id, issue.state, current_user.id) + end + end + def send_reassigned_email(issue) recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id && id != current_user.id } diff --git a/app/observers/merge_request_observer.rb b/app/observers/merge_request_observer.rb index 6d3c2bdd..d89e7734 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/issues/_show.html.haml b/app/views/issues/_show.html.haml index fa888618..3d1ecd43 100644 --- a/app/views/issues/_show.html.haml +++ b/app/views/issues/_show.html.haml @@ -8,10 +8,10 @@ %i.icon-comment = issue.notes.count - if can? current_user, :modify_issue, issue - - if issue.closed - = link_to 'Reopen', project_issue_path(issue.project, issue, issue: {closed: false }, status_only: true), method: :put, class: "btn btn-small grouped reopen_issue", remote: true + - if issue.closed? + = link_to 'Reopen', project_issue_path(issue.project, issue, issue: {state_event: :reopen }, status_only: true), method: :put, class: "btn btn-small grouped reopen_issue", remote: true - else - = link_to 'Close', project_issue_path(issue.project, issue, issue: {closed: true }, status_only: true), method: :put, class: "btn btn-small grouped close_issue", remote: true + = link_to 'Close', project_issue_path(issue.project, issue, issue: {state_event: :close }, status_only: true), method: :put, class: "btn btn-small grouped close_issue", remote: true = link_to edit_project_issue_path(issue.project, issue), class: "btn btn-small edit-issue-link grouped" do %i.icon-edit Edit diff --git a/app/views/issues/show.html.haml b/app/views/issues/show.html.haml index 474955cc..f1a97e10 100644 --- a/app/views/issues/show.html.haml +++ b/app/views/issues/show.html.haml @@ -7,10 +7,10 @@ %span.pull-right - if can?(current_user, :admin_project, @project) || @issue.author == current_user - - if @issue.closed - = link_to 'Reopen', project_issue_path(@project, @issue, issue: {closed: false }, status_only: true), method: :put, class: "btn grouped reopen_issue" + - if @issue.closed? + = link_to 'Reopen', project_issue_path(@project, @issue, issue: {state_event: :reopen }, status_only: true), method: :put, class: "btn grouped reopen_issue" - else - = link_to 'Close', project_issue_path(@project, @issue, issue: {closed: true }, status_only: true), method: :put, class: "btn grouped close_issue", title: "Close Issue" + = link_to 'Close', project_issue_path(@project, @issue, issue: {state_event: :close }, status_only: true), method: :put, class: "btn grouped close_issue", title: "Close Issue" - if can?(current_user, :admin_project, @project) || @issue.author == current_user = link_to edit_project_issue_path(@project, @issue), class: "btn grouped" do %i.icon-edit @@ -27,7 +27,7 @@ .ui-box.ui-box-show .ui-box-head %h4.box-title - - if @issue.closed + - if @issue.closed? .error.status_info Closed = gfm escape_once(@issue.title) diff --git a/app/views/merge_requests/_show.html.haml b/app/views/merge_requests/_show.html.haml index cefd33c0..ae2cfe92 100644 --- a/app/views/merge_requests/_show.html.haml +++ b/app/views/merge_requests/_show.html.haml @@ -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.state == MergeRequest::UNCHECKED ? "true" : "false"}, + check_enable: #{@merge_request.merge_status == MergeRequest::UNCHECKED ? "true" : "false"}, url_to_ci_check: "#{ci_status_project_merge_request_path(@project, @merge_request)}", ci_enable: #{@project.gitlab_ci? ? "true" : "false"}, - current_state: "#{@merge_request.human_state}", + current_status: "#{@merge_request.human_merge_status}", action: "#{controller.action_name}" }); }); diff --git a/app/views/merge_requests/show/_mr_accept.html.haml b/app/views/merge_requests/show/_mr_accept.html.haml index c2c04b86..64f25a51 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 644d7fcc..3b54f613 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,14 +21,14 @@ %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 - Merged by #{link_to_member(@project, @merge_request.merge_event.author)} - %small #{time_ago_in_words(@merge_request.merge_event.created_at)} ago. - - elsif @merge_request.closed_event - %span - Closed by #{link_to_member(@project, @merge_request.closed_event.author)} - %small #{time_ago_in_words(@merge_request.closed_event.created_at)} ago. + %span + Closed by #{link_to_member(@project, @merge_request.closed_event.author)} + %small #{time_ago_in_words(@merge_request.closed_event.created_at)} ago. + - if @merge_request.merged? + .ui-box-bottom + %span + Merged by #{link_to_member(@project, @merge_request.merge_event.author)} + %small #{time_ago_in_words(@merge_request.merge_event.created_at)} ago. diff --git a/app/views/merge_requests/show/_mr_ci.html.haml b/app/views/merge_requests/show/_mr_ci.html.haml index dd1e78a0..a8faa6ba 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 8119728d..3df7e4b2 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 @@ -17,7 +17,7 @@ %li= link_to "Email Patches", project_merge_request_path(@project, @merge_request, format: :patch) %li= link_to "Plain Diff", project_merge_request_path(@project, @merge_request, format: :diff) - = link_to 'Close', project_merge_request_path(@project, @merge_request, merge_request: {closed: true }, status_only: true), method: :put, class: "btn grouped btn-close", title: "Close merge request" + = link_to 'Close', project_merge_request_path(@project, @merge_request, merge_request: {state_event: :close }), method: :put, class: "btn grouped btn-close", title: "Close merge request" = link_to edit_project_merge_request_path(@project, @merge_request), class: "btn grouped" do %i.icon-edit diff --git a/app/views/milestones/_milestone.html.haml b/app/views/milestones/_milestone.html.haml index 00e20117..8a3727c6 100644 --- a/app/views/milestones/_milestone.html.haml +++ b/app/views/milestones/_milestone.html.haml @@ -1,12 +1,12 @@ -%li{class: "milestone milestone-#{milestone.closed ? 'closed' : 'open'}", id: dom_id(milestone) } +%li{class: "milestone milestone-#{milestone.closed? ? 'closed' : 'open'}", id: dom_id(milestone) } .pull-right - - if can?(current_user, :admin_milestone, milestone.project) and milestone.open? + - if can?(current_user, :admin_milestone, milestone.project) and milestone.active? = link_to edit_project_milestone_path(milestone.project, milestone), class: "btn btn-small edit-milestone-link grouped" do %i.icon-edit Edit %h4 = link_to_gfm truncate(milestone.title, length: 100), project_milestone_path(milestone.project, milestone) - - if milestone.expired? and not milestone.closed + - if milestone.expired? and not milestone.closed? %span.cred (Expired) %small = milestone.expires_at diff --git a/app/views/milestones/show.html.haml b/app/views/milestones/show.html.haml index 43d82a54..c2b09542 100644 --- a/app/views/milestones/show.html.haml +++ b/app/views/milestones/show.html.haml @@ -9,7 +9,7 @@ ← To milestones list .span6 .pull-right - - unless @milestone.closed + - unless @milestone.closed? = link_to new_project_issue_path(@project, issue: { milestone_id: @milestone.id }), class: "btn btn-small grouped", title: "New Issue" do %i.icon-plus New Issue @@ -25,12 +25,12 @@ %hr %p %span All issues for this milestone are closed. You may close milestone now. - = link_to 'Close Milestone', project_milestone_path(@project, @milestone, milestone: {closed: true }), method: :put, class: "btn btn-small btn-remove" + = link_to 'Close Milestone', project_milestone_path(@project, @milestone, milestone: {state_event: :close }), method: :put, class: "btn btn-small btn-remove" .ui-box.ui-box-show .ui-box-head %h4.box-title - - if @milestone.closed + - if @milestone.closed? .error.status_info Closed - elsif @milestone.expired? .error.status_info Expired @@ -63,7 +63,7 @@ %li=link_to('All Issues', '#') %ul.well-list - @issues.each do |issue| - %li{data: {closed: issue.closed}} + %li{data: {closed: issue.closed?}} = link_to [@project, issue] do %span.badge.badge-info ##{issue.id} – @@ -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/db/migrate/20130214154045_rename_state_to_merge_status_in_milestone.rb b/db/migrate/20130214154045_rename_state_to_merge_status_in_milestone.rb new file mode 100644 index 00000000..23797fe1 --- /dev/null +++ b/db/migrate/20130214154045_rename_state_to_merge_status_in_milestone.rb @@ -0,0 +1,5 @@ +class RenameStateToMergeStatusInMilestone < ActiveRecord::Migration + def change + rename_column :merge_requests, :state, :merge_status + end +end diff --git a/db/migrate/20130218140952_add_state_to_issue.rb b/db/migrate/20130218140952_add_state_to_issue.rb new file mode 100644 index 00000000..062103d0 --- /dev/null +++ b/db/migrate/20130218140952_add_state_to_issue.rb @@ -0,0 +1,5 @@ +class AddStateToIssue < ActiveRecord::Migration + def change + add_column :issues, :state, :string + end +end diff --git a/db/migrate/20130218141038_add_state_to_merge_request.rb b/db/migrate/20130218141038_add_state_to_merge_request.rb new file mode 100644 index 00000000..ac4108ee --- /dev/null +++ b/db/migrate/20130218141038_add_state_to_merge_request.rb @@ -0,0 +1,5 @@ +class AddStateToMergeRequest < ActiveRecord::Migration + def change + add_column :merge_requests, :state, :string + end +end diff --git a/db/migrate/20130218141117_add_state_to_milestone.rb b/db/migrate/20130218141117_add_state_to_milestone.rb new file mode 100644 index 00000000..c8403910 --- /dev/null +++ b/db/migrate/20130218141117_add_state_to_milestone.rb @@ -0,0 +1,5 @@ +class AddStateToMilestone < ActiveRecord::Migration + def change + add_column :milestones, :state, :string + end +end diff --git a/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb b/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb new file mode 100644 index 00000000..0614a5c0 --- /dev/null +++ b/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb @@ -0,0 +1,14 @@ +class ConvertClosedToStateInIssue < ActiveRecord::Migration + def up + Issue.transaction do + Issue.where(closed: true).update_all("state = 'closed'") + Issue.where(closed: false).update_all("state = 'opened'") + end + end + + def down + Issue.transaction do + Issue.where(state: :closed).update_all("closed = 1") + end + end +end diff --git a/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb b/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb new file mode 100644 index 00000000..4d5c6ee5 --- /dev/null +++ b/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb @@ -0,0 +1,16 @@ +class ConvertClosedToStateInMergeRequest < ActiveRecord::Migration + def up + MergeRequest.transaction do + MergeRequest.where("closed = 1 AND merged = 1").update_all("state = 'merged'") + MergeRequest.where("closed = 1 AND merged = 0").update_all("state = 'closed'") + MergeRequest.where("closed = 0").update_all("state = 'opened'") + end + end + + def down + MergeRequest.transaction do + MergeRequest.where(state: :closed).update_all("closed = 1") + MergeRequest.where(state: :merged).update_all("closed = 1, merged = 1") + end + end +end diff --git a/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb b/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb new file mode 100644 index 00000000..78096666 --- /dev/null +++ b/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb @@ -0,0 +1,14 @@ +class ConvertClosedToStateInMilestone < ActiveRecord::Migration + def up + Milestone.transaction do + Milestone.where(closed: false).update_all("state = 'opened'") + Milestone.where(closed: false).update_all("state = 'active'") + end + end + + def down + Milestone.transaction do + Milestone.where(state: :closed).update_all("closed = 1") + end + end +end diff --git a/db/migrate/20130218141444_remove_merged_from_merge_request.rb b/db/migrate/20130218141444_remove_merged_from_merge_request.rb new file mode 100644 index 00000000..a7bd82f5 --- /dev/null +++ b/db/migrate/20130218141444_remove_merged_from_merge_request.rb @@ -0,0 +1,9 @@ +class RemoveMergedFromMergeRequest < ActiveRecord::Migration + def up + remove_column :merge_requests, :merged + end + + def down + add_column :merge_requests, :merged, :boolean, default: true, null: false + end +end diff --git a/db/migrate/20130218141507_remove_closed_from_issue.rb b/db/migrate/20130218141507_remove_closed_from_issue.rb new file mode 100644 index 00000000..95cc0642 --- /dev/null +++ b/db/migrate/20130218141507_remove_closed_from_issue.rb @@ -0,0 +1,9 @@ +class RemoveClosedFromIssue < ActiveRecord::Migration + def up + remove_column :issues, :closed + end + + def down + add_column :issues, :closed, :boolean + end +end diff --git a/db/migrate/20130218141536_remove_closed_from_merge_request.rb b/db/migrate/20130218141536_remove_closed_from_merge_request.rb new file mode 100644 index 00000000..37183593 --- /dev/null +++ b/db/migrate/20130218141536_remove_closed_from_merge_request.rb @@ -0,0 +1,9 @@ +class RemoveClosedFromMergeRequest < ActiveRecord::Migration + def up + remove_column :merge_requests, :closed + end + + def down + add_column :merge_requests, :closed, :boolean + end +end diff --git a/db/migrate/20130218141554_remove_closed_from_milestone.rb b/db/migrate/20130218141554_remove_closed_from_milestone.rb new file mode 100644 index 00000000..e8dae4a1 --- /dev/null +++ b/db/migrate/20130218141554_remove_closed_from_milestone.rb @@ -0,0 +1,9 @@ +class RemoveClosedFromMilestone < ActiveRecord::Migration + def up + remove_column :milestones, :closed + end + + def down + add_column :milestones, :closed, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 0f07d2bc..f837e6ed 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20130131070232) do +ActiveRecord::Schema.define(:version => 20130218141554) do create_table "events", :force => true do |t| t.string "target_type" @@ -37,18 +37,17 @@ ActiveRecord::Schema.define(:version => 20130131070232) do t.integer "assignee_id" t.integer "author_id" t.integer "project_id" - t.datetime "created_at", :null => false - t.datetime "updated_at", :null => false - t.boolean "closed", :default => false, :null => false + t.datetime "created_at", :null => false + t.datetime "updated_at", :null => false t.integer "position", :default => 0 t.string "branch_name" t.text "description" t.integer "milestone_id" + t.string "state" end add_index "issues", ["assignee_id"], :name => "index_issues_on_assignee_id" add_index "issues", ["author_id"], :name => "index_issues_on_author_id" - add_index "issues", ["closed"], :name => "index_issues_on_closed" add_index "issues", ["created_at"], :name => "index_issues_on_created_at" add_index "issues", ["milestone_id"], :name => "index_issues_on_milestone_id" add_index "issues", ["project_id"], :name => "index_issues_on_project_id" @@ -69,25 +68,23 @@ ActiveRecord::Schema.define(:version => 20130131070232) 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.boolean "closed", :default => false, :null => false - 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.boolean "merged", :default => false, :null => false - t.integer "state", :default => 1, :null => false + t.integer "merge_status", :default => 1, :null => false t.integer "milestone_id" + t.string "state" end add_index "merge_requests", ["assignee_id"], :name => "index_merge_requests_on_assignee_id" add_index "merge_requests", ["author_id"], :name => "index_merge_requests_on_author_id" - add_index "merge_requests", ["closed"], :name => "index_merge_requests_on_closed" add_index "merge_requests", ["created_at"], :name => "index_merge_requests_on_created_at" add_index "merge_requests", ["milestone_id"], :name => "index_merge_requests_on_milestone_id" add_index "merge_requests", ["project_id"], :name => "index_merge_requests_on_project_id" @@ -96,13 +93,13 @@ ActiveRecord::Schema.define(:version => 20130131070232) do add_index "merge_requests", ["title"], :name => "index_merge_requests_on_title" create_table "milestones", :force => true do |t| - t.string "title", :null => false - t.integer "project_id", :null => false + t.string "title", :null => false + t.integer "project_id", :null => false t.text "description" t.date "due_date" - t.boolean "closed", :default => false, :null => false - 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.string "state" end add_index "milestones", ["due_date"], :name => "index_milestones_on_due_date" diff --git a/features/steps/project/project_issues.rb b/features/steps/project/project_issues.rb index 2103aeb1..7d540099 100644 --- a/features/steps/project/project_issues.rb +++ b/features/steps/project/project_issues.rb @@ -122,10 +122,9 @@ class ProjectIssues < Spinach::FeatureSteps And 'project "Shop" have "Release 0.3" closed issue' do project = Project.find_by_name("Shop") - create(:issue, + create(:closed_issue, :title => "Release 0.3", :project => project, - :author => project.users.first, - :closed => true) + :author => project.users.first) end end diff --git a/features/steps/project/project_merge_requests.rb b/features/steps/project/project_merge_requests.rb index 329261ad..ff95a47d 100644 --- a/features/steps/project/project_merge_requests.rb +++ b/features/steps/project/project_merge_requests.rb @@ -26,7 +26,7 @@ class ProjectMergeRequests < Spinach::FeatureSteps Then 'I should see closed merge request "Bug NS-04"' do mr = MergeRequest.find_by_title("Bug NS-04") - mr.closed.should be_true + mr.closed?.should be_true page.should have_content "Closed by" end @@ -80,11 +80,10 @@ class ProjectMergeRequests < Spinach::FeatureSteps And 'project "Shop" have "Feature NS-03" closed merge request' do project = Project.find_by_name("Shop") - create(:merge_request, + create(:closed_merge_request, title: "Feature NS-03", project: project, - author: project.users.first, - closed: true) + author: project.users.first) end And 'I switch to the diff tab' do diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 2cd8aa6c..b5dd033b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -35,12 +35,11 @@ module Gitlab class Group < Grape::Entity expose :id, :name, :path, :owner_id end - + class GroupDetail < Group expose :projects, using: Entities::Project end - class RepoObject < Grape::Entity expose :name, :commit expose :protected do |repo, options| @@ -63,7 +62,7 @@ module Gitlab class Milestone < Grape::Entity expose :id expose (:project_id) {|milestone| milestone.project.id} - expose :title, :description, :due_date, :closed, :updated_at, :created_at + expose :title, :description, :due_date, :state, :updated_at, :created_at end class Issue < Grape::Entity @@ -73,7 +72,7 @@ module Gitlab expose :label_list, as: :labels expose :milestone, using: Entities::Milestone expose :assignee, :author, using: Entities::UserBasic - expose :closed, :updated_at, :created_at + expose :state, :updated_at, :created_at end class SSHKey < Grape::Entity @@ -81,7 +80,7 @@ module Gitlab end class MergeRequest < Grape::Entity - expose :id, :target_branch, :source_branch, :project_id, :title, :closed, :merged + expose :id, :target_branch, :source_branch, :project_id, :title, :state expose :author, :assignee, using: Entities::UserBasic end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 4d832fbe..70bbf47e 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -69,14 +69,14 @@ module Gitlab # assignee_id (optional) - The ID of a user to assign issue # milestone_id (optional) - The ID of a milestone to assign issue # labels (optional) - The labels of an issue - # closed (optional) - The state of an issue (0 = false, 1 = true) + # state (optional) - The state of an issue (close|reopen) # Example Request: # PUT /projects/:id/issues/:issue_id put ":id/issues/:issue_id" do @issue = user_project.issues.find(params[:issue_id]) authorize! :modify_issue, @issue - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :closed] + attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event] attrs[:label_list] = params[:labels] if params[:labels].present? IssueObserver.current_user = current_user if @issue.update_attributes attrs diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 470cd1e1..7f763eb4 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -73,12 +73,12 @@ module Gitlab # target_branch - The target branch # assignee_id - Assignee user ID # title - Title of MR - # closed - Status of MR. true - closed + # state_event - Status of MR. (close|reopen|merge) # Example: # PUT /projects/:id/merge_request/:merge_request_id # put ":id/merge_request/:merge_request_id" do - attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :closed] + attrs = attributes_for_keys [:source_branch, :target_branch, :assignee_id, :title, :state_event] merge_request = user_project.merge_requests.find(params[:merge_request_id]) authorize! :modify_merge_request, merge_request diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb index 6aca9d01..eaf0d37c 100644 --- a/lib/api/milestones.rb +++ b/lib/api/milestones.rb @@ -59,14 +59,14 @@ module Gitlab # title (optional) - The title of a milestone # description (optional) - The description of a milestone # due_date (optional) - The due date of a milestone - # closed (optional) - The status of the milestone + # state (optional) - The status of the milestone (close|activate) # Example Request: # PUT /projects/:id/milestones/:milestone_id put ":id/milestones/:milestone_id" do authorize! :admin_milestone, user_project @milestone = user_project.milestones.find(params[:milestone_id]) - attrs = attributes_for_keys [:title, :description, :due_date, :closed] + attrs = attributes_for_keys [:title, :description, :due_date, :state_event] if @milestone.update_attributes attrs present @milestone, with: Entities::Milestone else diff --git a/spec/factories.rb b/spec/factories.rb index 17dbc796..b81984b5 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -54,10 +54,15 @@ FactoryGirl.define do project trait :closed do - closed true + state :closed + end + + trait :reopened do + state :reopened end factory :closed_issue, traits: [:closed] + factory :reopened_issue, traits: [:reopened] end factory :merge_request do @@ -67,10 +72,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 +86,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 @@ -159,6 +169,12 @@ FactoryGirl.define do factory :milestone do title project + + trait :closed do + state :closed + end + + factory :closed_milestone, traits: [:closed] end factory :system_hook do diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index b5d4bd7b..551e1753 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -15,7 +15,6 @@ describe Issue, "Issuable" do it { should validate_presence_of(:author) } it { should validate_presence_of(:title) } it { should ensure_length_of(:title).is_at_least(0).is_at_most(255) } - it { should ensure_inclusion_of(:closed).in_array([true, false]) } end describe "Scope" do diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 10db53e0..99d9f65b 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -9,7 +9,7 @@ # project_id :integer # created_at :datetime not null # updated_at :datetime not null -# closed :boolean default(FALSE), not null +# state :string default(FALSE), not null # position :integer default(0) # branch_name :string(255) # description :text @@ -44,34 +44,15 @@ describe Issue do end end - describe '#is_being_closed?' do - it 'returns true if the closed attribute has changed and is now true' do - subject.closed = true - subject.is_being_closed?.should be_true - end - it 'returns false if the closed attribute has changed and is now false' do - issue = create(:closed_issue) - issue.closed = false - issue.is_being_closed?.should be_false - end - it 'returns false if the closed attribute has not changed' do - subject.is_being_closed?.should be_false - end - end + describe '#is_being_reassigned?' do + it 'returnes issues assigned to user' do + user = create :user + 2.times do + issue = create :issue, assignee: user + end - describe '#is_being_reopened?' do - it 'returns true if the closed attribute has changed and is now false' do - issue = create(:closed_issue) - issue.closed = false - issue.is_being_reopened?.should be_true - end - it 'returns false if the closed attribute has changed and is now true' do - subject.closed = true - subject.is_being_reopened?.should be_false - end - it 'returns false if the closed attribute has not changed' do - subject.is_being_reopened?.should be_false + Issue.open_for(user).count.should eq 2 end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 41f4ede5..e61bf44c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -15,7 +15,7 @@ # st_commits :text(2147483647) # st_diffs :text(2147483647) # merged :boolean default(FALSE), not null -# state :integer default(1), not null +# merge_status :integer default(1), not null # milestone_id :integer # @@ -36,6 +36,10 @@ describe MergeRequest do it { should include_module(Issuable) } end + describe "#mr_and_commit_notes" do + + end + describe "#mr_and_commit_notes" do let!(:merge_request) { create(:merge_request) } @@ -62,35 +66,4 @@ describe MergeRequest do subject.is_being_reassigned?.should be_false end end - - describe '#is_being_closed?' do - it 'returns true if the closed attribute has changed and is now true' do - subject.closed = true - subject.is_being_closed?.should be_true - end - it 'returns false if the closed attribute has changed and is now false' do - merge_request = create(:closed_merge_request) - merge_request.closed = false - merge_request.is_being_closed?.should be_false - end - it 'returns false if the closed attribute has not changed' do - subject.is_being_closed?.should be_false - end - end - - - describe '#is_being_reopened?' do - it 'returns true if the closed attribute has changed and is now false' do - merge_request = create(:closed_merge_request) - merge_request.closed = false - merge_request.is_being_reopened?.should be_true - end - it 'returns false if the closed attribute has changed and is now true' do - subject.closed = true - subject.is_being_reopened?.should be_false - end - it 'returns false if the closed attribute has not changed' do - subject.is_being_reopened?.should be_false - end - end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 2ea2c56a..b473f843 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 # @@ -27,7 +27,6 @@ describe Milestone do describe "Validation" do it { should validate_presence_of(:title) } it { should validate_presence_of(:project) } - it { should ensure_inclusion_of(:closed).in_array([true, false]) } end let(:milestone) { create(:milestone) } @@ -41,7 +40,7 @@ describe Milestone do it "should count closed issues" do IssueObserver.current_user = issue.author - issue.update_attributes(closed: true) + issue.close milestone.issues << issue milestone.percent_complete.should == 100 end @@ -96,7 +95,7 @@ describe Milestone do describe :items_count do before do milestone.issues << create(:issue) - milestone.issues << create(:issue, closed: true) + milestone.issues << create(:closed_issue) milestone.merge_requests << create(:merge_request) end @@ -110,7 +109,35 @@ describe Milestone do it { milestone.can_be_closed?.should be_true } end - describe :open? do - it { milestone.open?.should be_true } + describe :is_empty? do + before do + issue = create :closed_issue, milestone: milestone + merge_request = create :merge_request, milestone: milestone + end + + it 'Should return total count of issues and merge requests assigned to milestone' do + milestone.total_items_count.should eq 2 + end end + + describe :can_be_closed? do + before do + milestone = create :milestone + create :closed_issue, milestone: milestone + + issue = create :issue + end + + it 'should be true if milestone active and all nestied issues closed' do + milestone.can_be_closed?.should be_true + end + + it 'should be false if milestone active and not all nestied issues closed' do + issue.milestone = milestone + issue.save + + milestone.can_be_closed?.should be_false + end + end + end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 4b620a2f..5c27f363 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/issue_observer_spec.rb b/spec/observers/issue_observer_spec.rb index 700c9a3a..e4e66917 100644 --- a/spec/observers/issue_observer_spec.rb +++ b/spec/observers/issue_observer_spec.rb @@ -1,10 +1,15 @@ require 'spec_helper' describe IssueObserver do - let(:some_user) { double(:user, id: 1) } - let(:assignee) { double(:user, id: 2) } - let(:author) { double(:user, id: 3) } - let(:issue) { double(:issue, id: 42, assignee: assignee, author: author) } + let(:some_user) { create :user } + let(:assignee) { create :user } + let(:author) { create :user } + let(:mock_issue) { double(:issue, id: 42, assignee: assignee, author: author) } + let(:assigned_issue) { create(:issue, assignee: assignee, author: author) } + let(:unassigned_issue) { create(:issue, author: author) } + let(:closed_assigned_issue) { create(:closed_issue, assignee: assignee, author: author) } + let(:closed_unassigned_issue) { create(:closed_issue, author: author) } + before(:each) { subject.stub(:current_user).and_return(some_user) } @@ -21,24 +26,66 @@ describe IssueObserver do end it 'sends an email to the assignee' do - Notify.should_receive(:new_issue_email).with(issue.id) + Notify.should_receive(:new_issue_email).with(mock_issue.id) - subject.after_create(issue) + subject.after_create(mock_issue) end it 'does not send an email to the assignee if assignee created the issue' do subject.stub(:current_user).and_return(assignee) Notify.should_not_receive(:new_issue_email) - subject.after_create(issue) + subject.after_create(mock_issue) + end + end + + context '#after_close' do + context 'a status "closed"' do + it 'note is created if the issue is being closed' do + Note.should_receive(:create_status_change_note).with(assigned_issue, some_user, 'closed') + + assigned_issue.close + end + + it 'notification is delivered if the issue being closed' do + Notify.should_receive(:issue_status_changed_email).twice + + assigned_issue.close + end + + it 'notification is delivered only to author if the issue being closed' do + Notify.should_receive(:issue_status_changed_email).once + Note.should_receive(:create_status_change_note).with(unassigned_issue, some_user, 'closed') + + unassigned_issue.close + end + end + + context 'a status "reopened"' do + it 'note is created if the issue is being reopened' do + Note.should_receive(:create_status_change_note).with(closed_assigned_issue, some_user, 'reopened') + + closed_assigned_issue.reopen + end + + it 'notification is delivered if the issue being reopened' do + Notify.should_receive(:issue_status_changed_email).twice + + closed_assigned_issue.reopen + end + + it 'notification is delivered only to author if the issue being reopened' do + Notify.should_receive(:issue_status_changed_email).once + Note.should_receive(:create_status_change_note).with(closed_unassigned_issue, some_user, 'reopened') + + closed_unassigned_issue.reopen + end end end context '#after_update' do before(:each) do - issue.stub(:is_being_reassigned?).and_return(false) - issue.stub(:is_being_closed?).and_return(false) - issue.stub(:is_being_reopened?).and_return(false) + mock_issue.stub(:is_being_reassigned?).and_return(false) end it 'is called when an issue is changed' do @@ -53,105 +100,17 @@ describe IssueObserver do context 'a reassigned email' do it 'is sent if the issue is being reassigned' do - issue.should_receive(:is_being_reassigned?).and_return(true) - subject.should_receive(:send_reassigned_email).with(issue) + mock_issue.should_receive(:is_being_reassigned?).and_return(true) + subject.should_receive(:send_reassigned_email).with(mock_issue) - subject.after_update(issue) + subject.after_update(mock_issue) end it 'is not sent if the issue is not being reassigned' do - issue.should_receive(:is_being_reassigned?).and_return(false) + mock_issue.should_receive(:is_being_reassigned?).and_return(false) subject.should_not_receive(:send_reassigned_email) - subject.after_update(issue) - end - end - - context 'a status "closed"' do - it 'note is created if the issue is being closed' do - issue.should_receive(:is_being_closed?).and_return(true) - Notify.should_receive(:issue_status_changed_email).twice - Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') - - subject.after_update(issue) - end - - it 'note is not created if the issue is not being closed' do - issue.should_receive(:is_being_closed?).and_return(false) - Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'closed') - - subject.after_update(issue) - end - - it 'notification is delivered if the issue being closed' do - issue.stub(:is_being_closed?).and_return(true) - Notify.should_receive(:issue_status_changed_email).twice - Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') - - subject.after_update(issue) - end - - it 'notification is not delivered if the issue not being closed' do - issue.stub(:is_being_closed?).and_return(false) - Notify.should_not_receive(:issue_status_changed_email) - Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'closed') - - subject.after_update(issue) - end - - it 'notification is delivered only to author if the issue being closed' do - issue_without_assignee = double(:issue, id: 42, author: author, assignee: nil) - issue_without_assignee.stub(:is_being_reassigned?).and_return(false) - issue_without_assignee.stub(:is_being_closed?).and_return(true) - issue_without_assignee.stub(:is_being_reopened?).and_return(false) - Notify.should_receive(:issue_status_changed_email).once - Note.should_receive(:create_status_change_note).with(issue_without_assignee, some_user, 'closed') - - subject.after_update(issue_without_assignee) - end - end - - context 'a status "reopened"' do - it 'note is created if the issue is being reopened' do - Notify.should_receive(:issue_status_changed_email).twice - issue.should_receive(:is_being_reopened?).and_return(true) - Note.should_receive(:create_status_change_note).with(issue, some_user, 'reopened') - - subject.after_update(issue) - end - - it 'note is not created if the issue is not being reopened' do - issue.should_receive(:is_being_reopened?).and_return(false) - Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'reopened') - - subject.after_update(issue) - end - - it 'notification is delivered if the issue being reopened' do - issue.stub(:is_being_reopened?).and_return(true) - Notify.should_receive(:issue_status_changed_email).twice - Note.should_receive(:create_status_change_note).with(issue, some_user, 'reopened') - - subject.after_update(issue) - end - - it 'notification is not delivered if the issue not being reopened' do - issue.stub(:is_being_reopened?).and_return(false) - Notify.should_not_receive(:issue_status_changed_email) - Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'reopened') - - subject.after_update(issue) - end - - it 'notification is delivered only to author if the issue being reopened' do - issue_without_assignee = double(:issue, id: 42, author: author, assignee: nil) - issue_without_assignee.stub(:is_being_reassigned?).and_return(false) - issue_without_assignee.stub(:is_being_closed?).and_return(false) - issue_without_assignee.stub(:is_being_reopened?).and_return(true) - Notify.should_receive(:issue_status_changed_email).once - Note.should_receive(:create_status_change_note).with(issue_without_assignee, some_user, 'reopened') - - subject.after_update(issue_without_assignee) + subject.after_update(mock_issue) end end end @@ -160,23 +119,23 @@ describe IssueObserver do let(:previous_assignee) { double(:user, id: 3) } before(:each) do - issue.stub(:assignee_id).and_return(assignee.id) - issue.stub(:assignee_id_was).and_return(previous_assignee.id) + mock_issue.stub(:assignee_id).and_return(assignee.id) + mock_issue.stub(:assignee_id_was).and_return(previous_assignee.id) end def it_sends_a_reassigned_email_to(recipient) - Notify.should_receive(:reassigned_issue_email).with(recipient, issue.id, previous_assignee.id) + Notify.should_receive(:reassigned_issue_email).with(recipient, mock_issue.id, previous_assignee.id) end def it_does_not_send_a_reassigned_email_to(recipient) - Notify.should_not_receive(:reassigned_issue_email).with(recipient, issue.id, previous_assignee.id) + Notify.should_not_receive(:reassigned_issue_email).with(recipient, mock_issue.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, issue) + subject.send(:send_reassigned_email, mock_issue) end context 'does not send an email to the user who made the reassignment' do @@ -185,14 +144,14 @@ describe IssueObserver 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, issue) + subject.send(:send_reassigned_email, mock_issue) 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, issue) + subject.send(:send_reassigned_email, mock_issue) end end end diff --git a/spec/observers/merge_request_observer_spec.rb b/spec/observers/merge_request_observer_spec.rb index 4841bf88..9d702107 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') + Note.should_receive(:create_status_change_note).with(assigned_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') - - 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') + Note.should_receive(:create_status_change_note).with(closed_assigned_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') - - 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 diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 781ebab0..630ac0f8 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -54,14 +54,24 @@ describe Gitlab::API do end end - describe "PUT /projects/:id/issues/:issue_id" do + describe "PUT /projects/:id/issues/:issue_id to update only title" do it "should update a project issue" do put api("/projects/#{project.id}/issues/#{issue.id}", user), - title: 'updated title', labels: 'label2', closed: 1 + title: 'updated title' response.status.should == 200 + json_response['title'].should == 'updated title' + end + end + + describe "PUT /projects/:id/issues/:issue_id to update state and label" do + it "should update a project issue" do + put api("/projects/#{project.id}/issues/#{issue.id}", user), + labels: 'label2', state_event: "close" + response.status.should == 200 + json_response['labels'].should == ['label2'] - json_response['closed'].should be_true + json_response['state'].should eq "closed" end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 5da54154..1abd7a20 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -43,6 +43,23 @@ describe Gitlab::API do end end + describe "PUT /projects/:id/merge_request/:merge_request_id to close MR" do + it "should return merge_request" do + put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), state_event: "close" + response.status.should == 200 + json_response['state'].should == 'closed' + end + end + + describe "PUT /projects/:id/merge_request/:merge_request_id to merge MR" do + it "should return merge_request" do + put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), state_event: "merge" + response.status.should == 200 + json_response['state'].should == 'merged' + end + end + + describe "PUT /projects/:id/merge_request/:merge_request_id" do it "should return merge_request" do put api("/projects/#{project.id}/merge_request/#{merge_request.id}", user), title: "New title" diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb index 80696671..d1b5e449 100644 --- a/spec/requests/api/milestones_spec.rb +++ b/spec/requests/api/milestones_spec.rb @@ -44,4 +44,14 @@ describe Gitlab::API do json_response['title'].should == 'updated title' end end + + describe "PUT /projects/:id/milestones/:milestone_id to close milestone" do + it "should update a project milestone" do + put api("/projects/#{project.id}/milestones/#{milestone.id}", user), + state_event: 'close' + response.status.should == 200 + + json_response['state'].should == 'closed' + end + end end diff --git a/spec/requests/issues_spec.rb b/spec/requests/issues_spec.rb index 2e94ffd0..6fff59f0 100644 --- a/spec/requests/issues_spec.rb +++ b/spec/requests/issues_spec.rb @@ -58,8 +58,7 @@ describe "Issues" do it "should be able to search on different statuses" do issue = Issue.first # with title 'foobar' - issue.closed = true - issue.save + issue.close visit project_issues_path(project) click_link 'Closed'