Issue uses StateMachine now

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

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,11 +19,29 @@
 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
 
+  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
+
+
   def self.open_for(user)
     opened.assigned(user)
   end

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

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)
+    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 }
 

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: :reopened }, 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: :closed }, 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

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: :reopened }, 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: :closed }, 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)
 

lib/api/entities.rb

@@ -40,7 +40,6 @@ module Gitlab
       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

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

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

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

spec/observers/issue_observer_spec.rb

 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,137 +26,91 @@ 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)
-    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)
-    end
-
-    it 'is called when an issue is changed' do
-      changed = create(:issue, project: create(:project))
-      subject.should_receive(:after_update)
-
-      Issue.observers.enable :issue_observer do
-        changed.description = 'I changed'
-        changed.save
-      end
-    end
-
-    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)
-
-        subject.after_update(issue)
-      end
-
-      it 'is not sent if the issue is not being reassigned' do
-        issue.should_receive(:is_being_reassigned?).and_return(false)
-        subject.should_not_receive(:send_reassigned_email)
-
-        subject.after_update(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
-        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')
+        Note.should_receive(:create_status_change_note).with(assigned_issue, some_user, 'closed')
 
-        subject.after_update(issue)
+        assigned_issue.close
       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)
+        assigned_issue.close
       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')
+        Note.should_receive(:create_status_change_note).with(unassigned_issue, some_user, 'closed')
 
-        subject.after_update(issue_without_assignee)
+        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
-        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)
+        closed_assigned_issue.reopen
       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')
+      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')
 
-        subject.after_update(issue)
+        closed_unassigned_issue.reopen
+      end
+    end
   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')
+  context '#after_update' do
+    before(:each) do
+      mock_issue.stub(:is_being_reassigned?).and_return(false)
+    end
+
+    it 'is called when an issue is changed' do
+      changed = create(:issue, project: create(:project))
+      subject.should_receive(:after_update)
 
-        subject.after_update(issue)
+      Issue.observers.enable :issue_observer do
+        changed.description = 'I changed'
+        changed.save
+      end
     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')
+    context 'a reassigned email' do
+      it 'is sent if the issue is being reassigned' do
+        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 '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')
+      it 'is not sent if the issue is not being reassigned' do
+        mock_issue.should_receive(:is_being_reassigned?).and_return(false)
+        subject.should_not_receive(:send_reassigned_email)
 
-        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

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
 

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'