diff --git a/app/workers/mails/member_created_job.rb b/app/workers/mails/member_created_job.rb index b523819138..e631ff9f35 100644 --- a/app/workers/mails/member_created_job.rb +++ b/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 diff --git a/app/workers/mails/member_job.rb b/app/workers/mails/member_job.rb index 163008a6b6..9286b205d9 100644 --- a/app/workers/mails/member_job.rb +++ b/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 diff --git a/app/workers/mails/member_roles_diff.rb b/app/workers/mails/member_roles_diff.rb new file mode 100644 index 0000000000..05f259531d --- /dev/null +++ b/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 diff --git a/spec/workers/mails/member_created_job_spec.rb b/spec/workers/mails/member_created_job_spec.rb index 4005775e66..041a080df6 100644 --- a/spec/workers/mails/member_created_job_spec.rb +++ b/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 diff --git a/spec/workers/mails/member_roles_diff_spec.rb b/spec/workers/mails/member_roles_diff_spec.rb new file mode 100644 index 0000000000..bc2e7b4aee --- /dev/null +++ b/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 diff --git a/spec/workers/mails/member_updated_job_spec.rb b/spec/workers/mails/member_updated_job_spec.rb index a49ff849f5..de04189c28 100644 --- a/spec/workers/mails/member_updated_job_spec.rb +++ b/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