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/models/issue.rb b/app/models/issue.rb index 07c04011..6c0f4da4 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,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 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/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/views/issues/_show.html.haml b/app/views/issues/_show.html.haml index fa888618..b078115a 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: :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 diff --git a/app/views/issues/show.html.haml b/app/views/issues/show.html.haml index 474955cc..ab10d619 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: :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) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8d965b60..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 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/spec/factories.rb b/spec/factories.rb index 92d250d6..3f357b79 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 diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index d795a81b..f72fed22 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 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/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/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'