diff --git a/app/services/groups/concerns/membership_manipulation.rb b/app/services/groups/concerns/membership_manipulation.rb index f16c4fcd22..5450fd6d9c 100644 --- a/app/services/groups/concerns/membership_manipulation.rb +++ b/app/services/groups/concerns/membership_manipulation.rb @@ -56,7 +56,7 @@ module Groups::Concerns touch_updated(affected_member_ids) - send_notifications(affected_member_ids, message) if affected_member_ids.any? && send_notifications + send_notifications(affected_member_ids, message, send_notifications) if affected_member_ids.any? && send_notifications end def modify_members_and_roles(_params) @@ -77,8 +77,8 @@ module Groups::Concerns .touch_all end - def send_notifications(member_ids, message) - Notifications::GroupMemberAlteredJob.perform_later(member_ids, message) + def send_notifications(member_ids, message, send_notifications) + Notifications::GroupMemberAlteredJob.perform_later(member_ids, message, send_notifications) end end end diff --git a/app/services/groups/update_roles_service.rb b/app/services/groups/update_roles_service.rb index eeae604650..9aeb95300a 100644 --- a/app/services/groups/update_roles_service.rb +++ b/app/services/groups/update_roles_service.rb @@ -54,7 +54,6 @@ module Groups execute_query(sql_query) end - # rubocop:disable Metrics/AbcSize def update_roles_cte <<~SQL WITH @@ -123,6 +122,5 @@ module Groups UNION SELECT member_id from members_with_added_roles SQL end - # rubocop:enable Metrics/AbcSize end end diff --git a/app/services/members/concerns/notification_sender.rb b/app/services/members/concerns/notification_sender.rb new file mode 100644 index 0000000000..eb4b889afe --- /dev/null +++ b/app/services/members/concerns/notification_sender.rb @@ -0,0 +1,56 @@ +#-- 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. +#++ + +module Members::Concerns::NotificationSender + extend ActiveSupport::Concern + + included do + def send_notification(member) + OpenProject::Notifications.send(event_type, + member: member, + message: notification_message, + send_notifications: send_notifications?) + end + + def set_attributes_params(params) + super.except(:notification_message, :send_notifications) + end + + def notification_message + params[:notification_message] + end + + def send_notifications? + params.fetch(:send_notifications, true) + end + + def event_type + raise NotImplementedError + end + end +end diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb index efe43ad9e1..02f73f5108 100644 --- a/app/services/members/create_service.rb +++ b/app/services/members/create_service.rb @@ -29,6 +29,8 @@ #++ class Members::CreateService < ::BaseServices::Create + include Members::Concerns::NotificationSender + around_call :post_process def post_process @@ -44,13 +46,6 @@ class Members::CreateService < ::BaseServices::Create protected - def send_notification(member) - OpenProject::Notifications.send(OpenProject::Events::MEMBER_CREATED, - member: member, - message: params[:notification_message], - send_notifications: params.fetch(:send_notifications, true)) - end - def add_group_memberships(member) return unless member.principal.is_a?(Group) @@ -59,7 +54,7 @@ class Members::CreateService < ::BaseServices::Create .call(ids: member.principal.user_ids, send_notifications: false) end - def set_attributes_params(params) - super.except(:notification_message, :send_notifications) + def event_type + OpenProject::Events::MEMBER_CREATED end end diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index a05e5606e8..3b203e2869 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -30,6 +30,7 @@ class Members::UpdateService < ::BaseServices::Update include Members::Concerns::CleanedUp + include Members::Concerns::NotificationSender around_call :post_process @@ -49,23 +50,13 @@ class Members::UpdateService < ::BaseServices::Update end end - def send_notification(member) - OpenProject::Notifications.send(OpenProject::Events::MEMBER_UPDATED, - member: member, - message: notification_message) - end - def update_group_roles(member) Groups::UpdateRolesService .new(member.principal, current_user: user, contract_class: EmptyContract) - .call(member: member, send_notifications: true, message: notification_message) - end - - def set_attributes_params(params) - super.except(:notification_message) + .call(member: member, send_notifications: send_notifications?, message: notification_message) end - def notification_message - params[:notification_message] + def event_type + OpenProject::Events::MEMBER_UPDATED end end diff --git a/app/workers/notifications/group_member_altered_job.rb b/app/workers/notifications/group_member_altered_job.rb index bda52a202b..5bbead47c0 100644 --- a/app/workers/notifications/group_member_altered_job.rb +++ b/app/workers/notifications/group_member_altered_job.rb @@ -31,11 +31,12 @@ class Notifications::GroupMemberAlteredJob < ApplicationJob queue_with_priority :notification - def perform(members_ids, message) + def perform(members_ids, message, send_notifications) each_member(members_ids) do |member| OpenProject::Notifications.send(event_type(member), member: member, - message: message) + message: message, + send_notifications: send_notifications) end end diff --git a/config/initializers/subscribe_listeners.rb b/config/initializers/subscribe_listeners.rb index 2b7555e038..9dc28a7c78 100644 --- a/config/initializers/subscribe_listeners.rb +++ b/config/initializers/subscribe_listeners.rb @@ -70,6 +70,8 @@ OpenProject::Notifications.subscribe(OpenProject::Events::MEMBER_CREATED) do |pa end OpenProject::Notifications.subscribe(OpenProject::Events::MEMBER_UPDATED) do |payload| + next unless payload[:send_notifications] + Mails::MemberUpdatedJob .perform_later(current_user: User.current, member: payload[:member], diff --git a/docs/api/apiv3/paths/membership.yml b/docs/api/apiv3/paths/membership.yml index a6f8a97f49..e0dd44e917 100644 --- a/docs/api/apiv3/paths/membership.yml +++ b/docs/api/apiv3/paths/membership.yml @@ -209,5 +209,7 @@ patch: By providing a `notificationMessage` within the `_meta` block of the payload, the client can include a customized message to the user of the updated membership. In case of a group, the message will be sent to every user belonging to the group. + + By including `{ "sendNotifications": false }` within the `_meta` block of the payload, no notifications is send out at all. operationId: Update_membership summary: Update membership diff --git a/docs/api/apiv3/paths/memberships.yml b/docs/api/apiv3/paths/memberships.yml index 4415b78c57..7fc7e390bb 100644 --- a/docs/api/apiv3/paths/memberships.yml +++ b/docs/api/apiv3/paths/memberships.yml @@ -174,9 +174,11 @@ post: description: |- Creates a new membership applying the attributes provided in the body. - You can use the form and schema to be retrieve the valid attribute values and by that be guided towards successful creation. + You can use the form and schema to retrieve the valid attribute values and by that be guided towards successful creation. By providing a `notificationMessage` within the `_meta` block of the payload, the client can include a customized message to the user of the newly created membership. In case of a group, the message will be sent to every user belonging to the group. + + By including `{ "sendNotifications": false }` within the `_meta` block of the payload, no notifications is send out at all. operationId: Create_membership summary: Create membership diff --git a/lib/api/v3/memberships/membership_meta_representer.rb b/lib/api/v3/memberships/membership_meta_representer.rb index 3648d3067b..bcc430f394 100644 --- a/lib/api/v3/memberships/membership_meta_representer.rb +++ b/lib/api/v3/memberships/membership_meta_representer.rb @@ -36,6 +36,8 @@ module API formattable_property :notification_message + property :send_notifications + def model_required? false end diff --git a/spec/lib/api/v3/memberships/membership_payload_representer_spec.rb b/spec/lib/api/v3/memberships/membership_payload_representer_spec.rb index ec54bab52a..bae11e8099 100644 --- a/spec/lib/api/v3/memberships/membership_payload_representer_spec.rb +++ b/spec/lib/api/v3/memberships/membership_payload_representer_spec.rb @@ -49,6 +49,19 @@ describe ::API::V3::Memberships::MembershipPayloadRepresenter do let(:value) { meta.notification_message } end end + + describe 'sendNotifications' do + let(:meta) { OpenStruct.new send_notifications: true } + let(:representer) do + described_class.create(membership, + meta: meta, + current_user: current_user) + end + + it_behaves_like 'property', :'_meta/sendNotifications' do + let(:value) { true } + end + end end end @@ -68,7 +81,8 @@ describe ::API::V3::Memberships::MembershipPayloadRepresenter do '_meta' => { 'notificationMessage' => { "raw" => 'Come to the dark side' - } + }, + 'sendNotifications' => true } } end @@ -77,6 +91,11 @@ describe ::API::V3::Memberships::MembershipPayloadRepresenter do expect(parsed.meta.notification_message) .to eql 'Come to the dark side' end + + it 'sets the notification sending configuration' do + expect(parsed.meta.send_notifications) + .to be_truthy + end end end end diff --git a/spec/requests/api/v3/groups/group_resource_spec.rb b/spec/requests/api/v3/groups/group_resource_spec.rb index 4ab6c7b817..a1d1de232b 100644 --- a/spec/requests/api/v3/groups/group_resource_spec.rb +++ b/spec/requests/api/v3/groups/group_resource_spec.rb @@ -186,7 +186,7 @@ describe 'API v3 Group resource', type: :request, content_type: :json do let(:body) do { _links: { - "members": [ + members: [ { href: api_v3_paths.user(members.last.id) }, @@ -258,15 +258,18 @@ describe 'API v3 Group resource', type: :request, content_type: :json do .to match_array [members.last, another_user] end - it 'sends a mail notifying of the added project memberships to the added user' do + it 'sends mails notifying of the added and updated project memberships to the added user' do expect(ActionMailer::Base.deliveries.size) - .to eql 1 + .to eq 2 expect(ActionMailer::Base.deliveries.map(&:to).flatten.uniq) .to match_array another_user.mail expect(ActionMailer::Base.deliveries.map(&:subject).flatten) - .to match_array [I18n.t(:'mail_member_updated_project.subject', project: project.name)] + .to match_array [ + I18n.t(:'mail_member_updated_project.subject', project: project.name), + I18n.t(:'mail_member_added_project.subject', project: other_project.name) + ] end end @@ -276,7 +279,7 @@ describe 'API v3 Group resource', type: :request, content_type: :json do let(:body) do { _links: { - "members": [ + members: [ { href: api_v3_paths.user(members.last.id) }, diff --git a/spec/requests/api/v3/membership_resources_spec.rb b/spec/requests/api/v3/membership_resources_spec.rb index d4dc7f77b0..84c6be15e4 100644 --- a/spec/requests/api/v3/membership_resources_spec.rb +++ b/spec/requests/api/v3/membership_resources_spec.rb @@ -29,6 +29,7 @@ require 'spec_helper' require 'rack/test' +# rubocop:disable RSpec/MultipleMemoizedHelpers describe 'API v3 memberships resource', type: :request, content_type: :json do include Rack::Test::Methods include API::V3::Utilities::PathHelper @@ -447,6 +448,36 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do context 'for a user' do it_behaves_like 'successful member creation' it_behaves_like 'sends mails' + + context 'when deactivating notification sending' do + let(:body) do + { + _links: { + project: { + href: api_v3_paths.project(project.id) + }, + principal: { + href: principal_path + }, + roles: [ + { + href: api_v3_paths.role(other_role.id) + } + ] + }, + _meta: { + sendNotifications: false + } + }.to_json + end + + it_behaves_like 'successful member creation' + + it 'sends no mail to the principal of the member' do + expect(ActionMailer::Base.deliveries) + .to be_empty + end + end end context 'for a group' do @@ -490,6 +521,36 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do .to be_present end end + + context 'when deactivating notification sending' do + let(:body) do + { + _links: { + project: { + href: api_v3_paths.project(project.id) + }, + principal: { + href: principal_path + }, + roles: [ + { + href: api_v3_paths.role(other_role.id) + } + ] + }, + _meta: { + sendNotifications: false + } + }.to_json + end + + it_behaves_like 'successful member creation' + + it 'sends no mail to the principal of the member' do + expect(ActionMailer::Base.deliveries) + .to be_empty + end + end end context 'for a placeholder user' do @@ -718,7 +779,7 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do let(:body) do { _links: { - "roles": [ + roles: [ { href: api_v3_paths.role(another_role.id) } @@ -783,6 +844,28 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do it_behaves_like 'sends mails' do let(:receivers) { [other_member.principal] } end + + context 'when deactivating notification sending' do + let(:body) do + { + _links: { + roles: [ + { + href: api_v3_paths.role(another_role.id) + } + ] + }, + _meta: { + sendNotifications: false + } + }.to_json + end + + it 'sends no mail to the principal of the member' do + expect(ActionMailer::Base.deliveries) + .to be_empty + end + end end context 'with a group' do @@ -862,13 +945,35 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do # Only sends to the second user since the first user's membership is unchanged let(:receivers) { [users.last] } end + + context 'when deactivating notification sending' do + let(:body) do + { + _links: { + roles: [ + { + href: api_v3_paths.role(another_role.id) + } + ] + }, + _meta: { + sendNotifications: false + } + }.to_json + end + + it 'sends no mail to the principal of the member' do + expect(ActionMailer::Base.deliveries) + .to be_empty + end + end end context 'if attempting to empty the roles' do let(:body) do { _links: { - "roles": [] + roles: [] } }.to_json end @@ -888,7 +993,7 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do let(:body) do { _links: { - "roles": [ + roles: [ { href: api_v3_paths.role(anonymous_role.id) } @@ -920,8 +1025,8 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do let(:body) do { _links: { - "project": { - "href": api_v3_paths.project(other_project.id) + project: { + href: api_v3_paths.project(other_project.id) } } @@ -939,8 +1044,8 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do let(:body) do { _links: { - "principal": { - "href": api_v3_paths.user(another_user.id) + principal: { + href: api_v3_paths.user(another_user.id) } } @@ -1049,3 +1154,4 @@ describe 'API v3 memberships resource', type: :request, content_type: :json do end end end +# rubocop:enable RSpec/MultipleMemoizedHelpers diff --git a/spec/services/groups/add_users_service_integration_spec.rb b/spec/services/groups/add_users_service_integration_spec.rb index 67c63ec113..cded240acc 100644 --- a/spec/services/groups/add_users_service_integration_spec.rb +++ b/spec/services/groups/add_users_service_integration_spec.rb @@ -77,7 +77,8 @@ describe Groups::AddUsersService, 'integration', type: :model do expect(Notifications::GroupMemberAlteredJob) .to have_received(:perform_later) .with(a_collection_containing_exactly(*ids), - message) + message, + true) end end diff --git a/spec/services/groups/cleanup_inherited_roles_service_integration_spec.rb b/spec/services/groups/cleanup_inherited_roles_service_integration_spec.rb index 4003d2c49f..3f649ba9e2 100644 --- a/spec/services/groups/cleanup_inherited_roles_service_integration_spec.rb +++ b/spec/services/groups/cleanup_inherited_roles_service_integration_spec.rb @@ -139,7 +139,8 @@ describe Groups::CleanupInheritedRolesService, 'integration', type: :model do expect(Notifications::GroupMemberAlteredJob) .to have_received(:perform_later) .with([first_user_member.id], - message) + message, + true) end end @@ -179,7 +180,8 @@ describe Groups::CleanupInheritedRolesService, 'integration', type: :model do expect(Notifications::GroupMemberAlteredJob) .to have_received(:perform_later) .with([first_user_member.id], - message) + message, + true) end end diff --git a/spec/services/groups/update_roles_service_integration_spec.rb b/spec/services/groups/update_roles_service_integration_spec.rb index 7675629b27..b966ed747a 100644 --- a/spec/services/groups/update_roles_service_integration_spec.rb +++ b/spec/services/groups/update_roles_service_integration_spec.rb @@ -83,7 +83,8 @@ describe Groups::UpdateRolesService, 'integration', type: :model do expect(Notifications::GroupMemberAlteredJob) .to have_received(:perform_later) .with(a_collection_containing_exactly(*Member.where(principal: user).pluck(:id)), - message) + message, + true) end end diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 44dcbdb40e..052026d72b 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -34,7 +34,11 @@ require 'services/base_services/behaves_like_update_service' describe Members::UpdateService, type: :model do it_behaves_like 'BaseServices update service' do let(:call_attributes) do - { role_ids: ["2"], notification_message: "Wish you where **here**." } + { + role_ids: ["2"], + notification_message: "Wish you where **here**.", + send_notifications: false + } end let!(:allow_notification_call) do @@ -48,7 +52,8 @@ describe Members::UpdateService, type: :model do .to receive(:send) .with(OpenProject::Events::MEMBER_UPDATED, member: model_instance, - message: call_attributes[:notification_message]) + message: call_attributes[:notification_message], + send_notifications: call_attributes[:send_notifications]) subject end diff --git a/spec/workers/notifications/group_member_altered_job_spec.rb b/spec/workers/notifications/group_member_altered_job_spec.rb index 951ce5fcff..0103a0ac45 100644 --- a/spec/workers/notifications/group_member_altered_job_spec.rb +++ b/spec/workers/notifications/group_member_altered_job_spec.rb @@ -32,8 +32,9 @@ require 'spec_helper' describe Notifications::GroupMemberAlteredJob, type: :model do subject(:service_call) do - described_class.new.perform(members_ids, message) + described_class.new.perform(members_ids, message, send_notification) end + let(:time) { Time.now } let(:member1) do FactoryBot.build_stubbed(:member, updated_at: time, created_at: time) @@ -44,6 +45,7 @@ describe Notifications::GroupMemberAlteredJob, type: :model do let(:members) { [member1, member2] } let(:members_ids) { members.map(&:id) } let(:message) { "Some message" } + let(:send_notification) { false } before do allow(OpenProject::Notifications) @@ -60,7 +62,7 @@ describe Notifications::GroupMemberAlteredJob, type: :model do expect(OpenProject::Notifications) .to have_received(:send) - .with(OpenProject::Events::MEMBER_CREATED, member: member1, message: message) + .with(OpenProject::Events::MEMBER_CREATED, member: member1, message: message, send_notifications: send_notification) end it 'sends an updated notification for the membership with the mismatching timestamps' do @@ -68,6 +70,6 @@ describe Notifications::GroupMemberAlteredJob, type: :model do expect(OpenProject::Notifications) .to have_received(:send) - .with(OpenProject::Events::MEMBER_UPDATED, member: member2, message: message) + .with(OpenProject::Events::MEMBER_UPDATED, member: member2, message: message, send_notifications: send_notification) end end