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

Prevents listing inactive users on member list (#792) #799

Merged
merged 4 commits into from
Sep 14, 2015
Merged

Prevents listing inactive users on member list (#792) #799

merged 4 commits into from
Sep 14, 2015

Conversation

gmsecrieru
Copy link
Contributor

Should close #792

@graywolf336
Copy link
Contributor

👍

@rodrigok
Copy link
Member

@gmsecrieru you've tested your solution? I think this will not work.

The field active of user's record is not sent from server to client.

@marceloschmidt
Copy link
Member

@gmsecrieru a possible solution for this would be to edit publications/activeUsers.coffee and add active: true to the Meteor.users.find query. This way the publication would not even send the inactive users to the client and there would be no need to check that on client side. Another option, though not recommended, would be to pass the active field in the fields list of this publication, but that would only be useful if you were doing anything with the inactive users, that would require them to be present on client side, which I do not believe we need right now.

@gmsecrieru
Copy link
Contributor Author

Thanks for your input and forgive me if my findings stated below are not correct as I am still learning/researching into our code base, but answering your comments:

@rodrigok Yes, I've tested and it does work here:

Searching for inactive user
screen shot 2015-09-14 at 14 23 22

Searching for active user
screen shot 2015-09-14 at 14 23 36

@marceloschmidt I think we do use inactive users at the moment. Check out the Admin's "Users" panel:
screen shot 2015-09-14 at 14 23 56

@marceloschmidt
Copy link
Member

@gmsecrieru When you are in the admin panel, once you select a user, it uses the fullUserData publication, which is only available for admins and that's why there you have the active flag. Try the following test: without being at the admin panel (or without having selected the testdummy), go to the same search and do your test again. You'll find that testdummy won't show either when activated or not activated. In fact, no user will return, unless you have selected it on admin panel.

@gmsecrieru
Copy link
Contributor Author

@marceloschmidt I have created a third user (which is not admin) and performed the following tests using its account:

Search for an active user
screen shot 2015-09-14 at 14 42 04

Search for an inactive user
screen shot 2015-09-14 at 14 42 15

Not sure if I am missing something or if we (even accidentally) already have this data on client somehow. I think I will be able to log into the Demo server in about an hour, maybe we can talk this through better!

Thanks again!

@marceloschmidt
Copy link
Member

@gmsecrieru I see the problem. In fact, when you used active: true on createChannelFlex and directMessagesFlex, you are actually using that on server subscription, where it is totally fine to use that flag. The problem I believe is with membersList. Have you successfully displayed more than yourself in members list? Try logging in with two users and check the members list, see if you can see both users online. I know I should have downloaded your code before trying to provide a solution for what I think doesn't work ;)

@gmsecrieru
Copy link
Contributor Author

@marceloschmidt Yes, it's there! For what it's worth, "george" is and Admin user, thus I don't know if that would reflect by any means in the result of "third.user" member list

screen shot 2015-09-14 at 15 12 01

@marceloschmidt
Copy link
Member

Ok! I'll give it a proper testing later on :) sorry for not doing it before.

Marcelo Schmidt

On Mon, Sep 14, 2015 at 3:14 PM, George Secrieru notifications@github.com
wrote:

@marceloschmidt https://github.com/marceloschmidt Yes, it's there! For
what it's worth, "george" is and Admin user, thus I don't know if that
would reflect by any means in the result of "third.user" member list

[image: screen shot 2015-09-14 at 15 12 01]
https://cloud.githubusercontent.com/assets/190883/9857812/4515670e-5af3-11e5-93d6-c523372d5e28.png


Reply to this email directly or view it on GitHub
#799 (comment)
.

@gmsecrieru
Copy link
Contributor Author

Take your time and thanks!

@marceloschmidt
Copy link
Member

My bad (and @rodrigok's), you were right! Since all your conditions belong to autocomplete filter settings, they are all run on server side, so they are right! This PR is ready for merge @engelgabriel.

engelgabriel added a commit that referenced this pull request Sep 14, 2015
Prevents listing inactive users on member list (#792)
@engelgabriel engelgabriel merged commit 2ddbb0b into RocketChat:master Sep 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants