FIX: Better handling of Group model state (#8356)

The group card and group members page were affecting each other and were
leaking members list and the query parameters which led to bad UX
experience and sub-optimal performance (client made more queries because
it was loading fewer members).

This commit refactors the group model to make it more consistent, remove
dead code, move error handling outside of model.

app/assets/javascripts/discourse/components/group-card-contents.js.es6

@@ -5,6 +5,7 @@ import { default as discourseComputed } from "discourse-common/utils/decorators"
 import CardContentsBase from "discourse/mixins/card-contents-base";
 import CleansUp from "discourse/mixins/cleans-up";
 import { groupPath } from "discourse/lib/url";
+import { Promise } from "rsvp";
 
 const maxMembersToDisplay = 10;
 
@@ -55,8 +56,9 @@ export default Component.extend(CardContentsBase, CleansUp, {
         if (!group.flair_url && !group.flair_bg_color) {
           group.set("flair_url", "fa-users");
         }
-        group.set("limit", maxMembersToDisplay);
-        return group.findMembers();
+        return group.members.length < maxMembersToDisplay
+          ? group.findMembers({ limit: maxMembersToDisplay }, true)
+          : Promise.resolve();
       })
       .catch(() => this._close())
       .finally(() => this.set("loading", null));

app/assets/javascripts/discourse/components/group-members-input.js.es6

@@ -1,91 +0,0 @@
-import discourseComputed from "discourse-common/utils/decorators";
-import { isEmpty } from "@ember/utils";
-import { lte } from "@ember/object/computed";
-import Component from "@ember/component";
-import { popupAjaxError } from "discourse/lib/ajax-error";
-import { propertyEqual } from "discourse/lib/computed";
-
-export default Component.extend({
-  classNames: ["group-members-input"],
-  addButton: true,
-
-  @discourseComputed("model.limit", "model.offset", "model.user_count")
-  currentPage(limit, offset, userCount) {
-    if (userCount === 0) {
-      return 0;
-    }
-
-    return Math.floor(offset / limit) + 1;
-  },
-
-  @discourseComputed("model.limit", "model.user_count")
-  totalPages(limit, userCount) {
-    if (userCount === 0) {
-      return 0;
-    }
-    return Math.ceil(userCount / limit);
-  },
-
-  @discourseComputed("model.usernames")
-  disableAddButton(usernames) {
-    return !usernames || !(usernames.length > 0);
-  },
-
-  showingFirst: lte("currentPage", 1),
-  showingLast: propertyEqual("currentPage", "totalPages"),
-
-  actions: {
-    next() {
-      if (this.showingLast) {
-        return;
-      }
-
-      const group = this.model;
-      const offset = Math.min(
-        group.get("offset") + group.get("limit"),
-        group.get("user_count")
-      );
-      group.set("offset", offset);
-
-      return group.findMembers();
-    },
-
-    previous() {
-      if (this.showingFirst) {
-        return;
-      }
-
-      const group = this.model;
-      const offset = Math.max(group.get("offset") - group.get("limit"), 0);
-      group.set("offset", offset);
-
-      return group.findMembers();
-    },
-
-    addMembers() {
-      if (isEmpty(this.get("model.usernames"))) {
-        return;
-      }
-      this.model.addMembers(this.get("model.usernames")).catch(popupAjaxError);
-      this.set("model.usernames", null);
-    },
-
-    removeMember(member) {
-      const message = I18n.t("groups.manage.delete_member_confirm", {
-        username: member.get("username"),
-        group: this.get("model.name")
-      });
-
-      return bootbox.confirm(
-        message,
-        I18n.t("no_value"),
-        I18n.t("yes_value"),
-        confirm => {
-          if (confirm) {
-            this.model.removeMember(member);
-          }
-        }
-      );
-    }
-  }
-});

app/assets/javascripts/discourse/components/groups-form-profile-fields.js.es6

@@ -6,6 +6,7 @@ import {
   observes
 } from "discourse-common/utils/decorators";
 import Group from "discourse/models/group";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 import discourseDebounce from "discourse/lib/debounce";
 import EmberObject from "@ember/object";
 
@@ -68,32 +69,34 @@ export default Component.extend({
     name = this.nameInput;
     if (isEmpty(name)) return;
 
-    Group.checkName(name).then(response => {
-      const validationName = "uniqueNameValidation";
+    Group.checkName(name)
+      .then(response => {
+        const validationName = "uniqueNameValidation";
 
-      if (response.available) {
-        this.set(
-          validationName,
-          EmberObject.create({
-            ok: true,
-            reason: I18n.t("admin.groups.new.name.available")
-          })
-        );
+        if (response.available) {
+          this.set(
+            validationName,
+            EmberObject.create({
+              ok: true,
+              reason: I18n.t("admin.groups.new.name.available")
+            })
+          );
 
-        this.set("disableSave", false);
-        this.set("model.name", this.nameInput);
-      } else {
-        let reason;
-
-        if (response.errors) {
-          reason = response.errors.join(" ");
+          this.set("disableSave", false);
+          this.set("model.name", this.nameInput);
         } else {
-          reason = I18n.t("admin.groups.new.name.not_available");
-        }
+          let reason;
 
-        this.set(validationName, this._failedInputValidation(reason));
-      }
-    });
+          if (response.errors) {
+            reason = response.errors.join(" ");
+          } else {
+            reason = I18n.t("admin.groups.new.name.not_available");
+          }
+
+          this.set(validationName, this._failedInputValidation(reason));
+        }
+      })
+      .catch(popupAjaxError);
   }, 500),
 
   _failedInputValidation(reason) {

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

@@ -1,27 +1,25 @@
+import Controller, { inject } from "@ember/controller";
 import { alias } from "@ember/object/computed";
-import { inject } from "@ember/controller";
-import Controller from "@ember/controller";
-import { popupAjaxError } from "discourse/lib/ajax-error";
-import Group from "discourse/models/group";
 import {
   default as discourseComputed,
   observes
 } from "discourse-common/utils/decorators";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 import discourseDebounce from "discourse/lib/debounce";
-import User from "discourse/models/user";
 
 export default Controller.extend({
+  application: inject(),
+
   queryParams: ["order", "desc", "filter"],
+
   order: "",
   desc: null,
-  loading: false,
-  limit: null,
-  offset: null,
-  isOwner: alias("model.is_group_owner"),
-  showActions: false,
   filter: null,
   filterInput: null,
-  application: inject(),
+
+  loading: false,
+  isOwner: alias("model.is_group_owner"),
+  showActions: false,
 
   @observes("filterInput")
   _setFilter: discourseDebounce(function() {
@@ -29,19 +27,33 @@ export default Controller.extend({
   }, 500),
 
   @observes("order", "desc", "filter")
-  refreshMembers() {
-    this.set("loading", true);
-    const model = this.model;
+  _filtersChanged() {
+    this.findMembers(true);
+  },
 
-    if (model && model.can_see_members) {
-      model.findMembers(this.memberParams).finally(() => {
-        this.set(
-          "application.showFooter",
-          model.members.length >= model.user_count
-        );
-        this.set("loading", false);
-      });
+  findMembers(refresh) {
+    if (this.loading) {
+      return;
     }
+
+    const model = this.model;
+    if (!model) {
+      return;
+    }
+
+    if (!refresh && model.members.length >= model.user_count) {
+      this.set("application.showFooter", true);
+      return;
+    }
+
+    this.set("loading", true);
+    model.findMembers(this.memberParams, refresh).finally(() => {
+      this.set(
+        "application.showFooter",
+        model.members.length >= model.user_count
+      );
+      this.set("loading", false);
+    });
   },
 
   @discourseComputed("order", "desc", "filter")
@@ -49,7 +61,7 @@ export default Controller.extend({
     return { order, desc, filter };
   },
 
-  @discourseComputed("model.members")
+  @discourseComputed("model.members.[]")
   hasMembers(members) {
     return members && members.length > 0;
   },
@@ -69,6 +81,10 @@ export default Controller.extend({
   },
 
   actions: {
+    loadMore() {
+      this.findMembers();
+    },
+
     toggleActions() {
       this.toggleProperty("showActions");
     },
@@ -93,38 +109,6 @@ export default Controller.extend({
           .then(() => this.set("usernames", []))
           .catch(popupAjaxError);
       }
-    },
-
-    loadMore() {
-      if (this.loading) {
-        return;
-      }
-      if (this.get("model.members.length") >= this.get("model.user_count")) {
-        this.set("application.showFooter", true);
-        return;
-      }
-
-      this.set("loading", true);
-
-      Group.loadMembers(
-        this.get("model.name"),
-        this.get("model.members.length"),
-        this.limit,
-        { order: this.order, desc: this.desc }
-      ).then(result => {
-        this.get("model.members").addObjects(
-          result.members.map(member => User.create(member))
-        );
-        this.setProperties({
-          loading: false,
-          user_count: result.meta.total,
-          limit: result.meta.limit,
-          offset: Math.min(
-            result.meta.offset + result.meta.limit,
-            result.meta.total
-          )
-        });
-      });
     }
   }
 });

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

@@ -1,25 +1,23 @@
-import { inject } from "@ember/controller";
-import Controller from "@ember/controller";
-import { ajax } from "discourse/lib/ajax";
-import { popupAjaxError } from "discourse/lib/ajax-error";
-import Group from "discourse/models/group";
+import Controller, { inject } from "@ember/controller";
 import {
   default as discourseComputed,
   observes
 } from "discourse-common/utils/decorators";
+import { ajax } from "discourse/lib/ajax";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 import discourseDebounce from "discourse/lib/debounce";
-import User from "discourse/models/user";
 
 export default Controller.extend({
+  application: inject(),
+
   queryParams: ["order", "desc", "filter"],
+
   order: "",
   desc: null,
-  loading: false,
-  limit: null,
-  offset: null,
   filter: null,
   filterInput: null,
-  application: inject(),
+
+  loading: false,
 
   @observes("filterInput")
   _setFilter: discourseDebounce(function() {
@@ -27,51 +25,41 @@ export default Controller.extend({
   }, 500),
 
   @observes("order", "desc", "filter")
-  refreshRequesters(force) {
-    if (this.loading || !this.model) {
+  _filtersChanged() {
+    this.findRequesters(true);
+  },
+
+  findRequesters(refresh) {
+    if (this.loading) {
       return;
     }
 
-    if (
-      !force &&
-      this.count &&
-      this.get("model.requesters.length") >= this.count
-    ) {
+    const model = this.model;
+    if (!model) {
+      return;
+    }
+
+    if (!refresh && model.members.length >= model.user_count) {
       this.set("application.showFooter", true);
       return;
     }
 
     this.set("loading", true);
-    this.set("application.showFooter", false);
-
-    Group.loadMembers(
-      this.get("model.name"),
-      force ? 0 : this.get("model.requesters.length"),
-      this.limit,
-      {
-        order: this.order,
-        desc: this.desc,
-        filter: this.filter,
-        requesters: true
-      }
-    ).then(result => {
-      const requesters = (!force && this.get("model.requesters")) || [];
-      requesters.addObjects(result.members.map(m => User.create(m)));
-      this.set("model.requesters", requesters);
-
-      this.setProperties({
-        loading: false,
-        count: result.meta.total,
-        limit: result.meta.limit,
-        offset: Math.min(
-          result.meta.offset + result.meta.limit,
-          result.meta.total
-        )
-      });
+    model.findRequesters(this.memberParams, refresh).finally(() => {
+      this.set(
+        "application.showFooter",
+        model.requesters.length >= model.user_count
+      );
+      this.set("loading", false);
     });
   },
 
-  @discourseComputed("model.requesters")
+  @discourseComputed("order", "desc", "filter")
+  memberParams(order, desc, filter) {
+    return { order, desc, filter };
+  },
+
+  @discourseComputed("model.requesters.[]")
   hasRequesters(requesters) {
     return requesters && requesters.length > 0;
   },
@@ -94,7 +82,7 @@ export default Controller.extend({
 
   actions: {
     loadMore() {
-      this.refreshRequesters();
+      this.findRequesters();
     },
 
     acceptRequest(user) {

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

@@ -21,10 +21,17 @@ export default Controller.extend({
   @discourseComputed(
     "showMessages",
     "model.user_count",
+    "model.request_count",
     "canManageGroup",
     "model.allow_membership_requests"
   )
-  tabs(showMessages, userCount, canManageGroup, allowMembershipRequests) {
+  tabs(
+    showMessages,
+    userCount,
+    requestCount,
+    canManageGroup,
+    allowMembershipRequests
+  ) {
     const membersTab = Tab.create({
       name: "members",
       route: "group.index",
@@ -41,7 +48,8 @@ export default Controller.extend({
         Tab.create({
           name: "requests",
           i18nKey: "requests.title",
-          icon: "user-plus"
+          icon: "user-plus",
+          count: requestCount
         })
       );
     }

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

@@ -1,31 +1,31 @@
+import EmberObject from "@ember/object";
+import { equal } from "@ember/object/computed";
 import { isEmpty } from "@ember/utils";
-import { notEmpty, equal } from "@ember/object/computed";
-import { ajax } from "discourse/lib/ajax";
 import {
   default as discourseComputed,
   observes
 } from "discourse-common/utils/decorators";
+import { ajax } from "discourse/lib/ajax";
+import Category from "discourse/models/category";
 import GroupHistory from "discourse/models/group-history";
 import RestModel from "discourse/models/rest";
-import Category from "discourse/models/category";
-import User from "discourse/models/user";
 import Topic from "discourse/models/topic";
-import { popupAjaxError } from "discourse/lib/ajax-error";
-import EmberObject from "@ember/object";
+import User from "discourse/models/user";
 
 const Group = RestModel.extend({
-  limit: 50,
-  offset: 0,
   user_count: 0,
+  limit: null,
+  offset: null,
+
+  request_count: 0,
+  requestersLimit: null,
+  requestersOffset: null,
 
   init() {
     this._super(...arguments);
-
-    this.set("owners", []);
+    this.setProperties({ members: [], requesters: [] });
   },
 
-  hasOwners: notEmpty("owners"),
-
   @discourseComputed("automatic_membership_email_domains")
   emailDomains(value) {
     return isEmpty(value) ? "" : value;
@@ -36,50 +36,76 @@ const Group = RestModel.extend({
     return automatic ? "automatic" : "custom";
   },
 
-  @discourseComputed("user_count")
-  userCountDisplay(userCount) {
-    // don't display zero its ugly
-    if (userCount > 0) {
-      return userCount;
+  findMembers(params, refresh) {
+    if (isEmpty(this.name) || !this.can_see_members) {
+      return Ember.RSVP.Promise.reject();
     }
+
+    if (refresh) {
+      this.setProperties({ limit: null, offset: null });
+    }
+
+    params = Object.assign(
+      { offset: (this.offset || 0) + (this.limit || 0) },
+      params
+    );
+
+    return Group.loadMembers(this.name, params).then(result => {
+      const ownerIds = new Set();
+      result.owners.forEach(owner => ownerIds.add(owner.id));
+
+      const members = refresh ? [] : this.members;
+      members.pushObjects(
+        result.members.map(member => {
+          member.owner = ownerIds.has(member.id);
+          return User.create(member);
+        })
+      );
+
+      this.setProperties({
+        members,
+        user_count: result.meta.total,
+        limit: result.meta.limit,
+        offset: result.meta.offset
+      });
+    });
   },
 
-  findMembers(params) {
+  findRequesters(params, refresh) {
     if (isEmpty(this.name) || !this.can_see_members) {
-      return;
+      return Ember.RSVP.Promise.reject();
     }
 
-    const offset = Math.min(this.user_count, Math.max(this.offset, 0));
+    if (refresh) {
+      this.setProperties({ requestersOffset: null, requestersLimit: null });
+    }
 
-    return Group.loadMembers(this.name, offset, this.limit, params).then(
-      result => {
-        const ownerIds = {};
-        result.owners.forEach(owner => (ownerIds[owner.id] = true));
-
-        this.setProperties({
-          user_count: result.meta.total,
-          limit: result.meta.limit,
-          offset: result.meta.offset,
-          members: result.members.map(member => {
-            if (ownerIds[member.id]) {
-              member.owner = true;
-            }
-            return User.create(member);
-          }),
-          owners: result.owners.map(owner => User.create(owner))
-        });
-      }
+    params = Object.assign(
+      {
+        offset: (this.requestersOffset || 0) + (this.requestersLimit || 0),
+        requesters: true
+      },
+      params
     );
+
+    return Group.loadMembers(this.name, params).then(result => {
+      const requesters = refresh ? [] : this.requesters;
+      requesters.pushObjects(result.members.map(m => User.create(m)));
+
+      this.setProperties({
+        requesters,
+        request_count: result.meta.total,
+        requestersLimit: result.meta.limit,
+        requestersOffset: result.meta.offset
+      });
+    });
   },
 
   removeOwner(member) {
     return ajax(`/admin/groups/${this.id}/owners.json`, {
       type: "DELETE",
       data: { user_id: member.id }
-    }).then(() => {
-      // reload member list
-      this.findMembers();
-    });
+    }).then(() => this.findMembers());
   },
 
   removeMember(member, params) {
@@ -272,16 +298,8 @@ Group.reopenClass({
     );
   },
 
-  loadMembers(name, offset, limit, params) {
-    return ajax(`/groups/${name}/members.json`, {
-      data: Object.assign(
-        {
-          limit: limit || 50,
-          offset: offset || 0
-        },
-        params || {}
-      )
-    });
+  loadMembers(name, opts) {
+    return ajax(`/groups/${name}/members.json`, { data: opts });
   },
 
   mentionable(name) {
@@ -293,9 +311,7 @@ Group.reopenClass({
   },
 
   checkName(name) {
-    return ajax("/groups/check-name", {
-      data: { group_name: name }
-    }).catch(popupAjaxError);
+    return ajax("/groups/check-name", { data: { group_name: name } });
   }
 });
 

app/assets/javascripts/discourse/routes/group-index.js.es6

@@ -19,7 +19,7 @@ export default DiscourseRoute.extend({
       filterInput: this._params.filter
     });
 
-    controller.refreshMembers();
+    controller.findMembers(true);
   },
 
   actions: {

app/assets/javascripts/discourse/routes/group-requests.js.es6

@@ -18,6 +18,6 @@ export default DiscourseRoute.extend({
       filterInput: this._params.filter
     });
 
-    controller.refreshRequesters(true);
+    controller.findRequesters(true);
   }
 });

app/assets/javascripts/discourse/templates/components/group-members-input.hbs

@@ -1,30 +0,0 @@
-<label>{{i18n 'groups.members.title'}} ({{model.user_count}})</label>
-
-{{#if model.members}}
-  <div>
-    <a class="previous {{if showingFirst 'disabled'}}" {{action "previous"}}>{{d-icon "fast-backward"}}</a>
-    {{currentPage}}/{{totalPages}}
-    <a class="next {{if showingLast 'disabled'}}" {{action "next"}}>{{d-icon "fast-forward"}}</a>
-  </div>
-  <div class="ac-wrap clearfix">
-    {{#each model.members as |member|}}
-      {{group-member member=member automatic=model.automatic removeAction=(action "removeMember")}}
-    {{/each}}
-  </div>
-{{/if}}
-
-{{#unless model.automatic}}
-  <div class="group-members-input-selector">
-    {{user-selector usernames=model.usernames
-          placeholderKey="groups.selector_placeholder"
-          id="member-selector"}}
-
-    {{#if addButton}}
-      {{d-button action=(action "addMembers")
-          class="add"
-          icon="plus"
-          disabled=disableAddButton
-          label="groups.manage.add_members"}}
-    {{/if}}
-  </div>
-{{/unless}}

app/assets/stylesheets/common/components/group-members-input.scss

@@ -1,9 +0,0 @@
-.group-members-input {
-  .group-members-input-selector {
-    margin-top: 10px;
-
-    .add {
-      margin-top: 7px;
-    }
-  }
-}

app/controllers/groups_controller.rb

@@ -208,20 +208,11 @@ class GroupsController < ApplicationController
 
     guardian.ensure_can_see_group_members!(group)
 
-    limit = (params[:limit] || 20).to_i
+    limit = (params[:limit] || 50).to_i
     offset = params[:offset].to_i
 
-    if limit < 0
-      raise Discourse::InvalidParameters.new(:limit)
-    end
-
-    if limit > 1000
-      raise Discourse::InvalidParameters.new(:limit)
-    end
-
-    if offset < 0
-      raise Discourse::InvalidParameters.new(:offset)
-    end
+    raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > 1000
+    raise Discourse::InvalidParameters.new(:offset) if offset < 0
 
     dir = (params[:desc] && !params[:desc].blank?) ? 'DESC' : 'ASC'
     order = ""

test/javascripts/acceptance/group-requests-test.js.es6

@@ -37,6 +37,7 @@ acceptance("Group Requests", {
           is_group_user: true,
           is_group_owner: true,
           is_group_owner_display: true,
+          can_see_members: true,
           mentionable: false,
           messageable: false
         },