Email: Reducing the number of emails sent to users (#773)

* removing an unused data controller attribute

* reducing sent manager emails

app/models/member.rb

@@ -192,9 +192,9 @@ def user_emails(namespace, locale)
       users.pluck(:email)
     end
 
-    def manager_emails(namespace, locale, member = nil) # rubocop:disable Metrics/AbcSize
+    def manager_emails(namespace, locale, access_level = Member::AccessLevel.manageable, member = nil) # rubocop:disable Metrics/AbcSize
       manager_memberships = Member.for_namespace_and_ancestors(namespace).not_expired
-                                  .where(access_level: Member::AccessLevel.manageable)
+                                  .where(access_level:)
       managers = if member
                    User.human_users.where(id: manager_memberships.select(:user_id), locale:)
                        .and(User.where.not(id: member.user.id)).distinct

app/models/namespace_group_link.rb

@@ -47,7 +47,7 @@ def send_access_revoked_emails
         GroupLinkMailer.access_revoked_user_email(user_emails, group, namespace, locale).deliver_later
       end
 
-      manager_emails = Member.manager_emails(namespace, locale)
+      manager_emails = Member.manager_emails(namespace, locale, Member::AccessLevel::OWNER)
       next if manager_emails.empty?
 
       GroupLinkMailer.access_revoked_manager_email(manager_emails, group, namespace, locale).deliver_later
@@ -61,7 +61,7 @@ def send_access_granted_emails
         GroupLinkMailer.access_granted_user_email(user_emails, group, namespace, locale).deliver_later
       end
 
-      manager_emails = Member.manager_emails(namespace, locale)
+      manager_emails = Member.manager_emails(namespace, locale, Member::AccessLevel::OWNER)
       next if manager_emails.empty?
 
       GroupLinkMailer.access_granted_manager_email(manager_emails, group, namespace, locale).deliver_later

app/services/members/create_service.rb

@@ -41,13 +41,6 @@ def execute # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Met
 
     def send_emails
       MemberMailer.access_granted_user_email(member, namespace).deliver_later
-
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(namespace, locale, member)
-        next if manager_emails.empty?
-
-        MemberMailer.access_granted_manager_email(member, manager_emails, namespace, locale).deliver_later
-      end
     end
   end
 end

app/services/members/destroy_service.rb

@@ -42,13 +42,6 @@ def send_emails
       return if Member.can_view?(member.user, namespace)
 
       MemberMailer.access_revoked_user_email(member, namespace).deliver_later
-
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(namespace, locale, member)
-        next if manager_emails.empty?
-
-        MemberMailer.access_revoked_manager_email(member, manager_emails, namespace, locale).deliver_later
-      end
     end
 
     def create_activities

app/views/groups/_edit_advanced_transfer.html.erb

@@ -7,38 +7,26 @@
     <% end %>
     <ul
       class="
-        mb-4
-        ml-4
-        space-y-1
-        text-slate-500
-        list-disc
-        list-inside
-        dark:text-slate-400
+        mb-4 ml-4 space-y-1 text-slate-500 list-disc list-inside dark:text-slate-400
       "
     >
       <% t("groups.edit.advanced.transfer.points").each do |item| %>
         <li><%= item %></li>
       <% end %>
     </ul>
 
-    <div data-controller="transfer">
+    <div>
       <div class="hidden confirm-transfer">
         <div
           class="
-            flex
-            p-4
-            mb-4
-            text-red-800
-            rounded-lg
-            bg-red-50
-            dark:bg-slate-800
+            flex p-4 mb-4 text-red-800 rounded-lg bg-red-50 dark:bg-slate-800
             dark:text-red-400
           "
         >
           <%= viral_icon(name: "exclamation_circle", classes: "h-6 w-6 flex-none") %>
           <span class="flex-1 ml-4"><%= t(
               "groups.edit.advanced.transfer.confirm.warning_html",
-              group_name: group.human_name
+              group_name: group.human_name,
             ) %></span>
         </div>
         <div class="mb-4">

app/views/projects/_transfer.html.erb

@@ -6,38 +6,26 @@
     <% end %>
     <ul
       class="
-        mb-4
-        ml-4
-        space-y-1
-        text-slate-500
-        list-disc
-        list-inside
-        dark:text-slate-400
+        mb-4 ml-4 space-y-1 text-slate-500 list-disc list-inside dark:text-slate-400
       "
     >
 
       <% t("projects.edit.advanced.transfer.points").each do |item| %>
         <li><%= item %></li>
       <% end %>
     </ul>
-    <div data-controller="transfer">
+    <div>
       <div class="hidden confirm-transfer">
         <div
           class="
-            flex
-            p-4
-            mb-4
-            text-red-800
-            rounded-lg
-            bg-red-50
-            dark:bg-slate-800
+            flex p-4 mb-4 text-red-800 rounded-lg bg-red-50 dark:bg-slate-800
             dark:text-red-400
           "
         >
           <%= viral_icon(name: "exclamation_circle", classes: "h-6 w-6 flex-none") %>
           <span class="flex-1 ml-4"><%= t(
               "projects.edit.advanced.transfer.confirm.warning_html",
-              project_name: @project.human_name
+              project_name: @project.human_name,
             ) %></span>
         </div>
         <div class="mb-4">

test/models/namespace_group_link_test.rb

@@ -119,7 +119,7 @@ def setup
 
       I18n.available_locales.each do |locale|
         user_emails = Member.user_emails(group_group_link.group, locale)
-        manager_emails = Member.manager_emails(group_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(group_group_link.namespace, locale, Member::AccessLevel::OWNER)
         unless user_emails.empty?
           assert_enqueued_email_with GroupLinkMailer, :access_revoked_user_email,
                                      args: [user_emails, group_group_link.group,
@@ -142,7 +142,7 @@ def setup
 
       I18n.available_locales.each do |locale|
         user_emails = Member.user_emails(group_group_link.group, locale)
-        manager_emails = Member.manager_emails(group_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(group_group_link.namespace, locale, Member::AccessLevel::OWNER)
         unless user_emails.empty?
           assert_enqueued_email_with GroupLinkMailer, :access_granted_user_email,
                                      args: [user_emails, group_group_link.group,

test/services/group_links/group_link_service_test.rb

@@ -25,7 +25,7 @@ def setup
                                      args: [user_emails, group, namespace, locale]
         end
 
-        manager_emails = Member.manager_emails(namespace, locale)
+        manager_emails = Member.manager_emails(namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_granted_manager_email,
@@ -111,7 +111,7 @@ def setup
                                      args: [user_emails, group, namespace, locale]
         end
 
-        manager_emails = Member.manager_emails(namespace, locale)
+        manager_emails = Member.manager_emails(namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_granted_manager_email,
@@ -152,7 +152,7 @@ def setup
                                      args: [user_emails, group, namespace, locale]
         end
 
-        manager_emails = Member.manager_emails(namespace, locale)
+        manager_emails = Member.manager_emails(namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_granted_manager_email,
@@ -210,7 +210,7 @@ def setup
                                      args: [user_emails, group, namespace, locale]
         end
 
-        manager_emails = Member.manager_emails(namespace, locale)
+        manager_emails = Member.manager_emails(namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_granted_manager_email,

test/services/group_links/group_unlink_service_test.rb

@@ -27,7 +27,7 @@ def setup
                                             locale]
         end
 
-        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_revoked_manager_email,
@@ -68,7 +68,7 @@ def setup
                                             locale]
         end
 
-        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_granted_manager_email,
@@ -128,7 +128,7 @@ def setup
                                             locale]
         end
 
-        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_revoked_manager_email,
@@ -157,7 +157,7 @@ def setup
                                             locale]
         end
 
-        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_revoked_manager_email,
@@ -198,7 +198,7 @@ def setup
                                             locale]
         end
 
-        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_granted_manager_email,
@@ -252,7 +252,7 @@ def setup
                                             locale]
         end
 
-        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale)
+        manager_emails = Member.manager_emails(namespace_group_link.namespace, locale, Member::AccessLevel::OWNER)
         next if manager_emails.empty?
 
         assert_enqueued_email_with GroupLinkMailer, :access_revoked_manager_email,

test/services/members/create_service_test.rb

@@ -21,15 +21,8 @@ def setup
         @new_member = Members::CreateService.new(@user, @group, valid_params, true).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_granted_user_email, args: [@new_member, @group]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(@group, locale, @new_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_granted_manager_email,
-                                   args: [@new_member, manager_emails, @group, locale]
-      end
     end
 
     test 'create group member with no email notification' do
@@ -52,15 +45,8 @@ def setup
         @new_member = Members::CreateService.new(@user, @project_namespace, valid_params, true).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_granted_user_email, args: [@new_member, @project_namespace]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(@project_namespace, locale, @new_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_granted_manager_email,
-                                   args: [@new_member, manager_emails, @project_namespace, locale]
-      end
     end
 
     test 'create group member with invalid params' do
@@ -185,15 +171,8 @@ def setup
         @new_member = Members::CreateService.new(@user, group, valid_params, true).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_granted_user_email, args: [@new_member, group]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(group, locale, @new_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_granted_manager_email,
-                                   args: [@new_member, manager_emails, group, locale]
-      end
     end
 
     test 'valid authorization to create project member' do
@@ -207,15 +186,8 @@ def setup
         @new_member = Members::CreateService.new(@user, @project_namespace, valid_params, true).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_granted_user_email, args: [@new_member, @project_namespace]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(@project_namespace, locale, @new_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_granted_manager_email,
-                                   args: [@new_member, manager_emails, @project_namespace, locale]
-      end
     end
 
     test 'create group member logged using logidze' do

test/services/members/destroy_service_test.rb

@@ -18,15 +18,8 @@ def setup
         Members::DestroyService.new(@group_member, @group, @user).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_revoked_user_email, args: [@group_member, @group]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(@group, locale, @group_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_revoked_manager_email,
-                                   args: [@group_member, manager_emails, @group, locale]
-      end
     end
 
     test 'remove themselves as a group member' do
@@ -35,15 +28,8 @@ def setup
         Members::DestroyService.new(@group_member, @group, user).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_revoked_user_email, args: [@group_member, @group]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(@group, locale, @group_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_revoked_manager_email,
-                                   args: [@group_member, manager_emails, @group, locale]
-      end
     end
 
     test 'remove group member when user does not have direct or inherited membership' do
@@ -77,15 +63,8 @@ def setup
         Members::DestroyService.new(@project_member, @project_namespace, @user).execute
       end
 
-      assert_enqueued_emails 3
+      assert_enqueued_emails 1
       assert_enqueued_email_with MemberMailer, :access_revoked_user_email, args: [@project_member, @project_namespace]
-      I18n.available_locales.each do |locale|
-        manager_emails = Member.manager_emails(@project_namespace, locale, @project_member)
-        next if manager_emails.empty?
-
-        assert_enqueued_email_with MemberMailer, :access_revoked_manager_email,
-                                   args: [@project_member, manager_emails, @project_namespace, locale]
-      end
     end
 
     test 'remove project member with incorrect permissions' do