diff --git a/Gemfile b/Gemfile index 9684892e6ff4aaa5daef4893e3701c6653b868d3..e7d93382f05918f92e9b1cca2c55e0512928a2a9 100644 --- a/Gemfile +++ b/Gemfile @@ -77,4 +77,5 @@ group :test do gem "simplecov", :require => false gem "shoulda-matchers" gem 'email_spec' + gem 'resque_spec' end diff --git a/Gemfile.lock b/Gemfile.lock index 7f252a66d12e3e96b884dd92bb516a2f98eaf143..c454a41ca0dbe37d4ecfb87f33ec13621c76da9b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -257,6 +257,9 @@ GEM resque_mailer (2.0.3) actionmailer (>= 3.0.0) resque (>= 1.2.3) + resque_spec (0.11.0) + resque (>= 1.19.0) + rspec (>= 2.5.0) rspec (2.10.0) rspec-core (~> 2.10.0) rspec-expectations (~> 2.10.0) @@ -385,6 +388,7 @@ DEPENDENCIES redcarpet (~> 2.1.1) resque (~> 1.20.0) resque_mailer + resque_spec rspec-rails sass-rails (= 3.2.5) seed-fu diff --git a/app/models/issue.rb b/app/models/issue.rb index 3ed47ef1bea2d7fd196b1262981adfd28cc037db..844d418b1e04d72cc3ff7b35e4a0d60f42a6923c 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -27,7 +27,7 @@ class Issue < ActiveRecord::Base validates :title, :presence => true, :length => { :within => 0..255 } - + validates :description, :length => { :within => 0..2000 } @@ -55,6 +55,18 @@ class Issue < ActiveRecord::Base def new? today? && created_at == updated_at end + + def is_being_reassigned? + assignee_id_changed? + end + + def is_being_closed? + closed_changed? && closed + end + + def is_being_reopened? + closed_changed? && !closed + end end # == Schema Information # diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb new file mode 100644 index 0000000000000000000000000000000000000000..fadedd301f4416025e948d5297d129fbded3b59b --- /dev/null +++ b/app/models/issue_observer.rb @@ -0,0 +1,23 @@ +class IssueObserver < ActiveRecord::Observer + cattr_accessor :current_user + + def after_create(issue) + Notify.new_issue_email(issue.id).deliver if issue.assignee != current_user + end + + def after_update(issue) + send_reassigned_email(issue) if issue.is_being_reassigned? + Note.create_status_change_note(issue, current_user, 'closed') if issue.is_being_closed? + Note.create_status_change_note(issue, current_user, 'reopened') if issue.is_being_reopened? + end + + protected + + def send_reassigned_email(issue) + recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id != current_user.id } + + recipient_ids.each do |recipient_id| + Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was).deliver + end + end +end diff --git a/app/models/mailer_observer.rb b/app/models/mailer_observer.rb index dbbd7d2f86ac3d338fa14eb23366ef68f8ad5bc5..880fd5026a48648e92d0661192a9619078c66997 100644 --- a/app/models/mailer_observer.rb +++ b/app/models/mailer_observer.rb @@ -1,31 +1,18 @@ class MailerObserver < ActiveRecord::Observer - observe :issue, :user, :note, :merge_request + observe :note, :merge_request cattr_accessor :current_user def after_create(model) - new_issue(model) if model.kind_of?(Issue) - new_user(model) if model.kind_of?(User) new_note(model) if model.kind_of?(Note) new_merge_request(model) if model.kind_of?(MergeRequest) end def after_update(model) changed_merge_request(model) if model.kind_of?(MergeRequest) - changed_issue(model) if model.kind_of?(Issue) end protected - def new_issue(issue) - if issue.assignee != current_user - Notify.new_issue_email(issue.id).deliver - end - end - - def new_user(user) - Notify.new_user_email(user.id, user.password).deliver - end - def new_note(note) if note.notify # Notify whole team except author of note @@ -65,12 +52,8 @@ class MailerObserver < ActiveRecord::Observer status_notify_and_comment merge_request, :reassigned_merge_request_email end - def changed_issue(issue) - status_notify_and_comment issue, :reassigned_issue_email - end - # This method used for Issues & Merge Requests - # + # # It create a comment for Issue or MR if someone close/reopen. # It also notify via email if assignee was changed # diff --git a/app/models/note.rb b/app/models/note.rb index c655eb807eb5abe9cff9a8a2cb02e799afd62921..aa034ef483e3139052a69d62475fa86db88dc507 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -42,6 +42,14 @@ class Note < ActiveRecord::Base mount_uploader :attachment, AttachmentUploader + def self.create_status_change_note(noteable, author, status) + create({ :noteable => noteable, + :project => noteable.project, + :author => author, + :note => "_Status changed to #{status}_" }, + :without_protection => true) + end + def notify @notify ||= false end diff --git a/app/models/user_observer.rb b/app/models/user_observer.rb new file mode 100644 index 0000000000000000000000000000000000000000..d12bcc99c83c980d9ebe5ba02e3e536c0b5bfae7 --- /dev/null +++ b/app/models/user_observer.rb @@ -0,0 +1,5 @@ +class UserObserver < ActiveRecord::Observer + def after_create(user) + Notify.new_user_email(user.id, user.password).deliver + end +end diff --git a/config/application.rb b/config/application.rb index dc6b734f0fa97d2464771bf6c1652d660e68583c..3585c4b0a87e836e453cbd4fe6d4dcf71b7193b2 100644 --- a/config/application.rb +++ b/config/application.rb @@ -23,7 +23,7 @@ module Gitlab # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer + config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer, :user_observer # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. diff --git a/config/initializers/resque_mailer.rb b/config/initializers/resque_mailer.rb new file mode 100644 index 0000000000000000000000000000000000000000..cec9dec9d0eff09f1fda46f89e6344d35511bf75 --- /dev/null +++ b/config/initializers/resque_mailer.rb @@ -0,0 +1 @@ +Resque::Mailer.excluded_environments = [] diff --git a/spec/models/activity_observer_spec.rb b/spec/models/activity_observer_spec.rb index 91c7d9147b2bcd42a70a566ae145d2b1857d395d..aed1b26d306191ef78f00982136495d00cd04ecd 100644 --- a/spec/models/activity_observer_spec.rb +++ b/spec/models/activity_observer_spec.rb @@ -9,9 +9,11 @@ describe ActivityObserver do end describe "Merge Request created" do - before do - @merge_request = Factory :merge_request, :project => project - @event = Event.last + before do + MergeRequest.observers.enable :activity_observer do + @merge_request = Factory :merge_request, :project => project + @event = Event.last + end end it_should_be_valid_event @@ -20,9 +22,11 @@ describe ActivityObserver do end describe "Issue created" do - before do - @issue = Factory :issue, :project => project - @event = Event.last + before do + Issue.observers.enable :activity_observer do + @issue = Factory :issue, :project => project + @event = Event.last + end end it_should_be_valid_event diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2b9798f7e53f62f2dbf26bb94faf249c4d7a193d --- /dev/null +++ b/spec/models/issue_observer_spec.rb @@ -0,0 +1,144 @@ +require 'spec_helper' + +describe IssueObserver do + let(:some_user) { double(:user, :id => 1) } + let(:assignee) { double(:user, :id => 2) } + let(:issue) { double(:issue, :id => 42, :assignee => assignee) } + + before(:each) { subject.stub(:current_user).and_return(some_user) } + + subject { IssueObserver.instance } + + describe '#after_create' do + + it 'is called when an issue is created' do + subject.should_receive(:after_create) + + Issue.observers.enable :issue_observer do + Factory.create(:issue, :project => Factory.create(:project)) + end + end + + it 'sends an email to the assignee' do + Notify.should_receive(:new_issue_email).with(issue.id). + and_return(double(:deliver => true)) + + subject.after_create(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 = Factory.create(:issue, :project => Factory.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) + end + end + + context 'a status "closed" note' do + it 'is created if the issue is being closed' do + issue.should_receive(:is_being_closed?).and_return(true) + Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') + + subject.after_update(issue) + end + + it '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 + end + + context 'a status "reopened" note' do + it 'is created if the issue is being reopened' do + 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 '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 + end + end + + describe '#send_reassigned_email' 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) + end + + def it_sends_a_reassigned_email_to(recipient) + Notify.should_receive(:reassigned_issue_email).with(recipient, issue.id, previous_assignee.id). + and_return(double(:deliver => true)) + 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) + 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) + end + + context 'does not send an email to the user who made the reassignment' do + it 'if the user is the assignee' do + subject.stub(:current_user).and_return(assignee) + 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) + 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) + end + end + end +end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 791e9cdeb9fac7b4c48f0bf194cdd56bd4c6f308..fd3af62d74d0fd4eba691dca0bb55f398e1a1f35 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -20,10 +20,60 @@ describe Issue do it { Issue.should respond_to :opened } end - it { Factory.create(:issue, - :author => Factory(:user), - :assignee => Factory(:user), - :project => Factory.create(:project)).should be_valid } + subject { Factory.create(:issue, + :author => Factory(:user), + :assignee => Factory(:user), + :project => Factory.create(:project)) } + it { should be_valid } + + describe '#is_being_reassigned?' do + it 'returns true if the issue assignee has changed' do + subject.assignee = Factory(:user) + subject.is_being_reassigned?.should be_true + end + it 'returns false if the issue assignee has not changed' 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 + issue = Factory.create(:issue, + :closed => true, + :author => Factory(:user), + :assignee => Factory(:user), + :project => Factory.create(:project)) + 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_reopened?' do + it 'returns true if the closed attribute has changed and is now false' do + issue = Factory.create(:issue, + :closed => true, + :author => Factory(:user), + :assignee => Factory(:user), + :project => Factory.create(:project)) + 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 + end + end describe "plus 1" do let(:project) { Factory(:project) } @@ -56,6 +106,7 @@ describe Issue do subject.upvotes.should == 2 end end + end # == Schema Information # diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index c74f72778104110690e7ce66515634451aa70237..af6ee9b889becebcd2c4f4ad5399ab2ac9500be2 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -70,6 +70,25 @@ describe Note do end end + describe '#create_status_change_note' do + let(:project) { Factory.create(:project) } + let(:thing) { Factory.create(:issue, :project => project) } + let(:author) { Factory(:user) } + let(:status) { 'new_status' } + + subject { Note.create_status_change_note(thing, author, status) } + + it 'creates and saves a Note' do + should be_a Note + subject.id.should_not be_nil + end + + its(:noteable) { should == thing } + its(:project) { should == thing.project } + its(:author) { should == author } + its(:note) { should =~ /Status changed to #{status}/ } + end + describe :authorization do before do @p1 = project diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6285a852c2335caa1cdd01515e875d21a52672ec..d28668b2445dce876ccd597acb5545126251a45b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -72,16 +72,29 @@ describe Project do end describe "last_activity" do - let(:project) { Factory :project } + let(:project) { Factory :project } + let(:last_event) { double } before do - @issue = Factory :issue, :project => project + project.stub(:events).and_return( [ double, double, last_event ] ) end - it { project.last_activity.should == Event.last } - it { project.last_activity_date.to_s.should == Event.last.created_at.to_s } + it { project.last_activity.should == last_event } end + describe 'last_activity_date' do + let(:project) { Factory :project } + + it 'returns the creation date of the project\'s last event if present' do + last_event = double(:created_at => 'now') + project.stub(:events).and_return( [double, double, last_event] ) + project.last_activity_date.should == last_event.created_at + end + + it 'returns the project\'s last update date if it has no events' do + project.last_activity_date.should == project.updated_at + end + end describe "fresh commits" do let(:project) { Factory :project } diff --git a/spec/models/user_observer_spec.rb b/spec/models/user_observer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..23dac98bb7438327c4e00d3e02a61b955cedff95 --- /dev/null +++ b/spec/models/user_observer_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe UserObserver do + subject { UserObserver.instance } + + it 'calls #after_create when new users are created' do + new_user = Factory.new(:user) + subject.should_receive(:after_create).with(new_user) + + User.observers.enable :user_observer do + new_user.save + end + end + + context 'when a new user is created' do + let(:user) { double(:user, id: 42, password: 'P@ssword!') } + let(:notification) { double :notification } + + it 'sends an email' do + notification.should_receive(:deliver) + Notify.should_receive(:new_user_email).with(user.id, user.password).and_return(notification) + + subject.after_create(user) + end + end +end diff --git a/spec/requests/admin/admin_projects_spec.rb b/spec/requests/admin/admin_projects_spec.rb index 9a33c6936a7041f9d518dad51b69e8819c42f04e..fb6577de32688b2da74ad48d0948e13b3002742e 100644 --- a/spec/requests/admin/admin_projects_spec.rb +++ b/spec/requests/admin/admin_projects_spec.rb @@ -88,6 +88,7 @@ describe "Admin::Projects" do fill_in 'Name', :with => 'NewProject' fill_in 'Code', :with => 'NPR' fill_in 'Path', :with => 'gitlabhq_1' + fill_in 'Description', :with => 'New Project Description' expect { click_button "Save" }.to change { Project.count }.by(1) @project = Project.last end diff --git a/spec/requests/admin/admin_users_spec.rb b/spec/requests/admin/admin_users_spec.rb index 91082a644b5fceaa675766627af0cd987dee37d1..ba6831e3d8b713132aa2a1e7ebcab2c8d01a594d 100644 --- a/spec/requests/admin/admin_users_spec.rb +++ b/spec/requests/admin/admin_users_spec.rb @@ -41,16 +41,23 @@ describe "Admin::Users" do it "should call send mail" do Notify.should_receive(:new_user_email).and_return(stub(:deliver => true)) - click_button "Save" + + User.observers.enable :user_observer do + click_button "Save" + end end it "should send valid email to user with email & password" do - click_button "Save" - user = User.last - email = ActionMailer::Base.deliveries.last - email.subject.should have_content("Account was created") - email.body.should have_content(user.email) - email.body.should have_content(@password) + User.observers.enable :user_observer do + with_resque do + click_button "Save" + end + user = User.last + email = ActionMailer::Base.deliveries.last + email.subject.should have_content("Account was created") + email.body.should have_content(user.email) + email.body.should have_content(@password) + end end end diff --git a/spec/requests/issues_spec.rb b/spec/requests/issues_spec.rb index 5c59675b4592cd6b8f03bcc78b674f5f3f526038..2c8650a8402e0b183d95ac3a7720d3c42363e28c 100644 --- a/spec/requests/issues_spec.rb +++ b/spec/requests/issues_spec.rb @@ -128,16 +128,22 @@ describe "Issues" do end it "should call send mail" do - Notify.should_receive(:new_issue_email).and_return(stub(:deliver => true)) - click_button "Submit new issue" + Issue.observers.enable :issue_observer do + Notify.should_receive(:new_issue_email).and_return(stub(:deliver => true)) + click_button "Submit new issue" + end end it "should send valid email to user" do - click_button "Submit new issue" - issue = Issue.last - email = ActionMailer::Base.deliveries.last - email.subject.should have_content("New Issue was created") - email.body.should have_content(issue.title) + Issue.observers.enable :issue_observer do + with_resque do + click_button "Submit new issue" + end + issue = Issue.last + email = ActionMailer::Base.deliveries.last + email.subject.should have_content("New Issue was created") + email.body.should have_content(issue.title) + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 18b7854d102d53b2a87182037dc4222c2d9589a9..5556798f5111a357dc60bf1b3472c86d766c4110 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -52,6 +52,7 @@ RSpec.configure do |config| DatabaseCleaner.start WebMock.disable_net_connect!(allow_localhost: true) + ActiveRecord::Base.observers.disable :all end config.after do