Merge pull request discourse#4569 from tgxworld/fix_n+1_queries

PERF: Fix N+1 queries when loading groups.

app/assets/javascripts/discourse/components/group-notifications-button.js.es6

@@ -2,7 +2,7 @@ import NotificationsButton from 'discourse/components/notifications-button';
 
 export default NotificationsButton.extend({
   classNames: ['notification-options', 'group-notification-menu'],
-  notificationLevel: Em.computed.alias('group.notification_level'),
+  notificationLevel: Em.computed.alias('group.group_user.notification_level'),
   i18nPrefix: 'groups.notifications',
 
   clicked(id) {

app/assets/javascripts/discourse/controllers/group.js.es6

@@ -41,8 +41,16 @@ export default Ember.Controller.extend({
     });
   },
 
-  @computed('model.is_member')
-  getTabs(isMember) {
-    return this.get('tabs').filter(t => isMember || !t.get('requiresMembership'));
+  @computed('model.is_group_user')
+  getTabs(isGroupUser) {
+    return this.get('tabs').filter(t => {
+      let isMember = false;
+
+      if (this.currentUser) {
+        isMember = this.currentUser.admin || isGroupUser;
+      }
+
+      return isMember || !t.get('requiresMembership');
+    });
   }
 });

app/assets/javascripts/discourse/models/group.js.es6

@@ -151,7 +151,7 @@ const Group = Discourse.Model.extend({
   },
 
   setNotification(notification_level) {
-    this.set("notification_level", notification_level);
+    this.set("group_user.notification_level", notification_level);
     return ajax(`/groups/${this.get("name")}/notifications`, {
       data: { notification_level },
       type: "POST"
@@ -181,7 +181,11 @@ Group.reopenClass({
         offset: offset || 0
       }
     });
-  }
+  },
+
+  mentionable(name) {
+    return ajax(`/groups/${name}/mentionable`, { data: { name } });
+  },
 });
 
 export default Group;

app/assets/javascripts/discourse/models/user.js.es6

@@ -341,7 +341,15 @@ const User = RestModel.extend({
       }
 
       if (!Em.isEmpty(json.user.groups)) {
-        json.user.groups = json.user.groups.map(g => Group.create(g));
+        const groups = [];
+
+        for(let i = 0; i < json.user.groups.length; i++) {
+          const group = Group.create(json.user.groups[i]);
+          group.group_user = json.user.group_users[i];
+          groups.push(group);
+        }
+
+        json.user.groups = groups;
       }
 
       if (json.user.invited_by) {

app/assets/javascripts/discourse/routes/new-message.js.es6

@@ -21,11 +21,11 @@ export default Discourse.Route.extend({
           });
         } else if (params.groupname) {
           // send a message to a group
-          Group.find(params.groupname).then(group => {
-            if (group.mentionable) {
-              Ember.run.next(() => e.send("createNewMessageViaParams", group.name, params.title, params.body));
+          Group.mentionable(params.groupname).then(result => {
+            if (result.mentionable) {
+              Ember.run.next(() => e.send("createNewMessageViaParams", params.groupname, params.title, params.body));
             } else {
-              bootbox.alert(I18n.t("composer.cant_send_pm", { username: group.name }));
+              bootbox.alert(I18n.t("composer.cant_send_pm", { username: params.groupname }));
             }
           }).catch(function() {
             bootbox.alert(I18n.t("generic_error"));

app/controllers/admin/groups_controller.rb

@@ -1,7 +1,7 @@
 class Admin::GroupsController < Admin::AdminController
 
   def index
-    groups = Group.order(:name).where("id <> ?", Group::AUTO_GROUPS[:everyone])
+    groups = Group.order(:name).where("groups.id <> ?", Group::AUTO_GROUPS[:everyone])
 
     if search = params[:search].to_s
       groups = groups.where("name ILIKE ?", "%#{search}%")

app/controllers/groups_controller.rb

@@ -1,10 +1,10 @@
 class GroupsController < ApplicationController
 
-  before_filter :ensure_logged_in, only: [:set_notifications]
+  before_filter :ensure_logged_in, only: [:set_notifications, :mentionable]
   skip_before_filter :preload_json, :check_xhr, only: [:posts_feed, :mentions_feed]
 
   def show
-    render_serialized(find_group(:id), BasicGroupSerializer)
+    render_serialized(find_group(:id), GroupShowSerializer, root: 'basic_group')
   end
 
   def counts
@@ -120,6 +120,16 @@ def add_members
     end
   end
 
+  def mentionable
+    group = find_group(:name)
+
+    if group
+      render json: { mentionable: Group.mentionable(current_user).where(id: group.id).present? }
+    else
+      raise Discourse::InvalidAccess.new
+    end
+  end
+
   def remove_member
     group = Group.find(params[:id])
     guardian.ensure_can_edit!(group)

app/controllers/session_controller.rb

@@ -169,7 +169,6 @@ def create
     login = params[:login].strip
     login = login[1..-1] if login[0] == "@"
 
-
     if user = User.find_by_username_or_email(login)
 
       # If their password is correct

app/models/group.rb

@@ -381,10 +381,6 @@ def bulk_add(user_ids)
     true
   end
 
-  def mentionable?(user, group_id)
-    Group.mentionable(user).where(id: group_id).exists?
-  end
-
   def staff?
     STAFF_GROUPS.include?(self.name.to_sym)
   end

app/serializers/basic_group_serializer.rb

@@ -11,41 +11,12 @@ class BasicGroupSerializer < ApplicationSerializer
              :title,
              :grant_trust_level,
              :incoming_email,
-             :notification_level,
              :has_messages,
-             :is_member,
-             :mentionable,
              :flair_url,
              :flair_bg_color,
              :flair_color
 
   def include_incoming_email?
     scope.is_staff?
   end
-
-  def notification_level
-    fetch_group_user&.notification_level
-  end
-
-  def include_notification_level?
-    scope.authenticated?
-  end
-
-  def mentionable
-    object.mentionable?(scope.user, object.id)
-  end
-
-  def is_member
-    scope.is_admin? || fetch_group_user.present?
-  end
-
-  def include_is_member?
-    scope.authenticated?
-  end
-
-  private
-
-  def fetch_group_user
-    @group_user ||= object.group_users.find_by(user_id: scope.user.id)
-  end
 end

app/serializers/basic_group_user_serializer.rb

@@ -0,0 +1,3 @@
+class BasicGroupUserSerializer < ApplicationSerializer
+  attributes :group_id, :user_id, :notification_level
+end

app/serializers/group_show_serializer.rb

@@ -0,0 +1,11 @@
+class GroupShowSerializer < BasicGroupSerializer
+  attributes :is_group_user
+
+  def include_is_group_user?
+    scope.authenticated?
+  end
+
+  def is_group_user
+    object.users.include?(scope.user)
+  end
+end

app/serializers/user_serializer.rb

@@ -73,6 +73,7 @@ def self.untrusted_attributes(*attrs)
 
   has_one :invited_by, embed: :object, serializer: BasicUserSerializer
   has_many :groups, embed: :object, serializer: BasicGroupSerializer
+  has_many :group_users, embed: :object, serializer: BasicGroupUserSerializer
   has_many :featured_user_badges, embed: :ids, serializer: UserBadgeSerializer, root: :user_badges
   has_one  :card_badge, embed: :object, serializer: BadgeSerializer
   has_one :user_option, embed: :object, serializer: UserOptionSerializer
@@ -127,13 +128,19 @@ def mailing_list_posts_per_day
   end
 
   def groups
+    groups = object.groups.order(:id)
+
     if scope.is_admin? || object.id == scope.user.try(:id)
-      object.groups
+      groups
     else
-      object.groups.where(visible: true)
+      groups.where(visible: true)
     end
   end
 
+  def group_users
+    object.group_users.order(:group_id)
+  end
+
   def include_email?
     object.id && object.id == scope.user.try(:id)
   end

config/routes.rb

@@ -398,7 +398,7 @@
   get "posts/:username/flagged" => "posts#flagged_posts", constraints: {username: USERNAME_ROUTE_FORMAT}
 
   get "groups/:id.json" => 'groups#show', constraints: {id: USERNAME_ROUTE_FORMAT}, defaults: {format: 'json'}
-  
+
   resources :groups, id: USERNAME_ROUTE_FORMAT do
     get "posts.rss" => "groups#posts_feed", format: :rss
     get "mentions.rss" => "groups#mentions_feed", format: :rss
@@ -409,6 +409,7 @@
     get 'mentions'
     get 'messages'
     get 'counts'
+    get 'mentionable'
 
     member do
       put "members" => "groups#add_members"

spec/controllers/admin/groups_controller_spec.rb

@@ -34,10 +34,7 @@
         "primary_group"=>false,
         "grant_trust_level"=>nil,
         "incoming_email"=>nil,
-        "notification_level"=>2,
         "has_messages"=>false,
-        "is_member"=>true,
-        "mentionable"=>false,
         "flair_url"=>nil,
         "flair_bg_color"=>nil,
         "flair_color"=>nil

spec/fabricators/email_token_fabricator.rb

@@ -0,0 +1,4 @@
+Fabricator(:email_token) do
+  user
+  email { |attrs| attrs[:user].email }
+end

spec/integration/groups_spec.rb

@@ -0,0 +1,36 @@
+require 'rails_helper'
+
+describe "Groups" do
+  describe "checking if a group can be mentioned" do
+    let(:password) { 'somecomplicatedpassword' }
+    let(:email_token) { Fabricate(:email_token, confirmed: true) }
+    let(:user) { email_token.user }
+    let(:group) { Fabricate(:group, name: 'test', users: [user]) }
+
+    before do
+      user.update_attributes!(password: password)
+    end
+
+    it "should return the right response" do
+      group
+
+      post "/session.json", { login: user.username, password: password }
+      expect(response).to be_success
+
+      get "/groups/test/mentionable.json", { name: group.name }
+
+      expect(response).to be_success
+
+      response_body = JSON.parse(response.body)
+      expect(response_body["mentionable"]).to eq(false)
+
+      group.update_attributes!(alias_level: Group::ALIAS_LEVELS[:everyone])
+
+      get "/groups/test/mentionable.json", { name: group.name }
+      expect(response).to be_success
+
+      response_body = JSON.parse(response.body)
+      expect(response_body["mentionable"]).to eq(true)
+    end
+  end
+end

test/javascripts/acceptance/groups-test.js.es6

@@ -1,4 +1,5 @@
-import { acceptance } from "helpers/qunit-helpers";
+import { acceptance, logIn } from "helpers/qunit-helpers";
+
 acceptance("Groups");
 
 test("Browsing Groups", () => {
@@ -24,6 +25,18 @@ test("Browsing Groups", () => {
 
   visit("/groups/discourse/messages");
   andThen(() => {
+    ok($('.action-list li').length === 4, 'it should not show messages tab');
     ok(count('.user-stream .item') > 0, "it lists stream items");
   });
 });
+
+test("Messages tab", () => {
+  logIn();
+  Discourse.reset();
+
+  visit("/groups/discourse");
+
+  andThen(() => {
+    ok($('.action-list li').length === 5, 'it should show messages tab if user is admin');
+  });
+});