diff --git a/app/models/user.rb b/app/models/user.rb index 9ce8404737..9c621a01ed 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -618,23 +618,7 @@ class User < Principal # Utility method to help check if a user should be notified about an # event. def notify_about?(object) - case mail_notification - when 'all' - true - when 'selected' - # user receives notifications for created/assigned issues on unselected projects - object.is_a?(WorkPackage) && (object.author == self || is_or_belongs_to?(object.assigned_to)) - when 'none' - false - when 'only_my_events' - object.is_a?(WorkPackage) && (object.author == self || is_or_belongs_to?(object.assigned_to)) - when 'only_assigned' - object.is_a?(WorkPackage) && is_or_belongs_to?(object.assigned_to) - when 'only_owner' - object.is_a?(WorkPackage) && object.author == self - else - false - end + active? && (mail_notification == 'all' || (object.is_a?(WorkPackage) && object.notify?(self))) end def reported_work_package_count diff --git a/app/models/work_package.rb b/app/models/work_package.rb index c4de8f3c55..cc6f2e7d3c 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -432,20 +432,26 @@ class WorkPackage < ActiveRecord::Base # >>> issues.rb >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> # Returns users that should be notified def recipients - notified = project.notified_users - # Author and assignee are always notified unless they have been - # locked or don't want to be notified - notified << author if author && author.active? && author.notify_about?(self) - if assigned_to - if assigned_to.is_a?(Group) - notified += assigned_to.users.select { |u| u.active? && u.notify_about?(self) } - else - notified << assigned_to if assigned_to.active? && assigned_to.notify_about?(self) - end - end + notified = project.notified_users + attribute_users.select { |u| u.notify_about?(self) } + notified.uniq! - # Remove users that can not view the issue - notified.select { |user| visible?(user) } + # Remove users that can not view the work package + notified & User.allowed(:view_work_packages, project) + end + + def notify?(user) + case user.mail_notification + when 'selected', 'only_my_events' + author == user || user.is_or_belongs_to?(assigned_to) || user.is_or_belongs_to?(responsible) + when 'none' + false + when 'only_assigned' + user.is_or_belongs_to?(assigned_to) || user.is_or_belongs_to?(responsible) + when 'only_owner' + author == user + else + false + end end def done_ratio @@ -936,4 +942,18 @@ class WorkPackage < ActiveRecord::Base .pluck('SUM(hours)') .first end + + def attribute_users + related = [author] + + [responsible, assigned_to].each do |user| + if user.is_a?(Group) + related += user.users + else + related << user + end + end + + related.select(&:present?) + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e096917fb5..42d2d5925b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -534,4 +534,112 @@ describe User, type: :model do it { expect(user.impaired?).to be_truthy } end end + + describe '#notify_about?' do + let(:work_package) do + FactoryGirl.build_stubbed(:work_package, + assigned_to: assignee, + responsible: responsible, + author: author) + end + let(:author) do + FactoryGirl.build_stubbed(:user) + end + let(:assignee) do + FactoryGirl.build_stubbed(:user) + end + let(:responsible) do + FactoryGirl.build_stubbed(:user) + end + let(:project) do + work_package.project + end + let(:role) do + FactoryGirl.build_stubbed(:role) + end + + it 'is false for an inactive user' do + user.status = User::STATUSES[:locked] + user.mail_notification = 'all' + expect(user.notify_about?({})).to be_falsey + end + + context 'Work package' do + it 'is true for a user with :all' do + author.mail_notification = 'all' + assert author.notify_about?(work_package) + end + + it 'is false for a user with :none' do + author.mail_notification = 'none' + expect(author.notify_about?(work_package)).to be_falsey + end + + it "is false for a user with :only_my_events who has no relation to the work package" do + user = FactoryGirl.build_stubbed(:user, mail_notification: 'only_my_events') + (Member.new.tap do |m| + m.attributes = { user: user, project: project, role_ids: [role.id] } + end) + expect(user.notify_about?(work_package)).to be_falsey + end + + it 'is true for a user with :only_my_events who is the author' do + author.mail_notification = 'only_my_events' + expect(author.notify_about?(work_package)).to be_truthy + end + + it 'is true for a user with :only_my_events who is the assignee' do + assignee.mail_notification = 'only_my_events' + expect(assignee.notify_about?(work_package)).to be_truthy + end + + it 'is true for a user with :only_assigned who is the assignee' do + assignee.mail_notification = 'only_assigned' + expect(assignee.notify_about?(work_package)).to be_truthy + end + + it 'is true for a user with :only_assigned who is the responsible' do + responsible.mail_notification = 'only_assigned' + expect(responsible.notify_about?(work_package)).to be_truthy + end + + it 'is false for a user with :only_assigned who is neither assignee nor responsible' do + author.mail_notification = 'only_assigned' + expect(author.notify_about?(work_package)).to be_falsey + end + + it 'is true for a user with :only_owner who is the author' do + author.mail_notification = 'only_owner' + expect(author.notify_about?(work_package)).to be_truthy + end + + it 'is false for a user with :only_owner who is not the author' do + assignee.mail_notification = 'only_owner' + expect(assignee.notify_about?(work_package)).to be_falsey + end + + it 'is true for a user with :selected who is the author' do + author.mail_notification = 'selected' + expect(author.notify_about?(work_package)).to be_truthy + end + + it 'is true for a user with :selected who is the assignee' do + assignee.mail_notification = 'selected' + expect(assignee.notify_about?(work_package)).to be_truthy + end + + it 'is true for a user with :selected who is the responsible' do + responsible.mail_notification = 'selected' + expect(responsible.notify_about?(work_package)).to be_truthy + end + + it "is false for a user with :only_my_events who has no relation to the work package" do + user = FactoryGirl.build(:user, mail_notification: 'selected') + (Member.new.tap do |m| + m.attributes = { user: user, project: project, role_ids: [role.id] } + end) + expect(user.notify_about?(work_package)).to be_falsey + end + end + end end diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index 9a303ecdcc..2f79a065d6 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -916,121 +916,140 @@ describe WorkPackage, type: :model do end describe '#recipients' do - let(:project) { FactoryGirl.create(:project) } - let(:member) { FactoryGirl.create(:user) } - let(:author) { FactoryGirl.create(:user) } - let(:assignee) { FactoryGirl.create(:user) } - let(:role) { - FactoryGirl.create(:role, - permissions: [:view_work_packages]) - } - let(:project_member) { - FactoryGirl.create(:member, - user: member, - project: project, - roles: [role]) - } - let(:project_author) { - FactoryGirl.create(:member, - user: author, - project: project, - roles: [role]) - } - let(:project_assignee) { - FactoryGirl.create(:member, - user: assignee, - project: project, - roles: [role]) - } - let(:work_package) { - FactoryGirl.create(:work_package, - author: author, - assigned_to: assignee, - project: project) - } - - shared_examples_for 'includes expected users' do - subject { work_package.recipients } - - it { is_expected.to include(*expected_users) } + let(:project) { FactoryGirl.build_stubbed(:project) } + let(:member) { FactoryGirl.build_stubbed(:user) } + let(:author) { FactoryGirl.build_stubbed(:user) } + let(:assignee) { FactoryGirl.build_stubbed(:user) } + let(:responsible) { FactoryGirl.build_stubbed(:user) } + let(:work_package) do + FactoryGirl.build_stubbed(:work_package, + author: author, + assigned_to: assignee, + responsible: responsible, + project: project) end - shared_examples_for 'includes not expected users' do - subject { work_package.recipients } - - it { is_expected.not_to include(*expected_users) } + let(:project_notified_users) do + [member] end - describe 'includes project recipients' do - before do project_member end - - context 'pre-condition' do - subject { project.recipients } + let(:users_with_view_permission) do + project_notified_users + [author, assignee, responsible] + end - it { is_expected.not_to be_empty } + before do + allow(project) + .to receive(:notified_users) + .and_return(project_notified_users) + + allow(User) + .to receive(:allowed) + .and_return users_with_view_permission + + [author, assignee, responsible].each do |user| + allow(user) + .to receive(:notify_about?) + .with(work_package) + .and_return(true) end - - let(:expected_users) { project.recipients } - - it_behaves_like 'includes expected users' end - describe 'includes work package author' do - before do project_author end - - context 'pre-condition' do - subject { work_package.author } + it 'contains author, assignee, responsible and all from project#notified_users' do + expect(work_package.recipients) + .to match_array users_with_view_permission + end - it { is_expected.not_to be_nil } + context 'with users lacking the view permission' do + let(:users_with_view_permission) do + [] end - let(:expected_users) { work_package.author } - - it_behaves_like 'includes expected users' + it 'does not contain such users' do + expect(work_package.recipients) + .to be_empty + end end - describe 'includes work package assignee' do - before do project_assignee end - - context 'pre-condition' do - subject { work_package.assigned_to } - - it { is_expected.not_to be_nil } + context 'with author, assignee, responsible not interested' do + before do + [author, assignee, responsible].each do |user| + allow(user) + .to receive(:notify_about?) + .with(work_package) + .and_return(false) + end end - let(:expected_users) { work_package.assigned_to } - - it_behaves_like 'includes expected users' + it 'does not contain such users' do + expect(work_package.recipients) + .to match_array project_notified_users + end end - context 'mail notification settings' do - before do - project_author - project_assignee + context 'with author, assignee, responsible also being in project#notified_users' do + let(:project_notified_users) do + [member] + [author, assignee, responsible] end - describe '#none' do - before do author.update_attribute(:mail_notification, :none) end + it 'contains the users but once' do + expect(work_package.recipients) + .to match_array project_notified_users + end + end - let(:expected_users) { work_package.author.mail } + context 'with a group' do + let(:user1) { FactoryGirl.build_stubbed(:user) } + let(:user2) { FactoryGirl.build_stubbed(:user) } + let(:user3) { FactoryGirl.build_stubbed(:user) } - it_behaves_like 'includes not expected users' + let(:users_with_view_permission) do + [user1, user3] end - describe '#only_assigned' do - before do author.update_attribute(:mail_notification, :only_assigned) end + before do + allow(user1) + .to receive(:notify_about?) + .with(work_package) + .and_return(false) + + [user2, user3].each do |user| + allow(user) + .to receive(:notify_about?) + .with(work_package) + .and_return(true) + end + end - let(:expected_users) { work_package.author.mail } + context 'for assignee' do + let(:assignee) do + group = FactoryGirl.build_stubbed(:group) + allow(group) + .to receive(:users) + .and_return([user1, user2, user3]) + group + end - it_behaves_like 'includes not expected users' + it 'returns those group members who want to be notified + and who have the permission to see the work package' do + expect(work_package.recipients) + .to match_array [user3] + end end - describe '#only_assigned' do - before do assignee.update_attribute(:mail_notification, :only_owner) end - - let(:expected_users) { work_package.assigned_to.mail } + context 'for responsible' do + let(:responsible) do + group = FactoryGirl.build_stubbed(:group) + allow(group) + .to receive(:users) + .and_return([user1, user2, user3]) + group + end - it_behaves_like 'includes not expected users' + it 'returns those group members who want to be notified + and who have the permission to see the work package' do + expect(work_package.recipients) + .to match_array [user3] + end end end end diff --git a/spec_legacy/unit/user_spec.rb b/spec_legacy/unit/user_spec.rb index 6b51100d04..ef6177abb2 100644 --- a/spec_legacy/unit/user_spec.rb +++ b/spec_legacy/unit/user_spec.rb @@ -476,81 +476,4 @@ describe User, type: :model do end end end - - context 'User#notify_about?' do - context 'Issues' do - before do - @project = Project.find(1) - @author = FactoryGirl.create(:user) - @assignee = FactoryGirl.create(:user) - @issue = FactoryGirl.create(:work_package, project: @project, assigned_to: @assignee, author: @author) - end - - it 'should be true for a user with :all' do - @author.update_attribute(:mail_notification, 'all') - assert @author.notify_about?(@issue) - end - - it 'should be false for a user with :none' do - @author.update_attribute(:mail_notification, 'none') - assert ! @author.notify_about?(@issue) - end - - it "should be false for a user with :only_my_events and isn't an author, creator, or assignee" do - @user = FactoryGirl.create(:user, mail_notification: 'only_my_events') - (Member.new.tap do |m| - m.attributes = { user: @user, project: @project, role_ids: [1] } - end).save! - assert ! @user.notify_about?(@issue) - end - - it 'should be true for a user with :only_my_events and is the author' do - @author.update_attribute(:mail_notification, 'only_my_events') - assert @author.notify_about?(@issue) - end - - it 'should be true for a user with :only_my_events and is the assignee' do - @assignee.update_attribute(:mail_notification, 'only_my_events') - assert @assignee.notify_about?(@issue) - end - - it 'should be true for a user with :only_assigned and is the assignee' do - @assignee.update_attribute(:mail_notification, 'only_assigned') - assert @assignee.notify_about?(@issue) - end - - it 'should be false for a user with :only_assigned and is not the assignee' do - @author.update_attribute(:mail_notification, 'only_assigned') - assert ! @author.notify_about?(@issue) - end - - it 'should be true for a user with :only_owner and is the author' do - @author.update_attribute(:mail_notification, 'only_owner') - assert @author.notify_about?(@issue) - end - - it 'should be false for a user with :only_owner and is not the author' do - @assignee.update_attribute(:mail_notification, 'only_owner') - assert ! @assignee.notify_about?(@issue) - end - - it 'should be true for a user with :selected and is the author' do - @author.update_attribute(:mail_notification, 'selected') - assert @author.notify_about?(@issue) - end - - it 'should be true for a user with :selected and is the assignee' do - @assignee.update_attribute(:mail_notification, 'selected') - assert @assignee.notify_about?(@issue) - end - - it 'should be false for a user with :selected and is not the author or assignee' do - @user = FactoryGirl.create(:user, mail_notification: 'selected') - (Member.new.tap do |m| - m.attributes = { user: @user, project: @project, role_ids: [1] } - end).save! - assert ! @user.notify_about?(@issue) - end - end - end end