allow suppressing notifications on member create/update

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

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

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

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

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

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
 

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],

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

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

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

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

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)
               },

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

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
 

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
 

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
 

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

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