From 71360436ff968cdbae2f346cfd95716cbf902c6a Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 26 Feb 2019 10:43:24 +0100 Subject: [PATCH] FIX: users list show was loading multiple times with different params (#7058) A first load was happening in route, which was setting properties on controller. These properties were observed on the controller and were triggering a reload of the AdminUser model. Not only was it doing loading two times it was also sometimes resulting on the controller model refresh end to happen after route has been changed, resulting in a wrong model. --- .../controllers/admin-users-list-show.js.es6 | 16 +++---- .../admin/routes/admin-users-list-show.js.es6 | 35 +++++++++------ .../acceptance/admin-users-list-test.js.es6 | 43 +++++++++++++++++++ .../helpers/create-pretender.js.es6 | 10 +++++ 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/app/assets/javascripts/admin/controllers/admin-users-list-show.js.es6 b/app/assets/javascripts/admin/controllers/admin-users-list-show.js.es6 index 16b148f951e..f80222cdc5a 100644 --- a/app/assets/javascripts/admin/controllers/admin-users-list-show.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-users-list-show.js.es6 @@ -1,12 +1,11 @@ import debounce from "discourse/lib/debounce"; import { i18n } from "discourse/lib/computed"; import AdminUser from "admin/models/admin-user"; -import { observes } from "ember-addons/ember-computed-decorators"; import CanCheckEmails from "discourse/mixins/can-check-emails"; export default Ember.Controller.extend(CanCheckEmails, { + model: null, query: null, - queryParams: ["order", "ascending"], order: null, ascending: null, showEmails: false, @@ -47,8 +46,7 @@ export default Ember.Controller.extend(CanCheckEmails, { this._refreshUsers(); }, 250).observes("listFilter"), - @observes("order", "ascending") - _refreshUsers: function() { + _refreshUsers() { this.set("refreshing", true); AdminUser.findAll(this.get("query"), { @@ -57,12 +55,8 @@ export default Ember.Controller.extend(CanCheckEmails, { order: this.get("order"), ascending: this.get("ascending") }) - .then(result => { - this.set("model", result); - }) - .finally(() => { - this.set("refreshing", false); - }); + .then(result => this.set("model", result)) + .finally(() => this.set("refreshing", false)); }, actions: { @@ -95,7 +89,7 @@ export default Ember.Controller.extend(CanCheckEmails, { showEmails: function() { this.set("showEmails", true); - this._refreshUsers(true); + this._refreshUsers(); } } }); diff --git a/app/assets/javascripts/admin/routes/admin-users-list-show.js.es6 b/app/assets/javascripts/admin/routes/admin-users-list-show.js.es6 index 88fb2049547..e893139fc48 100644 --- a/app/assets/javascripts/admin/routes/admin-users-list-show.js.es6 +++ b/app/assets/javascripts/admin/routes/admin-users-list-show.js.es6 @@ -1,17 +1,28 @@ -import AdminUser from "admin/models/admin-user"; - export default Discourse.Route.extend({ - model: function(params) { - this.userFilter = params.filter; - return AdminUser.findAll(params.filter); + queryParams: { + order: { refreshModel: true }, + ascending: { refreshModel: true } }, - setupController: function(controller, model) { - controller.setProperties({ - model: model, - query: this.userFilter, - showEmails: false, - refreshing: false - }); + // TODO: this has been introduced to fix a bug in admin-users-list-show + // loading AdminUser model multiple times without refactoring the controller + beforeModel(transition) { + const routeName = "adminUsersList.show"; + + if (transition.targetName === routeName) { + const params = transition.params[routeName]; + const controller = this.controllerFor(routeName); + if (controller) { + controller.setProperties({ + order: transition.queryParams.order, + ascending: transition.queryParams.ascending, + query: params.filter, + showEmails: false, + refreshing: false + }); + + controller._refreshUsers(); + } + } } }); diff --git a/test/javascripts/acceptance/admin-users-list-test.js.es6 b/test/javascripts/acceptance/admin-users-list-test.js.es6 index 0bbfb491daa..0a2b5c4ad78 100644 --- a/test/javascripts/acceptance/admin-users-list-test.js.es6 +++ b/test/javascripts/acceptance/admin-users-list-test.js.es6 @@ -8,3 +8,46 @@ QUnit.test("lists users", async assert => { assert.ok(exists(".users-list .user")); assert.ok(!exists(".user:eq(0) .email small"), "escapes email"); }); + +QUnit.test("switching tabs", async assert => { + const activeUser = "eviltrout@example.com"; + const suspectUser = "sam@example.com"; + const activeTitle = I18n.t("admin.users.titles.active"); + const suspectTitle = I18n.t("admin.users.titles.suspect"); + + await visit("/admin/users/list/active"); + + assert.equal(find(".admin-title h2").text(), activeTitle); + assert.ok( + find(".users-list .user:nth-child(1) .email") + .text() + .includes(activeUser) + ); + + await click('a[href="/admin/users/list/suspect"]'); + + assert.equal(find(".admin-title h2").text(), suspectTitle); + assert.ok( + find(".users-list .user:nth-child(1) .email") + .text() + .includes(suspectUser) + ); + + await click(".users-list .sortable:nth-child(4)"); + + assert.equal(find(".admin-title h2").text(), suspectTitle); + assert.ok( + find(".users-list .user:nth-child(1) .email") + .text() + .includes(suspectUser) + ); + + await click('a[href="/admin/users/list/active"]'); + + assert.equal(find(".admin-title h2").text(), activeTitle); + assert.ok( + find(".users-list .user:nth-child(1) .email") + .text() + .includes(activeUser) + ); +}); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 9bf0cf85b95..5cda6c828e1 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -458,6 +458,16 @@ export default function() { ]); }); + this.get("/admin/users/list/suspect.json", () => { + return response(200, [ + { + id: 2, + username: "sam", + email: "sam@example.com" + } + ]); + }); + this.get("/admin/customize/site_texts", request => { if (request.queryParams.overridden) { return response(200, { site_texts: [overridden] });