send emails when adding global role to a group

Also refactor the logic to decide if a 'membership created' or 'membership updated' email should be sent

app/workers/mails/member_created_job.rb

@@ -32,19 +32,11 @@ class Mails::MemberCreatedJob < Mails::MemberJob
   alias_method :send_for_project_user, :send_added_project
 
   def send_for_group_user(current_user, user_member, group_member, message)
-    if new_roles_added?(user_member, group_member)
-      send_updated_project(current_user, user_member, message)
-    elsif all_roles_added?(user_member, group_member)
+    difference = Mails::MemberRolesDiff.new(user_member, group_member)
+    if difference.roles_created?
       send_added_project(current_user, user_member, message)
+    elsif difference.roles_updated?
+      send_updated_project(current_user, user_member, message)
     end
   end
-
-  def new_roles_added?(user_member, group_member)
-    (group_member.member_roles.map(&:id) - user_member.member_roles.map(&:inherited_from)).length <
-      group_member.member_roles.length && user_member.member_roles.any? { |mr| mr.inherited_from.nil? }
-  end
-
-  def all_roles_added?(user_member, group_member)
-    (user_member.member_roles.map(&:inherited_from) - group_member.member_roles.map(&:id)).empty?
-  end
 end

app/workers/mails/member_job.rb

@@ -32,14 +32,22 @@ class Mails::MemberJob < ApplicationJob
   def perform(current_user:,
               member:,
               message: nil)
-    if member.project.nil?
-      send_updated_global(current_user, member, message)
-    elsif member.principal.is_a?(Group)
+    if member.principal.is_a?(Group)
       every_group_user_member(member) do |user_member|
-        send_for_group_user(current_user, user_member, member, message)
+        next unless roles_changed?(user_member, member)
+
+        if member.project.nil?
+          send_updated_global(current_user, user_member, message)
+        else
+          send_for_group_user(current_user, user_member, member, message)
+        end
       end
     elsif member.principal.is_a?(User)
-      send_for_project_user(current_user, member, message)
+      if member.project.nil?
+        send_updated_global(current_user, member, message)
+      else
+        send_for_project_user(current_user, member, message)
+      end
     end
   end
 
@@ -95,4 +103,8 @@ class Mails::MemberJob < ApplicationJob
       .where(project_id: nil, user_id: user_id)
       .exists?("membership_#{setting}" => false)
   end
+
+  def roles_changed?(user_member, group_member)
+    Mails::MemberRolesDiff.new(user_member, group_member).roles_changed?
+  end
 end

app/workers/mails/member_roles_diff.rb

@@ -0,0 +1,73 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+class Mails::MemberRolesDiff
+  attr_reader :user_member, :group_member
+
+  def initialize(user_member, group_member)
+    raise ArgumentError unless user_member.project_id == group_member.project_id
+
+    @user_member = user_member
+    @group_member = group_member
+  end
+
+  def roles_created?
+    result == :roles_created
+  end
+
+  def roles_updated?
+    result == :roles_updated
+  end
+
+  def roles_changed?
+    result != :roles_unchanged
+  end
+
+  def result
+    @result ||=
+      if user_previous_member_roles_ids.empty?
+        :roles_created
+      elsif (group_roles_ids - user_previous_member_roles_ids).any?
+        :roles_updated
+      else
+        :roles_unchanged
+      end
+  end
+
+  private
+
+  def user_previous_member_roles_ids
+    Set.new(user_member.member_roles
+      .reject { group_member.member_roles.map(&:id).include?(_1.inherited_from) }
+      .map(&:role_id).uniq)
+  end
+
+  def group_roles_ids
+    Set.new(group_member.member_roles.map(&:role_id))
+  end
+end

spec/workers/mails/member_created_job_spec.rb

@@ -36,9 +36,9 @@ describe Mails::MemberCreatedJob, type: :model do
     context 'with a group membership' do
       let(:member) do
         build_stubbed(:member,
-                                 project: project,
-                                 principal: group,
-                                 member_roles: group_member_roles)
+                      project: project,
+                      principal: group,
+                      member_roles: group_member_roles)
       end
 
       before do
@@ -48,8 +48,8 @@ describe Mails::MemberCreatedJob, type: :model do
       context 'with the user not having had a membership before the group`s membership was added' do
         let(:group_user_member_roles) do
           [build_stubbed(:member_role,
-                                    role: role,
-                                    inherited_from: group_member_roles.first.id)]
+                         role: role,
+                         inherited_from: group_member_roles.first.id)]
         end
 
         it 'sends mail' do
@@ -64,8 +64,8 @@ describe Mails::MemberCreatedJob, type: :model do
       context 'with the user having had a membership with the same roles before the group`s membership was added' do
         let(:group_user_member_roles) do
           [build_stubbed(:member_role,
-                                    role: role,
-                                    inherited_from: nil)]
+                         role: role,
+                         inherited_from: nil)]
         end
 
         it_behaves_like 'sends no mail'
@@ -75,32 +75,106 @@ describe Mails::MemberCreatedJob, type: :model do
                from another group before the group`s membership was added' do
         let(:group_user_member_roles) do
           [build_stubbed(:member_role,
-                                    role: role,
-                                    inherited_from: group_member_roles.first.id + 5)]
+                         role: role,
+                         inherited_from: group_member_roles.first.id + 5)]
         end
 
         it_behaves_like 'sends no mail'
       end
 
-      context 'with the user having had a membership before the group`s membership was added but now has additional roles' do
+      context 'with the user having had a membership before the group`s membership ' +
+          'was added but now has additional roles' do
         let(:other_role) { build_stubbed(:role) }
         let(:group_user_member_roles) do
           [build_stubbed(:member_role,
-                                    role: role,
-                                    inherited_from: group_member_roles.first.id),
+                         role: role,
+                         inherited_from: group_member_roles.first.id),
            build_stubbed(:member_role,
-                                    role: other_role,
-                                    inherited_from: nil)]
+                         role: other_role,
+                         inherited_from: nil)]
         end
 
         it 'sends mail' do
           run_job
 
+          expect(MemberMailer)
+            .not_to have_received(:added_project)
           expect(MemberMailer)
             .to have_received(:updated_project)
                   .with(current_user, group_user_member, message)
         end
       end
     end
+
+    context 'with a group global membership' do
+      let(:project) { nil }
+      let(:member) do
+        build_stubbed(:member,
+                      project: project,
+                      principal: group,
+                      member_roles: group_member_roles)
+      end
+
+      before do
+        group_user_member
+      end
+
+      context 'with the user not having had a membership before the group`s membership was added' do
+        let(:group_user_member_roles) do
+          [build_stubbed(:member_role,
+                         role: role,
+                         inherited_from: group_member_roles.first.id)]
+        end
+
+        it 'sends mail' do
+          run_job
+
+          expect(MemberMailer)
+            .to have_received(:updated_global)
+                  .with(current_user, group_user_member, message)
+        end
+      end
+
+      context 'with the user having had a membership with the same roles before the group`s membership was added' do
+        let(:group_user_member_roles) do
+          [build_stubbed(:member_role,
+                         role: role,
+                         inherited_from: nil)]
+        end
+
+        it_behaves_like 'sends no mail'
+      end
+
+      context 'with the user having had a membership with the same roles
+               from another group before the group`s membership was added' do
+        let(:group_user_member_roles) do
+          [build_stubbed(:member_role,
+                         role: role,
+                         inherited_from: group_member_roles.first.id + 5)]
+        end
+
+        it_behaves_like 'sends no mail'
+      end
+
+      context 'with the user having had a membership before the group`s membership was added but now has additional roles' do
+        let(:other_role) { build_stubbed(:role) }
+        let(:group_user_member_roles) do
+          [build_stubbed(:member_role,
+                         role: role,
+                         inherited_from: group_member_roles.first.id),
+           build_stubbed(:member_role,
+                         role: other_role,
+                         inherited_from: nil)]
+        end
+
+        it 'sends mail' do
+          run_job
+
+          expect(MemberMailer)
+            .to have_received(:updated_global)
+                  .with(current_user, group_user_member, message)
+        end
+      end
+    end
   end
 end

spec/workers/mails/member_roles_diff_spec.rb

@@ -0,0 +1,207 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require 'spec_helper'
+
+# rubocop:disable RSpec/MultipleMemoizedHelpers
+describe Mails::MemberRolesDiff, type: :model do
+  let(:project) { FactoryBot.build_stubbed(:project) }
+  let(:group) { FactoryBot.build_stubbed(:group) }
+  let(:user) { FactoryBot.build_stubbed(:user) }
+  let(:role) { FactoryBot.build_stubbed(:role) }
+  let(:role_other) { FactoryBot.build_stubbed(:role) }
+
+  let(:group_member_role) { FactoryBot.build_stubbed(:member_role, role: role) }
+  let(:group_member_role_other) { FactoryBot.build_stubbed(:member_role, role: role_other) }
+  let(:group_member_roles) { raise NotImplementedError('please set group_member_roles') }
+  let(:group_member) do
+    FactoryBot.build_stubbed(:member, principal: group, project: project, member_roles: group_member_roles)
+  end
+
+  let(:user_member_role) do
+    FactoryBot.build_stubbed(:member_role, role: role)
+  end
+  let(:user_member_role_inherited) do
+    FactoryBot.build_stubbed(:member_role, role: role, inherited_from: group_member_role.id)
+  end
+  let(:user_member_role_other) do
+    FactoryBot.build_stubbed(:member_role, role: role_other)
+  end
+  let(:user_member_role_other_inherited) do
+    FactoryBot.build_stubbed(:member_role, role: role_other, inherited_from: group_member_role_other.id)
+  end
+  let(:user_member_roles) { raise NotImplementedError('please set user_member_roles') }
+  let(:user_member) do
+    FactoryBot.build_stubbed(:member, principal: user, project: project, member_roles: user_member_roles)
+  end
+
+  subject(:difference) do
+    described_class.new(user_member, group_member)
+  end
+
+  shared_examples 'roles created' do
+    it 'results in roles created' do
+      expect(difference.result).to eq(:roles_created)
+    end
+  end
+
+  shared_examples 'roles updated' do
+    it 'results in roles updated' do
+      expect(difference.result).to eq(:roles_updated)
+    end
+  end
+
+  shared_examples 'roles unchanged' do
+    it 'results in roles unchanged' do
+      expect(difference.result).to eq(:roles_unchanged)
+    end
+  end
+
+  context 'when group has added all its roles to a user' do
+    let(:group_member_roles) { [group_member_role, group_member_role_other] }
+    let(:user_member_roles) do
+      [
+        user_member_role_inherited,
+        user_member_role_other_inherited
+      ]
+    end
+
+    include_examples 'roles created'
+  end
+
+  context 'when group has added all its roles to a user who already had some preexisting other roles' do
+    let(:group_member_roles) { [group_member_role] }
+    let(:user_member_roles) do
+      [
+        user_member_role_other,
+        user_member_role_inherited
+      ]
+    end
+
+    include_examples 'roles updated'
+  end
+
+  context 'when group has added a new role and an existing role to a user' do
+    let(:group_member_roles) { [group_member_role, group_member_role_other] }
+    let(:user_member_roles) do
+      [
+        user_member_role,
+        user_member_role_inherited,
+        user_member_role_other_inherited
+      ]
+    end
+
+    include_examples 'roles updated'
+  end
+
+  context 'when group has added already existing roles to a user' do
+    let(:group_member_roles) { [group_member_role, group_member_role_other] }
+    let(:user_member_roles) do
+      [
+        user_member_role,
+        user_member_role_other,
+        user_member_role_inherited,
+        user_member_role_other_inherited
+      ]
+    end
+
+    include_examples 'roles unchanged'
+  end
+
+  context 'when group did not add any roles' do
+    let(:group_member_roles) { [group_member_role, group_member_role_other] }
+    let(:user_member_roles) do
+      [
+        user_member_role,
+        user_member_role_other
+      ]
+    end
+
+    include_examples 'roles unchanged'
+  end
+
+  context 'when the projects are different between members' do
+    let(:group_member) do
+      FactoryBot.build_stubbed(
+        :member,
+        principal: group,
+        project: FactoryBot.create(:project)
+      )
+    end
+    let(:user_member) do
+      FactoryBot.build_stubbed(
+        :member,
+        principal: user,
+        project: FactoryBot.create(:project)
+      )
+    end
+
+    it 'raises ArgumentError' do
+      expect { difference.result }.to raise_error(ArgumentError)
+    end
+  end
+
+  context 'with another group defined' do
+    let(:other_group_member_role) { FactoryBot.build_stubbed(:member_role, role: role) }
+    let(:other_group_member_role_other) { FactoryBot.build_stubbed(:member_role, role: role_other) }
+    let(:user_member_role_inherited_from_other_group) do
+      FactoryBot.build_stubbed(:member_role, role: role, inherited_from: other_group_member_role.id)
+    end
+    let(:user_member_role_other_inherited_from_other_group) do
+      FactoryBot.build_stubbed(:member_role, role: role_other, inherited_from: other_group_member_role_other.id)
+    end
+
+    context 'when group has added to a user a new role and a role that already existed from another group membership' do
+      let(:group_member_roles) { [group_member_role, group_member_role_other] }
+      let(:user_member_roles) do
+        [
+          user_member_role_inherited_from_other_group,
+          user_member_role_inherited,
+          user_member_role_other_inherited
+        ]
+      end
+
+      include_examples 'roles updated'
+    end
+
+    context 'when group has added to a user some roles that already existed from another group membership' do
+      let(:group_member_roles) { [group_member_role, group_member_role_other] }
+      let(:user_member_roles) do
+        [
+          user_member_role_inherited_from_other_group,
+          user_member_role_other_inherited_from_other_group,
+          user_member_role_inherited,
+          user_member_role_other_inherited
+        ]
+      end
+
+      include_examples 'roles unchanged'
+    end
+  end
+end
+# rubocop:enable RSpec/MultipleMemoizedHelpers

spec/workers/mails/member_updated_job_spec.rb

@@ -31,7 +31,7 @@ require_relative 'shared/member_job'
 
 describe Mails::MemberUpdatedJob, type: :model do
   include_examples 'member job' do
-    let(:user_project_mail_method) { :updated_project}
+    let(:user_project_mail_method) { :updated_project }
 
     context 'with a group membership' do
       let(:member) do
@@ -100,5 +100,74 @@ describe Mails::MemberUpdatedJob, type: :model do
         it_behaves_like 'updated mail'
       end
     end
+
+    context 'with a group global membership' do
+      let(:project) { nil }
+      let(:member) do
+        FactoryBot.build_stubbed(:member,
+                                 project: project,
+                                 principal: group,
+                                 member_roles: group_member_roles)
+      end
+
+      shared_examples 'updated mail' do
+        it 'sends mail' do
+          run_job
+
+          expect(MemberMailer)
+            .to have_received(:updated_global)
+                  .with(current_user, group_user_member, message)
+        end
+      end
+
+      before do
+        group_user_member
+      end
+
+      context 'with the user not having had a membership before the group`s membership was added' do
+        let(:group_user_member_roles) do
+          [FactoryBot.build_stubbed(:member_role,
+                                    role: role,
+                                    inherited_from: group_member_roles.first.id)]
+        end
+
+        it_behaves_like 'updated mail'
+      end
+
+      context 'with the user having had a membership with the same roles before the group`s membership was added' do
+        let(:group_user_member_roles) do
+          [FactoryBot.build_stubbed(:member_role,
+                                    role: role,
+                                    inherited_from: nil)]
+        end
+
+        it_behaves_like 'sends no mail'
+      end
+
+      context 'with the user having had a membership with the same roles
+               from another group before the group`s membership was added' do
+        let(:group_user_member_roles) do
+          [FactoryBot.build_stubbed(:member_role,
+                                    role: role,
+                                    inherited_from: group_member_roles.first.id + 5)]
+        end
+
+        it_behaves_like 'sends no mail'
+      end
+
+      context 'with the user having had a membership before the group`s membership was added but now has additional roles' do
+        let(:other_role) { FactoryBot.build_stubbed(:role) }
+        let(:group_user_member_roles) do
+          [FactoryBot.build_stubbed(:member_role,
+                                    role: role,
+                                    inherited_from: group_member_roles.first.id),
+           FactoryBot.build_stubbed(:member_role,
+                                    role: other_role,
+                                    inherited_from: nil)]
+        end
+
+        it_behaves_like 'updated mail'
+      end
+    end
   end
 end