Merge remote-tracking branch 'origin/dev' into timeline

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

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

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

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

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