Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: users list show was loading multiple times with different params #7058

Merged
merged 3 commits into from Feb 26, 2019

Conversation

4 participants
@jjaffeux
Copy link
Contributor

commented Feb 24, 2019

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.

@discoursebot

This comment has been minimized.

Copy link

commented Feb 24, 2019

You've signed the CLA, jjaffeux. Thank you! This pull request is ready for review.

@jjaffeux jjaffeux requested a review from eviltrout Feb 24, 2019

FIX: users list show was loading multiple time with different params
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.

@jjaffeux jjaffeux force-pushed the jjaffeux:fix-controlleadmin-usr-list-show branch from 81b5e86 to c2a861b Feb 24, 2019

@discoursebot

This comment has been minimized.

Copy link

commented Feb 25, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/admin-users-tab-switching-breaks-content/109805/8

@discoursebot

This comment has been minimized.

Copy link

commented Feb 25, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/duplicate-network-calls-for-admin-email-sent-skipped-bounced-and-received/59882/5

Show resolved Hide resolved app/assets/javascripts/admin/routes/admin-users-list-show.js.es6
@@ -8,3 +8,42 @@ 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 = "<small>eviltrout@example.com</small>";

This comment has been minimized.

Copy link
@eviltrout

eviltrout Feb 25, 2019

Member

Weird, you are calling text and it's returning the <small> tags? I thought it only ever returned text.

Since this is the case, think it would be better if your selector was ... .email small instead so you can compare the email address without markup.

This comment has been minimized.

Copy link
@jjaffeux

jjaffeux Feb 25, 2019

Author Contributor

This is done this way because this is escaped and I would need to htmlilfy it first before being able to search inside it. See the existing test and the !exists :

QUnit.test("lists users", async assert => {
  await visit("/admin/users/list/active");

  assert.ok(exists(".users-list .user"));
-->  assert.ok(!exists(".user:eq(0) .email small"), "escapes email");
});

This comment has been minimized.

Copy link
@eviltrout

eviltrout Feb 25, 2019

Member

Understood, thanks for clarifying!

@jjaffeux jjaffeux merged commit 7136043 into discourse:master Feb 26, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jjaffeux jjaffeux deleted the jjaffeux:fix-controlleadmin-usr-list-show branch Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.