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.

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?

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

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

spec/serializers/group_show_serializer_spec.rb

@@ -44,4 +44,100 @@ describe GroupShowSerializer do
       expect(json[:group_show][:mentionable]).to eq(true)
     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.new, root: false, owner_group_ids: [group.id])
+      expect(subject.as_json[:automatic_membership_email_domains]).to eq(nil)
+    end
+  end
+
+  describe 'admin only fields' do
+    fab!(:group) { Fabricate(:group, email_username: 'foo@bar.com', email_password: 'pa$$w0rd') }
+    subject { described_class.new(group, scope: guardian, root: false) }
+
+    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
+
+  describe "default notification settings" do
+    subject { described_class.new(group, scope: guardian, root: false) }
+    let(:category1) { Fabricate(:category) }
+    let(:category2) { Fabricate(:category) }
+    let(:tag1) { Fabricate(:tag) }
+    let(:tag2) { Fabricate(:tag) }
+
+    before do
+      GroupCategoryNotificationDefault.create!(
+        group: group,
+        category: category1,
+        notification_level: GroupCategoryNotificationDefault.notification_levels[:watching]
+      )
+      GroupCategoryNotificationDefault.create!(
+        group: group,
+        category: category2,
+        notification_level: GroupCategoryNotificationDefault.notification_levels[:tracking]
+      )
+      GroupTagNotificationDefault.create!(
+        group: group,
+        tag: tag1,
+        notification_level: GroupTagNotificationDefault.notification_levels[:watching]
+      )
+      GroupTagNotificationDefault.create!(
+        group: group,
+        tag: tag2,
+        notification_level: GroupTagNotificationDefault.notification_levels[:tracking]
+      )
+    end
+
+    describe "for a user" do
+      let(:guardian) { Guardian.new(Fabricate(:user)) }
+
+      it "are not visible" do
+        expect(subject.as_json.keys.select { |k| k.to_s.ends_with?("_category_ids") }).to be_empty
+        expect(subject.as_json.keys.select { |k| k.to_s.ends_with?("_tags") }).to be_empty
+      end
+    end
+
+    describe "for admin" do
+      let(:guardian) { Guardian.new(Fabricate(:admin)) }
+
+      it "are correct" do
+        expect(subject.as_json[:watching_category_ids]).to eq([category1.id])
+        expect(subject.as_json[:tracking_category_ids]).to eq([category2.id])
+        expect(subject.as_json[:watching_first_post_category_ids]).to eq([])
+        expect(subject.as_json[:regular_category_ids]).to eq([])
+        expect(subject.as_json[:muted_category_ids]).to eq([])
+
+        expect(subject.as_json[:watching_tags]).to eq([tag1.name])
+        expect(subject.as_json[:tracking_tags]).to eq([tag2.name])
+        expect(subject.as_json[:watching_first_post_tags]).to eq([])
+        expect(subject.as_json[:regular_tags]).to eq([])
+        expect(subject.as_json[:muted_tags]).to eq([])
+      end
+    end
+  end
 end