FIX: N+1 for admins viewing groups page

FIX: N+1 for admins viewing groups page

Groups page was loading fields that are only used on the group show page, so move those fields to the GroupShowSerializer. Also only fetch the default category and tag notifications once.

diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb
index 3fee36f..e51e600 100644
--- a/app/serializers/basic_group_serializer.rb
+++ b/app/serializers/basic_group_serializer.rb
@@ -9,7 +9,6 @@ class BasicGroupSerializer < ApplicationSerializer
              :mentionable_level,
              :messageable_level,
              :visibility_level,
-             :automatic_membership_email_domains,
              :primary_group,
              :title,
              :grant_trust_level,
@@ -34,50 +33,6 @@ class BasicGroupSerializer < ApplicationSerializer
              :can_admin_group,
              :publish_read_state
 
-  def self.admin_attributes(*attrs)
-    attributes(*attrs)
-    attrs.each do |attr|
-      define_method "include_#{attr}?" do
-        scope.is_admin?
-      end
-    end
-  end
-
-  admin_attributes :automatic_membership_email_domains,
-                   :smtp_server,
-                   :smtp_port,
-                   :smtp_ssl,
-                   :imap_server,
-                   :imap_port,
-                   :imap_ssl,
-                   :imap_mailbox_name,
-                   :imap_mailboxes,
-                   :email_username,
-                   :email_password,
-                   :imap_last_error,
-                   :imap_old_emails,
-                   :imap_new_emails
-
-  def self.admin_or_owner_attributes(*attrs)
-    attributes(*attrs)
-    attrs.each do |attr|
-      define_method "include_#{attr}?" do
-        scope.is_admin? || (include_is_group_owner? && is_group_owner)
-      end
-    end
-  end
-
-  admin_or_owner_attributes :watching_category_ids,
-                            :tracking_category_ids,
-                            :watching_first_post_category_ids,
-                            :regular_category_ids,
-                            :muted_category_ids,
-                            :watching_tags,
-                            :watching_first_post_tags,
-                            :tracking_tags,
-                            :regular_tags,
-                            :muted_tags
-
   def include_display_name?
     object.automatic
   end
@@ -132,16 +87,6 @@ class BasicGroupSerializer < ApplicationSerializer
     scope.can_see_group_members?(object)
   end
 
-  [:watching, :regular, :tracking, :watching_first_post, :muted].each do |level|
-    define_method("#{level}_category_ids") do
-      GroupCategoryNotificationDefault.lookup(object, level).pluck(:category_id)
-    end
-
-    define_method("#{level}_tags") do
-      GroupTagNotificationDefault.lookup(object, level).joins(:tag).pluck('tags.name')
-    end
-  end
-
   private
 
   def staff?
diff --git a/app/serializers/group_show_serializer.rb b/app/serializers/group_show_serializer.rb
index 29a81c6..ee54b33 100644
--- a/app/serializers/group_show_serializer.rb
+++ b/app/serializers/group_show_serializer.rb
@@ -3,6 +3,50 @@
 class GroupShowSerializer < BasicGroupSerializer
   attributes :is_group_user, :is_group_owner, :is_group_owner_display, :mentionable, :messageable, :flair_icon, :flair_type
 
+  def self.admin_attributes(*attrs)
+    attributes(*attrs)
+    attrs.each do |attr|
+      define_method "include_#{attr}?" do
+        scope.is_admin?
+      end
+    end
+  end
+
+  admin_attributes :automatic_membership_email_domains,
+                   :smtp_server,
+                   :smtp_port,
+                   :smtp_ssl,
+                   :imap_server,
+                   :imap_port,
+                   :imap_ssl,
+                   :imap_mailbox_name,
+                   :imap_mailboxes,
+                   :email_username,
+                   :email_password,
+                   :imap_last_error,
+                   :imap_old_emails,
+                   :imap_new_emails
+
+  def self.admin_or_owner_attributes(*attrs)
+    attributes(*attrs)
+    attrs.each do |attr|
+      define_method "include_#{attr}?" do
+        scope.is_admin? || (include_is_group_owner? && is_group_owner)
+      end
+    end
+  end
+
+  admin_or_owner_attributes :watching_category_ids,
+                            :tracking_category_ids,
+                            :watching_first_post_category_ids,
+                            :regular_category_ids,
+                            :muted_category_ids,
+                            :watching_tags,
+                            :watching_first_post_tags,
+                            :tracking_tags,
+                            :regular_tags,
+                            :muted_tags
+
   def include_is_group_user?
     authenticated?
   end
@@ -51,6 +95,16 @@ class GroupShowSerializer < BasicGroupSerializer
     flair_type.present? && (is_group_owner || scope.is_admin?)
   end
 
+  [:watching, :regular, :tracking, :watching_first_post, :muted].each do |level|
+    define_method("#{level}_category_ids") do
+      group_category_notifications[NotificationLevels.all[level]] || []
+    end
+
+    define_method("#{level}_tags") do
+      group_tag_notifications[NotificationLevels.all[level]] || []
+    end
+  end
+
   private
 
   def authenticated?
@@ -61,4 +115,27 @@ class GroupShowSerializer < BasicGroupSerializer
     return @group_user if defined?(@group_user)
     @group_user = object.group_users.find_by(user: scope.user)
   end
+
+  def group_category_notifications
+    @group_category_notification_defaults ||=
+      GroupCategoryNotificationDefault.where(group_id: object.id)
+        .pluck(:notification_level, :category_id)
+        .inject({}) do |h, arr|
+          h[arr[0]] ||= []
+          h[arr[0]] << arr[1]
+          h
+        end
+  end
+
+  def group_tag_notifications
+    @group_tag_notification_defaults ||=
+      GroupTagNotificationDefault.where(group_id: object.id)
+        .joins(:tag)
+        .pluck(:notification_level, :name)
+        .inject({}) do |h, arr|
+          h[arr[0]] ||= []
+          h[arr[0]] << arr[1]
+          h
+        end
+  end
 end
diff --git a/spec/serializers/basic_group_serializer_spec.rb b/spec/serializers/basic_group_serializer_spec.rb
index 4000751..799d106 100644
--- a/spec/serializers/basic_group_serializer_spec.rb
+++ b/spec/serializers/basic_group_serializer_spec.rb
@@ -40,22 +40,6 @@ describe BasicGroupSerializer do
     end
   end
 
-  describe '#automatic_membership_email_domains' do
-    fab!(:group) { Fabricate(:group, automatic_membership_email_domains: 'ilovediscourse.com') }
-    let(:admin_guardian) { Guardian.new(Fabricate(:admin)) }
-
-    it 'should include email domains for admin' do
-      subject = described_class.new(group, scope: admin_guardian, root: false, owner_group_ids: [group.id])
-      expect(subject.as_json[:automatic_membership_email_domains]).to eq('ilovediscourse.com')
-    end
-
-    it 'should not include email domains for other users' do
-      subject = described_class.new(group, scope: guardian, root: false, owner_group_ids: [group.id])
-      expect(subject.as_json[:automatic_membership_email_domains]).to eq(nil)
-    end
-
-  end
-
   describe '#has_messages' do
     fab!(:group) { Fabricate(:group, has_messages: true) }
 
@@ -113,26 +97,4 @@ describe BasicGroupSerializer do
       end
     end
   end
-
-  describe 'admin only fields' do
-    fab!(:group) { Fabricate(:group, email_username: 'foo@bar.com', email_password: 'pa$$w0rd') }
-
-    describe 'for a user' do
-      let(:guardian) { Guardian.new(Fabricate(:user)) }
-
-      it 'are not visible' do
-        expect(subject.as_json[:email_username]).to be_nil
-        expect(subject.as_json[:email_password]).to be_nil
-      end
-    end
-
-    describe 'for an admin' do
-      let(:guardian) { Guardian.new(Fabricate(:admin)) }
-
-      it 'are visible' do
-        expect(subject.as_json[:email_username]).to eq('foo@bar.com')
-        expect(subject.as_json[:email_password]).to eq('pa$$w0rd')
-      end
-    end
-  end
 end

[... diff too long, it was truncated ...]

GitHub sha: 8333872e

1 Like