Address review feedback50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group

with some small updates

11 files changed, 71 insertions, 28 deletions

diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index faa4a8c19a6..1b6ad9c5a3f 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -172,18 +172,10 @@ class Namespace < ApplicationRecord
     end
   end
 
-  def emails_disabled=(value)
-    super
-
-    clear_memoization(:emails_disabled)
-  end
-
   # any ancestor can disable emails for all descendants
   def emails_disabled?
     strong_memoize(:emails_disabled) do
-      Namespace.where(id: self_and_ancestors.select(:id))
-        .where(emails_disabled: true)
-        .exists?
+      self_and_ancestors.where(emails_disabled: true).exists? && Feature.enabled?(:emails_disabled, self, default_enabled: true)
     end
   end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index e3ca513a8f9..3e6e5cfc8ad 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -632,8 +632,10 @@ class Project < ApplicationRecord
   alias_method :ancestors, :ancestors_upto
 
   def emails_disabled?
-    # disabling in the namespace overrides the project setting
-    super || namespace.emails_disabled?
+    strong_memoize(:emails_disabled) do
+      # disabling in the namespace overrides the project setting
+      (super || namespace.emails_disabled?) && Feature.enabled?(:emails_disabled, self, default_enabled: true)
+    end
   end
 
   def lfs_enabled?
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 42dc6eb753a..83710ffce2f 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -321,8 +321,8 @@ class NotificationService
   end
 
   def decline_project_invite(project_member)
-    # always send this one, since it's a response to the user's own
-    # action
+    # Must always send, regardless of project/namespace configuration since it's a
+    # response to the user's action.
 
     mailer.member_invite_declined_email(
       project_member.real_source_type,
@@ -354,8 +354,8 @@ class NotificationService
   end
 
   def decline_group_invite(group_member)
-    # always send this one, since it's a response to the user's own
-    # action
+    # Must always send, regardless of project/namespace configuration since it's a
+    # response to the user's action.
 
     mailer.member_invite_declined_email(
       group_member.real_source_type,
@@ -413,8 +413,8 @@ class NotificationService
   end
 
   def pipeline_finished(pipeline, recipients = nil)
-    # recipients could be just a list of emails from the PipelinesEmailService
-    # integration, so check for emails_disabled?
+    # Must always check project configuration since recipients could be a list of emails
+    # from the PipelinesEmailService integration.
     return if pipeline.project.emails_disabled?
 
     email_template = "pipeline_#{pipeline.status}_email"
diff --git a/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml b/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml
index afb5cb21288..9137e9339aa 100644
--- a/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml
+++ b/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml
@@ -1,5 +1,5 @@
 ---
-title: Allow email notifications to be disabled for all users of a group or project (backend only)
+title: Allow email notifications to be disabled for all members of a group or project
 merge_request: 30755
 author: Dustin Spicuzza
 type: added
diff --git a/db/migrate/20190715215532_add_project_emails_disabled.rb b/db/migrate/20190715215532_add_project_emails_disabled.rb
index 5a20f0a27a6..536ea34c0fb 100644
--- a/db/migrate/20190715215532_add_project_emails_disabled.rb
+++ b/db/migrate/20190715215532_add_project_emails_disabled.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 class AddProjectEmailsDisabled < ActiveRecord::Migration[5.2]
-  # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
   def change
diff --git a/db/migrate/20190715215549_add_group_emails_disabled.rb b/db/migrate/20190715215549_add_group_emails_disabled.rb
index 9a9814f2d1c..d3fd4d2d923 100644
--- a/db/migrate/20190715215549_add_group_emails_disabled.rb
+++ b/db/migrate/20190715215549_add_group_emails_disabled.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 class AddGroupEmailsDisabled < ActiveRecord::Migration[5.2]
-  # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
   def change
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index e59d2dda773..972f26ac745 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -886,5 +886,31 @@ describe Namespace do
         end
       end
     end
+
+    context 'when :emails_disabled feature flag is off' do
+      before do
+        stub_feature_flags(emails_disabled: false)
+      end
+
+      context 'when not a subgroup' do
+        it 'returns false' do
+          group = create(:group, emails_disabled: true)
+
+          expect(group.emails_disabled?).to be_falsey
+        end
+      end
+
+      context 'when a subgroup and ancestor emails are disabled' do
+        let(:grandparent) { create(:group) }
+        let(:parent)      { create(:group, parent: grandparent) }
+        let(:group)       { create(:group, parent: parent) }
+
+        it 'returns false' do
+          grandparent.update_attribute(:emails_disabled, true)
+
+          expect(group.emails_disabled?).to be_falsey
+        end
+      end
+    end
   end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index a72d5721c37..b4edbd97d20 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2319,7 +2319,7 @@ describe Project do
     let(:project) { create(:project, emails_disabled: false) }
 
     context 'emails disabled in group' do
-      it 'returns false' do
+      it 'returns true' do
         allow(project.namespace).to receive(:emails_disabled?) { true }
 
         expect(project.emails_disabled?).to be_truthy
@@ -2331,16 +2331,39 @@ describe Project do
         allow(project.namespace).to receive(:emails_disabled?) { false }
       end
 
-      it 'returns true' do
-        expect(project.emails_disabled).to be_falsey
+      it 'returns false' do
+        expect(project.emails_disabled?).to be_falsey
       end
 
-      it 'returns false' do
+      it 'returns true' do
         project.update_attribute(:emails_disabled, true)
 
         expect(project.emails_disabled?).to be_truthy
       end
     end
+
+    context 'when :emails_disabled feature flag is off' do
+      before do
+        stub_feature_flags(emails_disabled: false)
+      end
+
+      context 'emails disabled in group' do
+        it 'returns false' do
+          allow(project.namespace).to receive(:emails_disabled?) { true }
+
+          expect(project.emails_disabled?).to be_falsey
+        end
+      end
+
+      context 'emails enabled in group' do
+        it 'returns false' do
+          allow(project.namespace).to receive(:emails_disabled?) { false }
+          project.update_attribute(:emails_disabled, true)
+
+          expect(project.emails_disabled?).to be_falsey
+        end
+      end
+    end
   end
 
   describe '#lfs_enabled?' do
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 797dda2a618..12e9c2b2f3a 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -103,11 +103,11 @@ describe Groups::UpdateService do
     it 'updates the attribute' do
       internal_group.add_user(user, Gitlab::Access::OWNER)
 
-      expect { service.execute }.to change { internal_group.emails_disabled? }.to(true)
+      expect { service.execute }.to change { internal_group.emails_disabled }.to(true)
     end
 
     it 'does not update when not group owner' do
-      expect { service.execute }.not_to change { internal_group.emails_disabled? }
+      expect { service.execute }.not_to change { internal_group.emails_disabled }
     end
   end
 
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index de9b2b18517..31bd0f0f836 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -372,7 +372,7 @@ describe Projects::UpdateService do
     context 'when updating #emails_disabled' do
       it 'updates the attribute for the project owner' do
         expect { update_project(project, user, emails_disabled: true) }
-          .to change { project.emails_disabled? }
+          .to change { project.emails_disabled }
           .to(true)
       end
 
@@ -381,7 +381,7 @@ describe Projects::UpdateService do
         project.add_user(maintainer, :maintainer)
 
         expect { update_project(project, maintainer, emails_disabled: true) }
-          .not_to change { project.emails_disabled? }
+          .not_to change { project.emails_disabled }
       end
     end
 
diff --git a/spec/support/shared_examples/services/notification_service_shared_examples.rb b/spec/support/shared_examples/services/notification_service_shared_examples.rb
index c7bb4ad56fd..dd338ea47c7 100644
--- a/spec/support/shared_examples/services/notification_service_shared_examples.rb
+++ b/spec/support/shared_examples/services/notification_service_shared_examples.rb
@@ -8,6 +8,7 @@ shared_examples 'project emails are disabled' do
 
   before do
     reset_delivered_emails!
+    target_project.clear_memoization(:emails_disabled)
   end
 
   it 'sends no emails with project emails disabled' do
@@ -32,6 +33,7 @@ shared_examples 'group emails are disabled' do
 
   before do
     reset_delivered_emails!
+    target_group.clear_memoization(:emails_disabled)
   end
 
   it 'sends no emails with group emails disabled' do