From 352d43b101e20945eb1acbb22abe601b2a07f69a Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Mon, 18 Nov 2019 14:59:28 +0200 Subject: [PATCH] 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. --- .../components/group-card-contents.js.es6 | 6 +- .../components/group-members-input.js.es6 | 91 ------------- .../groups-form-profile-fields.js.es6 | 47 ++++--- .../discourse/controllers/group-index.js.es6 | 94 ++++++------- .../controllers/group-requests.js.es6 | 78 +++++------ .../discourse/controllers/group.js.es6 | 12 +- .../javascripts/discourse/models/group.js.es6 | 126 ++++++++++-------- .../discourse/routes/group-index.js.es6 | 2 +- .../discourse/routes/group-requests.js.es6 | 2 +- .../components/group-members-input.hbs | 30 ----- .../components/group-members-input.scss | 9 -- app/controllers/groups_controller.rb | 15 +-- .../acceptance/group-requests-test.js.es6 | 1 + 13 files changed, 188 insertions(+), 325 deletions(-) delete mode 100644 app/assets/javascripts/discourse/components/group-members-input.js.es6 delete mode 100644 app/assets/javascripts/discourse/templates/components/group-members-input.hbs delete mode 100644 app/assets/stylesheets/common/components/group-members-input.scss diff --git a/app/assets/javascripts/discourse/components/group-card-contents.js.es6 b/app/assets/javascripts/discourse/components/group-card-contents.js.es6 index 50ea8b9ab2..1838b32352 100644 --- a/app/assets/javascripts/discourse/components/group-card-contents.js.es6 +++ b/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)); diff --git a/app/assets/javascripts/discourse/components/group-members-input.js.es6 b/app/assets/javascripts/discourse/components/group-members-input.js.es6 deleted file mode 100644 index aedb43f2a6..0000000000 --- a/app/assets/javascripts/discourse/components/group-members-input.js.es6 +++ /dev/null @@ -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); - } - } - ); - } - } -}); diff --git a/app/assets/javascripts/discourse/components/groups-form-profile-fields.js.es6 b/app/assets/javascripts/discourse/components/groups-form-profile-fields.js.es6 index 9074ef2d63..fda6dd8ec6 100644 --- a/app/assets/javascripts/discourse/components/groups-form-profile-fields.js.es6 +++ b/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) { diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6 index 094c136597..887d5dbfcf 100644 --- a/app/assets/javascripts/discourse/controllers/group-index.js.es6 +++ b/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 - ) - }); - }); } } }); diff --git a/app/assets/javascripts/discourse/controllers/group-requests.js.es6 b/app/assets/javascripts/discourse/controllers/group-requests.js.es6 index ba8c7c971b..013c44f6d8 100644 --- a/app/assets/javascripts/discourse/controllers/group-requests.js.es6 +++ b/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) { diff --git a/app/assets/javascripts/discourse/controllers/group.js.es6 b/app/assets/javascripts/discourse/controllers/group.js.es6 index bd79392b85..b6e9a7ab8f 100644 --- a/app/assets/javascripts/discourse/controllers/group.js.es6 +++ b/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 }) ); } diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 4d6455e891..7822111134 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/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 } }); } }); diff --git a/app/assets/javascripts/discourse/routes/group-index.js.es6 b/app/assets/javascripts/discourse/routes/group-index.js.es6 index a8f630df0f..a3aed42816 100644 --- a/app/assets/javascripts/discourse/routes/group-index.js.es6 +++ b/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: { diff --git a/app/assets/javascripts/discourse/routes/group-requests.js.es6 b/app/assets/javascripts/discourse/routes/group-requests.js.es6 index b299bb01bf..548529cab0 100644 --- a/app/assets/javascripts/discourse/routes/group-requests.js.es6 +++ b/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); } }); diff --git a/app/assets/javascripts/discourse/templates/components/group-members-input.hbs b/app/assets/javascripts/discourse/templates/components/group-members-input.hbs deleted file mode 100644 index 73c5820a85..0000000000 --- a/app/assets/javascripts/discourse/templates/components/group-members-input.hbs +++ /dev/null @@ -1,30 +0,0 @@ - - -{{#if model.members}} -
- - {{currentPage}}/{{totalPages}} - -
-
- {{#each model.members as |member|}} - {{group-member member=member automatic=model.automatic removeAction=(action "removeMember")}} - {{/each}} -
-{{/if}} - -{{#unless model.automatic}} -
- {{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}} -
-{{/unless}} diff --git a/app/assets/stylesheets/common/components/group-members-input.scss b/app/assets/stylesheets/common/components/group-members-input.scss deleted file mode 100644 index fde660de04..0000000000 --- a/app/assets/stylesheets/common/components/group-members-input.scss +++ /dev/null @@ -1,9 +0,0 @@ -.group-members-input { - .group-members-input-selector { - margin-top: 10px; - - .add { - margin-top: 7px; - } - } -} diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index f0ed98e57d..1ca58471ce 100644 --- a/app/controllers/groups_controller.rb +++ b/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 = "" diff --git a/test/javascripts/acceptance/group-requests-test.js.es6 b/test/javascripts/acceptance/group-requests-test.js.es6 index 3e9c189dc7..68ccf7f867 100644 --- a/test/javascripts/acceptance/group-requests-test.js.es6 +++ b/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 },