From ece0edabac9c6125f6cde83cbd2e64d118d20cf2 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 12 Dec 2016 14:50:30 +0100 Subject: [PATCH 1/4] move spec from legacy --- spec/models/user_spec.rb | 88 +++++++++++++++++++++++++++++++++++ spec_legacy/unit/user_spec.rb | 77 ------------------------------ 2 files changed, 88 insertions(+), 77 deletions(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e096917fb5..da1def2cc5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -534,4 +534,92 @@ 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, + author: author) + end + let(:author) do + FactoryGirl.build_stubbed(:user) + end + let(:assignee) do + FactoryGirl.build_stubbed(:user) + end + let(:project) do + work_package.project + end + let(:role) do + FactoryGirl.build_stubbed(:role) + 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 and isn't an author, creator, or assignee" 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 and 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 and 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 and is the assignee' do + assignee.mail_notification = 'only_assigned' + expect(assignee.notify_about?(work_package)).to be_truthy + end + + it 'is false for a user with :only_assigned and is not the assignee' 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 and 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 and 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 and 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 and is the assignee' do + assignee.mail_notification = 'selected' + expect(assignee.notify_about?(work_package)).to be_truthy + end + + it 'is false for a user with :selected and is not the author or assignee' 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_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 From 3e4f465528653c856ece4b9ead9cd9aac28f32d7 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 12 Dec 2016 15:14:18 +0100 Subject: [PATCH 2/4] refactor check for active user --- app/models/user.rb | 18 +----------------- app/models/work_package.rb | 21 ++++++++++++++++++--- spec/models/user_spec.rb | 6 ++++++ 3 files changed, 25 insertions(+), 20 deletions(-) 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..b77c636265 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -435,12 +435,12 @@ class WorkPackage < ActiveRecord::Base 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) + notified << author if author && 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) } + notified += assigned_to.users.select { |u| u.notify_about?(self) } else - notified << assigned_to if assigned_to.active? && assigned_to.notify_about?(self) + notified << assigned_to if assigned_to.notify_about?(self) end end notified.uniq! @@ -448,6 +448,21 @@ class WorkPackage < ActiveRecord::Base notified.select { |user| visible?(user) } end + def notify?(user) + case user.mail_notification + when 'selected', 'only_my_events' + author == user || user.is_or_belongs_to?(assigned_to) + when 'none' + false + when 'only_assigned' + user.is_or_belongs_to?(assigned_to) + when 'only_owner' + author == user + else + false + end + end + def done_ratio if WorkPackage.use_status_for_done_ratio? && status && status.default_done_ratio status.default_done_ratio diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index da1def2cc5..1bc38c4bc0 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -554,6 +554,12 @@ describe User, type: :model 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' From defe6e75a8b6b1d76e766d27d430f1c266dacd26 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 12 Dec 2016 15:52:56 +0100 Subject: [PATCH 3/4] allow responsible to receive notifications --- app/models/work_package.rb | 35 ++++++++++++++++++-------------- spec/models/user_spec.rb | 34 ++++++++++++++++++++++--------- spec/models/work_package_spec.rb | 33 ++++++++++++++++++++++++------ 3 files changed, 71 insertions(+), 31 deletions(-) diff --git a/app/models/work_package.rb b/app/models/work_package.rb index b77c636265..cc6f2e7d3c 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -432,30 +432,21 @@ 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.notify_about?(self) - if assigned_to - if assigned_to.is_a?(Group) - notified += assigned_to.users.select { |u| u.notify_about?(self) } - else - notified << assigned_to if 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) + 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?(assigned_to) || user.is_or_belongs_to?(responsible) when 'only_owner' author == user else @@ -951,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 1bc38c4bc0..42d2d5925b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -539,6 +539,7 @@ describe User, type: :model do let(:work_package) do FactoryGirl.build_stubbed(:work_package, assigned_to: assignee, + responsible: responsible, author: author) end let(:author) do @@ -547,6 +548,9 @@ describe User, type: :model do let(:assignee) do FactoryGirl.build_stubbed(:user) end + let(:responsible) do + FactoryGirl.build_stubbed(:user) + end let(:project) do work_package.project end @@ -571,7 +575,7 @@ describe User, type: :model do expect(author.notify_about?(work_package)).to be_falsey end - it "is false for a user with :only_my_events and isn't an author, creator, or assignee" do + 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] } @@ -579,47 +583,57 @@ describe User, type: :model do expect(user.notify_about?(work_package)).to be_falsey end - it 'is true for a user with :only_my_events and is the author' do + 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 and is the assignee' do + 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 and is the assignee' do + 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 false for a user with :only_assigned and is not the assignee' do + 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 and is the author' do + 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 and is not the author' do + 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 and is the author' do + 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 and is the assignee' do + 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 false for a user with :selected and is not the author or assignee' do + 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] } diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index 9a303ecdcc..80472d6587 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -918,12 +918,10 @@ describe WorkPackage, type: :model do 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(:author) { FactoryGirl.create(:user, mail_notification: 'only_owner') } + let(:assignee) { FactoryGirl.create(:user, mail_notification: 'only_assigned') } + let(:responsible) { FactoryGirl.create(:user, mail_notification: 'only_assigned') } + let(:role) { FactoryGirl.create(:role, permissions: [:view_work_packages]) } let(:project_member) { FactoryGirl.create(:member, user: member, @@ -942,10 +940,17 @@ describe WorkPackage, type: :model do project: project, roles: [role]) } + let(:project_responsible) { + FactoryGirl.create(:member, + user: responsible, + project: project, + roles: [role]) + } let(:work_package) { FactoryGirl.create(:work_package, author: author, assigned_to: assignee, + responsible: responsible, project: project) } @@ -1003,6 +1008,22 @@ describe WorkPackage, type: :model do it_behaves_like 'includes expected users' end + describe 'includes work package responsible' do + before do + project_responsible + end + + context 'pre-condition' do + subject { work_package.responsible } + + it { is_expected.not_to be_nil } + end + + let(:expected_users) { work_package.responsible } + + it_behaves_like 'includes expected users' + end + context 'mail notification settings' do before do project_author From 4b1494a4d9da72ec1b46abba9285074763795411 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 12 Dec 2016 16:42:26 +0100 Subject: [PATCH 4/4] respec WorkPackage#recipients --- spec/models/work_package_spec.rb | 208 +++++++++++++++---------------- 1 file changed, 103 insertions(+), 105 deletions(-) diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index 80472d6587..2f79a065d6 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -916,142 +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, mail_notification: 'only_owner') } - let(:assignee) { FactoryGirl.create(:user, mail_notification: 'only_assigned') } - let(:responsible) { FactoryGirl.create(:user, mail_notification: 'only_assigned') } - 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(:project_responsible) { - FactoryGirl.create(:member, - user: responsible, - project: project, - roles: [role]) - } - let(:work_package) { - FactoryGirl.create(:work_package, - author: author, - assigned_to: assignee, - responsible: responsible, - 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 } - - it { is_expected.not_to be_empty } - end - - let(:expected_users) { project.recipients } - - it_behaves_like 'includes expected users' + let(:users_with_view_permission) do + project_notified_users + [author, assignee, responsible] end - describe 'includes work package author' do - before do project_author end - - context 'pre-condition' do - subject { work_package.author } - - it { is_expected.not_to be_nil } + 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) { work_package.author } - - it_behaves_like 'includes expected users' end - describe 'includes work package assignee' do - before do project_assignee end - - context 'pre-condition' do - subject { work_package.assigned_to } + 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.assigned_to } - - 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 responsible' do + context 'with author, assignee, responsible not interested' do before do - project_responsible + [author, assignee, responsible].each do |user| + allow(user) + .to receive(:notify_about?) + .with(work_package) + .and_return(false) + end end - context 'pre-condition' do - subject { work_package.responsible } - - it { is_expected.not_to be_nil } + it 'does not contain such users' do + expect(work_package.recipients) + .to match_array project_notified_users end - - let(:expected_users) { work_package.responsible } - - it_behaves_like 'includes expected users' 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